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

[Petr Rockai]

Hi,

David Roundy <droundy at darcs.net> writes:
> 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.
True, it might be a little tricky to avoid the export, as the function has a
couple uses in HashedIO and HashedRepo -- moreover, I see no obvious
higher-level replacement that could be exported to those two modules.

Maybe:
describeHashFile :: HashedDir -> String -> String
describeHashFile subdir hash = hash ++ " in subdir " ++ hashedDir subdir

that could be generally used by debugMessage (it would still be mostly useful
inside Cache, it seems) and a

hashedFilePath :: CacheLoc -> HashedDir -> String -> String
hashedFilePath (Cache Directory _ d) hd h = d ++ "/" ++ hashedDir hd ++ "/" ++ h
hashedFilePath (Cache Repo _ r) hd h = r ++ "/" ++ darcsdir "/" ++ hashedDir hd ++ "/" ++ h

which should be able to replace those "fn" locals duplicated a few times in
Cache.lhs (quick grep reveals 4 apparently identical instances). If we want to
push for removing the export, we might still need to drop some debugging output
and/or shuffle the code around a little, though. I'd probably defer that to
bundle with some possible future export refactoring in the neighbouring
modules. Probably haddocking functions as "appropriate only for Foo and Bar
modules" might be an acceptable interim solution that doesn't involve code
shuffles. Obviously, it won't be machine-checked, but sometimes you can't have
your cake and eat it too. (Yes, a way to restrict exports in the Haskell module
system would probably help us here. I don't think that's possible though?)

(And as a side-note #1, I'd probably add, that reducing export lists and
increasing module separation is, I believe, a good way to improve overall code
quality and comprehensibility: small, well-defined interfaces play a key role
in, at least my, ability to understanding code. Moreover, such code separation
tends to reduce impact of changes to smaller areas of code, which is good as
well.)

As a side-note #2, generally, the path handling seems to be a little fragile
overall, looking at all those examples of constructing paths. I'd probably
prefer an unified way to get paths into working and to repository, although the
access methods seem to be a mix of "assume our CWD is our repo", "wrap in
withCurrentDirectory, extract path from Repository" and possibly some others. A
cleanup on that front sounds like a non-trivial undertaking for sure, but would
improve code safety (and clarity), I believe. Maybe that's something the
RepoPath work has started to address, I haven't looked in much detail there
(but the quick look struck me as being a little hairy and that it might be
possible to replace most of the guts with System.FilePath -- something Eric has
proposed, along a safe-ish migration path, although laborious at best...).

Probably just a little someday-ish sigh.

>
>> 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 ++).
Right, if you'd ask me, I'd be able to tell that application takes precedence
here, I just sometimes have inclinations to add a little more parens than
needed, by default. I can try to keep them down for the future. :)

Yours, Petr
  (doing one too many things at once,
   hopefully the post still makes some sense).

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation