This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7730

rebase: make sure pruning does not confuse rebase (issue6180)
Needs RevisionPublic

Authored by khanchi97 on Dec 27 2019, 12:05 PM.

Details

Reviewers
martinvonz
marmoute
Group Reviewers
hg-reviewers
Summary

Before this patch, if a user is rebasing a stack of commits and
hit a conflict in between and decided to drop that commit (the commit
which is being rebased but hit conflict) and pruned it, now what
hg rebase --continue does is: skip that dropped commit and move
on to rebase the next commit and gets confused here because wdir
has two parents which is because while we skipped that dropped
commit wdir had two parents and we didn't update that to one parent.

Changes in test file demonstrate the fixed behavior.

Diff Detail

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

Event Timeline

khanchi97 created this revision.Dec 27 2019, 12:05 PM
martinvonz added inline comments.Jan 8 2020, 12:37 PM
hgext/rebase.py
597

I think it's incorrect that rebase sets two parents while the merge is being resolved, but that's out of scope for this patch.

598

Should this be self.wctx.parents() to work with in-memory rebase?

pulkit added a subscriber: pulkit.Jan 9 2020, 9:21 AM

Unrelated to the fix, we need better way to skip commits during rebasing. Pruning manually is not a good option, IIRC git rebase have a --skip flag.

khanchi97 updated this revision to Diff 19283.Jan 15 2020, 5:42 AM
khanchi97 added a comment.Jan 15 2020, 8:03 AM
In D7730#114953, @pulkit wrote:

Unrelated to the fix, we need better way to skip commits during rebasing. Pruning manually is not a good option, IIRC git rebase have a --skip flag.

Yeah, that's a good idea. We should also have --skip flag to skip the commit on which rebase got interrupted.

khanchi97 added inline comments.Jan 15 2020, 8:05 AM
hgext/rebase.py
597

I will look into it.

martinvonz added inline comments.Jan 15 2020, 11:35 AM
hgext/rebase.py
597

No need, I've already sent D7827

martinvonz added inline comments.Jan 15 2020, 11:38 AM
hgext/rebase.py
600

Actually, doesn't this need to be wctx.setparents() (which you can do now that D7822 has been queued) in order to work with in-memory rebase? Maybe time to add a test case with in-memory rebase?

marmoute added a subscriber: marmoute.Jan 23 2020, 11:35 AM

I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.

khanchi97 updated this revision to Diff 19816.Feb 1 2020, 1:40 PM
pulkit added a comment.Feb 4 2020, 6:26 AM
In D7730#117268, @marmoute wrote:

I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.

I agree. We should disallow prune if an unfinished operation exists.

marmoute requested changes to this revision.Feb 14 2020, 3:23 AM
In D7730#119191, @pulkit wrote:
In D7730#117268, @marmoute wrote:

I think is would be simpler and sfare to prevent unrelated operation during rebase. If the user cannot prune here we won't have to deal with it. This woudl also apply to other operation that can alter the repository, like another rebase, amend or a pull. Starting using a unified and safe approach seems to provide more benefit with less chance of UI inconsistency.

I agree. We should disallow prune if an unfinished operation exists.

Okay, lets to in this direction then.

This revision now requires changes to proceed.Feb 14 2020, 3:23 AM

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (4 lines)
Mtests/test-rebase-conflicts.t (7 lines)

Diff 19816

hgext/rebase.py

# Make clearrebased aware state[rev] is not a true successor # Make clearrebased aware state[rev] is not a true successor
self.skipped.add(rev) self.skipped.add(rev)
# Record rev as moved to its desired destination in self.state. # Record rev as moved to its desired destination in self.state.
# This helps bookmark and working parent movement. # This helps bookmark and working parent movement.
dest = max( dest = max(
adjustdest(repo, rev, self.destmap, self.state, self.skipped) adjustdest(repo, rev, self.destmap, self.state, self.skipped)
) )
self.state[rev] = dest self.state[rev] = dest
# since we are done, make sure wdir has one parent (issue6180)
martinvonzUnsubmitted
Not Done

I think it's incorrect that rebase sets two parents while the merge is being resolved, but that's out of scope for this patch.

martinvonz: I think it's incorrect that rebase sets two parents while the merge is being resolved, but…
khanchi97AuthorUnsubmitted
Done

I will look into it.

khanchi97: I will look into it.
martinvonzUnsubmitted
Not Done

No need, I've already sent D7827

martinvonz: No need, I've already sent D7827
if len(self.wctx.parents()) == 2:
martinvonzUnsubmitted
Not Done

Should this be self.wctx.parents() to work with in-memory rebase?

martinvonz: Should this be `self.wctx.parents()` to work with in-memory rebase?
p1 = self.wctx.p1().node()
self.wctx.setparents(p1)
martinvonzUnsubmitted
Not Done

Actually, doesn't this need to be wctx.setparents() (which you can do now that D7822 has been queued) in order to work with in-memory rebase? Maybe time to add a test case with in-memory rebase?

martinvonz: Actually, doesn't this need to be `wctx.setparents()` (which you can do now that D7822 has been…
elif self.state[rev] == revtodo: elif self.state[rev] == revtodo:
ui.status(_(b'rebasing %s\n') % desc) ui.status(_(b'rebasing %s\n') % desc)
progressfn(ctx) progressfn(ctx)
p1, p2, base = defineparents( p1, p2, base = defineparents(
repo, repo,
rev, rev,
self.destmap, self.destmap,
self.state, self.state,

tests/test-rebase-conflicts.t

$ echo 1 > a $ echo 1 > a
$ hg debugobsolete $(hg id -ir 2 --debug) $ hg debugobsolete $(hg id -ir 2 --debug)
1 new obsolescence markers 1 new obsolescence markers
obsoleted 1 changesets obsoleted 1 changesets
1 new orphan changesets 1 new orphan changesets
$ hg resolve -m $ hg resolve -m
(no more unresolved files) (no more unresolved files)
continue: hg rebase --continue continue: hg rebase --continue
XXX: it should have rebased revision 3 since it made changes unrelated to
destination, so no reason to say "its destination already has all its changes"
$ hg rebase -c $ hg rebase -c
note: not rebasing 2:06a50ac6b5ab "conflict in a", it has no successor note: not rebasing 2:06a50ac6b5ab "conflict in a", it has no successor
rebasing 3:aea370672fd7 "add b" (tip) rebasing 3:aea370672fd7 "add b" (tip)
note: not rebasing 3:aea370672fd7 "add b" (tip), its destination already has all its changes
$ hg tglog $ hg tglog
@ 1:draft 'edit a' @ 4:draft 'add b'
|
o 1:draft 'edit a'
| |
o 0:draft 'add a' o 0:draft 'add a'