[darcs-users] darcs patch: Change type of subdir parameter in Cache... (and 2 more)

[Jason Dagit]

On Tue, Sep 2, 2008 at 12:54 PM,  <me at mornfall.net> wrote:

My attempt at reviewing....

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

That sounds positive.

But, I wonder about this hunk:
hunk ./src/Darcs/Repository/HashedIO.lhs 70
 -}

 data HashDir r p = HashDir { permissions :: !r, cache :: !Cache,
-                             options :: ![DarcsFlag], rootHash :: !String,
-                             hashDir :: !String }
+                             options :: ![DarcsFlag], rootHash :: !String }
 type HashedIO r p = StateT (HashDir r p) IO

Why did you remove the hashDir from the record?  Why did you change
hashDir :: HashedDir?


> Tue Sep  2 20:57:45 CEST 2008  me at mornfall.net
>  * Refactor Cache's handling of hashed paths. No functional change.
>
>  Factored out the filepath building to a single place. This also led to folding
>  the explicit pattern matches on writability into a predicate, since the other
>  components of a CacheLoc are no longer useful in the function bodies.

This seems fine to me.

>
> Tue Sep  2 21:35:59 CEST 2008  me at mornfall.net
>  * Use qualified imports and shorter names on Cache functions. No functional change.

I like qualified imports just fine.  I think Data.ByteString sets a
good example here.  We don't do it much in darcs currently, but I
think I like this change.

That's my review.  I would accept all the above patches modulo the
hashDir change.

Thanks,
Jason