Patchwork [5,of,5] revset: remove unused lookup argument from _tokenizealias()

login
register
mail settings
Submitter Yuya Nishihara
Date March 29, 2016, 3:27 p.m.
Message ID <cf9abced341da9a63ad5.1459265270@mimosa>
Download mbox | patch
Permalink /patch/14143/
State Accepted
Headers show

Comments

Yuya Nishihara - March 29, 2016, 3:27 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1456728392 -32400
#      Mon Feb 29 15:46:32 2016 +0900
# Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
# Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
revset: remove unused lookup argument from _tokenizealias()

Since aliases are defined in configuration file, they should never have tokens
that require a repository-dependent lookup function.

This is purely a clean-up patch. Future patches won't have logical dependency
on this change.
Sean Farley - March 29, 2016, 8:10 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1456728392 -32400
> #      Mon Feb 29 15:46:32 2016 +0900
> # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> revset: remove unused lookup argument from _tokenizealias()
>
> Since aliases are defined in configuration file, they should never have tokens
> that require a repository-dependent lookup function.
>
> This is purely a clean-up patch. Future patches won't have logical dependency
> on this change.

I like the direction of this series!
Pierre-Yves David - March 30, 2016, 12:37 a.m.
On 03/29/2016 08:27 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1456728392 -32400
> #      Mon Feb 29 15:46:32 2016 +0900
> # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> revset: remove unused lookup argument from _tokenizealias()

Pushed, thanks. Very very nice cleanup.

Cheers,
Matt Mackall - April 5, 2016, 6:51 p.m.
On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1456728392 -32400
> #      Mon Feb 29 15:46:32 2016 +0900
> # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> revset: remove unused lookup argument from _tokenizealias()
> 
> Since aliases are defined in configuration file, they should never have tokens
> that require a repository-dependent lookup function.

That's not obvious to me.

It seems that if I have a branch named i-love-hyphens, and I can do:

$ hg log -r "i-love-hyphens% and author(mpm)"

..then I should be able to turn that into a revset alias without having to
change the quoting, right?

Going to drop this one for now.

-- 
Mathematics is the supreme nostalgia of our time.
Yuya Nishihara - April 6, 2016, 12:19 p.m.
On Tue, 05 Apr 2016 13:51:51 -0500, Matt Mackall wrote:
> On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1456728392 -32400
> > #      Mon Feb 29 15:46:32 2016 +0900
> > # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> > # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> > revset: remove unused lookup argument from _tokenizealias()
> > 
> > Since aliases are defined in configuration file, they should never have tokens
> > that require a repository-dependent lookup function.
> 
> That's not obvious to me.
> 
> It seems that if I have a branch named i-love-hyphens, and I can do:
> 
> $ hg log -r "i-love-hyphens% and author(mpm)"
> 
> ..then I should be able to turn that into a revset alias without having to
> change the quoting, right?

Yeah, that should be possible. _tokenizealias() is the tokenizer for alias
definition, which isn't involved while parsing a user input.

This patch just removed an unused argument. There should be no behavior change.
My commit message tries to express why we never ever need "lookup" argument.
Matt Mackall - April 6, 2016, 8:10 p.m.
On Wed, 2016-04-06 at 21:19 +0900, Yuya Nishihara wrote:
> On Tue, 05 Apr 2016 13:51:51 -0500, Matt Mackall wrote:
> > On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1456728392 -32400
> > > #      Mon Feb 29 15:46:32 2016 +0900
> > > # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> > > # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> > > revset: remove unused lookup argument from _tokenizealias()
> > > 
> > > Since aliases are defined in configuration file, they should never have
> > > tokens
> > > that require a repository-dependent lookup function.
> > 
> > That's not obvious to me.
> > 
> > It seems that if I have a branch named i-love-hyphens, and I can do:
> > 
> > $ hg log -r "i-love-hyphens% and author(mpm)"
> > 
> > ..then I should be able to turn that into a revset alias without having to
> > change the quoting, right?
> 
> Yeah, that should be possible. _tokenizealias() is the tokenizer for alias
> definition,

And I'm suggesting that the problematic branch name is -in- the alias. We can't
tokenize the alias correctly without knowing that "i-love-hyphens" is a valid
token.. and that requires a repo containing that token.

> This patch just removed an unused argument. There should be no behavior
> change.

Ok, but if the current behavior is buggy because it should be using the arg,
then it's not really a cleanup.
Yuya Nishihara - April 7, 2016, 12:02 p.m.
On Wed, 06 Apr 2016 15:10:13 -0500, Matt Mackall wrote:
> On Wed, 2016-04-06 at 21:19 +0900, Yuya Nishihara wrote:
> > On Tue, 05 Apr 2016 13:51:51 -0500, Matt Mackall wrote:  
> > > On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:  
> > > > # HG changeset patch
> > > > # User Yuya Nishihara <yuya@tcha.org>
> > > > # Date 1456728392 -32400
> > > > #      Mon Feb 29 15:46:32 2016 +0900
> > > > # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> > > > # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> > > > revset: remove unused lookup argument from _tokenizealias()
> > > > 
> > > > Since aliases are defined in configuration file, they should never have
> > > > tokens
> > > > that require a repository-dependent lookup function.  
> > > 
> > > That's not obvious to me.
> > > 
> > > It seems that if I have a branch named i-love-hyphens, and I can do:
> > > 
> > > $ hg log -r "i-love-hyphens% and author(mpm)"
> > > 
> > > ..then I should be able to turn that into a revset alias without having to
> > > change the quoting, right?  
> > 
> > Yeah, that should be possible. _tokenizealias() is the tokenizer for alias
> > definition,  
> 
> And I'm suggesting that the problematic branch name is -in- the alias. We can't
> tokenize the alias correctly without knowing that "i-love-hyphens" is a valid
> token.. and that requires a repo containing that token.

That means aliases are parsed differently across repositories, which is IMHO
troublesome rather than useful. You'll no longer be able to write a reliable
alias without quotes, e.g. "i"-"love"-"hyphens" since some repo might have
"i-love-hyphens" symbol.
Matt Mackall - April 7, 2016, 7:56 p.m.
On Thu, 2016-04-07 at 21:02 +0900, Yuya Nishihara wrote:
> On Wed, 06 Apr 2016 15:10:13 -0500, Matt Mackall wrote:
> > On Wed, 2016-04-06 at 21:19 +0900, Yuya Nishihara wrote:
> > > On Tue, 05 Apr 2016 13:51:51 -0500, Matt Mackall wrote:  
> > > > On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:  
> > > > > # HG changeset patch
> > > > > # User Yuya Nishihara <yuya@tcha.org>
> > > > > # Date 1456728392 -32400
> > > > > #      Mon Feb 29 15:46:32 2016 +0900
> > > > > # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> > > > > # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> > > > > revset: remove unused lookup argument from _tokenizealias()
> > > > > 
> > > > > Since aliases are defined in configuration file, they should never
> > > > > have
> > > > > tokens
> > > > > that require a repository-dependent lookup function.  
> > > > 
> > > > That's not obvious to me.
> > > > 
> > > > It seems that if I have a branch named i-love-hyphens, and I can do:
> > > > 
> > > > $ hg log -r "i-love-hyphens% and author(mpm)"
> > > > 
> > > > ..then I should be able to turn that into a revset alias without having
> > > > to
> > > > change the quoting, right?  
> > > 
> > > Yeah, that should be possible. _tokenizealias() is the tokenizer for alias
> > > definition,  
> > 
> > And I'm suggesting that the problematic branch name is -in- the alias. We
> > can't
> > tokenize the alias correctly without knowing that "i-love-hyphens" is a
> > valid
> > token.. and that requires a repo containing that token.
> 
> That means aliases are parsed differently across repositories, which is IMHO
> troublesome rather than useful. You'll no longer be able to write a reliable
> alias without quotes, e.g. "i"-"love"-"hyphens" since some repo might have
> "i-love-hyphens" symbol.

Sure.

However, we already know from the volume of complaints for the command line
version that that's not a real issue and that the former is. We had a steady
stream of complaints that i-love-hyphens didn't work in revsets and no one has
ever complained that their shortcut for "a and not b" didn't work because they
had a symbol "a-b".
Yuya Nishihara - April 8, 2016, 4:30 p.m.
On Thu, 07 Apr 2016 14:56:01 -0500, Matt Mackall wrote:
> On Thu, 2016-04-07 at 21:02 +0900, Yuya Nishihara wrote:
> > On Wed, 06 Apr 2016 15:10:13 -0500, Matt Mackall wrote:  
> > > On Wed, 2016-04-06 at 21:19 +0900, Yuya Nishihara wrote:  
> > > > On Tue, 05 Apr 2016 13:51:51 -0500, Matt Mackall wrote:    
> > > > > On Wed, 2016-03-30 at 00:27 +0900, Yuya Nishihara wrote:    
> > > > > > # HG changeset patch
> > > > > > # User Yuya Nishihara <yuya@tcha.org>
> > > > > > # Date 1456728392 -32400
> > > > > > #      Mon Feb 29 15:46:32 2016 +0900
> > > > > > # Node ID cf9abced341da9a63ad5daf49a908d39795aafb5
> > > > > > # Parent  092c90f4dad4e4e5870499819d4c0a2f4646e8fd
> > > > > > revset: remove unused lookup argument from _tokenizealias()
> > > > > > 
> > > > > > Since aliases are defined in configuration file, they should never
> > > > > > have
> > > > > > tokens
> > > > > > that require a repository-dependent lookup function.    
> > > > > 
> > > > > That's not obvious to me.
> > > > > 
> > > > > It seems that if I have a branch named i-love-hyphens, and I can do:
> > > > > 
> > > > > $ hg log -r "i-love-hyphens% and author(mpm)"
> > > > > 
> > > > > ..then I should be able to turn that into a revset alias without having
> > > > > to
> > > > > change the quoting, right?    
> > > > 
> > > > Yeah, that should be possible. _tokenizealias() is the tokenizer for alias
> > > > definition,    
> > > 
> > > And I'm suggesting that the problematic branch name is -in- the alias. We
> > > can't
> > > tokenize the alias correctly without knowing that "i-love-hyphens" is a
> > > valid
> > > token.. and that requires a repo containing that token.  
> > 
> > That means aliases are parsed differently across repositories, which is IMHO
> > troublesome rather than useful. You'll no longer be able to write a reliable
> > alias without quotes, e.g. "i"-"love"-"hyphens" since some repo might have
> > "i-love-hyphens" symbol.  
> 
> Sure.
> 
> However, we already know from the volume of complaints for the command line
> version that that's not a real issue and that the former is. We had a steady
> stream of complaints that i-love-hyphens didn't work in revsets and no one has
> ever complained that their shortcut for "a and not b" didn't work because they
> had a symbol "a-b".

I know that, but I've never heard complaints about revset aliases. And I think
aliases are more complicated because they are stored permanently in hgrc and
user variables are allowed.

  f(a, b) = a-b

That said, this patch has little benefit and it isn't a blocker of my template
alias series. So I agree to drop this.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2266,15 +2266,14 @@  def optimize(x, small):
 _aliassyminitletters = set(c for c in [chr(i) for i in xrange(256)]
                            if c.isalnum() or c in '._@$' or ord(c) > 127)
 
-def _tokenizealias(program, lookup=None):
+def _tokenizealias(program):
     """Parse alias declaration/definition into a stream of tokens
 
     This allows symbol names to use also ``$`` as an initial letter
     (for backward compatibility), and callers of this function should
     examine whether ``$`` is used also for unexpected symbols or not.
     """
-    return tokenize(program, lookup=lookup,
-                    syminitletters=_aliassyminitletters)
+    return tokenize(program, syminitletters=_aliassyminitletters)
 
 def _parsealiasdecl(decl):
     """Parse alias declaration ``decl``