D21: rebase: rewrite core algorithm (issue5578) (issue5630)

Sat Aug 12 21:16:53 UTC 2017

martinvonz added inline comments.

INLINE COMMENTS

> quark wrote in rebase.py:1004
> 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 rebase.py:1010
> 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 https://phab.mercurial-scm.org/D340 (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 rebase.py:1019
> 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?

REPOSITORY
  rHG Mercurial

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

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