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

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:392
>                              _('changesets'), total)
> -                p1, p2, base = defineparents(repo, rev, self.dest,
> -                                             self.state,
> -                                             self.destancestors,
> -                                             self.obsoletenotrebased)
> +                p1, p2, base = defineparents(repo, rev, self.dest, self.state)
>                  self.storestatus(tr=tr)

IIUC, the reason you no longer need to pass destancestors and obsoletenotrebased is because they are now calculated on the fly instead of upfront. It seems like that might be slower, but I haven't spent time thinking about under what circumstances it might be. Have you? Could it be noticeable even in the worst case you can think of, or would it be completely drowned by rebase's other costs (such as writing revlogs and working copy files)?

> rebase.py:1004
> +    cl = repo.changelog
> +    def ancestor(rev1, rev2):
> +        # take revision numbers instead of nodes

Should this be a wrapper of cl.isancestor() instead? Looks like that's how you use it.

> rebase.py:1009
> +    oldps = repo.changelog.parentrevs(rev) # old parents
> +    newps = list(oldps) # new parents
> +    bases = set() # merge base candidates

nit: Is this for converting to a list or for making a copy (of something that's already a list)? If the latter, I feel like oldps[:}  is clearer.

> rebase.py:1010
> +    newps = list(oldps) # new parents
> +    bases = set() # merge base candidates
> +    dests = adjustdest(repo, rev, dest, state)

I think each step of the rebase effectively applies the diff in (rev - base) onto p1 (and p2 is only artificially added later to produce the graph we want), so I think we need to keep better track of the bases than using a set.

Here's a test that will fail after this patch (the manifest will contain A as well):

  $ hg debugdrawdag <<'EOF'
  >   D
  >  /|
  > B C
  >  \|
  >   A E
  >   |/
  >   Q
  > EOF
  $ hg rebase -d E -r 'B + C + D'
  $ hg manifest --rev tip
  B
  C
  E
  Q

> rebase.py:1019
> +        # For non-merge changeset, just move p to adjusted dest as requested.
> +        if oldps[1] == nullrev:
> +            newps[i] = dests[i]

This looks a bit weird. Would it work the same if you move it outside? Something like:

  if oldps[1] == nullrev:
    assert dests[1] == nullrev
    newps = dests
    if newps != oldps: # maybe always true?
      bases.add(oldps[0])
  else:
    newps = oldps[:]
    for i, p in enumerate(oldps):
       ....

Having written that and looked at adjustdest(), I see that the assertion would fail because adjustdest() would return [dest,dest] for a non-merge source. That seems surprising to me. Should adjustdest() be changed not to do that?

> rebase.py:1025
> +        # parent", which is a surprise. This is a limit because "--dest" only
> +        # accepts one dest per src.
> +        #

I don't quite understand the responsibility of adjustdest() vs this method. Why would or why would we not let adjustdest() handle the merge cases too instead of doing that here?

> rebase.py:1057
> +            # right thing.
> +            newps.reverse()
>  

Here's another case where I believe bases should also be updated (they should be reversed).

> test-rebase-obsolete.t:1109
> +
> +Rebased F should have one parent, just like in the test case above
> +

nit: seems unnecessary to even mention this now (the other tests don't)

REPOSITORY
  rHG Mercurial

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

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