[darcs-users] [patch257] Resolve issue 1503

Eric Kow kowey at darcs.net
Wed Jun 2 10:30:32 UTC 2010


OK, I think I'm almost ready to apply this (no obvious bugs or things to
complain about).

But I have some design questions to ask you, so I'd like to wait for you
to tell me what is the right thing to do.  (This really is largely about
me not knowing how to manage certain tradeoffs, so what I'm trying to do
is make sure that you've at least thought about them and are not
counting on me to have the right answers).

An example of the kind of trade-off to manage is "How close to the core
should X bit of code be?  Do I want it in the surface-level code?  Or
does it belong deeper down?"  One could spend hours arguing about it,
which may not be productive.  Alternatively, we could at least make sure
we've made a sort of "best effort" at thinking about it and then just
make a decision and see how things go.  So please let me know what the
right thing to do is :-)

Resolve issue 1503: try local repos before remote ones.
-------------------------------------------------------
> +    pullingFrom <- mapM (fixUrl opts) repos
> +    withRepoLock opts $- \initialRepository -> 
> +      modifyCache initialRepository pullingFrom  >>= \repository -> 
> +      fetchPatches opts' repos "pull" repository >>= applyPatches opts' repository

OK, we still have the two repositories here but at least it's clearer
which one you should be using at all times.

You could get rid of the extra argument by reversing the definition of
modifyCache so it lends itself more to partial application.

> +-- | Add locals repositories to cache, ensuring that locals are tried before any remote one.
> +modifyCache ::  forall p C(r u). RepoPatch p => Repository p C(r u r) ->
> +                        [String] -> IO (Repository p C(r u r))
> +modifyCache (Repo dir opts rf(DarcsRepository pristine (Ca ccache))) repos =

I think modifyCache would be more useful as a higher order function
(otherwise, I would be not so inclined to have it in the
Darcs.Repository hierarchy).

Rather than being specifically about adding the local repositories to
the cache, it could just be about applying some (Cache -> Cache) on the
repository's cache.

> +  do
> +    let cache = Ca (addLocals ccache repos)
> +    return (Repo dir opts rf (DarcsRepository pristine cache))
> +    where
> +      addLocals = foldr (\a b -> if isFile a
> +                                 then Cache DarcsCache.Repo NotWritable a : b
> +                                 else b)

OK, you've gotten rid of the explicit recursion, but perhaps something
like this formulation would be clearer?

  addLocals xs ys = locals xs ++ ys
  locals = map (Cache DarcsCache.Repo NotWritable) . filter isFile

Or maybe

  addLocals xs ys = [ Cache DarcsCache.Repo NotWritable x | x <- xs, isFile x ] ++ ys

> +compareByLocality (Cache _ _  x) (Cache _ _ y)
> +  | isFile x && (isUrl y || isSsh y)  = LT
> +  | (isUrl x || isSsh x) && isFile y = GT
> +  | otherwise = EQ 

Every top level function should have a type signature.
You can refactor this slightly with something like

  where
   isRemote z = isUrl z || isSSh z

Or maybe it'd make sense in the interest of clarity to define both

  where
   isLocal  z = isFile z
   isRemote z = isUrl z || isSSh z

Or maybe we want to make it clear that the two cases are mutually
exclusive (in which case you need to reason first if that's what
you really want to say)

  where
    isLocal  = isFile
    isRemote = not . isLocal

>  copyFileUsingCache :: OrOnlySpeculate -> Cache -> HashedDir -> String -> IO ()
> -copyFileUsingCache oos (Ca cache) subdir f =
> -    do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f
> +copyFileUsingCache oos (Ca unsortedCache) subdir f =
> +    do let (Ca cache) = Ca (sortBy compareByLocality unsortedCache)
> +       debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f

Interesting.  One question: does this sorting belong here?  What if in
the future we want to sort by proximity to the repository directory?
How would we manage that?  What's the right attitude to adopt here?  One
possible answer, for example, is YAGNI.  Is that the right one?

>  fetchFileUsingCachePrivate :: FromWhere -> Cache -> HashedDir -> String -> IO (String, B.ByteString)
> -fetchFileUsingCachePrivate fromWhere (Ca cache) subdir f =
> +fetchFileUsingCachePrivate fromWhere (Ca unsortedCache) subdir f =
>      do when (fromWhere == Anywhere) $ copyFileUsingCache ActuallyCopy (Ca cache) subdir f
>         ffuc cache
>      `catchall` debugFail ("Couldn't fetch `"++f++"'\nin subdir "++(hashedDir subdir)++
> hunk ./src/Darcs/Repository/Cache.hs 265
>  
>            ffuc [] = debugFail $ "No sources from which to fetch file `"++f++"'\n"++ show (Ca cache)
>  
> +          Ca cache = Ca (sortBy compareByLocality unsortedCache)

What's the difference between these two functions?

-- 
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: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100602/78b94923/attachment.pgp>


More information about the darcs-users mailing list