quark added 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!

  rHG Mercurial


To: quark, durin42, #hg-reviewers
Cc: martinvonz, durin42, mercurial-devel
Mercurial-devel mailing list

Reply via email to