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

[David Roundy]

On Tue, Sep 2, 2008 at 11:08 AM, Petr Rockai <me at mornfall.net> wrote:
> 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.

I see.  I hadn't actually looked at those uses...

> 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?)

Yeah, this does seem like a reasonable (albeit not compelling) refactor.

> (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.)

Yes.  One hokey approach for some of these issues is to
enforce/document import rules in tests/haskell_policy.sh.  This isn't
a substitute for small clean interfaces, but could help a bit to keep
excessive export lists from causing trouble.  A quick grep shows, for
instance, that Repository.Cache is imported twice from code outside
the Darcs.Repository hierarchy.  Once in Repair and once in ShowRepo.
The latter case I'm fixing, and a refactor of repair will fix the
former, and then we could put a rule into haskell_policy.sh.

I'd prefer for the module hierarchy itself to reflect import rules:
That module Darcs.x.y should only be imported in Darcs.x or Darcs.x.z,
etc, so that the hierarchy really conveyed useful information.  But it
wasn't originally set up with this as a goal (so far as I can tell),
and we've no clear distinction (clear to anyone but myself, that is)
between "internal subsystem APIs" and "preferred exported APIs".  :(
Except in the case of "Internal" and "Private" in module names (which
seems pretty clear to me), and even that isn't currently obeyed by the
code (specifically, ShowRepo and Repair).

David