D340: rebase: prefer choosing merge base with successor in destination

[quark (Jun Wu)]

quark planned changes to this revision.
quark added a comment.


  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.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1115
> 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).

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

> martinvonz wrote in 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.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D340

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel