Patchwork checkheads: extract obsolete post processing in its own function

login
register
mail settings
Submitter Pierre-Yves David
Date March 21, 2017, 10:42 p.m.
Message ID <787354f0d60eccda66ba.1490136145@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/19540/
State Superseded
Headers show

Comments

Pierre-Yves David - March 21, 2017, 10:42 p.m.
# 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 787354f0d60eccda66ba0de4db8e6e47897acc7c
# 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 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. 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.
Ryan McElroy - March 22, 2017, 10:04 a.m.
On 3/21/17 10:42 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 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
>
Pierre-Yves David - March 22, 2017, 4:14 p.m.
On 03/22/2017 11:04 AM, Ryan McElroy wrote:
> On 3/21/17 10:42 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 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.

Actually, we should probably pass a the pushoperation instead of the 
repository. That logic will needs to poke at the remote repository (eg: 
Sean told me about a bug reported to bitbucket were this logic kicks in 
even if obsolescence markers are not going to be exchanged.

V2 incoming.

Cheers,

Patch

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