Re: [PATCH 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
On 18/01/2019 13:26, Yuya Nishihara wrote: >> On Thu, Jan 17, 2019, 11:32 Boris FELD wrote: >>> On 13/01/2019 10:12, Yuya Nishihara wrote: On Sun, 13 Jan 2019 08:37:47 +0100, Boris FELD wrote: > On 12/01/2019 05:04, Yuya Nishihara wrote: >> On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: >>> # HG changeset patch >>> # User Boris Feld >>> # Date 1547130238 -3600 >>> # Thu Jan 10 15:23:58 2019 +0100 >>> # Node ID 38733dd8559578267617514a42f253efabb6 >>> # Parent 427247e84e29c144321d21a825d371458b5d3e1a >>> # EXP-Topic revs-efficiency >>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ >>> -r 38733dd85595 >>> revset: enforce "%d" to be interpreted as literal revision number >>> (API) >> New behavior looks saner. Please also flag this as (BC). It's exposed >>> as >> revset() template function. >> >>> %r = revset expression, parenthesized >>> -%d = int(arg), no quoting >>> +%d = rev(int(arg)), no quoting >> 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. >> Suppose it's pretty much a coding error to pass in an invalid revision >>> to >> repo.revs(), we'll probably want aborts. >> >> Maybe we'll need an internal '_rev()' function? > %ld silently pass on these too, so I would rather have %ld and %d > consistent here (and align on the silence %ld behavior). Indeed. I never thought %ld would behave in such way. Consistent behavior should be better. >>> Through some buggy revset usage in evolve we discovered that the >>> optimization to passthrough input directly for %ld changed this >>> behavior. In these cases, the revisions are passed as is and >>> filtered/out-of-bound revision raise their usual error. >>> >>> Reintroducing the silent filtering behavior for %ld seems possible (it >>> will have a performance impact, but still faster than the previous >>> serialization). >>> >>> However, it is probably more efficient and saner to have these errors >>> raised. In this case, we should align all %ld case and %d behaviors on >>> the error raising version. What do you think? > On Thu, 17 Jan 2019 12:36:50 -0800, Martin von Zweigbergk wrote: >> Makes sense to me. That's how it would work on the command line too. > +1 I dunno if we should go the whole way and manually check that every single input is valid, or if we should just forward the faulty input and let lower level user crash as expected? Doing the manual is more thoughtful, but also more expensive and will defeat revset laziness in multiple cases. Since this is mostly an internal API, I feel like the "forward and let other complains" is better. However as a side effect, it means that invalid inputs might go unnoticed because they are filtered by the previous subset: For example → "1000:2000 and %ld" % [1500, 99], with 999 being invalid will simply return [1500] as 99 is not included in 1000:2000 In the 5.0 cycle, we could try to have some better tracking of "sane" inputs that don't need filtering and the one who does. We can also improve the performance impact of this filtering. Series coming to implement the "simple" approach. It is anyway a pre-requires for the more strict approach. > ___ > 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 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
> On Thu, Jan 17, 2019, 11:32 Boris FELD wrote: > > On 13/01/2019 10:12, Yuya Nishihara wrote: > > > On Sun, 13 Jan 2019 08:37:47 +0100, Boris FELD wrote: > > >> On 12/01/2019 05:04, Yuya Nishihara wrote: > > >>> On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: > > # HG changeset patch > > # User Boris Feld > > # Date 1547130238 -3600 > > # Thu Jan 10 15:23:58 2019 +0100 > > # Node ID 38733dd8559578267617514a42f253efabb6 > > # Parent 427247e84e29c144321d21a825d371458b5d3e1a > > # EXP-Topic revs-efficiency > > # Available At https://bitbucket.org/octobus/mercurial-devel/ > > # hg pull https://bitbucket.org/octobus/mercurial-devel/ > > -r 38733dd85595 > > revset: enforce "%d" to be interpreted as literal revision number > > (API) > > >>> New behavior looks saner. Please also flag this as (BC). It's exposed > > as > > >>> revset() template function. > > >>> > > %r = revset expression, parenthesized > > -%d = int(arg), no quoting > > +%d = rev(int(arg)), no quoting > > >>> 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. > > >>> Suppose it's pretty much a coding error to pass in an invalid revision > > to > > >>> repo.revs(), we'll probably want aborts. > > >>> > > >>> Maybe we'll need an internal '_rev()' function? > > >> %ld silently pass on these too, so I would rather have %ld and %d > > >> consistent here (and align on the silence %ld behavior). > > > Indeed. I never thought %ld would behave in such way. Consistent behavior > > > should be better. > > > > Through some buggy revset usage in evolve we discovered that the > > optimization to passthrough input directly for %ld changed this > > behavior. In these cases, the revisions are passed as is and > > filtered/out-of-bound revision raise their usual error. > > > > Reintroducing the silent filtering behavior for %ld seems possible (it > > will have a performance impact, but still faster than the previous > > serialization). > > > > However, it is probably more efficient and saner to have these errors > > raised. In this case, we should align all %ld case and %d behaviors on > > the error raising version. What do you think? On Thu, 17 Jan 2019 12:36:50 -0800, Martin von Zweigbergk wrote: > Makes sense to me. That's how it would work on the command line too. +1 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
Makes sense to me. That's how it would work on the command line too. On Thu, Jan 17, 2019, 11:32 Boris FELD wrote: > > On 13/01/2019 10:12, Yuya Nishihara wrote: > > On Sun, 13 Jan 2019 08:37:47 +0100, Boris FELD wrote: > >> On 12/01/2019 05:04, Yuya Nishihara wrote: > >>> On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: > # HG changeset patch > # User Boris Feld > # Date 1547130238 -3600 > # Thu Jan 10 15:23:58 2019 +0100 > # Node ID 38733dd8559578267617514a42f253efabb6 > # Parent 427247e84e29c144321d21a825d371458b5d3e1a > # EXP-Topic revs-efficiency > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull https://bitbucket.org/octobus/mercurial-devel/ > -r 38733dd85595 > revset: enforce "%d" to be interpreted as literal revision number > (API) > >>> New behavior looks saner. Please also flag this as (BC). It's exposed > as > >>> revset() template function. > >>> > %r = revset expression, parenthesized > -%d = int(arg), no quoting > +%d = rev(int(arg)), no quoting > >>> 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. > >>> Suppose it's pretty much a coding error to pass in an invalid revision > to > >>> repo.revs(), we'll probably want aborts. > >>> > >>> Maybe we'll need an internal '_rev()' function? > >> %ld silently pass on these too, so I would rather have %ld and %d > >> consistent here (and align on the silence %ld behavior). > > Indeed. I never thought %ld would behave in such way. Consistent behavior > > should be better. > > Through some buggy revset usage in evolve we discovered that the > optimization to passthrough input directly for %ld changed this > behavior. In these cases, the revisions are passed as is and > filtered/out-of-bound revision raise their usual error. > > Reintroducing the silent filtering behavior for %ld seems possible (it > will have a performance impact, but still faster than the previous > serialization). > > However, it is probably more efficient and saner to have these errors > raised. In this case, we should align all %ld case and %d behaviors on > the error raising version. What do you think? > > I'll try to get a patch out tomorrow to align things in a direction or > the other. > > ___ > > 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 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
On 13/01/2019 10:12, Yuya Nishihara wrote: > On Sun, 13 Jan 2019 08:37:47 +0100, Boris FELD wrote: >> On 12/01/2019 05:04, Yuya Nishihara wrote: >>> On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: # HG changeset patch # User Boris Feld # Date 1547130238 -3600 # Thu Jan 10 15:23:58 2019 +0100 # Node ID 38733dd8559578267617514a42f253efabb6 # Parent 427247e84e29c144321d21a825d371458b5d3e1a # EXP-Topic revs-efficiency # Available At https://bitbucket.org/octobus/mercurial-devel/ # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 38733dd85595 revset: enforce "%d" to be interpreted as literal revision number (API) >>> New behavior looks saner. Please also flag this as (BC). It's exposed as >>> revset() template function. >>> %r = revset expression, parenthesized -%d = int(arg), no quoting +%d = rev(int(arg)), no quoting >>> 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. >>> Suppose it's pretty much a coding error to pass in an invalid revision to >>> repo.revs(), we'll probably want aborts. >>> >>> Maybe we'll need an internal '_rev()' function? >> %ld silently pass on these too, so I would rather have %ld and %d >> consistent here (and align on the silence %ld behavior). > Indeed. I never thought %ld would behave in such way. Consistent behavior > should be better. Through some buggy revset usage in evolve we discovered that the optimization to passthrough input directly for %ld changed this behavior. In these cases, the revisions are passed as is and filtered/out-of-bound revision raise their usual error. Reintroducing the silent filtering behavior for %ld seems possible (it will have a performance impact, but still faster than the previous serialization). However, it is probably more efficient and saner to have these errors raised. In this case, we should align all %ld case and %d behaviors on the error raising version. What do you think? I'll try to get a patch out tomorrow to align things in a direction or the other. > ___ > 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 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
On Sun, 13 Jan 2019 08:37:47 +0100, Boris FELD wrote: > > On 12/01/2019 05:04, Yuya Nishihara wrote: > > On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: > >> # HG changeset patch > >> # User Boris Feld > >> # Date 1547130238 -3600 > >> # Thu Jan 10 15:23:58 2019 +0100 > >> # Node ID 38733dd8559578267617514a42f253efabb6 > >> # Parent 427247e84e29c144321d21a825d371458b5d3e1a > >> # EXP-Topic revs-efficiency > >> # Available At https://bitbucket.org/octobus/mercurial-devel/ > >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r > >> 38733dd85595 > >> revset: enforce "%d" to be interpreted as literal revision number (API) > > New behavior looks saner. Please also flag this as (BC). It's exposed as > > revset() template function. > > > >> %r = revset expression, parenthesized > >> -%d = int(arg), no quoting > >> +%d = rev(int(arg)), no quoting > > 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. > > Suppose it's pretty much a coding error to pass in an invalid revision to > > repo.revs(), we'll probably want aborts. > > > > Maybe we'll need an internal '_rev()' function? > %ld silently pass on these too, so I would rather have %ld and %d > consistent here (and align on the silence %ld behavior). Indeed. I never thought %ld would behave in such way. Consistent behavior should be better. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
On 12/01/2019 05:04, Yuya Nishihara wrote: > On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: >> # HG changeset patch >> # User Boris Feld >> # Date 1547130238 -3600 >> # Thu Jan 10 15:23:58 2019 +0100 >> # Node ID 38733dd8559578267617514a42f253efabb6 >> # Parent 427247e84e29c144321d21a825d371458b5d3e1a >> # EXP-Topic revs-efficiency >> # Available At https://bitbucket.org/octobus/mercurial-devel/ >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r >> 38733dd85595 >> revset: enforce "%d" to be interpreted as literal revision number (API) > New behavior looks saner. Please also flag this as (BC). It's exposed as > revset() template function. > >> %r = revset expression, parenthesized >> -%d = int(arg), no quoting >> +%d = rev(int(arg)), no quoting > 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. > Suppose it's pretty much a coding error to pass in an invalid revision to > repo.revs(), we'll probably want aborts. > > Maybe we'll need an internal '_rev()' function? %ld silently pass on these too, so I would rather have %ld and %d consistent here (and align on the silence %ld behavior). > >> %s = string(arg), escaped and single-quoted >> %b = arg.branch(), escaped and single-quoted >> %n = hex(arg), single-quoted > We might want to map %n to _node() as well, but that isn't required in this > series. Yeah, good point. I'll keep this series focused on %d/%ld but will submit a follow up once this is in. > ___ > 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 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)
On Fri, 11 Jan 2019 12:29:06 +0100, Boris Feld wrote: > # HG changeset patch > # User Boris Feld > # Date 1547130238 -3600 > # Thu Jan 10 15:23:58 2019 +0100 > # Node ID 38733dd8559578267617514a42f253efabb6 > # Parent 427247e84e29c144321d21a825d371458b5d3e1a > # EXP-Topic revs-efficiency > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r > 38733dd85595 > revset: enforce "%d" to be interpreted as literal revision number (API) New behavior looks saner. Please also flag this as (BC). It's exposed as revset() template function. > %r = revset expression, parenthesized > -%d = int(arg), no quoting > +%d = rev(int(arg)), no quoting 'rev(n)' returns an empty set if n is out of range, whereas 'n' aborts. Suppose it's pretty much a coding error to pass in an invalid revision to repo.revs(), we'll probably want aborts. Maybe we'll need an internal '_rev()' function? > %s = string(arg), escaped and single-quoted > %b = arg.branch(), escaped and single-quoted > %n = hex(arg), single-quoted We might want to map %n to _node() as well, but that isn't required in this series. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
