Patchwork [V2] checkheads: extract obsolete post processing in its own function

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1490135413 -3600
#      Tue Mar 21 23:30:13 2017 +0100
# Node ID 72c820f77671d4495d96e7aeeba9237573dbce85
# Parent  66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d
# EXP-Topic checkheads
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 72c820f77671
checkheads: extract obsolete post processing in its own function

The checkheads function is long and complex, extract that logic in a subfunction
is win in itself. As the comment in the code says, this postprocessing is
currently very basic and either misbehave or fails to detect valid push in many
cases. My deeper motive for this extraction is to be make it easier to provide
extensive testing of this case and strategy to cover them. Final test and logic
will makes it to core once done.

On 3/22/17 4:31 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1490135413 -3600
> #      Tue Mar 21 23:30:13 2017 +0100
> # Node ID 72c820f77671d4495d96e7aeeba9237573dbce85
> # Parent  66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d
> # EXP-Topic checkheads
>
> The checkheads function is long and complex, extract that logic in a subfunction
> is win in itself. As the comment in the code says, this postprocessing is
> currently very basic and either misbehave or fails to detect valid push in many
> cases. My deeper motive for this extraction is to be make it easier to provide
> extensive testing of this case and strategy to cover them. Final test and logic
> will makes it to core once done.
>
> diff --git a/mercurial/discovery.py b/mercurial/discovery.py
> --- a/mercurial/discovery.py
> +++ b/mercurial/discovery.py
> @@ -343,38 +343,13 @@ def checkheads(pushop):
>           oldhs.update(unsyncedheads)
>           candidate_newhs.update(unsyncedheads)
>           dhs = None # delta heads, the new heads on branch
> -        discardedheads = set()
>           if not repo.obsstore:
> +            discardedheads = set()
>               newhs = candidate_newhs
>           else:
> -            # remove future heads which are actually obsoleted by another
> -            # pushed element:
> -            #
> -            # XXX as above, There are several cases this code does not handle
> -            # XXX properly
> -            #
> -            # (1) if <nh> is public, it won't be affected by obsolete marker
> -            #     and a new is created
> -            #
> -            # (2) if the new heads have ancestors which are not obsolete and
> -            #     not ancestors of any other heads we will have a new head too.
> -            #
> -            # These two cases will be easy to handle for known changeset but
> -            # much more tricky for unsynced changes.
> -            #
> -            # In addition, this code is confused by prune as it only looks for
> -            # successors of the heads (none if pruned) leading to issue4354
> -            newhs = set()
> -            for nh in candidate_newhs:
> -                if nh in repo and repo[nh].phase() <= phases.public:
> -                    newhs.add(nh)
> -                else:
> -                    for suc in obsolete.allsuccessors(repo.obsstore, [nh]):
> -                        if suc != nh and suc in allfuturecommon:
> -                            discardedheads.add(nh)
> -                            break
> -                    else:
> -                        newhs.add(nh)
> +            newhs, discardedheads = _postprocessobsolete(pushop,
> +                                                         allfuturecommon,
> +                                                         candidate_newhs)
>           unsynced = sorted(h for h in unsyncedheads if h not in discardedheads)
>           if unsynced:
>               if None in unsynced:
> @@ -434,3 +409,42 @@ def checkheads(pushop):
>                   repo.ui.note((" %s\n") % short(h))
>       if errormsg:
>           raise error.Abort(errormsg, hint=hint)
> +
> +def _postprocessobsolete(pushop, futurecommon, candidate_newhs):
Indeed, passing pushop does seem better.

> +    """post process the list of new heads with obsolescence information
> +
> +    Exist as a subfunction to contains the complexity and allow extensions to
Grammar nit: "Exists as a subfunction to contain..." (move the s from 
contains to exist).

That can be fixed in flight and isn't too important for understanding 
anyway.

> +    experiment with smarter logic.
> +    Returns (newheads, discarded_heads) tuple
> +    """
> +    # remove future heads which are actually obsoleted by another
> +    # pushed element:
> +    #
> +    # XXX as above, There are several cases this code does not handle
> +    # XXX properly
> +    #
> +    # (1) if <nh> is public, it won't be affected by obsolete marker
> +    #     and a new is created
> +    #
> +    # (2) if the new heads have ancestors which are not obsolete and
> +    #     not ancestors of any other heads we will have a new head too.
> +    #
> +    # These two cases will be easy to handle for known changeset but
> +    # much more tricky for unsynced changes.
> +    #
> +    # In addition, this code is confused by prune as it only looks for
> +    # successors of the heads (none if pruned) leading to issue4354
> +    repo = pushop.repo
> +    newhs = set()
> +    discarded = set()
> +    for nh in candidate_newhs:
> +        if nh in repo and repo[nh].phase() <= phases.public:
> +            newhs.add(nh)
> +        else:
> +            for suc in obsolete.allsuccessors(repo.obsstore, [nh]):
> +                if suc != nh and suc in futurecommon:
> +                    discarded.add(nh)
> +                    break
> +            else:
> +                newhs.add(nh)
> +    return newhs, discarded
>
Nice cleanup, lgtm.

On Thu, Mar 23, 2017 at 09:52:59AM +0000, Ryan McElroy wrote:
> On 3/22/17 4:31 PM, Pierre-Yves David wrote:
> ># HG changeset patch
> ># User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> ># Date 1490135413 -3600
> >#      Tue Mar 21 23:30:13 2017 +0100
> ># Node ID 72c820f77671d4495d96e7aeeba9237573dbce85
> ># Parent  66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d
> ># EXP-Topic checkheads
> >
> >The checkheads function is long and complex, extract that logic in a subfunction
> >is win in itself. As the comment in the code says, this postprocessing is
> >currently very basic and either misbehave or fails to detect valid push in many
> >cases. My deeper motive for this extraction is to be make it easier to provide
> >extensive testing of this case and strategy to cover them. Final test and logic
> >will makes it to core once done.

[...]

> Nice cleanup, lgtm.

Queued, thanks.


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel