Patchwork [V3] revrange: don't parse revset aliases as hash prefixes (issue4553)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date Feb. 26, 2015, 6:58 p.m.
Message ID <be1e1c998407c4b35c9d.1424977094@Iris>
Download mbox | patch
Permalink /patch/7840/
State Accepted
Headers show

Comments

Jordi Gutiérrez Hermoso - Feb. 26, 2015, 6:58 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1424905921 18000
#      Wed Feb 25 18:12:01 2015 -0500
# Node ID be1e1c998407c4b35c9df52be7ce8f4c87ef417c
# Parent  f15891e7f658fbc0351589887760548cab2c41f8
revrange: don't parse revset aliases as hash prefixes (issue4553)

If a user has a revsetalias defined, it is their explicit wish for
this alias to be parsed as a revset and nothing else. Although the
case of the alias being short enough and only contain the letters a-f
is probably kind of rare, it may still happen.
Yuya Nishihara - Feb. 28, 2015, 3:37 a.m.
On Thu, 26 Feb 2015 13:58:14 -0500, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1424905921 18000
> #      Wed Feb 25 18:12:01 2015 -0500
> # Node ID be1e1c998407c4b35c9df52be7ce8f4c87ef417c
> # Parent  f15891e7f658fbc0351589887760548cab2c41f8
> revrange: don't parse revset aliases as hash prefixes (issue4553)
> 
> If a user has a revsetalias defined, it is their explicit wish for
> this alias to be parsed as a revset and nothing else. Although the
> case of the alias being short enough and only contain the letters a-f
> is probably kind of rare, it may still happen.
> 
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -628,12 +628,22 @@ def revrange(repo, revs):
>          return repo[val].rev()
>  
>      seen, l = set(), revset.baseset([])
> +
> +    revsetaliases = [alias for (alias, _) in
> +                     repo.ui.configitems("revsetalias")]

"_" shadows _() function and could cause problem in future.

>      for spec in revs:
>          if l and not seen:
>              seen = set(l)
>          # attempt to parse old-style ranges first to deal with
>          # things like old-tag which contain query metacharacters
>          try:
> +            # ... except for revset aliases without arguments. These
> +            # should be parsed as soon as possible, because they might
> +            # clash with a hash prefix.
> +            if spec in revsetaliases:
> +                raise error.RepoLookupError
> +
>              if isinstance(spec, int):
>                  seen.add(spec)
>                  l = l + revset.baseset([spec])
> @@ -641,6 +651,9 @@ def revrange(repo, revs):
>  
>              if _revrangesep in spec:
>                  start, end = spec.split(_revrangesep, 1)
> +                if start in revsetaliases or end in revsetaliases:
> +                    raise error.RepoLookupError
> +
>                  start = revfix(repo, start, 0)
>                  end = revfix(repo, end, len(repo) - 1)
>                  if end == nullrev and start < 0:

I don't know which is better, but both "x in revsetliases" checks can be done
by revfix().

Regards,
Yuya Nishihara - Feb. 28, 2015, 9:04 a.m.
On Sat, 28 Feb 2015 12:37:19 +0900, Yuya Nishihara wrote:
> On Thu, 26 Feb 2015 13:58:14 -0500, Jordi Gutiérrez Hermoso wrote:
> >      for spec in revs:
> >          if l and not seen:
> >              seen = set(l)
> >          # attempt to parse old-style ranges first to deal with
> >          # things like old-tag which contain query metacharacters
> >          try:
> > +            # ... except for revset aliases without arguments. These
> > +            # should be parsed as soon as possible, because they might
> > +            # clash with a hash prefix.
> > +            if spec in revsetaliases:
> > +                raise error.RepoLookupError
> > +
> >              if isinstance(spec, int):
> >                  seen.add(spec)
> >                  l = l + revset.baseset([spec])
> > @@ -641,6 +651,9 @@ def revrange(repo, revs):
> >  
> >              if _revrangesep in spec:
> >                  start, end = spec.split(_revrangesep, 1)
> > +                if start in revsetaliases or end in revsetaliases:
> > +                    raise error.RepoLookupError
> > +
> >                  start = revfix(repo, start, 0)
> >                  end = revfix(repo, end, len(repo) - 1)
> >                  if end == nullrev and start < 0:
> 
> I don't know which is better, but both "x in revsetliases" checks can be done
> by revfix().

Oops, my suggestion didn't work because "spec in repo" may raise LookupError.
It seems "spec in repo" is unnecessary, though.
Jordi Gutiérrez Hermoso - March 2, 2015, 6:28 p.m.
On Sat, 2015-02-28 at 18:04 +0900, Yuya Nishihara wrote:
> On Sat, 28 Feb 2015 12:37:19 +0900, Yuya Nishihara wrote:
> > I don't know which is better, but both "x in revsetliases" checks can be done
> > by revfix().
> 
> Oops, my suggestion didn't work because "spec in repo" may raise LookupError.
> It seems "spec in repo" is unnecessary, though.

I don't understand what you're suggesting with revfix. Do you think I
can ignore your suggestion? :-)
Pierre-Yves David - March 2, 2015, 8:57 p.m.
On 02/26/2015 06:58 PM, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1424905921 18000
> #      Wed Feb 25 18:12:01 2015 -0500
> # Node ID be1e1c998407c4b35c9df52be7ce8f4c87ef417c
> # Parent  f15891e7f658fbc0351589887760548cab2c41f8
> revrange: don't parse revset aliases as hash prefixes (issue4553)

Looks like an interesting things to have. Had to look the code a bit to 
convince me that the LookupError raising was not an abomination.

Pushed to the clowncopter.
Yuya Nishihara - March 3, 2015, 1:54 p.m.
On Mon, 02 Mar 2015 13:28:32 -0500, Jordi Gutiérrez Hermoso wrote:
> On Sat, 2015-02-28 at 18:04 +0900, Yuya Nishihara wrote:
> > On Sat, 28 Feb 2015 12:37:19 +0900, Yuya Nishihara wrote:
> > > I don't know which is better, but both "x in revsetliases" checks can be done
> > > by revfix().
> > 
> > Oops, my suggestion didn't work because "spec in repo" may raise LookupError.
> > It seems "spec in repo" is unnecessary, though.
> 
> I don't understand what you're suggesting with revfix. Do you think I
> can ignore your suggestion? :-)

Yes, it's not important. Just it could be written as follows.

+    revsetaliases = [alias for (alias, _definition) in
+                     repo.ui.configitems("revsetalias")]
+
     def revfix(repo, val, defval):
         if not val and val != 0 and defval is not None:
             return defval
+        if val in revsetaliases:
+            raise error.RepoLookupError
         return repo[val].rev()
 
@@ -659,7 +665,7 @@ def revrange(repo, revs):
                     seen = newrevs
                 l = l + revset.baseset(sorted(newrevs, reverse=start > end))
                 continue
-            elif spec and spec in repo: # single unquoted rev
+            elif spec: # single unquoted rev
                 rev = revfix(repo, spec, None)
                 if rev in seen:
                     continue

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -628,12 +628,22 @@  def revrange(repo, revs):
         return repo[val].rev()
 
     seen, l = set(), revset.baseset([])
+
+    revsetaliases = [alias for (alias, _) in
+                     repo.ui.configitems("revsetalias")]
+
     for spec in revs:
         if l and not seen:
             seen = set(l)
         # attempt to parse old-style ranges first to deal with
         # things like old-tag which contain query metacharacters
         try:
+            # ... except for revset aliases without arguments. These
+            # should be parsed as soon as possible, because they might
+            # clash with a hash prefix.
+            if spec in revsetaliases:
+                raise error.RepoLookupError
+
             if isinstance(spec, int):
                 seen.add(spec)
                 l = l + revset.baseset([spec])
@@ -641,6 +651,9 @@  def revrange(repo, revs):
 
             if _revrangesep in spec:
                 start, end = spec.split(_revrangesep, 1)
+                if start in revsetaliases or end in revsetaliases:
+                    raise error.RepoLookupError
+
                 start = revfix(repo, start, 0)
                 end = revfix(repo, end, len(repo) - 1)
                 if end == nullrev and start < 0:
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -1128,6 +1128,62 @@  far away.
   3
   2
 
+issue4553: check that revset aliases override existing hash prefix
+
+  $ hg log -qr e
+  6:e0cc66ef77e8
+
+  $ hg log -qr e --config revsetalias.e="all()"
+  0:2785f51eece5
+  1:d75937da8da0
+  2:5ed5505e9f1c
+  3:8528aa5637f2
+  4:2326846efdab
+  5:904fa392b941
+  6:e0cc66ef77e8
+  7:013af1973af4
+  8:d5d0dcbdc4d9
+  9:24286f4ae135
+
+  $ hg log -qr e: --config revsetalias.e="0"
+  0:2785f51eece5
+  1:d75937da8da0
+  2:5ed5505e9f1c
+  3:8528aa5637f2
+  4:2326846efdab
+  5:904fa392b941
+  6:e0cc66ef77e8
+  7:013af1973af4
+  8:d5d0dcbdc4d9
+  9:24286f4ae135
+
+  $ hg log -qr :e --config revsetalias.e="9"
+  0:2785f51eece5
+  1:d75937da8da0
+  2:5ed5505e9f1c
+  3:8528aa5637f2
+  4:2326846efdab
+  5:904fa392b941
+  6:e0cc66ef77e8
+  7:013af1973af4
+  8:d5d0dcbdc4d9
+  9:24286f4ae135
+
+  $ hg log -qr e:
+  6:e0cc66ef77e8
+  7:013af1973af4
+  8:d5d0dcbdc4d9
+  9:24286f4ae135
+
+  $ hg log -qr :e
+  0:2785f51eece5
+  1:d75937da8da0
+  2:5ed5505e9f1c
+  3:8528aa5637f2
+  4:2326846efdab
+  5:904fa392b941
+  6:e0cc66ef77e8
+
 issue2549 - correct optimizations
 
   $ log 'limit(1 or 2 or 3, 2) and not 2'