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

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> quark wrote in rebase.py:1019
> 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.

Yes, I think handling the single-parent case separately makes sense. The problem with the old structure was not that it handled  1 vs 2 parents differently but that it handled the 1st vs the 2nd parent differently. (I'm sure *you* have realized that, I'm just trying to help other readers who have been less involved.)

> rebase.py:1051
> +            # graph. Here, only record bases where changelog graph cannot
> +            # desired result (i.e. isancestor(p, np) is Fales). For example,
> +            #

missing "find" (?) before "desired" and misspelled "False"

> rebase.py:1069
> +                    continue
> +                if x == np or (x > np and isancestor(np, x)):
> +                    np = nullrev

nit: move "x > np" into isancestor()

> rebase.py:1070
> +                if x == np or (x > np and isancestor(np, x)):
> +                    np = nullrev
> +                elif x < np and isancestor(x, np):

In this case, maybe we shouldn't have added `p` to `bases` above? Maybe that block should be moved after this loop? Maybe it would be easier to keep track of the bases in a list that's initialized to [None,None]? Or maybe if the order of `newps` always matches `oldps`, can `bases` be calculated later and not mixed in with the calculation of `newps`?

> rebase.py:1134
> +
> +        if siderevs:
> +            repo.ui.warn(_('warning: rebasing %d:%s may inlcude unwanted '

Thanks for catching this case and warning about it! I wonder if we should even error out early instead? Even if we decide to do that, feel free to leave it for a followup.

> rebase.py:1135
> +        if siderevs:
> +            repo.ui.warn(_('warning: rebasing %d:%s may inlcude unwanted '
> +                           'changes from revision %s\n')

Typo in "inlcude".

> rebase.py:1136
> +            repo.ui.warn(_('warning: rebasing %d:%s may inlcude unwanted '
> +                           'changes from revision %s\n')
> +                         % (rev, repo[rev],

It would probably be good to include the nodeid here just like we do on the line above, especially since the revision number may point to something else after the rebase is complete (if not using obsmarkers).

REPOSITORY
  rHG Mercurial

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

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