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

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

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,

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.

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? :-)

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.

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