[PATCH 2 of 4 V3 part 2] ancestor: add lazy membership testing to lazyancestors

Siddharth Agarwal sid0 at fb.com
Tue Dec 18 12:02:47 CST 2012


On 12/18/2012 08:43 AM, Kevin Bullock wrote:
> Looks good, but some comments below. I don't see any obvious code 
> paths that would suffer from the latter case, do you have a sense of 
> this? 

Well, I guess you could hit it if you're e.g. rebasing from tip to 0, 
but then you have bigger problems. In general we're almost always better 
off.

> I'm uneasy about this change being included in this patch. It should 
> at least be referenced in the commit message, if not moved to a 
> subsequent patch. 

It is referenced already in the commit message, isn't it? That's where 
the perf numbers come from.

> I'd prefer if these were simply named self._seen and self._visit, with 
> a comment that they aren't to be used for iteration. 

I'm torn here -- that is what I originally had, but I thought that was 
somewhat confusing even with a comment.

>> +def isinlazyset(s, l):
>> +    print [n for n in l if n in s]
> The name of this function is entirely opaque to me. Something more like 'printlazyancestors' would be an improvement.

Yes, the name is a leftover from an early iteration. I'll fix this.


More information about the Mercurial-devel mailing list