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

[Yuya Nishihara]

On Thu, 16 Apr 2015 19:24:48 +0300, Alexander Drozdov wrote:
> On Thu, 16 Apr 2015 22:07:27 +0900, Yuya Nishihara wrote:
> > On Thu, 16 Apr 2015 07:49:15 +0300, Alexander Drozdov wrote:
> >> # HG changeset patch
> >> # User Alexander Drozdov <al.drozdov at gmail.com>
> >> # Date 1429159436 -10800
> >> #      Thu Apr 16 07:43:56 2015 +0300
> >> # Node ID a4bd33c69fc54357e5002bb320f7abd762010e37
> >> # Parent  c560d8c687916cb70a6d54c2c9ddcb5c9e457be2
> >> revset: make id() to not abort on an unknown 40-byte hexadecimal string
> >>
> >> Instead, just return the empty set in the case as for less-then-40-byte
> >> strings.
> >>
> >> diff --git a/mercurial/revset.py b/mercurial/revset.py
> >> --- a/mercurial/revset.py
> >> +++ b/mercurial/revset.py
> >> @@ -1294,7 +1294,13 @@ def node_(repo, subset, x):
> >>       # i18n: "id" is a keyword
> >>       n = getstring(l[0], _("id requires a string"))
> >>       if len(n) == 40:
> >> -        rn = repo[n].rev()
> >> +        try:
> >> +            rn = repo.changelog.rev(node.bin(n))
> >> +        except (TypeError):
> >> +            # i18n: "id" is a keyword
> >> +            raise error.ParseError(_("id expects a hexadecimal string"))
> >
> > If we raise ParseError here, short non-hexadecimal hash should also be handled
> > the same way. But node.bin() can't be applied to odd-length hex. My suggestion
> > was to make separate patches so that the first one, no abort on unknown hash,
> > would likely be accepted.
> 
> I think extra checks for strings shorter than 40 characters here may cause a
> performance penalty as partialmatch() also do the checks. Instead, I think it is
> better to raise (actually to just re-raise) an exception from partialmatch() to
> just catch it here. C and pure python partialmatch() code should be changed.

I'm not sure if partialmatch() should be changed that way. And I guess the
extra checks would be much cheaper compared to partialmatch().

> That change also requires to review all the code calling partialmatch(). And I
> thing that change is what should be done later by a separate patch, isn't it?

Yes, by a separate patch, So for now, I think ParseError shouldn't be raised
at all.

This patch (partially) addresses the following issues.

 a. id(unknown_full_hash) aborts, but id(unknown_short_hash) doesn't
 b. id(40byte_tag_or_bookmark) returns tagged/bookmarked revision
 c. id(non_hexadecimal_string) should raise ParseError just like
    rev(non_integer) does?

Perhaps (a) and (b) will be accepted for stable, but I don't think (c) will.
I think (c) is correct, but it's only my opinion.

Regards,