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

[Yuya Nishihara]

On Wed, 15 Apr 2015 08:27:15 +0300, Alexander Drozdov wrote:
> 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?

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,