| 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 <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!
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,
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.
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.
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.
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.
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".
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``