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

Yuya Nishihara yuya at tcha.org
Sat Jul 4 08:29:09 CDT 2015


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

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.

> 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. ;)


More information about the Mercurial-devel mailing list