{"thread":{"id":"46383","subject":"[PATCH 0/2] Fix regression: CamelCased aliases","startedAt":"2017-07-14T08:39:38Z","lastAt":"2017-07-15T00:01:44Z","messageCount":10,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","astian"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"324492","messageId":"cover.1500021526.git.johannes.schindelin@gmx.de","threadId":"46383","inReplyTo":null,"subject":"[PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-14T08:39:24Z","receivedAt":"2017-07-14T08:39:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was possible before v2.13.3 to invoke:\n\n\tgit config alias.CamelCased <something>\n\tgit CamelCased\n\nThis regressed (due to a stupid mistake of mine that was not caught in\npatch review, sadly) in v2.13.3.\n\nAnd this patch series fixes it again, introducing a regression test to\nensure that it does not get broken again.\n\n\nJohannes Schindelin (2):\n  t1300: demonstrate that CamelCased aliases regressed\n  alias: compare alias name *case-insensitively*\n\n alias.c                | 2 +-\n t/t1300-repo-config.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\n\nbase-commit: f3da2b79be9565779e4f76dc5812c68e156afdf0\nPublished-As: https://github.com/dscho/git/releases/tag/CamelCased-aliases-v1\nFetch-It-Via: git fetch https://github.com/dscho/git CamelCased-aliases-v1\n-- \n2.13.3.windows.1.13.gaf0c2223da0\n\n"},{"id":"324493","messageId":"79af768e29d878da995df553638b18fc6d7a5190.1500021526.git.johannes.schindelin@gmx.de","threadId":"46383","inReplyTo":"cover.1500021526.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-14T08:39:29Z","receivedAt":"2017-07-14T08:39:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is totally legitimate to add CamelCased aliases, but due to the way\nconfig keys are compared, the case does not matter.\n\nExcept that now it does: the alias name is expected to be all\nlower-case. This is a regression introduced by a9bcf6586d1 (alias: use\nthe early config machinery to expand aliases, 2017-06-14).\n\nNoticed by Alejandro Pauly, diagnosed by Kevin Willford.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t1300-repo-config.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex a37ef042221..0df71b84ccf 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1075,6 +1075,13 @@ test_expect_success 'git -c works with aliases of builtins' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'aliases can be CamelCased' '\n+\ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n+\tgit CamelCased >out &&\n+\tgit rev-parse HEAD >expect &&\n+\ttest_cmp expect out\n+'\n+\n test_expect_success 'git -c does not split values on equals' '\n \techo \"value with = in it\" >expect &&\n \tgit -c core.foo=\"value with = in it\" config core.foo >actual &&\n-- \n2.13.3.windows.1.13.gaf0c2223da0\n\n\n"},{"id":"324494","messageId":"75bfe3653dd65e4219a78cea8e9308060a84af4f.1500021526.git.johannes.schindelin@gmx.de","threadId":"46383","inReplyTo":"cover.1500021526.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] alias: compare alias name *case-insensitively*","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-14T08:39:38Z","receivedAt":"2017-07-14T08:40:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is totally legitimate to add CamelCased aliases, but due to the way\nconfig keys are compared, the case does not matter.\n\nTherefore, we must compare the alias name insensitively to the config\nkeys.\n\nThis fixes a regression introduced by a9bcf6586d1 (alias: use\nthe early config machinery to expand aliases, 2017-06-14).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n alias.c                | 2 +-\n t/t1300-repo-config.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 39f622e4141..bf146e52632 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -11,7 +11,7 @@ static int config_alias_cb(const char *key, const char *value, void *d)\n \tstruct config_alias_data *data = d;\n \tconst char *p;\n \n-\tif (skip_prefix(key, \"alias.\", &p) && !strcmp(p, data->alias))\n+\tif (skip_prefix(key, \"alias.\", &p) && !strcasecmp(p, data->alias))\n \t\treturn git_config_string((const char **)&data->v, key, value);\n \n \treturn 0;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 0df71b84ccf..364a537000b 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1075,7 +1075,7 @@ test_expect_success 'git -c works with aliases of builtins' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'aliases can be CamelCased' '\n+test_expect_success 'aliases can be CamelCased' '\n \ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n \tgit CamelCased >out &&\n \tgit rev-parse HEAD >expect &&\n-- \n2.13.3.windows.1.13.gaf0c2223da0\n"},{"id":"324496","messageId":"20170714090256.ne4gqgppt2qshtak@sigill.intra.peff.net","threadId":"46383","inReplyTo":"cover.1500021526.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T09:02:56Z","receivedAt":"2017-07-14T09:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 14, 2017 at 10:39:24AM +0200, Johannes Schindelin wrote:\n\n> It was possible before v2.13.3 to invoke:\n> \n> \tgit config alias.CamelCased <something>\n> \tgit CamelCased\n> \n> This regressed (due to a stupid mistake of mine that was not caught in\n> patch review, sadly) in v2.13.3.\n\nInteresting. I don't think this was ever intended to work. Prior to\nv2.2.0 it did not, and it was \"fixed\" inadvertently by 111791559\n(alias.c: replace `git_config()` with `git_config_get_string()`,\n2014-08-07).\n\nIt's a known limitation of the alias config scheme that it cannot\ndistinguish between \"camelcase\" and \"CamelCase\" (nor represent commands\nwith underscores or other characters that are invalid in a keyname). In\nthe long run we'd want to allow \"alias.CamelCase.command\" or similar to\nmake this work correctly.\n\nIt probably doesn't hurt anything to make this old ambiguous style work\nin the meantime if people are using it and finding it convenient.\n\n> Johannes Schindelin (2):\n>   t1300: demonstrate that CamelCased aliases regressed\n>   alias: compare alias name *case-insensitively*\n\nThe patches look obviously correct.\n\nAs a meta-comment, I find splitting the tests from the fix like this\nmakes review more tedious. The commit messages have to repeat the exact\nsame reasoning.\n\nThe only value I think it brings is that you can confirm in the\nfirst commit that the expect_failure does indeed fail. I guess that has\nsome value, though I usually do it by running the test against the\nversion of git built from HEAD^.\n\n-Peff\n"},{"id":"324526","messageId":"xmqq60evrqyd.fsf@gitster.mtv.corp.google.com","threadId":"46383","inReplyTo":"20170714090256.ne4gqgppt2qshtak@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-14T15:14:18Z","receivedAt":"2017-07-14T15:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jul 14, 2017 at 10:39:24AM +0200, Johannes Schindelin wrote:\n>\n>> It was possible before v2.13.3 to invoke:\n>> \n>> \tgit config alias.CamelCased <something>\n>> \tgit CamelCased\n>> \n>> This regressed (due to a stupid mistake of mine that was not caught in\n>> patch review, sadly) in v2.13.3.\n>\n> Interesting. I don't think this was ever intended to work.\n> ...\n> The patches look obviously correct.\n\nHow can something be \"(n)ever intended to work\" and yet patches to\nmake it work be \"obviously correct\"? ;-)\n\nMy first/knee-jerk reation to the title of the series also was\n\"letter cases are not supposed to work in aliases\", but that depends\non the definition of \"work\".  When you add 'alias.Foo', you are not\nsupposed to be able to make 'git foo' behave differently from that\nalias you defined.  In order to make that, which is not supposed to\nwork, work, we'd need to introduce alias.Foo.commmand, as you said.\n\nBut I think that it is still reasonable for an end user to expect\nthat 'git Foo' would trigger that alias.  And that is what was\nrecently changed, inadvertently.\n\nSo the problem may need to be explained better in this series, but I\nthink the usage was expected to work and the series is fixing a real\nregression.\n\nDo we want to promise to keep the following \"working\"?\n\n    git config alias.Foo <something>\n    git foo\n\nBy designing the system in such a way that an alias is created with\na two-level name in our system, we are saying that alias names are\ncase insensitive to the end users, so I _think_ the above is\nintended to work, and we are effectively promising that it will keep\nworking.\n\nIt is a different matter if that design decision was sensible,\nthough.\n\n> As a meta-comment, I find splitting the tests from the fix like this\n> makes review more tedious.\n\nI agree.\n"},{"id":"324531","messageId":"20170714153015.crklfqaa2qc573zo@sigill.intra.peff.net","threadId":"46383","inReplyTo":"xmqq60evrqyd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T15:30:16Z","receivedAt":"2017-07-14T15:30:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 14, 2017 at 08:14:18AM -0700, Junio C Hamano wrote:\n\n> >> It was possible before v2.13.3 to invoke:\n> >> \n> >> \tgit config alias.CamelCased <something>\n> >> \tgit CamelCased\n> >> \n> >> This regressed (due to a stupid mistake of mine that was not caught in\n> >> patch review, sadly) in v2.13.3.\n> >\n> > Interesting. I don't think this was ever intended to work.\n> > ...\n> > The patches look obviously correct.\n> \n> How can something be \"(n)ever intended to work\" and yet patches to\n> make it work be \"obviously correct\"? ;-)\n\nYou snipped the part where I said \"this is probably reasonable to do in\nthe meantime.\" :)\n\nMy \"obviously correct\" is only that the patches fulfill that.\n\n> But I think that it is still reasonable for an end user to expect\n> that 'git Foo' would trigger that alias.  And that is what was\n> recently changed, inadvertently.\n> \n> So the problem may need to be explained better in this series, but I\n> think the usage was expected to work and the series is fixing a real\n> regression.\n\nSort of. It did not work until it accidentally did work, and now it\naccidentally does not work again. So \"usage was expected to work\" was\ncertainly not true for Git developers (or at least nobody intentionally\nwrote code to make it so). And anybody relying on it started doing so\nsince v2.2.0.\n\nBut like I said, it's probably reasonable to make it work. There's\nlittle harm in doing so.  The only downside I can see is that doing:\n\n  git config alias.foo <something>\n  git Foo\n\nnow triggers the alias. That seems like at worst a minor bug, and\npossibly even the right thing to do (see below).\n\n> Do we want to promise to keep the following \"working\"?\n> \n>     git config alias.Foo <something>\n>     git foo\n> \n> By designing the system in such a way that an alias is created with\n> a two-level name in our system, we are saying that alias names are\n> case insensitive to the end users, so I _think_ the above is\n> intended to work, and we are effectively promising that it will keep\n> working.\n\nYes, I think we must. Keys are case-insensitive, and you are allowed to\nwrite them in whatever case you like. The more interesting case is the\nreverse, that I showed above. I think there are basically two mental\nmodels that are reasonable:\n\n  1. Uppercase in key names is treated the same as lowercase. Therefore\n     we must allow \"alias.Foo\" to match \"git foo\", but \"git Foo\" can\n     never have a match (in the current schema).\n\n  2. Keys are case-insensitive, and anything that matches them is\n     considered case-insensitive, too. That means \"Foo\" and \"foo\" are\n     identical for these purposes, and you can never have two aliases\n     \"Foo\" and \"foo\".\n\nIn either mental model, \"alias.Foo\" for \"git foo\" must work. But the\nreverse only works in (2).\n\nI think either model is fine. These patches push us into (2).\n\n-Peff\n"},{"id":"324536","messageId":"xmqqh8yfq8pr.fsf@gitster.mtv.corp.google.com","threadId":"46383","inReplyTo":"20170714153015.crklfqaa2qc573zo@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-14T16:33:36Z","receivedAt":"2017-07-14T16:33:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think there are basically two mental models that are reasonable:\n>\n>   1. Uppercase in key names is treated the same as lowercase. Therefore\n>      we must allow \"alias.Foo\" to match \"git foo\", but \"git Foo\" can\n>      never have a match (in the current schema).\n>\n>   2. Keys are case-insensitive, and anything that matches them is\n>      considered case-insensitive, too. That means \"Foo\" and \"foo\" are\n>      identical for these purposes, and you can never have two aliases\n>      \"Foo\" and \"foo\".\n>\n> In either mental model, \"alias.Foo\" for \"git foo\" must work. But the\n> reverse only works in (2).\n>\n> I think either model is fine. These patches push us into (2).\n\nI've always thought that the promise we give our end users is that\nthese keys are case insensitive, and that the fact that we downcase\nthe key before calling config callback is merely an implementation\ndetail.  That is why I never considered the possibility that (1) can\nbe a valid mental model.\n\nThere are other possible implementations of case insensitivity.  We\ncould have been upcasing instead before calling config callback and\nthe users wouldn't have noticed.\n\nSo I'd consider that pushing us into (2) is a good thing.\n\nThanks.\n"},{"id":"324538","messageId":"1d79b017-38b3-e1cb-a4bf-1153dc4fdbca@eclipso.at","threadId":"46383","inReplyTo":"xmqqh8yfq8pr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"astian","fromEmail":"astian@eclipso.at","sentAt":"2017-07-14T17:26:00Z","receivedAt":"2017-07-14T17:27:08Z","isPatch":true,"sender":{"key":"astian@eclipso.at","avatar":null},"body":"FWIW, I don't like 2, I don't like the irregularity in the invocation:\n\n  $ git branch\n  * master\n  $ git BRANCH\n  git: 'BRANCH' is not a git command. See 'git --help'.\n  $ git config alias.br 'branch -v'\n  $ git br\n  * master 51c785c initial\n  $ git BR\n  * master 51c785c initial\n\nThere is also this:\n\n  $ git branch\n  * master\n  $ git BRANCH\n  git: 'BRANCH' is not a git command. See 'git --help'.\n  $ git config alias.branch 'branch -v'\n  $ git branch\n  * master\n  $ git BRANCH\n  * master 51c785c initial\n\n\n"},{"id":"324541","messageId":"20170714174002.xjizkofjxlirgnaj@sigill.intra.peff.net","threadId":"46383","inReplyTo":"1d79b017-38b3-e1cb-a4bf-1153dc4fdbca@eclipso.at","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T17:40:02Z","receivedAt":"2017-07-14T17:40:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 14, 2017 at 05:26:00PM +0000, astian wrote:\n\n> FWIW, I don't like 2, I don't like the irregularity in the invocation:\n\nWithout quoting, it took me a second to figure out what you meant. But I\nthink \"2\" here is the \"mental model 2\" I mentioned in my earlier email.\n\n>   $ git branch\n>   * master\n>   $ git BRANCH\n>   git: 'BRANCH' is not a git command. See 'git --help'.\n>   $ git config alias.br 'branch -v'\n>   $ git br\n>   * master 51c785c initial\n>   $ git BR\n>   * master 51c785c initial\n> \n> There is also this:\n> \n>   $ git branch\n>   * master\n>   $ git BRANCH\n>   git: 'BRANCH' is not a git command. See 'git --help'.\n>   $ git config alias.branch 'branch -v'\n>   $ git branch\n>   * master\n>   $ git BRANCH\n>   * master 51c785c initial\n\nThat is an interesting side effect, especially the latter BRANCH/branch\none. We usually do not allow overrides of actual git commands, but this\n\"fools\" that check.\n\nI agree it's an unexpected fallout. On the other hand, unless you are\n_trying_ to do something funny, I don't think you'd ever hit on this\nbehavior. And if you are trying to do something funny, I think this\nbehaves in a reasonable and predictable manner.\n\n-Peff\n"},{"id":"324565","messageId":"xmqqzic6pnz4.fsf@gitster.mtv.corp.google.com","threadId":"46383","inReplyTo":"20170714174002.xjizkofjxlirgnaj@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Fix regression: CamelCased aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-15T00:01:35Z","receivedAt":"2017-07-15T00:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> There is also this:\n>> \n>>   $ git branch\n>>   * master\n>>   $ git BRANCH\n>>   git: 'BRANCH' is not a git command. See 'git --help'.\n>>   $ git config alias.branch 'branch -v'\n>>   $ git branch\n>>   * master\n>>   $ git BRANCH\n>>   * master 51c785c initial\n>\n> That is an interesting side effect, especially the latter BRANCH/branch\n> one. We usually do not allow overrides of actual git commands, but this\n> \"fools\" that check.\n\nThis one is tricky.  An obvious fix would be, because users expect\nthat their aliases are case insensitive, the rule that forbids and\nignores an alias that masks a real git subcommand should also apply\ncase insensitively.  But that opens another can of worms.  If \"git\nBranch\" is forbidden to be an alias, it should not just fail but\ninvoke \"git branch\", which would mean that Git subcommands should\nalso be looked up case insensitively!  I am not sure if we want to\ngo there.\n\n> I agree it's an unexpected fallout. On the other hand, unless you are\n> _trying_ to do something funny, I don't think you'd ever hit on this\n> behavior. And if you are trying to do something funny, I think this\n> behaves in a reasonable and predictable manner.\n\nOK.\n"}]}