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

David Roundy droundy at darcs.net
Tue Sep 2 13:34:07 UTC 2008


On Tue, Sep 02, 2008 at 11:18:41AM +0100, Eric Y. Kow wrote:
> Just reviewing.  
> 
> > Tue Sep  2 02:44:02 CEST 2008  me at mornfall.net
> >   * Rename hashSlurped, slurpHashed and syncHashed to writeHashedPristine, slurpHashedPristine and syncHashedPristine, respectively.
> 
> Just a straightforward rename.  In fact, since David's already oked this
> in principle, I would just fast-track.
>  
> > 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.
> 
> I like the idea behind this refactor.  It also falls into a general
> trend of using explicit types rather than bools or strings to make
> things explicit and more resistant to future-us making silly mistakes.
>    
> > Tue Sep  2 04:05:32 CEST 2008  me at mornfall.net
> >   * Haddock the {slurp,write,sync}HashedPristine functions in HashedIO.
> 
> Another fast-trackee
> 
> Content-Description: A darcs patch for your repository!
> 
> > [Rename hashSlurped, slurpHashed and syncHashed to writeHashedPristine, slurpHashedPristine and syncHashedPristine, respectively.
> > me at mornfall.net**20080902004402] hunk ./src/Darcs/Repository/HashedIO.lhs 326
> 
> >  syncHashed :: Cache -> Slurpy -> String -> IO ()
> >  syncHashed c s r = do runStateT sh $ HashDir {permissions=RW, cache=c, options=[], rootHash=r, hashDir="pristine.hashed" }
> > -                      return ()
> > +                              return ()
> 
> :-) darcs replace won't fix indentation for you
> 
> > replace ./src/Darcs/Repository/HashedIO.lhs [A-Za-z_0-9] hashSlurped writeHashedPristine
> > replace ./src/Darcs/Repository/HashedIO.lhs [A-Za-z_0-9] slurpHashed slurpHashedPristine
> > replace ./src/Darcs/Repository/HashedIO.lhs [A-Za-z_0-9] syncHashed syncHashedPristine
> > replace ./src/Darcs/Repository/HashedRepo.lhs [A-Za-z_0-9] hashSlurped writeHashedPristine
> > replace ./src/Darcs/Repository/HashedRepo.lhs [A-Za-z_0-9] slurpHashed slurpHashedPristine
> > replace ./src/Darcs/Repository/HashedRepo.lhs [A-Za-z_0-9] syncHashed syncHashedPristine
> 
> Looks good.
> 
> > +data HashedDir = HashedPristineDir | HashedPatchesDir | HashedInventoriesDir
> 
> There is a potential confusion between HashedDir and HashDir.
> 
> But looking at the rest of the code, I see that this HashDir data type
> is only used for interacting with pristine (and not, for example, with
> the patch dir), so maybe the latter should be renamed to reflect this.
> For that matter, it seems like there's a lot of HashedIO functions that
> will also want renaming if we're going to de-parameterise them.

Hmmm.  I hadn't seen this.  It's a definite point of possible
confusion, but doesn't seem too bad, given that HashDir isn't exported
from HashedIO, which itself isn't imported into (I think) anything but
HashedRepo.  It might be worth renaming HashDir to PristineDir or
something.  I'm not sure, though.

> > -findFileMtimeUsingCache :: Cache -> String -> String -> IO EpochTime
> > +findFileMtimeUsingCache :: Cache -> HashedDir -> String -> IO EpochTime
> >  findFileMtimeUsingCache (Ca cache) subdir f = mt cache
> >      where mt [] = return undefined_time
> >            mt (Cache Repo Writable r:_) = (modificationTime `fmap`
> > hunk ./src/Darcs/Repository/Cache.lhs 97
> > -                                          getSymbolicLinkStatus (r++"/"++darcsdir++"/"++subdir++"/"++f))
> > +                                          getSymbolicLinkStatus (r++"/"++darcsdir++"/"++(hashedDir subdir)++"/"++f))
> 
> Silly style detail: you don't need the parens around (hashedDir subdir)
> because in Haskell, function application has higher precedence than any
> infix operator

:)

> > +gethashmtime h = do HashDir _ c _ _ <- get
> > +                    lift $ unsafeInterleaveIO $ findFileMtimeUsingCache c HashedPristineDir h
> 
> Here's an example of the kind of function which probably should be
> renamed.  getPristineHashMtime?

I personally prefer to restrict theseCaps to functions that are
actually exported.  Given that gethashmtime is only called once, a
better option might be to define it locally in a where clause, if
we're going to refactor it at all.

David


More information about the darcs-users mailing list