[darcs-users] [patch282] Resolve issue 1176: caches interfere wit... (and 1 more)

Eric Kow kowey at darcs.net
Wed Jun 16 20:53:35 UTC 2010

On Wed, Jun 16, 2010 at 17:20:26 +0000, Adolfo Builes wrote:
> Tue Jun 15 09:44:02 COT 2010  builes.adolfo at googlemail.com
>   * Resolve issue 1176: caches interfere with --remote-repo flag

I already reviewed this, so I'll skip it.

> Wed Jun 16 11:23:30 COT 2010  builes.adolfo at googlemail.com
>   * Removes duplicated entries after any modification is done to the cache

Almost there.  Just one more change (to the haddock)

Removes duplicated entries after any modification is done to the cache
> +-- | @modifyCache repository@ function@ modifies the cache of
> +--   @repository@ with @function at .  After the modification is done the
> +--   duplicated entries are removed and then the cache is sorted with
> +--   local < http < ssh

There are three problems with the haddock that I can see:

 - You have a syntax error in the top line (note that I never ask people
   to amend haddock syntax errors, A. because I'm so grateful they even
   bother to write them and B. because it's more efficient for me to
   just fix it, point it out and be done with it)

 - You are repeating the documentation for
   'Darcs.Repository.Cache.compareByLocality' when you
   could just as easily link to it.  Duplication is the root of all
   evil.  Imagine we one day decide to change 'compareByLocality' and we
   (inevitably) forget to update this haddock.  It's the kind of thing
   that's subtle, we'll never notice it for years until somebody does
   something wrong...

 - Your comment reveals too much about the implementation of the
   function.  Your job is not to tell me what the code says, but
   I as a user of the function can expect from it.
   In particular, X happens, then Y and Z is not entirely appropriate.
   The subtlety here is that (f . sort) is not the same as (sort . f).
   Maybe we should think about this on IRC

Can you fix these?  I think once you address the two key issues above,
I can just apply what you have and maybe do some minor cleaning up

> -  where dr       = DarcsRepository pristine newCache
> -        newCache = ( \ (Ca c) -> Ca $ sortBy compareByLocality c) $ f cache
> +  where dr                = DarcsRepository pristine $ nubAndSort $ f cache
> +        nubAndSort (Ca c) = Ca $ sortBy compareByLocality $ nub c

This seems fine.  I think the way I would written it was something like
the below (again with the Functor idea), but it could just be my taste,
which is flawed :-)
  dr = DarcsRepository pristine
     $ cmap (nub . sortBy compareByLocality .  f) cache
  cmap f (Ca c) = Ca (f c)

But you should probably ignore my comment above

Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100616/19219479/attachment.pgp>

More information about the darcs-users mailing list