quark added inline comments. INLINE COMMENTS
> martinvonz wrote in rebase.py:1004 > It looks like it effectively is. You can rewrite it using isancestor(). The > reason I brought this up at all was that cl.ancestor() returns a single > ancestor even if there are multiple (as in criss-cross merges), which made me > wonder what would happen if the wrong ancestor was returned. I don't think > there can be a "wrong" ancestor for the uses here, but using isancestor() > instead makes that clearer. > > Also, it seems like isancestor(a,b) can be made faster than ancestors(a,b)==a > can because it can stop walking when when the rev is lower than a's rev. Good point. I was aware that isancestor could be slightly faster. I'll rewrite the merge base logic so it only uses `isancestor`. > martinvonz wrote in rebase.py:1010 > I'm not sure I understand, but I don't think the user can influence the > result (and I'm assuming you mean "as *D*'s parent", otherwise I can't make > any sense of it). It should be fine to use either B' or C' as the new p1, as > long as base is set to B or C (respectively). Sorry I wasn't clear. When there are multiple merge base candidates, we perform rules in this order: 1. remove ancestors of source and destination since they can be decided by the `merge.update(ancestor=None)` without us passing an explicit `ancestor`. 2. remove obsoleted parents with successor in destination, as explained by the next patch. 3. if there are still non-unique candidates, what to do? The current code for 3 will give up and let `merge.update` decide, which produces the sub-optimal result in your case. Choosing p1 (if changed) blindly is better then the current code as step 3. I feel it's worth a warning since the merge result is sub-optimal any way. The case I wanted to make was to say "1" must happen before "3". But it seems obvious so the case is not helpful. Instead of picking p1 blindly, I wonder if we want passing all merge base candidates to the merge code to decide. The merge code seems to have "calculating bids for ancestor" logic suitable for this case. I'm trying that approach to see how it works. > martinvonz wrote in rebase.py:1019 > True, I can see the point about [nullrev,nullrev] being valid implying that > any [X,X] should be valid. > > How about the rest of my comment? I didn't understand what you meant by the > first part of you reply. I concluded that part of the reasons that the old code is messy is because it uses too many `if`s and relies heavily on the fact that Mercurial has only 2 parents. So I started with `for` as top-level block as a hint to make people think about making the code work for more than 2 parents. But it does not seem to serve that purpose well now. Single-parent case is still special, I'll change the structure. > martinvonz wrote in test-rebase-newancestor.t:287 > Sorry, I forgot to ask before, but why did this change? Good catch! It turns out to be Line 1088, `ancestor` returns a single node where in this case we want multiple ancestor nodes. That should be solved by the `isancestor` change. Thanks! REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D21 To: quark, durin42, #hg-reviewers Cc: martinvonz, durin42, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
