[darcs-users] [patch257] Resolve issue 1503

Eric Kow kowey at darcs.net
Tue Jun 1 17:40:32 UTC 2010

On Tue, Jun 01, 2010 at 16:28:54 +0000, Adolfo Builes wrote:
> Mon May 31 20:15:34 COT 2010  builes.adolfo at googlemail.com
>   * Resolve issue 1503

Hooray, first GSoC patch!

OK, some minor things to fix at least:

 * the patch name
 * separate out unrelated patches from content patch

Things to think about and discuss if needed:

 * design/functionality questions
 * stylistic changes
 * testing

As for testing: Is there any reasonable way you can think of to test
this patch automatically?  See the network directory for some examples.
We could set up some test repositories on darcs.net if you need.

Resolve issue 1503
This should have a slightly more descriptive title.  For example,

 Resolve issue1503: treat pull repos as source entries 

You're not always going to be able to fit everything into the patch
name, but just get the important points down so that people know what
it is at a glance.  You could always say more in the patch log if you
feel the need.


> -import Darcs.Repository.Merge

Cleanup changes like this (and your trailing whitespace cleanup) should
be done in a separate patch so that it's clearer what the actual meat of
your work is.

> +import qualified Darcs.Repository.InternalTypes as DRI

Importing these internal modules is a often bad sign (sometimes it's a
necessary evil, because the Darcs.Repository.* layer is not very clean).
Do we really need it?  Can we import from Darcs.Repository instead?

>  pullCmd :: [DarcsFlag] -> [String] -> IO ()
>  pullCmd opts repos =
> -    withRepoLock opts $- \repository ->
> -        fetchPatches opts' repos "pull" repository >>= applyPatches opts' repository
> +    withRepoLock opts $- \repository -> do
> +      repository' <- addLocalReposToCache  repository repos
> +      out <- fetchPatches opts' repos "pull" repository'
> +      applyPatches opts' repository' out

Note that in situations like this, it's easy (for me) to accidentally
use the wrong identifier (eg. repository when I mean repository').

I wonder if there's a way you can get rid of one of these points
somehow.  Maybe some sort of partial application thing like

  withRepoLock opts $- addLocalReposToCache repos >>= $ \repository ->

> hunk ./src/Darcs/Commands/Pull.lhs 186
> +-- | If we are pulling from local repositories, they are appended to
> +-- the list of the caches loaded from sources, the idea behind is to
> +-- avoid going to remote repositories, as darcs always try first the
> +-- ones read from sources, before trying the one given as arguments
> +-- with the command pull.

Thanks for the haddock.

This is interesting because your code makes this only half-true now, in
the sense that Darcs will only do this if the repos we are pulling from
are remote ones.  So maybe a clearer way here is not to talk about how
Darcs behaves in general and just focus on the specific case.  For
example, "this is to ensure that local repositories will be tried first,
even if there are already remote repositories in the sources."

> +addLocalReposToCache ::  forall p C(r u). RepoPatch p => Repository p C(r u r) ->
> +                        [String] -> IO (Repository p C(r u r))
> +addLocalReposToCache (DRI.Repo dir opts rf(DRI.DarcsRepository pristine (DRC.Ca ccache))) repos =
> +  do
> +    pullingFrom <-  (mapM (fixUrl opts) repos)
> +    let cache = DRC.Ca (appendRepos ccache pullingFrom)
> +    return (DRI.Repo dir opts rf (DRI.DarcsRepository pristine cache))

I wonder if it would be better to have a function on the
Darcs.Repository layer like

modifyCache :: forall p C(r u t). RepoPatch p => (Cache -> Cache)
            -> Repository p C(r u t)
            -> Repository p C(r u t)

> +    where
> +      appendRepos cache [] = cache
> +      appendRepos cache (x:xs) = if isFile x
> +                                   then
> +                                    (DRC.Cache DRC.Repo DRC.NotWritable x) : appendRepos cache xs
> +                                   else
> +                                    appendRepos cache xs

Hmm, this seems like a step in the right direction because it would
ensure that when pulling from local repositories, we will not
accidentally prioritise any remote repositories in the sources list.

It sounds like this would solve this particular bug.  But how about a
more general solution which divides cache entries by locality?

For example you could say something like

 compareByLocality x y
  | isLocal x && isRemote y = LT
  | isRemote x && isLocal y = GT
  | otherwise = compare x y

Your approach is a bit more conservative, but I think the approach I
suggest is a bit more general as it will cause Darcs to systematically
prefer all local cache entries to remote ones.

Stylistic issues [hlint could be helpful here]:

* I think you mean prepend instead of append (I think appending xs to ys
  means ys ++ xs).
* It seems like appendRepos would be a simpler with filter/map instead
  of explicit recursion.
* Superfluous parentheses (mapM...)

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/20100601/faf54e69/attachment.pgp>

More information about the darcs-users mailing list