[darcs-users] darcs patch: Rename hashSlurped, slurpHashed and sync... (and 2 more)

[David Roundy]

On Tue, Sep 02, 2008 at 03:30:42AM +0200, me at mornfall.net wrote:
> Hi,
> 
> this bundle implements the discussed refactor, mostly a prelude to newer,
> better and shinier fix for issue971. I have thrown in some haddocks, just to
> please Kowey. (Can't you see what a karma whore I have become? But, I blame
> Lispy. Braaaii^WCoookieeezzz!)
> 
> Yours,
>    Petr.
> 
> Tue Sep  2 02:44:02 CEST 2008  me at mornfall.net
>   * Rename hashSlurped, slurpHashed and syncHashed to writeHashedPristine, slurpHashedPristine and syncHashedPristine, respectively.
> 
> Tue Sep  2 03:04:05 CEST 2008  me at mornfall.net
>   * Change type of subdir parameter in Cache/HashedIO functions from String to HashedDir.
>   
>   This refactor should make calling the Cache and HashedIO functions safer: you
>   should be no longer able to swap hash and subdir accidentally in the call site,
>   or mistype the subdirectory name.
> 
> Tue Sep  2 03:24:39 CEST 2008  me at mornfall.net
>   * Haddock the {slurp,write,sync}HashedPristine functions in HashedIO.

Looks good on the whole (and applied).  I have a few comments, below...

> hunk ./src/Darcs/Repository/Cache.lhs 7
>  module Darcs.Repository.Cache (
>                     cacheHash, okayHash, takeHash,
>                     Cache(..), CacheType(..), CacheLoc(..), WritableOrNot(..),
> +                   HashedDir(..), hashedDir,
>                     unionCaches, cleanCaches,
>                     fetchFileUsingCache, speculateFileUsingCache, writeFileUsingCache,
>                     findFileMtimeUsingCache, setFileMtimeUsingCache, peekInCache,

I wonder if it might be a good idea to not export hashedDir? It
wouldn't gain us much in the way of safety, but I don't know that we
need hashedDir outside Repository.Cache, and generally it seems a good
idea to avoid exporting functions that aren't needed:  it makes the
export list more useful to folks reading the code, and it also
prevents possible abuses.  In particular, it seems like a bad idea for
code outside of Repository.Cache to ever read or write directly in the
hashed directories--the whole point of this module is to govern the
interaction with hashed files, for instance, so that we'll
automatically download hashed files that we might happen to be
missing, properly using a global cache (if configured) and all that.

> hunk ./src/Darcs/Repository/Cache.lhs 118
>            cacheHasIt (Cache t Writable d:cs) = do ex <- doesFileExist (fn t d)
>                                                    if ex then return True
>                                                          else cacheHasIt cs
> -          fn Directory d = d ++ "/" ++ subdir ++ "/" ++ f
> -          fn Repo r = r ++ "/"++darcsdir++"/" ++ subdir ++ "/" ++ f
> +          fn Directory d = d ++ "/" ++ (hashedDir subdir) ++ "/" ++ f
> +          fn Repo r = r ++ "/"++darcsdir++"/" ++ (hashedDir subdir) ++ "/" ++ f

Note that the parens around hashedDir subdir aren't needed.  This
isn't something that needs fixing, but in general I prefer to avoid
unnecessary parens.  It makes the code a bit more concise, and for me
is easier to parse (partly because I know the precedence rules... but
it's definitely worth learning the precedence rules, particularly for
: and ++).

Thanks again! (and I'll read the next patch bundle that includes this
one next)

David