Page MenuHomePhabricator

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

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?

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.Sat, Feb 1, 1:40 PM
pulkit added a comment.Tue, Feb 4, 6:26 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.

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

marmoute requested changes to this revision.Fri, Feb 14, 3:23 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.

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.Fri, Feb 14, 3:23 AM