[PATCH] rebase: clarify comment about merge ancestor when rebasing merges

[Augie Fackler]

On Sun, Jan 04, 2015 at 01:58:21AM +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski at unity3d.com>
> # Date 1420331347 -3600
> #      Sun Jan 04 01:29:07 2015 +0100
> # Node ID c9d31cdeb6cc91c745d570b542aff07c2cac47e6
> # Parent  ff6694de19f3c7f3829ef65f504fb3ad573deb70
> rebase: clarify comment about merge ancestor when rebasing merges

queued with a handful of english tweaks, thanks

(I agree, based on the new comments this code is ripe for replacement.)

>
> The code for picking a merge ancestor when rebasing merges had a long and
> incorrect comment.
>
> The comment would perhaps have been fine as commit message but do not make the
> code more readable or maintainable and is a bad substitute for correct and
> readable code.
>
> The correct essense of the comment is quite trivial: a merge of an ancestor of
> the rebase destination and an 'outside' revision can be rebased as if it was a
> linear change, using 'destination ancestor parent' as base and pretty much
> ignoring the 'outside' revision.
>
> The code path where the comment is placed is however also used for other kinds
> of merge rebases. The comment is thus not really correct and not helpful. I
> think it would be better to drop the comment and rewrite the code.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -645,17 +645,19 @@ def defineparents(repo, rev, target, sta
>          # Case (2) detaching the node with a single parent, use this parent
>          base = repo[rev].p1().rev()
>      else:
> -        # In case of merge, we need to pick the right parent as merge base.
> +        # Assuming there is a p1, this is the case where there also is a p2.
> +        # We are thus rebasing a merge and need to pick the right merge base.
>          #
>          # Imagine we have:
> -        # - M: currently rebase revision in this step
> +        # - M: current rebase revision in this step
>          # - A: one parent of M
> -        # - B: second parent of M
> +        # - B: other parent of M
>          # - D: destination of this merge step (p1 var)
>          #
> -        # If we are rebasing on D, D is the successors of A or B. The right
> -        # merge base is the one D succeed to. We pretend it is B for the rest
> -        # of this comment
> +        # Consider the case where D is a descendant of A or B and the other is
> +        # 'outside'. In this case, the right merge base is the D ancestor.
> +        #
> +        # An informal proof, assuming A is 'outside' and B is the D ancestor:
>          #
>          # If we pick B as the base, the merge involves:
>          # - changes from B to M (actual changeset payload)
> @@ -663,12 +665,20 @@ def defineparents(repo, rev, target, sta
>          #   version of B)
>          # Which exactly represent the rebase operation.
>          #
> -        # If we pick the A as the base, the merge involves
> +        # If we pick A as the base, the merge involves:
>          # - changes from A to M (actual changeset payload)
>          # - changes from A to D (with include changes between unrelated A and B
>          #   plus changes induced by rebase)
>          # Which does not represent anything sensible and creates a lot of
> -        # conflicts.
> +        # conflicts. A is thus not the right choice - B is.
> +        #
> +        # Note: The base found in this 'proof' is only correct in the specified
> +        # case. This base do not make sense if D not is a descendant of A or B
> +        # or if the other parent not is 'outside' (especially not if the other
> +        # parent has been rebased). The current implementation do however not
> +        # make it feasible to consider different cases separately. In these
> +        # other changes we currently just leave it to the user to correctly
> +        # resolve an impossible merge using a wrong ancestor.
>          for p in repo[rev].parents():
>              if state.get(p.rev()) == p1:
>                  base = p.rev()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel