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