Patchwork D7177: [PoC] allow providing explicit mapping for parents of merge commits

login
register
mail settings
Submitter phabricator
Date Oct. 28, 2019, 4:17 p.m.
Message ID <differential-rev-PHID-DREV-mmfkz365brbciro7hgve-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42608/
State New
Headers show

Comments

phabricator - Oct. 28, 2019, 4:17 p.m.
joerg.sonnenberger created this revision.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Consider the following DAG:
  
  C   C'
  
  | \ / |
  |
  
  A B D
  
  with the goal of rebasing C to C' while switching the A parent to D.
  This can happen when dealing with manual rebases of merges without
  obsolescence markers.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: joerg.sonnenberger, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 30, 2019, 5:38 a.m.
martinvonz added a comment.


  I changed the commit message to indent the graph by two spaces. That way it renders better here in Phabricator.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 16, 2020, 10:08 p.m.
martinvonz added a comment.


  I personally haven't queued this yet because I don't really like the UI. Some things I recently worked on made me realize that `hg graft` doesn't currently preserve merges, but we could probably easily add a `--preserve-merges` option to it, so `hg co D; hg graft --base B --preserve-merges` would take care of the case in the commit message.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 23, 2020, 4:39 p.m.
marmoute added a comment.


  I like the idea of using adding this to graft.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - April 22, 2020, 3:52 p.m.
Herald added a subscriber: mercurial-patches.
This revision now requires changes to proceed.
baymax added a comment.
baymax requested changes to this revision.


  There seems to have been no activities on this Diff for the past 3 Months.
  
  By policy, we are automatically moving it out of the `need-review` state.
  
  Please, move it back to `need-review` without hesitation if this diff should still be discussed.
  
  :baymax:need-review-idle:

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - May 30, 2020, 6:08 p.m.
martinvonz added a comment.


  Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - June 9, 2020, 11:44 a.m.
marmoute added a comment.


  In D7177#128389 <https://phab.mercurial-scm.org/D7177#128389>, @martinvonz wrote:
  
  > Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).
  
  That's interresting. How would that works with a practical case ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - June 9, 2020, 2:26 p.m.
martinvonz added a comment.


  In D7177#128740 <https://phab.mercurial-scm.org/D7177#128740>, @marmoute wrote:
  
  > In D7177#128389 <https://phab.mercurial-scm.org/D7177#128389>, @martinvonz wrote:
  >
  >> Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).
  >
  > That's interresting. How would that works with a practical case ?
  
  The example I have was meant to be a practical example for how to do it in the case given in the patch description :) But let me know if you meant some other aspect of "how it works". As I said, I'm not sure ant the BC bit of it at least.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - June 9, 2020, 3:47 p.m.
marmoute added a comment.


  In D7177#128756 <https://phab.mercurial-scm.org/D7177#128756>, @martinvonz wrote:
  
  > In D7177#128740 <https://phab.mercurial-scm.org/D7177#128740>, @marmoute wrote:
  >
  >> In D7177#128389 <https://phab.mercurial-scm.org/D7177#128389>, @martinvonz wrote:
  >>
  >>> Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).
  >>
  >> That's interresting. How would that works with a practical case ?
  >
  > The example I have was meant to be a practical example for how to do it in the case given in the patch description :) But let me know if you meant some other aspect of "how it works". As I said, I'm not sure ant the BC bit of it at least.
  
  I meant how that would work in general, esepcially case more complicated than the basic base shown by @joerg.sonnenberger.
  
  Not that in @joerg base, B being ancestors of both C and C', we could probably behave better by default directly. Could we not ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - June 9, 2020, 4:15 p.m.
martinvonz added a comment.


  In D7177#128757 <https://phab.mercurial-scm.org/D7177#128757>, @marmoute wrote:
  
  > In D7177#128756 <https://phab.mercurial-scm.org/D7177#128756>, @martinvonz wrote:
  >
  >> In D7177#128740 <https://phab.mercurial-scm.org/D7177#128740>, @marmoute wrote:
  >>
  >>> In D7177#128389 <https://phab.mercurial-scm.org/D7177#128389>, @martinvonz wrote:
  >>>
  >>>> Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).
  >>>
  >>> That's interresting. How would that works with a practical case ?
  >>
  >> The example I have was meant to be a practical example for how to do it in the case given in the patch description :) But let me know if you meant some other aspect of "how it works". As I said, I'm not sure ant the BC bit of it at least.
  >
  > I meant how that would work in general, esepcially case more complicated than the basic base shown by @joerg.sonnenberger.
  
  I think the parents chosen by `-d` should only apply to the roots of the rebase set. If your rebase set has multiple roots and only some of them are merge commits, then we should probably error out.
  
  Anyway, I'm not sure it's realistic to make it work that way now, especially because of how inconsistent it would be with other arguments that accept revsets and only care about the highest revnum in the set (I think that was a mistake to do, but I understand why it was done that way and it's obviously too late to change it).
  
  > Not that in @joerg base, B being ancestors of both C and C', we could probably behave better by default directly. Could we not ?
  
  As @joerg said in the patch description, there's no reasonable way to choose which of A and B to preserve (the current version of rebase wouldn't preserve either and would instead rebase all of A, B, and C).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - June 10, 2020, 9:15 a.m.
marmoute added a comment.


  In D7177#128760 <https://phab.mercurial-scm.org/D7177#128760>, @martinvonz wrote:
  
  > In D7177#128757 <https://phab.mercurial-scm.org/D7177#128757>, @marmoute wrote:
  >
  >> In D7177#128756 <https://phab.mercurial-scm.org/D7177#128756>, @martinvonz wrote:
  >>
  >>> In D7177#128740 <https://phab.mercurial-scm.org/D7177#128740>, @marmoute wrote:
  >>>
  >>>> In D7177#128389 <https://phab.mercurial-scm.org/D7177#128389>, @martinvonz wrote:
  >>>>
  >>>>> Maybe another option is to allow multiple `-d` arguments for this case? Something like `hg rebase -r C -d B -d D`. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support `hg rebase -r C -d 'B + D'` for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).
  >>>>
  >>>> That's interresting. How would that works with a practical case ?
  >>>
  >>> The example I have was meant to be a practical example for how to do it in the case given in the patch description :) But let me know if you meant some other aspect of "how it works". As I said, I'm not sure ant the BC bit of it at least.
  >>
  >> I meant how that would work in general, esepcially case more complicated than the basic base shown by @joerg.sonnenberger.
  >
  > I think the parents chosen by `-d` should only apply to the roots of the rebase set. If your rebase set has multiple roots and only some of them are merge commits, then we should probably error out.
  
  So you suggestion would be to allow to specify two -d that would definive the two new parents of the root ? I suspect it would be a bit too narrow compared to the varienty of case we might have to handle. Maybe this is a mission for PlanPlan.
  
  > Anyway, I'm not sure it's realistic to make it work that way now, especially because of how inconsistent it would be with other arguments that accept revsets and only care about the highest revnum in the set (I think that was a mistake to do, but I understand why it was done that way and it's obviously too late to change it).
  
  I think the multiple -d value approach would works fine. (but for the other issue).
  
  >> Not that in @joerg base, B being ancestors of both C and C', we could probably behave better by default directly. Could we not ?
  >
  > As @joerg said in the patch description, there's no reasonable way to choose which of A and B to preserve (the current version of rebase wouldn't preserve either and would instead rebase all of A, B, and C).
  
  The description got me confused, (I though C was rebased on C' (finding the naming strange)) while this is about C being rebased on D.

INLINE COMMENTS

> rebase.py:884
> +            _(b'map old parent to new parent (ADVANCED)'),
> +            _(b'REV:REV')),
>          (b'k', b'keep', False, _(b'keep original changesets')),

`REV:REV` is valid revset so the result is quite confusing, if we go that route, we need anothr syntax.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - Sept. 17, 2021, 1:59 p.m.
joerg.sonnenberger added a comment.


  From the VC call today: The use case can be addressed by marking parents as to-be-preserved, so that for merge commits rebase can decide on the parent link is should operate on. It covers a general DAG and is somewhat easier to use in terms of UX than the parentmap, but can provide the same functionality with a multi-step rebase.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel
phabricator - Sept. 17, 2021, 2:02 p.m.
marmoute added a comment.


  In D7177#175507 <https://phab.mercurial-scm.org/D7177#175507>, @joerg.sonnenberger wrote:
  
  > From the VC call today: The use case can be addressed by marking parents as to-be-preserved, so that for merge commits rebase can decide on the parent link is should operate on. It covers a general DAG and is somewhat easier to use in terms of UX than the parentmap, but can provide the same functionality with a multi-step rebase.
  
  Yeah, something like `--preserve-parent XXX` where XXX is a set a revs to "preserve". When rebasing a revision, if it has parents in XXX we keep them (and rebase the other). if no parent are in XXX, we don't care.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7177/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7177

To: joerg.sonnenberger, martinvonz, #hg-reviewers, baymax
Cc: mercurial-patches, marmoute, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -189,6 +189,13 @@ 
         self.activebookmark = None
         self.destmap = {}
         self.skipped = set()
+        self.parentmap = {}
+        for pm in opts.get(b'parentmap', []):
+            if pm.count(':') != 1:
+                raise error.Abort(_(b"--parentmap takes a pair of revisions"
+                                    b"separated by colon"))
+            oldp, newp = pm.split(":")
+            self.parentmap[int(oldp)] = int(newp)
 
         self.collapsef = opts.get(b'collapse', False)
         self.collapsemsg = cmdutil.logmessage(ui, opts)
@@ -614,6 +621,7 @@ 
                 self.state,
                 self.skipped,
                 self.obsoletenotrebased,
+                self.parentmap,
             )
             if not self.inmemory and len(repo[None].parents()) == 2:
                 repo.ui.debug(b'resuming interrupted rebase\n')
@@ -870,6 +878,7 @@ 
             _(b'read collapse commit message from file'),
             _(b'FILE'),
         ),
+        (b'p', b'parentmap', [], _(b'map old parent to new parent'), _(b'REV:REV')),
         (b'k', b'keep', False, _(b'keep original changesets')),
         (b'', b'keepbranches', False, _(b'keep original branch names')),
         (b'D', b'detach', False, _(b'(DEPRECATED)')),
@@ -1649,7 +1658,7 @@ 
             yield nodemap[s]
 
 
-def defineparents(repo, rev, destmap, state, skipped, obsskipped):
+def defineparents(repo, rev, destmap, state, skipped, obsskipped, parentmap):
     """Return new parents and optionally a merge base for rev being rebased
 
     The destination specified by "dest" cannot always be used directly because
@@ -1707,6 +1716,8 @@ 
                 np = dests[i]
             elif p in state and state[p] > 0:
                 np = state[p]
+            elif p in parentmap:
+                np = parentmap[p]
 
             # "bases" only record "special" merge bases that cannot be
             # calculated from changelog DAG (i.e. isancestor(p, np) is False).