[PATCH 2 of 2 NOPUSH stable] fix bundle error (issue XXX)

[Sune Foldager]

On 2012-05-12 17:22, Benoit Boissinot wrote:
>On Sat, May 12, 2012 at 5:04 PM, Sune Foldager <cryo at cyanite.org> wrote:
>
>> On 2012-05-12 16:56, Benoit Boissinot wrote:
>>
>>> On Sat, May 12, 2012 at 4:45 PM, Sune Foldager <cryo at cyanite.org> wrote:
>>>
>>>  On 2012-05-12 16:05, Benoit Boissinot wrote:
>>>>
>>>>  On Sat, May 12, 2012 at 3:39 PM, Sune Foldager <cryo at cyanite.org>
>>>>> wrote:
>>>>>
>>>>>  # HG changeset patch
>>>>>
>>>>>> # User Sune Foldager <cryo at cyanite.org>
>>>>>> # Date 1336829974 -7200
>>>>>> # Branch stable
>>>>>> # Node ID b81e7e187080391f0608e28fda5d11****fb06c8b7ee
>>>>>> # Parent  381f8c47ba233d1c80489172d59a22****2944f465f0
>>>>>>
>>>>>> fix bundle error (issue XXX)
>>>>>>
>>>>>> diff -r 381f8c47ba23 -r b81e7e187080 mercurial/localrepo.py
>>>>>> --- a/mercurial/localrepo.py    Sat May 12 15:33:50 2012 +0200
>>>>>> +++ b/mercurial/localrepo.py    Sat May 12 15:39:34 2012 +0200
>>>>>> @@ -1782,11 +1782,7 @@
>>>>>>       common = set(cl.ancestors(*[cl.rev(n) for n in bases]))
>>>>>>       return self._changegroupsubset(****common, csets, heads, source)
>>>>>>
>>>>>>
>>>>>> -    def getlocalbundle(self, source, outgoing):
>>>>>> -        """Like getbundle, but taking a discovery.outgoing as an
>>>>>> argument.
>>>>>> -
>>>>>> -        This is only implemented for local repos and reuses
>>>>>> potentially
>>>>>> -        precomputed sets in outgoing."""
>>>>>> +    def _getbundle(self, source, outgoing):
>>>>>>       if not outgoing.missing:
>>>>>>           return None
>>>>>>       return self._changegroupsubset(****outgoing.common,
>>>>>>
>>>>>> @@ -1794,6 +1790,18 @@
>>>>>>                                      outgoing.missingheads,
>>>>>>                                      source)
>>>>>>
>>>>>> +    def getlocalbundle(self, source, outgoing):
>>>>>>
>>>>>>
>>>>>>  You forgot to switch push() to _getbundle().
>>>>>
>>>>>
>>>> Right.
>>>>
>>>>
>>>>  I don't think that's the right place to fix it. I'd do it in
>>>> discovery.py
>>>>
>>>>> and only change bundle() in commands.py. That should be much simpler.
>>>>>
>>>>>
>>>> The part in getlocalbundle could be moved, sure, for instance to
>>>> commands.
>>>> I don't see how it would be simpler in discovery; you seem to lack the
>>>> context
>>>> there. Unless you propose adding a new command for it or something?
>>>>
>>>>
>>> Yes, adding a method to prune common to the set of nodes you want to
>>> support for "portable" bundle (common intersected with ancestors of
>>> missing).
>>>
>>
>> In practice, I don't intersect but just use ancestors of missing for
>> common,
>> since this is a subset of common anyway, but yes. Sure, it can be done
>> in discovery.. I guess I don't see the advantage over doing it in commands,
>> maybe. Maybe to prevent internal access to the outgoing class.
>
>
>Yes, it is much cleaner in discovery, since the dag related operation are
>going to be more encapsulated there.

The thing is, the new function would change outgoing.common to really just
contain ancestors, so I don't think it's that clean to have it in disco,
but I don't have too strong feelings. What would you call such a function?

>>  The part where it skips the fast path in _changegroupsubset, I don't see
>>>> how
>>>> to move very easily.
>>>>
>>>
>>>
>>> I don't think that part is needed, if it is you're papering over a bug
>>> which can be triggered outside of bundle.
>>>
>>
>> It's definitely needed. Note that in my reproduction scenario I just do
>> "hg bundle test.hg foo2", which triggers the fast path (since there are
>> no heads given). Since the fast path decision is only based on heads,
>> it can also be triggered in the --base -r scenario.
>>
>
>I just had a look, it's a bug in discovery I guess. missingheads should be
>a subset of missing.

Yeah, I also noticed that earlier; wasn't sure if it was a bug or not. Since
this is also used by push, and since getbundle uses this to decide whether
or not to use the fast path, wouldn't it prevent it from using the fast path
in some (or all) situations? I should test that.

-s