[PATCH 0/2] Fix regression: CamelCased aliases

[PATCH 0/2] Fix regression: CamelCased aliases

[Johannes Schindelin]

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

[PATCH 1/2] t1300: demonstrate that CamelCased aliases regressed

[Johannes Schindelin]

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

[PATCH 2/2] alias: compare alias name *case-insensitively*

[Johannes Schindelin]

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

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Jeff King]

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

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Junio C Hamano]

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.

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Jeff King]

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

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Junio C Hamano]

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.

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[astian]

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

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Jeff King]

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

Re: [PATCH 0/2] Fix regression: CamelCased aliases

[Junio C Hamano]

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.