[PATCH V2] revset: make id() to not abort on an unknown 40-byte string

[Alexander Drozdov]

On 14.04.2015 16:25:33 +0300 FUJIWARA Katsunori <foozy at lares.dti.ne.jp> wrote:
>
> At Tue, 14 Apr 2015 06:02:40 +0000,
> Martin von Zweigbergk wrote:
>>
>> On Mon, Apr 13, 2015 at 10:55 PM Ryan McElroy <rm at fb.com> wrote:
>>
>>> On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
>>>> # HG changeset patch
>>>> # User Alexander Drozdov <al.drozdov at gmail.com>
>>>> # Date 1428988858 -10800
>>>> #      Tue Apr 14 08:20:58 2015 +0300
>>>> # Node ID 31e872ad0473ed27cf48b518fee28f34049a9698
>>>> # Parent  52ff737c63d2b2cb41185549aa9c35bc47317032
>>>> revset: make id() to not abort on an unknown 40-byte string
>>>>
>>>> Instead, just return an empty set in the case as for less-then-40-byte
>>>> strings.
>>>>
>>>> diff -r 52ff737c63d2 -r 31e872ad0473 mercurial/revset.py
>>>> --- a/mercurial/revset.py     Thu Apr 09 16:18:38 2015 -0400
>>>> +++ b/mercurial/revset.py     Tue Apr 14 08:20:58 2015 +0300
>>>> @@ -1293,7 +1293,10 @@
>>>>        # i18n: "id" is a keyword
>>>>        n = getstring(l[0], _("id requires a string"))
>>>>        if len(n) == 40:
>>>> -        rn = repo[n].rev()
>>>> +        try:
>>>> +            rn = repo[n].rev()
>>>> +        except error.RepoLookupError:
>>>> +            rn = None
>>>>        else:
>>>>            rn = None
>>>>            pm = repo.changelog._partialmatch(n)
>
> I think that "changelog.rev()" should be used instead of "repo[n]",
> because the latter causes some redundant examinations before and after
> looking up by "40-byte hash" (= "if len(changeid) == 40" block) in
> "changectx.__init__()".
>
>      http://selenic.com/repo/hg/file/52ff737c63d2/mercurial/context.py#l372
>
> The former also avoids that a name (bookmarks/tags/branches) in
> 40-byte is accepted accidentally, as Yuya described at previous post.
>
> BTW, TypeError raised by "node.bin(n)" for "changelog.rev()" has to be
> caught and re-raised as ParserError like other parameter checking.

For now, "id(zzz)" just returns the empty set. Isn't be reasonable to not
re-raise TypeError for a 40-byte non-hexadecimal string but to return the
empty set too?

>
>
>>>> diff -r 52ff737c63d2 -r 31e872ad0473 tests/test-revset.t
>>>> --- a/tests/test-revset.t     Thu Apr 09 16:18:38 2015 -0400
>>>> +++ b/tests/test-revset.t     Tue Apr 14 08:20:58 2015 +0300
>>>> @@ -554,6 +554,21 @@
>>>>      hg: parse error: rev expects a number
>>>>      [255]
>>>>
>>>> +Test hexadecimal revision
>>>> +  $ log 'id(2)'
>>>> +  abort: 00changelog.i at 2: ambiguous identifier!
>>>> +  [255]
>>>> +  $ log 'id(23268)'
>>>> +  4
>>>> +  $ log 'id(2785f51eece)'
>>>> +  0
>>>> +  $ log 'id(d5d0dcbdc4d9ff5dbb2d336f32f0bb561c1a532c)'
>>>> +  8
>>>> +  $ log 'id(d5d0dcbdc4a)'
>>>> +  $ log 'id(d5d0dcbdc4w)'
>>>> +  $ log 'id(d5d0dcbdc4d9ff5dbb2d336f32f0bb561c1a532d)'
>>>> +  $ log 'id(d5d0dcbdc4d9ff5dbb2d336f32f0bb561c1a532q)'
>>>> +
>>>>    Test null revision
>>>>      $ log '(null)'
>>>>      -1
>>>>
>>> This is definitely one of the cases where I'd love to see the
>>> before-to-after behavior diff in the test. That being said, this change
>>> seems fine to me.
>>>
>>> It still seems a little weird that an ambiguous identifier is a hard
>>> error (abort [255]) but a totally missing identifier is not an error at
>>> all, but I'm not sure if there's anything we can or should do about that.
>>>
>>
>> I agree that it is a little weird, but I also don't know what to do about
>> it. I think hidden revs get reported before this change but I think they're
>> silent after. That also seems weird, but at least consistently weird.
>
> Alexander's work seems to be reversed approach of mine (I tried to
> make "id()" and "rev()" abort for hidden/unknown revision).
>
>    http://selenic.com/pipermail/mercurial-devel/2015-March/067894.html
>    http://selenic.com/pipermail/mercurial-devel/2015-March/067897.html
>
>  From the point of view of "equivalence between id() and rev()",
> returning empty set for unknown/hidden revision seems reasonable.
>
>
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel at selenic.com
>>> http://selenic.com/mailman/listinfo/mercurial-devel
>>>
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp
>