Patchwork revset: fix resolving strings from a list

login
register
mail settings
Submitter Durham Goode
Date Sept. 1, 2015, 11:53 p.m.
Message ID <d4ca5e9300bd7a689834.1441151598@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/10364/
State Accepted
Headers show

Comments

Durham Goode - Sept. 1, 2015, 11:53 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1441151165 25200
#      Tue Sep 01 16:46:05 2015 -0700
# Node ID d4ca5e9300bd7a689834ff1158bf9fada625028e
# Parent  049005de325ea400893f45bd6221215cc9b26db0
revset: fix resolving strings from a list

When using multiple revsets that get optimized into a list (like
hg log -r r1235 -r r1237 in hgsubversion), the revset list code was assuming the
strings were resolvable via repo[X]. hgsubversion and other extensions override
def stringset() to allow processing different revision identifiers (such as
r1235 or g<githash>), and there for the _list() implementation was circumventing
that resolution.

The fix is to just call stringset(). The default implementaiton does the same
thing that _list was already doing (namely repo[X]).

This has always been broken, but it was recently exposed by 4ee4f7415095 which
made "--rev X --rev Y" produce a combined revset "X | Y".
Yuya Nishihara - Sept. 2, 2015, 2:41 p.m.
On Tue, 1 Sep 2015 16:53:18 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1441151165 25200
> #      Tue Sep 01 16:46:05 2015 -0700
> # Node ID d4ca5e9300bd7a689834ff1158bf9fada625028e
> # Parent  049005de325ea400893f45bd6221215cc9b26db0
> revset: fix resolving strings from a list
> 
> When using multiple revsets that get optimized into a list (like
> hg log -r r1235 -r r1237 in hgsubversion), the revset list code was assuming the
> strings were resolvable via repo[X]. hgsubversion and other extensions override
> def stringset() to allow processing different revision identifiers (such as
> r1235 or g<githash>), and there for the _list() implementation was circumventing
> that resolution.
> 
> The fix is to just call stringset(). The default implementaiton does the same
> thing that _list was already doing (namely repo[X]).
> 
> This has always been broken, but it was recently exposed by 4ee4f7415095 which
> made "--rev X --rev Y" produce a combined revset "X | Y".
> 
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2067,14 +2067,17 @@ def _list(repo, subset, x):
>              r = int(t)
>              if str(r) != t or r not in cl:
>                  raise ValueError
> +            revs = [r]
>          except ValueError:
> -            r = repo[t].rev()
> -        if r in seen:
> -            continue
> -        if (r in subset
> -            or r == node.nullrev and isinstance(subset, fullreposet)):
> -            ls.append(r)
> -        seen.add(r)
> +            revs = stringset(repo, subset, t)
> +
> +        for r in revs:
> +            if r in seen:
> +                continue
> +            if (r in subset
> +                or r == node.nullrev and isinstance(subset, fullreposet)):
> +                ls.append(r)
> +            seen.add(r)

It is slightly slower because stringset() have to create a baseset object for
every single revision.

% echo "null$(hg log -r 0:1000 -T '+{node}')" | ./contrib/revsetbenchmarks.py

   plain
0) 0.033776
1) 0.035655 105%
Matt Mackall - Sept. 2, 2015, 8:16 p.m.
On Tue, 2015-09-01 at 16:53 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1441151165 25200
> #      Tue Sep 01 16:46:05 2015 -0700
> # Node ID d4ca5e9300bd7a689834ff1158bf9fada625028e
> # Parent  049005de325ea400893f45bd6221215cc9b26db0
> revset: fix resolving strings from a list

I've queued this, thanks. We can address the minor perf regression in a
follow-up.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2067,14 +2067,17 @@  def _list(repo, subset, x):
             r = int(t)
             if str(r) != t or r not in cl:
                 raise ValueError
+            revs = [r]
         except ValueError:
-            r = repo[t].rev()
-        if r in seen:
-            continue
-        if (r in subset
-            or r == node.nullrev and isinstance(subset, fullreposet)):
-            ls.append(r)
-        seen.add(r)
+            revs = stringset(repo, subset, t)
+
+        for r in revs:
+            if r in seen:
+                continue
+            if (r in subset
+                or r == node.nullrev and isinstance(subset, fullreposet)):
+                ls.append(r)
+            seen.add(r)
     return baseset(ls)
 
 # for internal use