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

[Yuya Nishihara]

On Thu, 12 Apr 2018 11:32:23 +0200, Feld Boris wrote:
> On 11/04/2018 17:16, Yuya Nishihara wrote:
> > On Wed, 11 Apr 2018 11:36:05 +0200, Feld Boris wrote:
> >>>> The proposal here is to define a prefix for which we break backward
> >>>> compatibility. If we do so, people with a "<set>" label will have to use:
> >>>>
> >>>>      "<set>":whatever
> >>>>
> >>>> to get a similar effect.
> >>> IIRC x:y was the most important syntax that needed a strong BC guarantee, so
> >>> this proposal doesn't sound good.
> >> Indeed, the `x:y` is important and we don't want to break the BC
> >> guarantee of it.
> >>
> >> The proposal is less impacting, only people using 'set' as labels and
> >> using it at the beginning of a revsetwould be impacted. This prefix has
> >> the advantage of being concise and coherent with whatfilesetuse.
> > Doesn't '-r set:foo' look like a range?
> >
> > I don't like an idea of introducing another ambiguous syntax to resolve
> > ambiguity, but furthermore "set:foo" seems confusing to humans.
> >
> > IIUC, we have "set:" for filesets only because that's the syntax to specify
> > file patterns. If we really want to add something to force revset evaluation,
> > I think it has to be compatible with the current syntax, such as "(EXPR)" or
> > "revset(EXPR)".
> 
> `(EXPR)` seemtoo likely to introduce a BC breakage, as people are more 
> likely to have a tag that looks like `(xxx)` than a 'set' tag IMHO.

Ugh, I never assumed that would actually happen, but since you face to more
real users having various backgrounds than me, I might be wrong.

> In a previous discussion, you pointed out that `revset(EXPR)` would be 
> painful because we would have to escape everything within EXPR. What 
> makes you consider it again? Do you mean that if a revset has this form 
> `revset((.*))`; we just evaluate the contents inside parenthesis?

My proposal is 'revset(EXPR)', not 'revset("EXPR")'. It's an identity function
similar to present(), but also disables the legacy lookup. I don't wanna add
such magic, but 'revset(EXPR)' seems less bad than 'set:EXPR' or ' EXPR'.

> Agreed that `set:foo` looks like a range, maybe we need to use a less 
> ambiguous operator?
> 
> `=foo+bar`
> `#foo+bar`
> `#revset# foo+bar`
> 
> (Nothing really stands out as pretty, but trying to extend our search 
> area here.)

Yeah, they look weird.

FWIW, I prefer not reviewing this sort of patches just before the freeze.
The patch itself is simple, but we have to carefully think about UX and
syntactic consistency.