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.
Details
- Reviewers
- None
- Group Reviewers
hg-reviewers - Commits
- rHG3160876c6e4e: rebase: choose merge base without unwanted revisions
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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. |
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 |
Yes. In that case maybe either way is suboptimal. The troublesome cases are:
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.
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. |
Sounds good to me. Thanks!
The test cases still seem useful, so maybe you can keep that part of the patch?
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).