[PATCH 2 of 2 V2] revset: skip old style lookup if external whitespace are detected

[Yuya Nishihara]

On Mon, 26 Feb 2018 11:45:03 +0100, Feld Boris wrote:
> On 13/02/2018 12:47, Yuya Nishihara wrote:
> > On Mon, 12 Feb 2018 18:00:52 +0100, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld at octobus.net>
> >> # Date 1518448909 -3600
> >> #      Mon Feb 12 16:21:49 2018 +0100
> >> # Node ID b0f45e1376e2d0f32023e197c51802bc21c60490
> >> # Parent  f02fd7ca256d044c4a51c3f3fc0ecaf95d23e03d
> >> # EXP-Topic noname
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r b0f45e1376e2
> >> revset: skip old style lookup if external whitespace are detected
> >>
> >> Since label cannot contains leading or trailing whitespace we can skip looking
> >> for them. This is useful in repository with slow labels (eg: special type of
> >> tags). Short command running on a specific revision can benefit from such
> >> shortcut.
> >>
> >> eg on a repository where loading tags take 0.4s:
> >>
> >> 1: hg log --template '{node}\n' --rev 'rev(0)'
> >>     0.560 seconds
> >>
> >> 2: hg log --template '{node}\n' --rev ' rev(0)'
> >>     0.109 seconds
> > Seems okay, but isn't it too obscure that prefixing with ' ' is the fast
> > way of querying?
> 
> Yes, it is a bit obscure but it's the best solution we came up with 
> existing code. I sent a new patch that implements the fast path at the 
> individual name-space level in a way that seems cleaner and more useful.
> 
> Another solution is we could force revset evaluation using a `set:` (or 
> `revset:`) prefix. This way we could have both a clean and explicit way 
> of implementing the fast path.

That isn't possible because "set:whatever" can be a range between "set"
and whatever. ;)

> > Instead, maybe we can make lookup() to not search slow labels assuming these
> > labeling schemes didn't exist in pre-revset era.
> We are afraid it is a bit more complex than that. Because `rev(0)` is a 
> valid tag and a valid bookmark, we don't think we can ever skip this 
> lookup call (yes, in our case, both tags and bookmarks are expensive to 
> load).

If loading plain tags and bookmarks is expensive, yeah, there would be no
fast path of lookup().

> > Alternatively, we could add
> > a config knob to switch off the old-style range support.
> Having a config knob for this seems weird. We don't expect users to find 
> it and really understand what the config is about. It would be useful 
> for large corporate users with centralized config, but they would need 
> to set the flag on every one of their scripts/servers involving Mercurial.

IMHO, config knob is easier to learn than using the ' ' prefix. I would say
WTF if I taught to use the ' ' to make hg fast. And I think this config can
be switched on by tweakdefaults because lookup() exists only for backward
compatibility. (We might still want to allow dashes in symbols, though.)