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

[Yuya Nishihara]

On Fri, 13 Apr 2018 19:56:21 +0200, Feld Boris wrote:
> > We love the 'revset(EXPR)' idea and we have a v4 series that is ready 
> > to send.

Can you mark it as "(EXPERIMENTAL)" in case we find a better way to work
around the problem?

> By re-reading your comments on the V3 series, I realized that the second 
> changeset is not necessary since repo is never passed for internal lookup

I'm thinking of renaming repo argument to lookup=lookupfn(repo) to clarify
that.

> I will update the series and prepare a new V4.

Thanks.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Feld Boris]

On 13/04/2018 15:48, Feld Boris wrote:

On 12/04/2018 13:09, Yuya Nishihara wrote:

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 "" label will have to use:

  "":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.


We love the 'revset(EXPR)' idea and we have a v4 series that is ready 
to send.


It is okay to send it before the freeze? If not, would it be possible 
to take the first two changesets of the V3 series? They will likely 
bring some speed improvements for big repositories where name lookups 
are slow and for commands that don't need to load them.


By re-reading your comments on the V3 series, I realized that the second 
changeset is not necessary since repo is never passed for internal lookup


I will update the series and prepare a new V4.




___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel




___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Feld Boris]

On 12/04/2018 13:09, Yuya Nishihara wrote:

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 "" label will have to use:

  "":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.


We love the 'revset(EXPR)' idea and we have a v4 series that is ready to 
send.


It is okay to send it before the freeze? If not, would it be possible to 
take the first two changesets of the V3 series? They will likely bring 
some speed improvements for big repositories where name lookups are slow 
and for commands that don't need to load them.



___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [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 "" label will have to use:
> 
>   "":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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Feld Boris]

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 "" label will have to use:

     "":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.


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?


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



Since "foo(bar)" needs quotes in revset query (except for x and x:y), it would
makes some sense to add an option to disable the compatibility hack at all.

We cannot see a way to make the config option both easily discoverable
and constrained. There is lot of labels that includes `-`, `+` and other
symbols that will be impacted.

sigh.


___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

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 "" label will have to use:
> >>
> >>     "":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)".

> > Since "foo(bar)" needs quotes in revset query (except for x and x:y), it 
> > would
> > makes some sense to add an option to disable the compatibility hack at all.
> 
> We cannot see a way to make the config option both easily discoverable 
> and constrained. There is lot of labels that includes `-`, `+` and other 
> symbols that will be impacted.

sigh.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Feld Boris]

On 02/03/2018 05:58, Yuya Nishihara wrote:

On Thu, 1 Mar 2018 15:48:47 -0500, Feld Boris wrote:

On 26/02/2018 08:11, Yuya Nishihara wrote:

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

The proposal here is to define a prefix for which we break backward
compatibility. If we do so, people with a "" label will have to use:

    "":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.



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

Dropping the older lookup methods seems impractical. Right now, `rev(0)`
is a valid tag. So dropping legacy lookup means all these things will
have to be heavily quoted: (eg: hg log -r '"version-4(candidate2)"').

Yes. I assume that "foo(bar)" tags or branches wouldn't be used widely.

Since "foo(bar)" needs quotes in revset query (except for x and x:y), it would
makes some sense to add an option to disable the compatibility hack at all.


We cannot see a way to make the config option both easily discoverable 
and constrained. There is lot of labels that includes `-`, `+` and other 
symbols that will be impacted.


Having a revset prefix would allow some revsets to be parsed with the 
legacy lookup (likely the user inputs) while other could be parsed 
explicitly as revsets (for exampleconfig options like pushrev) to avoid 
any collision with potential labels.



A longer terms solution would be to support configuring constraints on
labels. If tags can be configured to only match a specific pattern we
could skip lookup for data that does not match it very quickly. To be
fully enforced it probably need to a part of the .hg/requires so it is a
heavier change. It also needs to be more than just a hook so that the
lookup logic is aware of it and can fast path it. Such approach work in
our use-case but is also more specific and requires more
configuration(work?).
We could use the same approach as the V2 series proposed here:
https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-February/112087.html

Just curious. Can't we make the name lookup instant?


Sadly no.



However, we think it still makes sense to have an easy and standard way
to force something to be evaluated as a revset as it's not possible as
of today.

Here is a summary of the idea we've discussed so far:

1) Using some kind of prefix, breaking BC for people using the prefix as
a label:

    `revs:rev(42)` → directly evaluated as `rev(42)`
    `"revs":rev(42)` → force 

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

[Yuya Nishihara]

On Thu, 1 Mar 2018 15:48:47 -0500, Feld Boris wrote:
> On 26/02/2018 08:11, Yuya Nishihara wrote:
> > 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 
>  # 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. ;)
> 
> The proposal here is to define a prefix for which we break backward 
> compatibility. If we do so, people with a "" label will have to use:
> 
>    "":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.

> >>> 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.)
> Dropping the older lookup methods seems impractical. Right now, `rev(0)` 
> is a valid tag. So dropping legacy lookup means all these things will 
> have to be heavily quoted: (eg: hg log -r '"version-4(candidate2)"').

Yes. I assume that "foo(bar)" tags or branches wouldn't be used widely.

Since "foo(bar)" needs quotes in revset query (except for x and x:y), it would
makes some sense to add an option to disable the compatibility hack at all.

> A longer terms solution would be to support configuring constraints on 
> labels. If tags can be configured to only match a specific pattern we 
> could skip lookup for data that does not match it very quickly. To be 
> fully enforced it probably need to a part of the .hg/requires so it is a 
> heavier change. It also needs to be more than just a hook so that the 
> lookup logic is aware of it and can fast path it. Such approach work in 
> our use-case but is also more specific and requires more 
> configuration(work?).
> We could use the same approach as the V2 series proposed here: 
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-February/112087.html

Just curious. Can't we make the name lookup instant?

> However, we think it still makes sense to have an easy and standard way 
> to force something to be evaluated as a revset as it's not possible as 
> of today.
> 
> Here is a summary of the idea we've discussed so far:
> 
> 1) Using some kind of prefix, breaking BC for people using the prefix as 
> a label:
> 
>    `revs:rev(42)` → directly evaluated as `rev(42)`
>    `"revs":rev(42)` → force "revs" as a label
> 
> 2) Adding a special revset for revset evaluation.
> 
>    `revset("rev(42)")` → directly evaluated `rev(42)`
> 
> 3) adding strange special case: (leading space, parents).
> 
>    ` rev(42)` → directly evaluated as `rev(42)`
>    `(rev(42))` → directly evaluated as `rev(42)`
> 
> 4) Config flag to disable legacy parsing (force quote for strange labels).
> 
> 5) Full-featured label patterns enforcement as 

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

[Feld Boris]

On 26/02/2018 08:11, Yuya Nishihara wrote:

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


The proposal here is to define a prefix for which we break backward 
compatibility. If we do so, people with a "" label will have to use:


  "":whatever

to get a similar effect.

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.)
Dropping the older lookup methods seems impractical. Right now, `rev(0)` 
is a valid tag. So dropping legacy lookup means all these things will 
have to be heavily quoted: (eg: hg log -r '"version-4(candidate2)"').


(In practice, even `"rev(0)"` is also a valid tag, starting and escaping 
fest).


A longer terms solution would be to support configuring constraints on 
labels. If tags can be configured to only match a specific pattern we 
could skip lookup for data that does not match it very quickly. To be 
fully enforced it probably need to a part of the .hg/requires so it is a 
heavier change. It also needs to be more than just a hook so that the 
lookup logic is aware of it and can fast path it. Such approach work in 
our use-case but is also more specific and requires more 
configuration(work?).
We could use the same approach as the V2 series proposed here: 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-February/112087.html


However, we think it still makes sense to have an easy and standard way 
to force something to be evaluated as a revset as it's not possible as 
of today.


Here is a summary of the idea we've discussed so far:

1) Using some kind of prefix, breaking BC for people using the prefix as 
a label:


  `revs:rev(42)` → directly evaluated as `rev(42)`
  `"revs":rev(42)` → force "revs" as a label

2) Adding a special revset for revset evaluation.

  `revset("rev(42)")` → directly evaluated `rev(42)`

3) adding strange special case: (leading space, parents).

  ` rev(42)` → directly evaluated as `rev(42)`
  `(rev(42))` → directly evaluated as `rev(42)`

4) Config flag to disable legacy parsing (force quote for strange labels).

5) Full-featured label patterns enforcement as described earlier in this 
email.




We agree fast-pathing on (3) is a bit too obscure.
We find (4) to be impractical (and likely to break unexpected stuff).

(5) is a good solution but significantly heavier to write and much more 
specific.


So we would be happy if we could agree on a way to go 

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

[Matt Harbison]

On Mon, 26 Feb 2018 05:45:03 -0500, 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 
# 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.


I meant to chime in the first time I saw this, but this isn't necessarily  
true.  I converted a bzr repo a few weeks ago, and it carried over not  
only trailing spaces, but LF in the middle of the tag name.  That made the  
.hgtags file... entertaining.  That can be fixed, but there might be such  
labels in the wild.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

On Mon, 26 Feb 2018 21:42:01 -0500, Matt Harbison wrote:
> On Mon, 26 Feb 2018 05:45:03 -0500, 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 
> >>> # 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.
> 
> I meant to chime in the first time I saw this, but this isn't necessarily  
> true.  I converted a bzr repo a few weeks ago, and it carried over not  
> only trailing spaces, but LF in the middle of the tag name.  That made the  
> .hgtags file... entertaining.  That can be fixed, but there might be such  
> labels in the wild.

Ugh, I didn't know that, thanks. So we can't assume existing "names" have no
leading/trailing spaces, even though we can enforce that for new ones.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [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 
> >> # 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.)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Feld Boris]

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 
# 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.

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

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.


Cheers,
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

On Mon, 12 Feb 2018 18:00:52 +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld 
> # 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?

Instead, maybe we can make lookup() to not search slow labels assuming these
labeling schemes didn't exist in pre-revset era. Alternatively, we could add
a config knob to switch off the old-style range support.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel