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

login
register
mail settings
Submitter Alexander Drozdov
Date April 13, 2015, 6:11 a.m.
Message ID <c2e3c9bce437a01584e4.1428905462@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/8627/
State Changes Requested
Headers show

Comments

Alexander Drozdov - April 13, 2015, 6:11 a.m.
# HG changeset patch
# User Alexander Drozdov <al.drozdov@gmail.com>
# Date 1428905039 -10800
#      Mon Apr 13 09:03:59 2015 +0300
# Node ID c2e3c9bce437a01584e4c32469c0f5d0fcf566d7
# 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.
Yuya Nishihara - April 13, 2015, 2:04 p.m.
On Mon, 13 Apr 2015 09:11:02 +0300, Alexander Drozdov wrote:
> # HG changeset patch
> # User Alexander Drozdov <al.drozdov@gmail.com>
> # Date 1428905039 -10800
> #      Mon Apr 13 09:03:59 2015 +0300
> # Node ID c2e3c9bce437a01584e4c32469c0f5d0fcf566d7
> # 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 --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -1292,13 +1292,10 @@ def node_(repo, subset, x):
>      l = getargs(x, 1, 1, _("id requires one argument"))
>      # i18n: "id" is a keyword
>      n = getstring(l[0], _("id requires a string"))
> -    if len(n) == 40:
> -        rn = repo[n].rev()
> -    else:
> -        rn = None
> -        pm = repo.changelog._partialmatch(n)
> -        if pm is not None:
> -            rn = repo.changelog.rev(pm)
> +    rn = None
> +    pm = repo.changelog._partialmatch(n)
> +    if pm is not None:
> +        rn = repo.changelog.rev(pm)

I think "len(n) == 40" is necessary to avoid slow _partialmatch().

FWIW, this patch also fixes the bug of 40-byte tag or bookmark.

  $ hg tag xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
  $ hg log -r 'id(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx)'

Regards,
Martin von Zweigbergk - April 13, 2015, 2:39 p.m.
Would "if len(n) == 40 and n in repo:" be better?

On Mon, Apr 13, 2015, 07:06 Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 13 Apr 2015 09:11:02 +0300, Alexander Drozdov wrote:
> > # HG changeset patch
> > # User Alexander Drozdov <al.drozdov@gmail.com>
> > # Date 1428905039 -10800
> > #      Mon Apr 13 09:03:59 2015 +0300
> > # Node ID c2e3c9bce437a01584e4c32469c0f5d0fcf566d7
> > # 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 --git a/mercurial/revset.py b/mercurial/revset.py
> > --- a/mercurial/revset.py
> > +++ b/mercurial/revset.py
> > @@ -1292,13 +1292,10 @@ def node_(repo, subset, x):
> >      l = getargs(x, 1, 1, _("id requires one argument"))
> >      # i18n: "id" is a keyword
> >      n = getstring(l[0], _("id requires a string"))
> > -    if len(n) == 40:
> > -        rn = repo[n].rev()
> > -    else:
> > -        rn = None
> > -        pm = repo.changelog._partialmatch(n)
> > -        if pm is not None:
> > -            rn = repo.changelog.rev(pm)
> > +    rn = None
> > +    pm = repo.changelog._partialmatch(n)
> > +    if pm is not None:
> > +        rn = repo.changelog.rev(pm)
>
> I think "len(n) == 40" is necessary to avoid slow _partialmatch().
>
> FWIW, this patch also fixes the bug of 40-byte tag or bookmark.
>
>   $ hg tag xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>   $ hg log -r 'id(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx)'
>
> Regards,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - April 13, 2015, 3:33 p.m.
On Mon, 13 Apr 2015 14:39:07 +0000, Martin von Zweigbergk wrote:
> Would "if len(n) == 40 and n in repo:" be better?

"n in repo" just does "try repo[n] except RepoLookupError", so I'd rather do
"try rn = repo[n].rev() except ...".

It would be nice if revlog or changelog had the function to look up node
only by full or partial nodeid, but I'm not sure how it should be implemented.
I think foozy had thoughts around it.

Regards,
Alexander Drozdov - April 13, 2015, 6:32 p.m.
On Tue, 14 Apr 2015 00:33:55 +0900, Yuya Nishihara <yuya@tcha.org> wrote:
> On Mon, 13 Apr 2015 14:39:07 +0000, Martin von Zweigbergk wrote:
>> Would "if len(n) == 40 and n in repo:" be better?
>
> "n in repo" just does "try repo[n] except RepoLookupError", so I'd rather do
> "try rn = repo[n].rev() except ...".

I'll rewrite the patch, thanks!

Also, my current patch don't not work in pure Python as revlog's _partialmatch()
don't try to match 40-byte identifiers.

> It would be nice if revlog or changelog had the function to look up node
> only by full or partial nodeid, but I'm not sure how it should be implemented.
> I think foozy had thoughts around it.
>
> Regards,
>

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1292,13 +1292,10 @@  def node_(repo, subset, x):
     l = getargs(x, 1, 1, _("id requires one argument"))
     # i18n: "id" is a keyword
     n = getstring(l[0], _("id requires a string"))
-    if len(n) == 40:
-        rn = repo[n].rev()
-    else:
-        rn = None
-        pm = repo.changelog._partialmatch(n)
-        if pm is not None:
-            rn = repo.changelog.rev(pm)
+    rn = None
+    pm = repo.changelog._partialmatch(n)
+    if pm is not None:
+        rn = repo.changelog.rev(pm)
 
     if rn is None:
         return baseset()
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -554,6 +554,21 @@  Test explicit numeric revision
   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