[PATCH 4 of 5 v3] rebase: refactor computerebase to case-by-case handling

[Pierre-Yves David]

On 12/04/2014 05:20 PM, Mads Kiilerich wrote:
> On 12/05/2014 01:26 AM, Pierre-Yves David wrote:
>>
>>
>> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski at unity3d.com>
>>> # Date 1416364731 -3600
>>> #      Wed Nov 19 03:38:51 2014 +0100
>>> # Node ID 742988c7a05cb1bd49f9be0f057a9c8ef4e20d95
>>> # Parent  e031c5201577b359af981396957a0b9a9a5f5300
>>> rebase: refactor computerebase to case-by-case handling
>>>
>>> This makes it simpler to review and tweak handling of different cases.
>>
>> What was wrong with the previous implementation, what makes yours better?
>
> You saw that in v1 of the patch series ... and complained about it.

Information relevant to a changesets should be in the changesets 
description…

>>> This implementation was created using the old implementation as black
>>> box. It
>>> started out with how I think it should be, and was tweaked until it
>>> matched how
>>> it actually is. Further tweaking can take us close to where we want
>>> to be, step
>>> by step.
>>
>> Now that we have the full list of case, are we sure they are all
>> properly tested? I would makes me more comfortable in such refactoring.
>
> The cases that not are tested have a "untested" warning and a comment
> explaining why.

Ok, probably worth mentioning that your check that all testable case 
were tested.

The untestable (because mercurial does not not use them) should probably 
just fails for now. This would simplify your table a little.

>>> The only case where the test suite coverage gives different result from
>>> computerebase is in test-rebase-obsolete.t in a case where None as
>>> ancestor
>>> would give the same ancestor as the one previously explicitly specified.
>>
>> Why does it change? What does it mean? Is that bad? Why don't we get
>> it right in the first place?
>
> The old algorithm was IMO a mess that tried to cover all the very
> different scenarios with the same algorithm. I gave up untangling it.
> Apparently, sometimes it would use None as ancestor (and thus
> automatically finding one in the merge algorithm) when rebasing an
> ancestor child, and sometimes it would explicitly use the parent as
> ancestor. In the end it would use the same ancestor and give the same
> result. In the next change we make it use the explicit ancestor in all
> cases.

Please update your description with these details.

I'm still wondering why you are not making it right from the start.


>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -25,6 +25,7 @@ import os, errno
>>>
>>>   nullmerge = -2
>>>   revignored = -3
>>> +outside = -4 # in computerebase: not in state, thus outside rebase set
>>>
>>>   cmdtable = {}
>>>   command = cmdutil.command(cmdtable)
>>> @@ -586,88 +587,93 @@ def computerebase(repo, rev, target, sta
>>>       '''Return the merge revisions and new parent relationship of
>>> rev rebased
>>>       to target:
>>>           merge parent 1, merge parent 2, ancestor, new parent 1, new
>>> parent 2
>>> +
>>> +    The following table shows how it is - not necessarily how it
>>> should be.
>>> +
>>> +    Rebasing of rev with parents p1 and p2 onto target as rev' is
>>> written:
>>> +        m1,m2|a
>>> +        p1',p2'
>>> +
>>> +        \  p2     null         ancestor        rebased outside
>>> +      p1 \
>>> +    null       target,rev|  target,rev|None  p2',rev|p2 target,rev|p2
>>> +                 target         target           p2' target
>>> +
>>> +    ancestor target,rev|Non target,rev|None  p2',rev|p2 target,rev|None
>>> +                 target         target           p2' target,p2
>>> +
>>> +    rebased    p1',rev|p1     p1',rev|p1    target,rev|p1 p1',rev|p1
>>> +                   p1'            p1'          p1',p2 p1',p2
>>> +
>>> +    outside   target,rev|p1 target,rev|None  p2',rev|p2 abort
>>> +                 target        target,p1       p2',p1       3 parents
>>
>> This table is fairly hard to read can we more to a version with:
>
> It contains a lot of information.
>
>> 1) clearer boundary
>> 2) fixed size, aligned value. There is various option for this I think
>> my currently preferred is using T (target), R (rev), 1 (p1), 2 (p2)
>> and - (no value, applies for None)
>
> I don't understand what you mean.

You table is too hard to read. It needs vertically align content easily 
comparable.

eg: (half convincing one)

   ancestors | Ta Re -- Ta -- | Ta Re -- Ta -- |
             |                |                |
   rebased   | 1' Re p1 1' -- | 1' Re p1 1' -- |

-- 
Pierre-Yves David