[PATCH 4 of 8] revset: enforce "%d" to be interpreted as literal revision number (API)

[Boris FELD]

On 18/01/2019 13:26, Yuya Nishihara wrote:
>> On Thu, Jan 17, 2019, 11:32 Boris FELD <boris.feld at octobus.net> 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 <boris.feld at octobus.net>
>>>>>>> # Date 1547130238 -3600
>>>>>>> #      Thu Jan 10 15:23:58 2019 +0100
>>>>>>> # Node ID 38733dd85595782676175141111a42f253efabb6
>>>>>>> # 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, 999999], with 9999999 being invalid will
simply return [1500] as 999999 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel