Page MenuHomePhabricator

rebase: introduce optional parent mapping
Needs RevisionPublic

Authored by joerg.sonnenberger on Oct 28 2019, 12:17 PM.

Details

Reviewers
martinvonz
baymax
Group Reviewers
hg-reviewers
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.
Out of the box, rebase will fail here as it doesn't know which parent of
C to rewrite. With the new --parentmap option, this can be specified
explicitly. Use cases for this functionality are dealing with manual
rebases without obsolescence markers, e.g. git-style forced commits.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

joerg.sonnenberger retitled this revision from [PoC] allow providing explicit mapping for parents of merge commits to rebase: introduce optional parent mapping.Oct 28 2019, 4:02 PM
joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger updated this revision to Diff 17411.
martinvonz edited the summary of this revision. (Show Details)Oct 30 2019, 1:37 AM

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

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.

I like the idea of using adding this to graft.

baymax requested changes to this revision.Apr 22 2020, 11:52 AM

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:

This revision now requires changes to proceed.Apr 22 2020, 11:52 AM

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

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 ?

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.

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 ?

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

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.

hgext/rebase.py
884

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