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

login
register
mail settings
Submitter Alexander Drozdov
Date April 14, 2015, 5:40 a.m.
Message ID <31e872ad0473ed27cf48.1428990048@km33436-05.keymachine.de>
Download mbox | patch
Permalink /patch/8646/
State Changes Requested
Headers show

Comments

Alexander Drozdov - April 14, 2015, 5:40 a.m.
# HG changeset patch
# User Alexander Drozdov <al.drozdov@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.
Ryan McElroy - April 14, 2015, 5:55 a.m.
On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
> # HG changeset patch
> # User Alexander Drozdov <al.drozdov@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)
> 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@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.
Martin von Zweigbergk - April 14, 2015, 6:02 a.m.
On Mon, Apr 13, 2015 at 10:55 PM Ryan McElroy <rm@fb.com> wrote:

> On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
> > # HG changeset patch
> > # User Alexander Drozdov <al.drozdov@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)
> > 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@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.


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Katsunori FUJIWARA - April 14, 2015, 1:25 p.m.
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@fb.com> wrote:
> 
> > On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
> > > # HG changeset patch
> > > # User Alexander Drozdov <al.drozdov@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.


> > > 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@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@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> >

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Alexander Drozdov - April 14, 2015, 3:15 p.m.
On Tue, 14 Apr 2015 22:25:33 +0900, FUJIWARA Katsunori 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@fb.com> wrote:
>>
>>> On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
>>>> # HG changeset patch
>>>> # User Alexander Drozdov <al.drozdov@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.

Thanks! I'll resend.

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

The repository I'm working on use transplant/graft intensively. I'm trying to
extend the 'default' template by printing 'transplant_source'/'source' extras
with corresponded revision numbers. Some of the revisions stored in extras are
not present in the repository, so I want to print the hexadecimal values only
for them, without numbers. I think getting empty set by calling revset() from
the template is the only way to do that now:
{revset("id(%s)", get(extras, "source"))}. To make it to work right now I
pass 'get(extras, "source")|short', but it looks ugly for me.

>
>
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@selenic.com
>>> http://selenic.com/mailman/listinfo/mercurial-devel
>>>
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
>
Yuya Nishihara - April 14, 2015, 3:30 p.m.
On Tue, 14 Apr 2015 18:15:57 +0300, Alexander Drozdov wrote:
> On Tue, 14 Apr 2015 22:25:33 +0900, FUJIWARA Katsunori wrote:
> > 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.
> 
> The repository I'm working on use transplant/graft intensively. I'm trying to
> extend the 'default' template by printing 'transplant_source'/'source' extras
> with corresponded revision numbers. Some of the revisions stored in extras are
> not present in the repository, so I want to print the hexadecimal values only
> for them, without numbers. I think getting empty set by calling revset() from
> the template is the only way to do that now:
> {revset("id(%s)", get(extras, "source"))}. To make it to work right now I
> pass 'get(extras, "source")|short', but it looks ugly for me.

For now, you can use present() to silence RepoLookupError.
Alexander Drozdov - April 14, 2015, 3:46 p.m.
On Wed, 15 Apr 2015 00:30:32 +0900, Yuya Nishihara wrote:
> On Tue, 14 Apr 2015 18:15:57 +0300, Alexander Drozdov wrote:
>> On Tue, 14 Apr 2015 22:25:33 +0900, FUJIWARA Katsunori wrote:
>>> 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.
>>
>> The repository I'm working on use transplant/graft intensively. I'm trying to
>> extend the 'default' template by printing 'transplant_source'/'source' extras
>> with corresponded revision numbers. Some of the revisions stored in extras are
>> not present in the repository, so I want to print the hexadecimal values only
>> for them, without numbers. I think getting empty set by calling revset() from
>> the template is the only way to do that now:
>> {revset("id(%s)", get(extras, "source"))}. To make it to work right now I
>> pass 'get(extras, "source")|short', but it looks ugly for me.
>
> For now, you can use present() to silence RepoLookupError.

Thanks!
Alexander Drozdov - April 15, 2015, 5:27 a.m.
On 14.04.2015 16:25:33 +0300 FUJIWARA Katsunori <foozy@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@fb.com> wrote:
>>
>>> On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
>>>> # HG changeset patch
>>>> # User Alexander Drozdov <al.drozdov@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@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@selenic.com
>>> http://selenic.com/mailman/listinfo/mercurial-devel
>>>
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
>
Yuya Nishihara - April 15, 2015, 12:29 p.m.
On Wed, 15 Apr 2015 08:27:15 +0300, Alexander Drozdov wrote:
> On 14.04.2015 16:25:33 +0300 FUJIWARA Katsunori <foozy@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@fb.com> wrote:
> >>> On 4/13/2015 10:40 PM, Alexander Drozdov wrote:
> >>>> # HG changeset patch
> >>>> # User Alexander Drozdov <al.drozdov@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?

I prefer ParseError for any non-hexadecimal strings because rev() does for
non-integer strings. I think you can make two patches that will fix a) unknown
40-byte string, then b) parse hexadecimal string strictly.

Regards,

Patch

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