Patchwork [4,of,5] branchmap: simply update code

login
register
mail settings
Submitter Pierre-Yves David
Date Jan. 7, 2014, 1:38 a.m.
Message ID <5edde891b488a474c9b6.1389058706@marginatus.fb.com>
Download mbox | patch
Permalink /patch/3264/
State Superseded
Headers show

Comments

Pierre-Yves David - Jan. 7, 2014, 1:38 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1389047209 28800
#      Mon Jan 06 14:26:49 2014 -0800
# Node ID 5edde891b488a474c9b677d0cd52569f79c44c7e
# Parent  30aff651beece3d31ab3eb70b6cb40bd6b8bdf88
branchmap: simply update code

We drop iterrevs which are not needed anymore. The know head are never a
descendant of the updated set. It was possible with the old strip code. This
simplification make the code easier to read an update.
Simon Heimberg - Jan. 9, 2014, 3:50 p.m.
Two general comments below, I can not say anything about the
functionality.

On Monday 06.01.2014, 17:38 -0800 pierre-yves.david@ens-lyon.org wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1389047209 28800
> #      Mon Jan 06 14:26:49 2014 -0800
> # Node ID 5edde891b488a474c9b677d0cd52569f79c44c7e
> # Parent  30aff651beece3d31ab3eb70b6cb40bd6b8bdf88
> branchmap: simply update code
> 
> We drop iterrevs which are not needed anymore. The know head are never a
> descendant of the updated set. It was possible with the old strip code. This
> simplification make the code easier to read an update.
> 
> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -21,11 +21,11 @@ def timer(func, title=None):
>          count += 1
>          a, b = ostart, ostop
>          results.append((cstop - cstart, b[0] - a[0], b[1]-a[1]))
>          if cstop - begin > 3 and count >= 100:
>              break
> -        if cstop - begin > 10 and count >= 3:
> +        if cstop - begin > 10 and count >= 25:

I do not see how this change is related to the topic. It looks like this
is used for all perf measurements.

>              break
>      if title:
>          sys.stderr.write("! %s\n" % title)
>      if r:
>          sys.stderr.write("! result: %s\n" % r)
> @@ -394,10 +394,11 @@ def perfbranchmap(ui, repo, full=False):
>              view.branchmap()
>          return d
>      # add filter in smaller subset to bigger subset
>      possiblefilters = set(repoview.filtertable)
>      allfilters = []
> +    possiblefilters = ['base']

Why is possiblefilters defined two lines above and overwritten here? Is
the first one unnecessary?

>      while possiblefilters:
>          for name in possiblefilters:
>              subset = branchmap.subsettable.get(name)
>              if subset not in possiblefilters:
>                  break
> diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
> --- a/mercurial/branchmap.py
> +++ b/mercurial/branchmap.py
> @@ -214,11 +214,11 @@ class branchcache(dict):
>              # Abort may be raise by read only opener
>              pass
<snip>
Pierre-Yves David - Jan. 9, 2014, 7:01 p.m.
On 01/09/2014 07:50 AM, Simon Heimberg wrote:
> Two general comments below, I can not say anything about the
> functionality.

That is a totally wrong change that sneaked in :-) Thanks for pointing 
them out :-)

Will resend a version of this patch.

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -21,11 +21,11 @@  def timer(func, title=None):
         count += 1
         a, b = ostart, ostop
         results.append((cstop - cstart, b[0] - a[0], b[1]-a[1]))
         if cstop - begin > 3 and count >= 100:
             break
-        if cstop - begin > 10 and count >= 3:
+        if cstop - begin > 10 and count >= 25:
             break
     if title:
         sys.stderr.write("! %s\n" % title)
     if r:
         sys.stderr.write("! result: %s\n" % r)
@@ -394,10 +394,11 @@  def perfbranchmap(ui, repo, full=False):
             view.branchmap()
         return d
     # add filter in smaller subset to bigger subset
     possiblefilters = set(repoview.filtertable)
     allfilters = []
+    possiblefilters = ['base']
     while possiblefilters:
         for name in possiblefilters:
             subset = branchmap.subsettable.get(name)
             if subset not in possiblefilters:
                 break
diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -214,11 +214,11 @@  class branchcache(dict):
             # Abort may be raise by read only opener
             pass
 
     def update(self, repo, revgen):
         """Given a branchhead cache, self, that may have extra nodes or be
-        missing heads, and a generator of nodes that are at least a superset of
+        missing heads, and a generator of nodes that are strictly a superset of
         heads missing, this function updates self to be correct.
         """
         cl = repo.changelog
         # collect new branch entries
         newbranches = {}
@@ -232,36 +232,30 @@  class branchcache(dict):
         # really branchheads. Note checking parents is insufficient:
         # 1 (branch a) -> 2 (branch b) -> 3 (branch a)
         for branch, newheadrevs in newbranches.iteritems():
             bheads = self.setdefault(branch, [])
             bheadrevs = [cl.rev(node) for node in bheads]
-            ctxisnew = bheadrevs and min(newheadrevs) > max(bheadrevs)
-            # Remove duplicates - nodes that are in newheadrevs and are already
-            # in bheadrevs.  This can happen if you strip a node whose parent
-            # was already a head (because they're on different branches).
-            bheadrevs = sorted(set(bheadrevs).union(newheadrevs))
 
-            # Starting from tip means fewer passes over reachable.  If we know
-            # the new candidates are not ancestors of existing heads, we don't
-            # have to examine ancestors of existing heads
-            if ctxisnew:
-                iterrevs = sorted(newheadrevs)
-            else:
-                iterrevs = list(bheadrevs)
+            # This have been tested True on all internal usage of this function.
+            # run it again in case of doubt
+            # assert not (set(bheadrevs) & set(newheadrevs))
+            newheadrevs.sort()
+            bheadrevs.extend(newheadrevs)
+            bheadrevs.sort()
 
             # This loop prunes out two kinds of heads - heads that are
             # superseded by a head in newheadrevs, and newheadrevs that are not
             # heads because an existing head is their descendant.
-            while iterrevs:
-                latest = iterrevs.pop()
+            while newheadrevs:
+                latest = newheadrevs.pop()
                 if latest not in bheadrevs:
                     continue
                 ancestors = set(cl.ancestors([latest], bheadrevs[0]))
                 if ancestors:
                     bheadrevs = [b for b in bheadrevs if b not in ancestors]
             self[branch] = [cl.node(rev) for rev in bheadrevs]
-            tiprev = max(bheadrevs)
+            tiprev = bheadrevs[-1]
             if tiprev > self.tiprev:
                 self.tipnode = cl.node(tiprev)
                 self.tiprev = tiprev
 
         if not self.validfor(repo):