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

[Bryan O'Sullivan]

On Tue, Jun 5, 2012 at 1:38 PM, Joshua Redstone <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
> +++ b/mercurial/discovery.py    Fri Jun 01 08:56:17 2012 -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/20120606/17275f11/attachment.html>