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