martinvonz added inline comments.


> quark wrote in
> Line 1088 is not `isancestor`. I'm okay with having two functions here. Let 
> me know what do you prefer.

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.

> quark wrote in
> Interesting. I didn't know that we treat p1 specially here. If so, users can 
> always construct an asymmetric case that feels wrong. For example, if we use 
> B or C as E's parent in the above graph, p1 may not get chosen in one of the 
> case.
> In this diff, it's Line 1091 trying to be conservative. I'll add tests and a 
> warning like (Diff 770) line 1115.

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

> quark wrote in
> Maybe `assert i == 0` is a better assertion here. If `nullrev` is considered 
> part of the DAG, then I think it makes sense for `adjustdest` to return 
> `dest` for p2.

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.

> test-rebase-newancestor.t:287
>    removing other
> -  note: merging f9daf77ffe76+ and 4c5f12f25ebe using bids from ancestors 
> a60552eb93fb and f59da8fc0fcf
> -  

Sorry, I forgot to ask before, but why did this change?

  rHG Mercurial


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

Reply via email to