[PATCH 6 of 6 v6] strip: incrementally update the branchheads cache after a strip

[Joshua Redstone]

From: Bryan O'Sullivan <bos at serpentine.com<mailto:bos at serpentine.com>>
Date: Thursday, June 14, 2012 8:14 PM
To: Joshua Redstone <joshua.redstone at fb.com<mailto:joshua.redstone at fb.com>>
Cc: "mpm at selenic.com<mailto:mpm at selenic.com>" <mpm at selenic.com<mailto:mpm at selenic.com>>, "mercurial-devel at selenic.com<mailto:mercurial-devel at selenic.com>" <mercurial-devel at selenic.com<mailto:mercurial-devel at selenic.com>>
Subject: Re: [PATCH 6 of 6 v6] strip: incrementally update the branchheads cache after a strip

On Fri, Jun 8, 2012 at 2:27 PM, Joshua Redstone <joshua.redstone at fb.com<mailto:joshua.redstone at fb.com>> wrote:

+            bheadrevs = [self.changelog.rev(node) for node in bheads \
+                             if self.changelog.hasnode(node)]
+            newheadrevs = [self.changelog.rev(node) for node in newnodes \
+                             if self.changelog.hasnode(node)]

The backslashes are not necessary here, and the "if" lines should be aligned with "self" on the preceding lines.
k

+            ctxisnew = True
+            if len(bheadrevs) > 0:
+                ctxisnew = min(newheadrevs) > max(bheadrevs)

ctxisnew = bheadrevs and min(newheadrevs) > max(bheadrevs)
good idiom

+            bheadrevs = list(set(bheadrevs).union(newheadrevs))
            bheadrevs.sort()

bheadrevs = sorted(set(bheadrevs).union(newheadrevs))
nice

+            # 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, reverse=True)
+            else:
+                iterrevs = sorted(bheadrevs, reverse=True)

Your comment about starting from tip seems to contradict the code that follows...
right - removed reverse

+            while iterrevs:
+                latest = iterrevs.pop()

...which due to the sort ordering is starting from the *furthest* from tip.

+        for branch in partial.keys():

for branch in partial:

+            nodes = [head for head in partial[branch] \
+                         if self.changelog.hasnode(head)]

Drop the backslash.

+            if len(nodes) < 1:

if not nodes:

+        else:
+            # No info to update the cache.  If nodes were destroyed, the cache
+            # is stale and this will be caught the next time it is read.
+            pass

This is a little stylistically odd.
adjusted

+    # Generate set of branches who will have nodes stripped.
+    stringstriplist = [str(rev) for rev in striplist]
+    striprevs = set(repo.revs("%lr::", stringstriplist,
+                                stringstriplist))
+    stripbranches = set([repo[rev].branch() for rev in striprevs])

This could be a simple one-liner, I think:

stripbranches = set([repo[rev].branch() for rev in repo.revs('%ld::', striplist)])

didn't know about %ld.  I think still need striprevs around for code below

+    newheadrevs = set(repo.revs("parents(%lr::) - %lr::", stringstriplist,
+                                stringstriplist))

This doesn't need to be a set, and you can still avoid stringstriplist:

newheadrevs = repo.revs("parents(%ld::) - %ld::", striprevs, striprevs)
right

+    newheadbranches = set([repo[rev].branch() for rev in newheadrevs])

newheadbranches does not need to be a set.
I think it does. otherwise if there are duplicates then the equality check below fails..

+    if len(stripbranches) == 1 and len(newheadbranches) == 1 \
+            and stripbranches == newheadbranches:

if len(stripbranches) == 1 and stripbranches == newheadbranches:
right
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120615/e838e40c/attachment.html>