1 patch for repository http://darcs.net:

Thu Jun 10 16:28:29 COT 2010  builes.adolfo@googlemail.com
  * Resolve issue 1176: caches interfere with --remote-repo flag

Attachments

• resolve-issue-1176_-caches-interfere-with-__remote_repo-flag.dpatch
• unnamed

On Thu, Jun 10, 2010 at 21:46:22 +0000, Adolfo Builes wrote:
> -      addLocal repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [Cache DarcsCache.Repo NotWritable r | r <- repos, isFile r ] ++ cache
> +      addLocal repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [Cache DarcsCache.Repo NotWritable r | r <- repos, isFile r ] ++
> +                                                                   [Cache DarcsCache.Repo NotWritable r | RemoteRepo r <- opts]  ++ cache

So in this code, we assume that the remote repository is more easily
accessible than the other entries in the sources file.  Is that
desirable?

Code-wise: it's probably good to make Cache DarcsCache.Repo NotWritable
a helper function.

Also on IRC, we'd discussed a possible simplification to this scheme
where we add *all* of the explicitly requested repositories (not just
the local ones) and then sort everything.  (Why?) What I would suggest
is doing something like that first and then building this patch on top
of that work.

Also, not related to this patch review, I claim that there is a real
world (in other words, not contrived) scenario in which you will
actually find both a remote-repo flag *and* some explicitly
requested repositories.  Can you find it?  It may be a useful exercise
to think about what that may me (It takes some more familiarity with
Darcs as a user than you may have, so don't worry if you don't find it,
but if you do find it, I think the "ah-hah!" moment may be moderately
rewarding...)

I also think it'd be useful if you could explain to me why we add the
explicitly requested repos and the remote repo to the these caches,
but not the defaultrepo.  I have a reason that makes sense to me, but
I'd like to see what you think just to double check.

>  compareByLocality (Cache _ _  x) (Cache _ _ y)
>    | isLocal x &&  isRemote y  = LT
>    | isRemote x && isLocal y = GT
> +  | isUrl x && isSsh y = LT
> +  | isSsh x && isUrl y = GT

That seems to make sense.  You may want to haddock this
function, maybe just saying the order things are sorted in,
ie.  local < http < ssh

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Hi Kowey



> I also think it'd be useful if you could explain to me why we add the
> explicitly requested repos and the remote repo to the these caches,
> but not the defaultrepo.  I have a reason that makes sense to me, but
> I'd like to see what you think just to double check.
>
>
 The defaultrepo appear as the "explicitly requested" when none is given as
argument, so it will be added to the caches too, however ,It would be
already there because the defaultrepo also is saved in sources. So

> why we add the explicitly requested repos and the remote repo to the these
> caches
>

Is basically a trick which would allows to avoid going to remote repos if we
have local ones in the list, or what was happening in 1176 that It would
first try the ones in the sources before the remote, and because the
previous default which was remote was there, you were asked for it even when
it wasn't what you were expecting.

Attachments

• unnamed

1 patch for repository http://darcs.net:

Sun Jun 13 14:50:59 COT 2010  builes.adolfo@googlemail.com
  * Resolve issue 1176: caches interfere with --remote-repo flag

Attachments

• resolve-issue-1176_-caches-interfere-with-__remote_repo-flag.dpatch
• unnamed

On Sun, Jun 13, 2010 at 20:06:13 +0000, Adolfo Builes wrote:
> -      addLocal repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [Cache DarcsCache.Repo NotWritable r | r <- repos, isFile r ] ++ cache
> +      addRepos repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [ toCache r | r <- repos ] ++

Hmm, now you're adding all the explicitly requested repos (which is
fine), but you're sorting them... shouldn't these be sorted in locality
order?

> +      toCache r = Cache DarcsCache.Repo NotWritable r

Maybe this should be called toReadOnlyCache if you're amending anyway

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

1 patch for repository http://darcs.net:

Mon Jun 14 21:07:26 COT 2010  builes.adolfo@googlemail.com
  *  Resolve issue 1176: caches interfere with --remote-repo flag

Attachments

• -resolve-issue-1176_-caches-interfere-with-__remote_repo-flag.dpatch
• unnamed

Hi Eric

On Sun, Jun 13, 2010 at 3:27 PM, Eric Kow <kowey@darcs.net> wrote:

> On Sun, Jun 13, 2010 at 20:06:13 +0000, Adolfo Builes wrote:
> > -      addLocal repo repos = modifyCache repo $ \ (Ca cache) -> Ca $
> [Cache DarcsCache.Repo NotWritable r | r <- repos, isFile r ] ++ cache
> > +      addRepos repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [
> toCache r | r <- repos ] ++
>
> Hmm, now you're adding all the explicitly requested repos (which is
> fine), but you're sorting them... shouldn't these be sorted in locality
> order?
>
>
I realized I don't need to add "[ toCache r | RemoteRepo r <- opts]" since
that would be a contradiction  to what the manual says

  On the other hand, if any other repositories are supplied as command line
> arguments, this flag will be ignored (and the default repository may be
> overwritten).


And if not command line arguments are given so the repos content would be
the one you give it with --remote-repo :).

So actually to solved this problem is just question of having the sources
sorted as discussed: local < http < ssh.

What I did is that I put the function to keep the caches sorted in the
modifyCache function, so no matter what is done in the cache, it will be
always sorted.

And about the trivia I just dig in changes and ended up in Neil Mitchell's
blog, I see now the need  for --remote-repo ;).
I think the main point here is the fact that  explicitly passed rewrite you
default-repo and you don't want that to happen when the repo you are pulling
from is just  a mean to avoid firewalls, certainly we all have had that bad
experience of ssh port blocked, than happens in my university as well :S.

Attachments

• unnamed

On Mon, Jun 14, 2010 at 21:24:55 -0500, Adolfo Builes wrote:
> I realized I don't need to add "[ toCache r | RemoteRepo r <- opts]" since
> that would be a contradiction  to what the manual says
> 
>   On the other hand, if any other repositories are supplied as command line
> > arguments, this flag will be ignored (and the default repository may be
> > overwritten).

That sounds right, but then how are you handling the issue1176 case,
where I supply --remote-repo and no explicit arguments?

> So actually to solved this problem is just question of having the sources
> sorted as discussed: local < http < ssh.

That does not sound like the whole picture.  So right now, the problem
is that the defaultrepo is in the sources, but the remote repo is not.
How should we deal with that?

> And about the trivia I just dig in changes and ended up in Neil Mitchell's
> blog, I see now the need  for --remote-repo ;).

:-)  I should have mentioned it

> I think the main point here is the fact that  explicitly passed rewrite you
> default-repo and you don't want that to happen when the repo you are pulling
> from is just  a mean to avoid firewalls, certainly we all have had that bad
> experience of ssh port blocked, than happens in my university as well :S.

Not sure I understand.  It wasn't just a means to avoid firewalls for
what it's worth. It's also about not having to type in your password (or
maybe use a slower ssh connection) when all you want to do is darcs
pull.

Thanks,

Eric

PS. In plain-text mode in mutt, your quoting structure seems a little
    weird, because the first line of what you're quoting is not quoted
    (which makes your messages slightly harder to read).

    I suspect it's something related to the gmail web interface, but I'm
    not sure :-/


-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

2 patches for repository http://darcs.net:

Tue Jun 15 09:44:02 COT 2010  builes.adolfo@googlemail.com
  * Resolve issue 1176: caches interfere with --remote-repo flag

Tue Jun 15 10:03:06 COT 2010  builes.adolfo@googlemail.com
  * Removing duplicated entries in addReposToCache

Attachments

• resolve-issue-1176_-caches-interfere-with-__remote_repo-flag.dpatch
• unnamed

I'll apply this first patch, but I have question about the second one

Resolve issue 1176: caches interfere with --remote-repo flag
------------------------------------------------------------
> -      addLocal repo repos = modifyCache repo $ \ (Ca cache) -> Ca $ [Cache DarcsCache.Repo NotWritable r | r <- repos, isFile r ] ++ cache
> +      addReposToCache repos (Ca cache) = Ca $ [ toReadOnlyCache r | r <- repos ] ++  cache
> +      toReadOnlyCache = Cache DarcsCache.Repo NotWritable

Adolfo explained to me on IRC that the defaultrepo/remote repo are
actually treated as explicitly requested repos (via Darcs.RunCommand)
if no other arguments are passed.

So this looks fine.

I'll just say that

> +      addReposToCache repos (Ca cache) = Ca $ [ toReadOnlyCache r | r <- repos ] ++  cache

probably is cleaner written like

         addReposToCache repos (Ca cache) = Ca $ map toReadOnlyCache repos ++ cache

but I don't mind either way

> --- | Modifies the cache of a given repository with the function f
> +-- | Modifies the cache of a given repository with the function f.
> +--   No matter which modification is done over the cache, it always
> +--   returns a sorted cache where local < http < ssh
>  modifyCache :: FORALL(p r u t) (RepoPatch p)  => Repository p C(r u t) -> (Cache -> Cache) -> Repository p C(r u t)
> hunk ./src/Darcs/Repository/InternalTypes.hs 53
> -modifyCache (Repo dir opts rf (DarcsRepository pristine cache)) f =
> -  Repo dir opts rf (DarcsRepository pristine (f cache))
> -
> +modifyCache (Repo dir opts rf (DarcsRepository pristine cache)) f = Repo dir opts rf dr
> +  where dr       = DarcsRepository pristine newCache
> +        newCache = ( \ (Ca c) -> Ca $ sortBy compareByLocality c) $ f cache

Maybe Ca should be a Functor so you could just fmap (sortBy compareByLocality)
on it?  (This could end up being one of those really bad ideas)

Removing duplicated entries in addReposToCache
----------------------------------------------
> builes.adolfo@googlemail.com**20100615150306
>  Ignore-this: 47fbcc3221eee93719681bad059dafe2
> ] hunk ./src/Darcs/Commands/Pull.lhs 181
>        applyPatches opts' repository r
>      where
>        opts' = mergeOpts opts
> -      addReposToCache repos (Ca cache) = Ca $ [ toReadOnlyCache r | r <- repos ] ++  cache
> +      addReposToCache repos (Ca cache) = Ca $ nub $ [ toReadOnlyCache r | r <- repos ] ++  cache
>        toReadOnlyCache = Cache DarcsCache.Repo NotWritable

This seems like an improvement anyway, but why don't we nub in
modifyCache since we're happy to sort there?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Hi


>
>
This seems like an improvement anyway, but why don't we nub in
> modifyCache since we're happy to sort there?
>
>
Yeah that totally makes sense, I will send it like a new patch, and the
stylistic changes will be other ( although I will think through about making
Ca a functor), I also want to apply some style suggestions from Petr.

btw: Is the quoting fixed ? I did a trick maybe It will work now.

Adolfo

Attachments

• unnamed

Hi Eric, a quick question with this one

I'll apply this first patch, but I have question about the second on



Should I wait for you to apply that change and then send the patch for the
nub in modifyCache ? I was trying to resend that particular patch ( the one
with nub in modifyCache ) and It would make me send also the patch "Resolve
issue 1176" again.

Adolfo

Attachments

• unnamed

On Wed, Jun 16, 2010 at 01:06:55 -0500, Adolfo Builes wrote:
> Should I wait for you to apply that change and then send the patch for the
> nub in modifyCache ? I was trying to resend that particular patch ( the one
> with nub in modifyCache ) and It would make me send also the patch "Resolve
> issue 1176" again.

No need to wait; just send it again.  Darcs is smart enough to know I've
already applied the patch if you don't amend.

Yeah, there are some wishlist items for being able to co-operate more
with the patch tracker, but that's for some distant future, I expect.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9