[PATCH 2 of 2 v5] revlog: remove revlog.reachable()

[Joshua Redstone]

K, will split up patches as you suggest and also add incancestors.

From: Bryan O'Sullivan <bos at serpentine.com<mailto:bos at serpentine.com>>
Date: Wednesday, June 6, 2012 5:52 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 2 of 2 v5] revlog: remove revlog.reachable()

On Tue, Jun 5, 2012 at 1:38 PM, Joshua Redstone <joshua.redstone at fb.com<mailto:joshua.redstone at fb.com>> wrote:

    def applied(self, repo, node, parent):
-        '''returns True if a node is already an ancestor of parent
-        or has already been transplanted'''
+        '''returns True if a node is already an ancestor of parent,
+        including parent, or has already been transplanted'''

It's a shame that we have these dual needs, for both inclusive and exclusive ancestors and descendants. Seems to me we'd be better off to add an incancestors method to revlog that provided the "inclusive ancestors" functionality, rather than having every call site uglified as in the hunk that follows:

+            if stoprev == parentrev or \
+                    stoprev in repo.changelog.ancestors([parentrev], stoprev):

(Style nit: there are very few line-ending backslashes in the code base, and it would be nice to not have them proliferate. Could you maybe use parentheses here instead?)

--- a/mercurial/discovery.py    Mon Jun 04 17:57:57 2012 -0500<tel:2012%20-0500>
+++ b/mercurial/discovery.py    Fri Jun 01 08:56:17 2012 -0700<tel:2012%20-0700>
@@ -208,17 +208,19 @@
        # Construct {old,new}map with branch = None (topological branch).
        # (code based on _updatebranchcache)
        oldheads = set(h for h in remoteheads if h in cl.nodemap)
-        newheads = oldheads.union(outgoing.missing)
-        if len(newheads) > 1:
-            for latest in reversed(outgoing.missing):
-                if latest not in newheads:
+        oldheadrevs = set([cl.rev(node) for node in oldheads])
+        missingrevs = [cl.rev(node) for node in outgoing.missing]
+        newheadrevs = oldheadrevs.union(missingrevs)
+        if len(newheadrevs) > 1:
+            missingrevs.sort(reverse=True)
+            for latest in missingrevs:
+                if latest not in newheadrevs:
                    continue
-                minhrev = min(cl.rev(h) for h in newheads)
-                reachable = cl.reachable(latest, cl.node(minhrev))
-                reachable.remove(latest)
-                newheads.difference_update(reachable)
+                minhrev = min(newheadsrevs) # <-- TYPO
+                reachable = set(cl.ancestors([latest], minhrev))
+                newheadrevs.difference_update(reachable)

I think this can't be correct, as there's a typo: "newheadsrevs" 3 lines from the bottom. Also, are you sure that the old and new code paths process nodes in the same order and give the same result, or if not, that this is okay?

Also, this hunk is complex, but doesn't actually seem to make the code any simpler. Is it necessary for some reason? And is it relevant to the rest of this patch?

Oh wait, I see what's going on: you dropped reachable, and then switched all the surrounding code from nodespace to revspace. That's good, but it does make this patch big, intimidating, and hard to follow. Maybe this would be best done in a series of patches:

  *   One patch that drops reachable, converts all callers to using ancestors instead, but immediately converts the revs returned by ancestors into nodes. This should make each callsite easier to visually inspect for correctness.
  *   N patches, each converting a single former callsite from nodespace to revspace.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120608/da9f9be5/attachment.html>