[PATCH 4 of 8 V2] revset: update origin() predicate so everything is more consistent with grafts

[Matt Harbison]

Patrick Mézard wrote:
> Le 08/06/12 06:58, Matt Harbison a écrit :
>> # HG changeset patch
>> # User Matt Harbison<matt_harbison at yahoo.com>
>> # Date 1337735103 14400
>> # Node ID 2cf3fa0007c22037a8e3a1beaecec29d0bbc03ed
>> # Parent  9eb8327a534e2f16d66b036771ebaceac93d4b2a
>> revset: update origin() predicate so everything is more consistent with grafts
[...]
>
> [...]
>
>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>> --- a/mercurial/revset.py
>> +++ b/mercurial/revset.py
>> @@ -888,7 +888,19 @@
>>       else:
>>           s = tuple(p.rev() for p in repo[None].parents())
>>
>> -    return [r for r in xrange(0, len(repo)) if _getrevsource(repo, r) in s]
>> +    l = list()
>> +
>> +    for r in xrange(0, len(repo)):
>
> Use subset as mentioned in a previous message. Note you cannot assume anything about subset order, and probably have to sort it.
>

Sort the 'subset' parameter?  Wouldn't this break any previous sorting 
or ordering done on it?

>> +        src = _getrevsource(repo, r)
>> +
>> +        if src is not -1:
>
> if src != -1:

Oops.  Good catch.  I think I originally had _getrevsource() return 
None, but checkcode complained so I switched it.
>
>> +            # Select r if src is already selected (ie r was transitively copied
>> +            # from a given origin), or if src is itself a given origin that was
>> +            # NOT copied (to exclude a transplanted cset as an origin).
>> +            if src in l or (src in s and _getrevsource(repo, src) is None):
>> +                l.append(r)
>
> Couldn't "src in l" become expensive? What about turning it into a set and replace the last statement with:
>

That's a good point.  I wasn't considering performance issues with list 
vs set because I'm still learning pretty basic python with a C++/Java 
background and frankly, I didn't know how much 'magic' was involved with 
set() like list comprehensions and generators.

My question is, was the above construct for populating 'l' a) valid (I'm 
thinking not anymore, now that it isn't looping over every rev) and b) 
understandable?  I have no idea how revs get assigned in the repo, and 
as noted in the patch 0 message, don't know if there are ways that they 
can get reordered such that the above is invalid.  I traced through 
localrepo.__getitem__() at one point and thought it better to do that as 
little as possible.  But since Matt edited the previous patch to do the 
lookup twice, I guess maybe it is pretty cheap and the less fancy way of 
continually calling _getrevsource() until -1 is returned is safer?

Thanks to you and Matt for the comments.  I had pretty much tapped out 
the trial and error method of learning this.  But it's very cool once 
you do start to understand.

--Matt