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

[Ryan McElroy]

On 3/21/17 10:42 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1490135413 -3600
> #      Tue Mar 21 23:30:13 2017 +0100
> # Node ID 787354f0d60eccda66ba0de4db8e6e47897acc7c
> # Parent  66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d
> # EXP-Topic checkheads
> # Available At https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=X1PCqIb1foE38fk_XWYf-aaAwDmJ4P0klZVtI7TGEWk&s=BDwmpGmjYfrQ5_BZQ-19-vlGJwbvEH2-rewqp3-9cCI&e=
> #              hg pull https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_repo_users_marmoute_mercurial_&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=X1PCqIb1foE38fk_XWYf-aaAwDmJ4P0klZVtI7TGEWk&s=BDwmpGmjYfrQ5_BZQ-19-vlGJwbvEH2-rewqp3-9cCI&e=  -r 787354f0d60e
> 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.

I agree, this looks good to me.

> 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,12 @@ 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(repo, allfuturecommon,
> +                                                         candidate_newhs)
>           unsynced = sorted(h for h in unsyncedheads if h not in discardedheads)
>           if unsynced:
>               if None in unsynced:
> @@ -434,3 +408,41 @@ def checkheads(pushop):
>                   repo.ui.note((" %s\n") % short(h))
>       if errormsg:
>           raise error.Abort(errormsg, hint=hint)
> +
> +def _postprocessobsolete(repo, common, candidate):
> +    """post process the list of new heads with obsolescence information
> +
> +    Exist as a subfunction to contains the complexity and allow extensions to
> +    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
> +    newhs = set()
> +    discarded = set()
> +    for nh in candidate:
> +        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 common:
> +                    discarded.add(nh)
> +                    break
> +            else:
> +                newhs.add(nh)
> +    return newhs, discarded
>