# Patchwork [1,of,5,v3] rebase: move base calculation from rebasenode() to defineparents()

Submitter Mads Kiilerich Dec. 4, 2014, 3:12 a.m. mbox | patch /patch/6988/ Accepted cf3495dfd7ed5d1a6f94ff8589d95bb2efd64bb5 show

Mads Kiilerich - Dec. 4, 2014, 3:12 a.m.
```# HG changeset patch
# Date 1416364731 -3600
#      Wed Nov 19 03:38:51 2014 +0100
# Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
# Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
rebase: move base calculation from rebasenode() to defineparents()

We want to collect all calculation in one place.
```
Pierre-Yves David - Dec. 5, 2014, 12:09 a.m.
```On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # Date 1416364731 -3600
> #      Wed Nov 19 03:38:51 2014 +0100
> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
> rebase: move base calculation from rebasenode() to defineparents()
>
> We want to collect all calculation in one place.

We want to? Why? Can you be a bit more verbose about the goal you are
aiming to ?
```
Pierre-Yves David - Dec. 5, 2014, 12:27 a.m.
```On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
> @@ -509,7 +509,8 @@ def externalparent(repo, state, targetan
>                         ', '.join(str(p) for p in sorted(parents))))
>
>   def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None):
> -    '''Commit the changes and store useful information in extra.
> +    '''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
> +    but also store useful information in extra.
>       Return node of committed revision.'''
>       try:
>           repo.dirstate.beginparentchange()

also: looks like unrelated changes sneaked in.
```
Mads Kiilerich - Dec. 5, 2014, 12:51 a.m.
```On 12/05/2014 01:09 AM, Pierre-Yves David wrote:
> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # Date 1416364731 -3600
>> #      Wed Nov 19 03:38:51 2014 +0100
>> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
>> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
>> rebase: move base calculation from rebasenode() to defineparents()
>>
>> We want to collect all calculation in one place.
>
> We want to? Why? Can you be a bit more verbose about the goal you are
> aiming to ?

It is just a refactoring preparing for the next patches that change the
whole calculation.

```
Pierre-Yves David - Dec. 5, 2014, 12:53 a.m.
```On 12/04/2014 04:51 PM, Mads Kiilerich wrote:
> On 12/05/2014 01:09 AM, Pierre-Yves David wrote:
>> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # Date 1416364731 -3600
>>> #      Wed Nov 19 03:38:51 2014 +0100
>>> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
>>> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
>>> rebase: move base calculation from rebasenode() to defineparents()
>>>
>>> We want to collect all calculation in one place.
>>
>> We want to? Why? Can you be a bit more verbose about the goal you are
>> aiming to ?
>
> It is just a refactoring preparing for the next patches that change the
> whole calculation.

next patches that are not so much explanatory them self. I assume you
are not doing all that churn for fun.
```
Pierre-Yves David - Dec. 5, 2014, 1:42 a.m.
```On 12/04/2014 04:51 PM, Mads Kiilerich wrote:
> On 12/05/2014 01:09 AM, Pierre-Yves David wrote:
>> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # Date 1416364731 -3600
>>> #      Wed Nov 19 03:38:51 2014 +0100
>>> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
>>> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
>>> rebase: move base calculation from rebasenode() to defineparents()
>>>
>>> We want to collect all calculation in one place.
>>
>> We want to? Why? Can you be a bit more verbose about the goal you are
>> aiming to ?
>
> It is just a refactoring preparing for the next patches that change the
> whole calculation.

Hints:
- You are not charged at description length. You can add more details there.
- I still do not get what is you actual plan beside rewriting this whole
logic.

(I assume your end call it is related to issue4076 but I've no idea what
you plan to do here. I would naively expect some bid merge related
solution from you)
```
Mads Kiilerich - Dec. 5, 2014, 3:52 p.m.
```On 12/05/2014 02:42 AM, Pierre-Yves David wrote:
>
>
> On 12/04/2014 04:51 PM, Mads Kiilerich wrote:
>> On 12/05/2014 01:09 AM, Pierre-Yves David wrote:
>>> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # Date 1416364731 -3600
>>>> #      Wed Nov 19 03:38:51 2014 +0100
>>>> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
>>>> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
>>>> rebase: move base calculation from rebasenode() to defineparents()
>>>>
>>>> We want to collect all calculation in one place.
>>>
>>> We want to? Why? Can you be a bit more verbose about the goal you are
>>> aiming to ?
>>
>> It is just a refactoring preparing for the next patches that change the
>> whole calculation.
>
> Hints:
> - You are not charged at description length. You can add more details
> there.
> - I still do not get what is you actual plan beside rewriting this
> whole logic.

The rewrite simplifies the code - I think that has value in itself.

It also makes it easy to handle special cases separately and in special
ways - for example
http://selenic.com/pipermail/mercurial-devel/2014-November/064134.html

> (I assume your end call it is related to issue4076 but I've no idea
> what you plan to do here. I would naively expect some bid merge
> related solution from you)

http://selenic.com/pipermail/mercurial-devel/2014-November/064134.html
was one attempt at solving my problem ... which probably is the same as
4076. That attempt was however wrong. We can discuss it in details once
we have something we can point at and discuss separately.

```
Matt Mackall - Dec. 5, 2014, 8:13 p.m.
```On Thu, 2014-12-04 at 04:12 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # Date 1416364731 -3600
> #      Wed Nov 19 03:38:51 2014 +0100
> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
> rebase: move base calculation from rebasenode() to defineparents()
>
> We want to collect all calculation in one place.

I've queued this one as it looks straightforward enough and lets us make
forward progress. Check-code sends its regards.
```
Pierre-Yves David - Dec. 5, 2014, 9:15 p.m.
```On 12/05/2014 07:52 AM, Mads Kiilerich wrote:
> On 12/05/2014 02:42 AM, Pierre-Yves David wrote:
>> On 12/04/2014 04:51 PM, Mads Kiilerich wrote:
>>> On 12/05/2014 01:09 AM, Pierre-Yves David wrote:
>>>> On 12/03/2014 07:12 PM, Mads Kiilerich wrote:
>>>>> # HG changeset patch
>>>>> # Date 1416364731 -3600
>>>>> #      Wed Nov 19 03:38:51 2014 +0100
>>>>> # Node ID a20a470ffce95d5266b8e332c0e01c72604fc528
>>>>> # Parent  c237499a7fba65c88a2da721a22b66df4f39cf4e
>>>>> rebase: move base calculation from rebasenode() to defineparents()
>>>>>
>>>>> We want to collect all calculation in one place.
>>>>
>>>> We want to? Why? Can you be a bit more verbose about the goal you are
>>>> aiming to ?
>>>
>>> It is just a refactoring preparing for the next patches that change the
>>> whole calculation.
>>
>> Hints:
>> - You are not charged at description length. You can add more details
>> there.
>> - I still do not get what is you actual plan beside rewriting this
>> whole logic.
>
> The rewrite simplifies the code - I think that has value in itself.

The value of the rewrite is not obvious.

- You gather logic without justification of why they are related or what
benefit we get to group them.
- You drop documentation about what the values means and why we pick it.

The previous code is not the more readable one but is it is still
information. It may misbehave in some corner case but is served use well.

You new code (patch 4):

- Does not includes document what are the values semantic and what we
try to achieves by picking them. It just drop a godly table and its dry
implementation.
- Does not document any of the special case

Sure, having a nice table and changing its code look good idea. But I've
really no element from you to judge it this is the right change.
From my end, this just introduce a lot of churn without any articulated
explanation. The hand wavy "trust me, I'm an programmer" is not going to
work here. You are touching the critical part of the rebase logic and I
need to trust your change to accept it.

> It also makes it easy to handle special cases separately and in special
> ways - for example
> http://selenic.com/pipermail/mercurial-devel/2014-November/064134.html

Saying: There is "problem", so I'm "changing things" is not enough. I
need you to show that you understand the issue properly and are able to
to understand what the solution should be. Producing this table is a
good step as it show you looked at the current behavior. But it is not
enough.

Especially since your previous attempt as fixing that was deficient. I
requested such explanation the first week of November, I'm still waiting
for it and I'll still wait for it. The second attempt (linked patches)
is not more convincing because you do not explain why it is a better
solution (neither you explain a solution to what).

> http://selenic.com/pipermail/mercurial-devel/2014-November/064134.html
> was one attempt at solving my problem ... which probably is the same as
> 4076. That attempt was however wrong. We can discuss it in details once
> we have something we can point at and discuss separately.

Your use of "Which probably is the same" is a good example of why I'm
not trusting your solution for now and expect you write something about
the situation, the reason it is bad and how we could makes it better.
This would be a something concrete we could point at and discuss
(something we are both dying for apparently).

If you already have a good mental image of what is going on there, you
just have to spend ½ of the time you already spent arguing about review
to write that down. Otherwise, get one and write it down.
```

## Patch

```diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -370,8 +370,8 @@  def rebase(ui, repo, **opts):
if state[rev] == -1:
ui.progress(_("rebasing"), pos, ("%d:%s" % (rev, repo[rev])),
_('changesets'), total)
-                p1, p2 = defineparents(repo, rev, target, state,
-                                                        targetancestors)
+                p1, p2, base = defineparents(repo, rev, target, state,
+                                             targetancestors)
storestatus(repo, originalwd, target, state, collapsef, keepf,
keepbranchesf, external, activebookmark)
if len(repo.parents()) == 2:
@@ -380,7 +380,7 @@  def rebase(ui, repo, **opts):
try:
ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
'rebase')
-                        stats = rebasenode(repo, rev, p1, state, collapsef,
+                        stats = rebasenode(repo, rev, p1, base, state, collapsef,
target)
if stats and stats[3] > 0:
raise error.InterventionRequired(
@@ -414,8 +414,8 @@  def rebase(ui, repo, **opts):
ui.note(_('rebase merging completed\n'))

if collapsef and not keepopen:
-            p1, p2 = defineparents(repo, min(state), target,
-                                                        state, targetancestors)
+            p1, p2, _base = defineparents(repo, min(state), target,
+                                          state, targetancestors)
editopt = opts.get('edit')
editform = 'rebase.collapse'
if collapsemsg:
@@ -509,7 +509,8 @@  def externalparent(repo, state, targetan
', '.join(str(p) for p in sorted(parents))))

def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None):
-    '''Commit the changes and store useful information in extra.
+    '''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
+    but also store useful information in extra.
Return node of committed revision.'''
try:
repo.dirstate.beginparentchange()
@@ -539,8 +540,8 @@  def concludenode(repo, rev, p1, p2, comm
repo.dirstate.invalidate()
raise

-def rebasenode(repo, rev, p1, state, collapse, target):
-    'Rebase a single revision'
+def rebasenode(repo, rev, p1, base, state, collapse, target):
+    'Rebase a single revision rev on top of p1 using base as merge ancestor'
# Merge phase
# Update to target and merge it with local
if repo['.'].rev() != p1:
@@ -550,48 +551,6 @@  def rebasenode(repo, rev, p1, state, col
repo.dirstate.write()
repo.ui.debug(" merge against %d:%s\n" % (rev, repo[rev]))
-    if rev == min(state):
-        # Case (1) initial changeset of a non-detaching rebase.
-        # Let the merge mechanism find the base itself.
-        base = None
-    elif not repo[rev].p2():
-        # Case (2) detaching the node with a single parent, use this parent
-        base = repo[rev].p1().rev()
-    else:
-        # In case of merge, we need to pick the right parent as merge base.
-        #
-        # Imagine we have:
-        # - M: currently rebase revision in this step
-        # - A: one parent of M
-        # - B: second parent of M
-        # - D: destination of this merge step (p1 var)
-        #
-        # If we are rebasing on D, D is the successors of A or B. The right
-        # merge base is the one D succeed to. We pretend it is B for the rest
-        # of this comment
-        #
-        # If we pick B as the base, the merge involves:
-        # - changes from B to M (actual changeset payload)
-        # - changes from B to D (induced by rebase) as D is a rebased
-        #   version of B)
-        # Which exactly represent the rebase operation.
-        #
-        # If we pick the A as the base, the merge involves
-        # - changes from A to M (actual changeset payload)
-        # - changes from A to D (with include changes between unrelated A and B
-        #   plus changes induced by rebase)
-        # Which does not represent anything sensible and creates a lot of
-        # conflicts.
-        for p in repo[rev].parents():
-            if state.get(p.rev()) == p1:
-                base = p.rev()
-                break
-            base = None
-
-            # Raise because this function is called wrong (see issue 4106)
-            raise AssertionError('no base found to rebase on '
-                                 '(rebasenode called wrong)')
if base is not None:
repo.ui.debug("   detach base %d:%s\n" % (base, repo[base]))
# When collapsing in-place, the parent is the common ancestor, we
@@ -660,7 +619,50 @@  def defineparents(repo, rev, target, sta
p2 = p2n
repo.ui.debug(" future parents are %d and %d\n" %
(repo[p1].rev(), repo[p2].rev()))
-    return p1, p2
+
+    if rev == min(state):
+        # Case (1) initial changeset of a non-detaching rebase.
+        # Let the merge mechanism find the base itself.
+        base = None
+    elif not repo[rev].p2():
+        # Case (2) detaching the node with a single parent, use this parent
+        base = repo[rev].p1().rev()
+    else:
+        # In case of merge, we need to pick the right parent as merge base.
+        #
+        # Imagine we have:
+        # - M: currently rebase revision in this step
+        # - A: one parent of M
+        # - B: second parent of M
+        # - D: destination of this merge step (p1 var)
+        #
+        # If we are rebasing on D, D is the successors of A or B. The right
+        # merge base is the one D succeed to. We pretend it is B for the rest
+        # of this comment
+        #
+        # If we pick B as the base, the merge involves:
+        # - changes from B to M (actual changeset payload)
+        # - changes from B to D (induced by rebase) as D is a rebased
+        #   version of B)
+        # Which exactly represent the rebase operation.
+        #
+        # If we pick the A as the base, the merge involves
+        # - changes from A to M (actual changeset payload)
+        # - changes from A to D (with include changes between unrelated A and B
+        #   plus changes induced by rebase)
+        # Which does not represent anything sensible and creates a lot of
+        # conflicts.
+        for p in repo[rev].parents():
+            if state.get(p.rev()) == p1:
+                base = p.rev()
+                break