* [PATCH 0/2] Fix regression: CamelCased aliases @ 2017-07-14 8:39 Johannes Schindelin 2017-07-14 8:39 ` [PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed Johannes Schindelin ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Johannes Schindelin @ 2017-07-14 8:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano It was possible before v2.13.3 to invoke: git config alias.CamelCased <something> git CamelCased This regressed (due to a stupid mistake of mine that was not caught in patch review, sadly) in v2.13.3. And this patch series fixes it again, introducing a regression test to ensure that it does not get broken again. Johannes Schindelin (2): t1300: demonstrate that CamelCased aliases regressed alias: compare alias name *case-insensitively* alias.c | 2 +- t/t1300-repo-config.sh | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) base-commit: f3da2b79be9565779e4f76dc5812c68e156afdf0 Published-As: https://github.com/dscho/git/releases/tag/CamelCased-aliases-v1 Fetch-It-Via: git fetch https://github.com/dscho/git CamelCased-aliases-v1 -- 2.13.3.windows.1.13.gaf0c2223da0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed 2017-07-14 8:39 [PATCH 0/2] Fix regression: CamelCased aliases Johannes Schindelin @ 2017-07-14 8:39 ` Johannes Schindelin 2017-07-14 8:39 ` [PATCH 2/2] alias: compare alias name *case-insensitively* Johannes Schindelin 2017-07-14 9:02 ` [PATCH 0/2] Fix regression: CamelCased aliases Jeff King 2 siblings, 0 replies; 10+ messages in thread From: Johannes Schindelin @ 2017-07-14 8:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano It is totally legitimate to add CamelCased aliases, but due to the way config keys are compared, the case does not matter. Except that now it does: the alias name is expected to be all lower-case. This is a regression introduced by a9bcf6586d1 (alias: use the early config machinery to expand aliases, 2017-06-14). Noticed by Alejandro Pauly, diagnosed by Kevin Willford. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t1300-repo-config.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index a37ef042221..0df71b84ccf 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1075,6 +1075,13 @@ test_expect_success 'git -c works with aliases of builtins' ' test_cmp expect actual ' +test_expect_failure 'aliases can be CamelCased' ' + test_config alias.CamelCased "rev-parse HEAD" && + git CamelCased >out && + git rev-parse HEAD >expect && + test_cmp expect out +' + test_expect_success 'git -c does not split values on equals' ' echo "value with = in it" >expect && git -c core.foo="value with = in it" config core.foo >actual && -- 2.13.3.windows.1.13.gaf0c2223da0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] alias: compare alias name *case-insensitively* 2017-07-14 8:39 [PATCH 0/2] Fix regression: CamelCased aliases Johannes Schindelin 2017-07-14 8:39 ` [PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed Johannes Schindelin @ 2017-07-14 8:39 ` Johannes Schindelin 2017-07-14 9:02 ` [PATCH 0/2] Fix regression: CamelCased aliases Jeff King 2 siblings, 0 replies; 10+ messages in thread From: Johannes Schindelin @ 2017-07-14 8:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano It is totally legitimate to add CamelCased aliases, but due to the way config keys are compared, the case does not matter. Therefore, we must compare the alias name insensitively to the config keys. This fixes a regression introduced by a9bcf6586d1 (alias: use the early config machinery to expand aliases, 2017-06-14). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- alias.c | 2 +- t/t1300-repo-config.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/alias.c b/alias.c index 39f622e4141..bf146e52632 100644 --- a/alias.c +++ b/alias.c @@ -11,7 +11,7 @@ static int config_alias_cb(const char *key, const char *value, void *d) struct config_alias_data *data = d; const char *p; - if (skip_prefix(key, "alias.", &p) && !strcmp(p, data->alias)) + if (skip_prefix(key, "alias.", &p) && !strcasecmp(p, data->alias)) return git_config_string((const char **)&data->v, key, value); return 0; diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 0df71b84ccf..364a537000b 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1075,7 +1075,7 @@ test_expect_success 'git -c works with aliases of builtins' ' test_cmp expect actual ' -test_expect_failure 'aliases can be CamelCased' ' +test_expect_success 'aliases can be CamelCased' ' test_config alias.CamelCased "rev-parse HEAD" && git CamelCased >out && git rev-parse HEAD >expect && -- 2.13.3.windows.1.13.gaf0c2223da0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 8:39 [PATCH 0/2] Fix regression: CamelCased aliases Johannes Schindelin 2017-07-14 8:39 ` [PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed Johannes Schindelin 2017-07-14 8:39 ` [PATCH 2/2] alias: compare alias name *case-insensitively* Johannes Schindelin @ 2017-07-14 9:02 ` Jeff King 2017-07-14 15:14 ` Junio C Hamano 2 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2017-07-14 9:02 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Junio C Hamano On Fri, Jul 14, 2017 at 10:39:24AM +0200, Johannes Schindelin wrote: > It was possible before v2.13.3 to invoke: > > git config alias.CamelCased <something> > git CamelCased > > This regressed (due to a stupid mistake of mine that was not caught in > patch review, sadly) in v2.13.3. Interesting. I don't think this was ever intended to work. Prior to v2.2.0 it did not, and it was "fixed" inadvertently by 111791559 (alias.c: replace `git_config()` with `git_config_get_string()`, 2014-08-07). It's a known limitation of the alias config scheme that it cannot distinguish between "camelcase" and "CamelCase" (nor represent commands with underscores or other characters that are invalid in a keyname). In the long run we'd want to allow "alias.CamelCase.command" or similar to make this work correctly. It probably doesn't hurt anything to make this old ambiguous style work in the meantime if people are using it and finding it convenient. > Johannes Schindelin (2): > t1300: demonstrate that CamelCased aliases regressed > alias: compare alias name *case-insensitively* The patches look obviously correct. As a meta-comment, I find splitting the tests from the fix like this makes review more tedious. The commit messages have to repeat the exact same reasoning. The only value I think it brings is that you can confirm in the first commit that the expect_failure does indeed fail. I guess that has some value, though I usually do it by running the test against the version of git built from HEAD^. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 9:02 ` [PATCH 0/2] Fix regression: CamelCased aliases Jeff King @ 2017-07-14 15:14 ` Junio C Hamano 2017-07-14 15:30 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2017-07-14 15:14 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Schindelin, git Jeff King <peff@peff.net> writes: > On Fri, Jul 14, 2017 at 10:39:24AM +0200, Johannes Schindelin wrote: > >> It was possible before v2.13.3 to invoke: >> >> git config alias.CamelCased <something> >> git CamelCased >> >> This regressed (due to a stupid mistake of mine that was not caught in >> patch review, sadly) in v2.13.3. > > Interesting. I don't think this was ever intended to work. > ... > The patches look obviously correct. How can something be "(n)ever intended to work" and yet patches to make it work be "obviously correct"? ;-) My first/knee-jerk reation to the title of the series also was "letter cases are not supposed to work in aliases", but that depends on the definition of "work". When you add 'alias.Foo', you are not supposed to be able to make 'git foo' behave differently from that alias you defined. In order to make that, which is not supposed to work, work, we'd need to introduce alias.Foo.commmand, as you said. But I think that it is still reasonable for an end user to expect that 'git Foo' would trigger that alias. And that is what was recently changed, inadvertently. So the problem may need to be explained better in this series, but I think the usage was expected to work and the series is fixing a real regression. Do we want to promise to keep the following "working"? git config alias.Foo <something> git foo By designing the system in such a way that an alias is created with a two-level name in our system, we are saying that alias names are case insensitive to the end users, so I _think_ the above is intended to work, and we are effectively promising that it will keep working. It is a different matter if that design decision was sensible, though. > As a meta-comment, I find splitting the tests from the fix like this > makes review more tedious. I agree. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 15:14 ` Junio C Hamano @ 2017-07-14 15:30 ` Jeff King 2017-07-14 16:33 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2017-07-14 15:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git On Fri, Jul 14, 2017 at 08:14:18AM -0700, Junio C Hamano wrote: > >> It was possible before v2.13.3 to invoke: > >> > >> git config alias.CamelCased <something> > >> git CamelCased > >> > >> This regressed (due to a stupid mistake of mine that was not caught in > >> patch review, sadly) in v2.13.3. > > > > Interesting. I don't think this was ever intended to work. > > ... > > The patches look obviously correct. > > How can something be "(n)ever intended to work" and yet patches to > make it work be "obviously correct"? ;-) You snipped the part where I said "this is probably reasonable to do in the meantime." :) My "obviously correct" is only that the patches fulfill that. > But I think that it is still reasonable for an end user to expect > that 'git Foo' would trigger that alias. And that is what was > recently changed, inadvertently. > > So the problem may need to be explained better in this series, but I > think the usage was expected to work and the series is fixing a real > regression. Sort of. It did not work until it accidentally did work, and now it accidentally does not work again. So "usage was expected to work" was certainly not true for Git developers (or at least nobody intentionally wrote code to make it so). And anybody relying on it started doing so since v2.2.0. But like I said, it's probably reasonable to make it work. There's little harm in doing so. The only downside I can see is that doing: git config alias.foo <something> git Foo now triggers the alias. That seems like at worst a minor bug, and possibly even the right thing to do (see below). > Do we want to promise to keep the following "working"? > > git config alias.Foo <something> > git foo > > By designing the system in such a way that an alias is created with > a two-level name in our system, we are saying that alias names are > case insensitive to the end users, so I _think_ the above is > intended to work, and we are effectively promising that it will keep > working. Yes, I think we must. Keys are case-insensitive, and you are allowed to write them in whatever case you like. The more interesting case is the reverse, that I showed above. I think there are basically two mental models that are reasonable: 1. Uppercase in key names is treated the same as lowercase. Therefore we must allow "alias.Foo" to match "git foo", but "git Foo" can never have a match (in the current schema). 2. Keys are case-insensitive, and anything that matches them is considered case-insensitive, too. That means "Foo" and "foo" are identical for these purposes, and you can never have two aliases "Foo" and "foo". In either mental model, "alias.Foo" for "git foo" must work. But the reverse only works in (2). I think either model is fine. These patches push us into (2). -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 15:30 ` Jeff King @ 2017-07-14 16:33 ` Junio C Hamano 2017-07-14 17:26 ` astian 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2017-07-14 16:33 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Schindelin, git Jeff King <peff@peff.net> writes: > I think there are basically two mental models that are reasonable: > > 1. Uppercase in key names is treated the same as lowercase. Therefore > we must allow "alias.Foo" to match "git foo", but "git Foo" can > never have a match (in the current schema). > > 2. Keys are case-insensitive, and anything that matches them is > considered case-insensitive, too. That means "Foo" and "foo" are > identical for these purposes, and you can never have two aliases > "Foo" and "foo". > > In either mental model, "alias.Foo" for "git foo" must work. But the > reverse only works in (2). > > I think either model is fine. These patches push us into (2). I've always thought that the promise we give our end users is that these keys are case insensitive, and that the fact that we downcase the key before calling config callback is merely an implementation detail. That is why I never considered the possibility that (1) can be a valid mental model. There are other possible implementations of case insensitivity. We could have been upcasing instead before calling config callback and the users wouldn't have noticed. So I'd consider that pushing us into (2) is a good thing. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 16:33 ` Junio C Hamano @ 2017-07-14 17:26 ` astian 2017-07-14 17:40 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: astian @ 2017-07-14 17:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git FWIW, I don't like 2, I don't like the irregularity in the invocation: $ git branch * master $ git BRANCH git: 'BRANCH' is not a git command. See 'git --help'. $ git config alias.br 'branch -v' $ git br * master 51c785c initial $ git BR * master 51c785c initial There is also this: $ git branch * master $ git BRANCH git: 'BRANCH' is not a git command. See 'git --help'. $ git config alias.branch 'branch -v' $ git branch * master $ git BRANCH * master 51c785c initial ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 17:26 ` astian @ 2017-07-14 17:40 ` Jeff King 2017-07-15 0:01 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2017-07-14 17:40 UTC (permalink / raw) To: astian; +Cc: Junio C Hamano, Johannes Schindelin, git On Fri, Jul 14, 2017 at 05:26:00PM +0000, astian wrote: > FWIW, I don't like 2, I don't like the irregularity in the invocation: Without quoting, it took me a second to figure out what you meant. But I think "2" here is the "mental model 2" I mentioned in my earlier email. > $ git branch > * master > $ git BRANCH > git: 'BRANCH' is not a git command. See 'git --help'. > $ git config alias.br 'branch -v' > $ git br > * master 51c785c initial > $ git BR > * master 51c785c initial > > There is also this: > > $ git branch > * master > $ git BRANCH > git: 'BRANCH' is not a git command. See 'git --help'. > $ git config alias.branch 'branch -v' > $ git branch > * master > $ git BRANCH > * master 51c785c initial That is an interesting side effect, especially the latter BRANCH/branch one. We usually do not allow overrides of actual git commands, but this "fools" that check. I agree it's an unexpected fallout. On the other hand, unless you are _trying_ to do something funny, I don't think you'd ever hit on this behavior. And if you are trying to do something funny, I think this behaves in a reasonable and predictable manner. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix regression: CamelCased aliases 2017-07-14 17:40 ` Jeff King @ 2017-07-15 0:01 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2017-07-15 0:01 UTC (permalink / raw) To: Jeff King; +Cc: astian, Johannes Schindelin, git Jeff King <peff@peff.net> writes: >> There is also this: >> >> $ git branch >> * master >> $ git BRANCH >> git: 'BRANCH' is not a git command. See 'git --help'. >> $ git config alias.branch 'branch -v' >> $ git branch >> * master >> $ git BRANCH >> * master 51c785c initial > > That is an interesting side effect, especially the latter BRANCH/branch > one. We usually do not allow overrides of actual git commands, but this > "fools" that check. This one is tricky. An obvious fix would be, because users expect that their aliases are case insensitive, the rule that forbids and ignores an alias that masks a real git subcommand should also apply case insensitively. But that opens another can of worms. If "git Branch" is forbidden to be an alias, it should not just fail but invoke "git branch", which would mean that Git subcommands should also be looked up case insensitively! I am not sure if we want to go there. > I agree it's an unexpected fallout. On the other hand, unless you are > _trying_ to do something funny, I don't think you'd ever hit on this > behavior. And if you are trying to do something funny, I think this > behaves in a reasonable and predictable manner. OK. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-07-15 0:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-14 8:39 [PATCH 0/2] Fix regression: CamelCased aliases Johannes Schindelin 2017-07-14 8:39 ` [PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed Johannes Schindelin 2017-07-14 8:39 ` [PATCH 2/2] alias: compare alias name *case-insensitively* Johannes Schindelin 2017-07-14 9:02 ` [PATCH 0/2] Fix regression: CamelCased aliases Jeff King 2017-07-14 15:14 ` Junio C Hamano 2017-07-14 15:30 ` Jeff King 2017-07-14 16:33 ` Junio C Hamano 2017-07-14 17:26 ` astian 2017-07-14 17:40 ` Jeff King 2017-07-15 0:01 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).