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

login
register
mail settings
Submitter Mads Kiilerich
Date Dec. 4, 2014, 3:12 a.m.
Message ID <a20a470ffce95d5266b8.1417662765@localhost.localdomain>
Download mbox | patch
Permalink /patch/6988/
State Accepted
Commit cf3495dfd7ed5d1a6f94ff8589d95bb2efd64bb5
Headers show

Comments

Mads Kiilerich - Dec. 4, 2014, 3:12 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# 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
> # User Mads Kiilerich <madski@unity3d.com>
> # 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
>> # User Mads Kiilerich <madski@unity3d.com>
>> # 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.

/Mads
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
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # 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
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # 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
>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>> # 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.

/Mads
Matt Mackall - Dec. 5, 2014, 8:13 p.m.
On Thu, 2014-12-04 at 04:12 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # 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
>>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>>> # 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 
readable. Comments are maybe not 100% accurate but they provide valuable 
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.ui.debug(" already in target\n")
     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
-        else: # fallback when base not found
-            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
+        else: # fallback when base not found
+            base = None
+
+            # Raise because this function is called wrong (see issue 4106)
+            raise AssertionError('no base found to rebase on '
+                                 '(defineparents called wrong)')
+    return p1, p2, base
 
 def isagitpatch(repo, patchname):
     'Return true if the given patch is in git format'