| 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
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 ?
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.
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
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.
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)
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
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.
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'