[PATCH 6 of 6] revset: fix a crash with 'roots(wdir())'

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sat Jul 18 10:16:14 CDT 2015



On 07/04/2015 03:29 PM, Yuya Nishihara wrote:
> On Fri, 03 Jul 2015 09:51:31 -0700, Pierre-Yves David wrote:
>> On 07/01/2015 06:28 PM, Matt Harbison wrote:
>>> On Wed, 01 Jul 2015 09:01:17 -0400, Yuya Nishihara <yuya at tcha.org> wrote:
>>>> 3) can we have a function that is as fast as cl.parentrevs(r) and can
>>>> process
>>>>     wdir() ?
>>>
>>> Something like this in revset.py?  It looks like an optimization to not
>>> go through context.
>>>
>>> def _parentrevs(repo, cl, r):
>>>       if r is not None:
>>>           return cl.parentrevs(r)
>>>       else:
>>>           return [p.rev() for p in repo[r].parents()]
>>>
>>> The only reason I didn't do it was because with the local changelog
>>> caching optimization, I assumed calling a function over and over would
>>> be unacceptable overhead.
>
> It could be a generator to minimize the number of repo.changelog calls, but
> I agree it would be still slower than the inlined version.
>
>    def _iterparentrevs(repo, subset):
>        cl = repo.changelog
>        for r in subset:
>            if r is not None:
>                yield cl.parentrevs(r)
>            else:
>                yield [p.rev() for p in repo[r].parents()]
>
>> So I think we should have the API not panic when handed and
>> 'wdirrev'/'wdirid' (the same way we probably want to detect 'nullrev'/….
>>
>> I see possibles path for this:
>>
>> 1) Have the low level API support the 'wdirrev' by providing them some
>> access to the dirstate data. That would be the "best" as it would ensure
>> transparent support for 'wdir' everywhere (mostly). However, this would
>> probably requires a massive number of layer violation and mudding.
>>
>> 2) Have low level API detect 'wdirrev' case and "ignore" it, issue a
>> devel-warn for developer to spot such case. This will provide a middle
>> ground between full support (1) and plain crash (what we have now).
>
> I think (1) would be the hard way. My current idea is similar to (2), but
> raising exception instead of devel-warning which needs 'ui' object.
>
>    cl.node(wdirrev) -> wdirid
>    cl.rev(wdirnode) -> wdirrev
>    cl.parentrevs(wdirrev) -> UnsupportedNodeError

The goal here would be to avoid to crash (crash to the user are bad) in 
multiple place where people will be using old code (especially 
extensions). Raising an exception would not help in this regard (except 
it would be a dedicated error)

But as you pointed, cl have neither a repo or a ui, which limit our 
options a lot,

> Then, cl.parentrevs(r) will be able to fall back to repo[r].parents():
>
>    try:
>        ps.add(cl.parentrevs(r)[0])
>    except error.UnsupportedNodeError:
>        ps.add(repo[r].p1().rev())
>
> The overhead will be minimal because 'wdir()' will be rarely used.

Setting up the exception catching will likely have an overhead:

no-except: 1.03 ms

   for i in xrange(2**16)
     i

with-except: 1.57 ms

   for i in xrange(2**16)
     try:
       i
     except:
       pass

>
>> 3) As you guys have been working on it more closely than I did, you will
>> likely comes with another better solution.
>>
>> 4) I got convinced that my worries are groundless.
>
> I'm sure (4) never ever happen. ;)

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list