This is an archive of the discontinued Mercurial Phabricator instance.

rebase: choose merge base without unwanted revisions
ClosedPublic

Authored by quark on Aug 11 2017, 1:41 AM.

Details

Summary

Previously, when there are 2 merge base candidates, we choose p1 blindly,
which may make the merge result to have "unwanted content". This patch makes
rebase smarter - choose a merge base that does not have "unwanted revs" if
possible. Since we don't really have a good solution when there are
"unwanted revs", abort in that case.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Aug 11 2017, 1:41 AM
quark updated this revision to Diff 768.Aug 11 2017, 1:44 AM
quark updated this revision to Diff 770.Aug 11 2017, 2:39 AM
quark edited the summary of this revision. (Show Details)Aug 13 2017, 12:10 AM
quark updated this revision to Diff 830.
quark updated this revision to Diff 868.Aug 14 2017, 10:24 AM
quark updated this revision to Diff 870.Aug 14 2017, 11:05 AM
martinvonz added inline comments.
hgext/rebase.py
1112

And if there are more ancestors of B, will the "rebasing %d:%s may include unwanted changes" warning appear?

Regardless, if B adds file B and the merge in C involves removing file B, then the total diff will be empty. Does that mean that C' will be empty? It should be reverting B', right? (The same thing applies if it's not a whole file that gets added/removed, of course.) I think I'd prefer to be conservative here and error out until we have a safe answer to these merges that won't risk resulting in surprising contents. In this particular case, the user can work around it by first rebasing "B+C" only (but that may be tricky to realize).

tests/test-rebase-obsolete.t
1180

Why do we want/need rebaseskipobsolete? I was expecting to see a test case like then one in the documentation you added i rebase.py.

quark planned changes to this revision.Aug 15 2017, 7:25 PM

Since the "unwanted" warning "covers" the case this patch handles. The behavior is still "correct" (since the warning is printed) without this patch. I'm leaning towards dropping this patch.

For errors vs warnings, I still think it's better to give power users a chance to proceed. How do you think about this approach?

  • If there are multiple merge base candidates, calculate "unwanted" revsets for both, and if only one of them is empty, pick that one automatically and proceed.
  • If both merge base candidates have "unwanted" revsets, raise error.InterventionRequired so power users could still have a chance to proceed fixing them manually.
hgext/rebase.py
1112

And if there are more ancestors of B, will the "rebasing %d:%s may include unwanted changes" warning appear?

Yes. In that case maybe either way is suboptimal. The troublesome cases are:

  • A merge base candidate has a successor in destination with *different* content
  • A merge base candidate has "unwanted" ancestors that are not covered by rebaseset

I think the problem of this patch is it assumes "different" content blindly. It would be better to actually have a quick check about whether that is the case.

Thinking about it again, most cases the reason a commit is in destination is because of a rebase so its content does not change. Therefore this patch is about a very corner case that may not worth the complexity.

Regardless, if B adds file B and the merge in C involves removing file B, then the total diff will be empty.

That is a surprise to me. Apparently you know merge better than I am :)

tests/test-rebase-obsolete.t
1180

Not sure why I added that. But it does seem to be unnecessary.

In D340#6330, @quark wrote:

Since the "unwanted" warning "covers" the case this patch handles. The behavior is still "correct" (since the warning is printed) without this patch. I'm leaning towards dropping this patch.
For errors vs warnings, I still think it's better to give power users a chance to proceed. How do you think about this approach?

  • If there are multiple merge base candidates, calculate "unwanted" revsets for both, and if only one of them is empty, pick that one automatically and proceed.
  • If both merge base candidates have "unwanted" revsets, raise error.InterventionRequired so power users could still have a chance to proceed fixing them manually.

Sounds good to me. Thanks!

The test cases still seem useful, so maybe you can keep that part of the patch?

quark edited the summary of this revision. (Show Details)Aug 19 2017, 1:53 AM
quark retitled this revision from rebase: prefer choosing merge base with successor in destination to rebase: choose merge base without unwanted revisions.
quark updated this revision to Diff 1085.
This revision was automatically updated to reflect the committed changes.