[PATCH] pretty: find pretty formats case-insensitively

[PATCH] pretty: find pretty formats case-insensitively

[Brian Lyles]

User-defined pretty formats are stored in config, which is meant to use
case-insensitive matching for names as noted in config.txt's 'Syntax'
section:

    All the other lines [...] are recognized as setting variables, in
    the form 'name = value' [...]. The variable names are
    case-insensitive, [...].

When a user specifies one of their format aliases with an uppercase in
it, however, it is not found.

    $ git config pretty.testAlias %h
    $ git config --list | grep pretty
    pretty.testalias=%h
    $ git log --format=testAlias -1
    fatal: invalid --pretty format: testAlias
    $ git log --format=testalias -1
    3c2a3fdc38

This is true whether the name in the config file uses any uppercase
characters or not.

Normalize the format name specified via `--format` to lowercase so that
format aliases are found case-insensitively. The format aliases loaded
from config against which this name is compared are already normalized
to lowercase since they are loaded through `git_config()`.

`xstrdup_tolower` is used instead of modifying the string in-place to
ensure that the error shown to the user when the format is not found has
the same casing that the user entered. Otherwise, the mismatch may be
confusing to the user.

Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 pretty.c                      | 12 +++++++++++-
 t/t4205-log-pretty-formats.sh |  7 +++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index cf964b060c..78ec7a75ff 100644
--- a/pretty.c
+++ b/pretty.c
@@ -168,10 +168,20 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
 
 static struct cmt_fmt_map *find_commit_format(const char *sought)
 {
+	struct cmt_fmt_map *result;
+	char *sought_lower;
+
 	if (!commit_formats)
 		setup_commit_formats();
 
-	return find_commit_format_recursive(sought, sought, 0);
+	/*
+	 * The sought name will be compared to config names that have already
+	 * been normalized to lowercase.
+	 */
+	sought_lower = xstrdup_tolower(sought);
+	result = find_commit_format_recursive(sought_lower, sought_lower, 0);
+	free(sought_lower);
+	return result;
 }
 
 void get_commit_format(const char *arg, struct rev_info *rev)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e3d655e6b8..321e305979 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -59,6 +59,13 @@ test_expect_success 'alias user-defined format' '
 	test_cmp expected actual
 '
 
+test_expect_success 'alias user-defined format is matched case-insensitively' '
+	git log --pretty="format:%h" >expected &&
+	git config pretty.testalias "format:%h" &&
+	git log --pretty=testAlias >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '
 	git config i18n.logOutputEncoding $test_encoding &&
 	git log --oneline >expected-s &&
-- 
2.43.2

Re: [PATCH] pretty: find pretty formats case-insensitively

[Jeff King]

On Sun, Mar 24, 2024 at 04:43:09PM -0500, Brian Lyles wrote:

> User-defined pretty formats are stored in config, which is meant to use
> case-insensitive matching for names as noted in config.txt's 'Syntax'
> section:
> 
>     All the other lines [...] are recognized as setting variables, in
>     the form 'name = value' [...]. The variable names are
>     case-insensitive, [...].
> 
> When a user specifies one of their format aliases with an uppercase in
> it, however, it is not found.
> 
>     $ git config pretty.testAlias %h
>     $ git config --list | grep pretty
>     pretty.testalias=%h
>     $ git log --format=testAlias -1
>     fatal: invalid --pretty format: testAlias
>     $ git log --format=testalias -1
>     3c2a3fdc38

Yeah, I agree that case-insensitive matching makes more sense here due
to the nature of config keys, especially given this:

> This is true whether the name in the config file uses any uppercase
> characters or not.

I.e., the config code is going to normalize the variable names already,
so we must match (even if the user consistently specifies camelCase).

But...

>  static struct cmt_fmt_map *find_commit_format(const char *sought)
>  {
> +	struct cmt_fmt_map *result;
> +	char *sought_lower;
> +
>  	if (!commit_formats)
>  		setup_commit_formats();
>  
> -	return find_commit_format_recursive(sought, sought, 0);
> +	/*
> +	 * The sought name will be compared to config names that have already
> +	 * been normalized to lowercase.
> +	 */
> +	sought_lower = xstrdup_tolower(sought);
> +	result = find_commit_format_recursive(sought_lower, sought_lower, 0);
> +	free(sought_lower);
> +	return result;
>  }

The mention of "recursive" in the function we call made me what wonder
if we'd need more normalization. And I think we do. Try this
modification to your test:

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 321e305979..be549b1d4b 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '
 
 test_expect_success 'alias user-defined format is matched case-insensitively' '
 	git log --pretty="format:%h" >expected &&
-	git config pretty.testalias "format:%h" &&
-	git log --pretty=testAlias >actual &&
+	git config pretty.testone "format:%h" &&
+	git config pretty.testtwo testOne &&
+	git log --pretty=testTwo >actual &&
 	test_cmp expected actual
 '
 

which fails because looking up "testOne" in the recursion won't work. So
I think we'd want to simply match case-insensitively inside the
function, like:

diff --git a/pretty.c b/pretty.c
index 50825c9d25..10f71ee004 100644
--- a/pretty.c
+++ b/pretty.c
@@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
 	for (i = 0; i < commit_formats_len; i++) {
 		size_t match_len;
 
-		if (!starts_with(commit_formats[i].name, sought))
+		if (!istarts_with(commit_formats[i].name, sought))
 			continue;
 
 		match_len = strlen(commit_formats[i].name);

And then you would not even need to normalize it in
find_commit_format().

> +test_expect_success 'alias user-defined format is matched case-insensitively' '
> +	git log --pretty="format:%h" >expected &&
> +	git config pretty.testalias "format:%h" &&
> +	git log --pretty=testAlias >actual &&
> +	test_cmp expected actual
> +'

Modern style would be to use "test_config" here (or just "git -c"), but
I see the surrounding tests are too old to do so. So I'd be OK with
matching them (but cleaning up all of the surrounding ones would be
nice, too).

-Peff

PS The matching rules in find_commit_format_recursive() seem weird
   to me. We do a prefix match, and then return the entry whose name is
   the shortest? And break ties based on which came first? So:

     git -c pretty.abcd=format:one \
         -c pretty.abc=format:two \
         -c pretty.abd=format:three \
	 log -1 --format=ab

   quietly chooses "two". I guess the "shortest wins" is meant to allow
   "foo" to be chosen over "foobar" if you specify the whole name. But
   the fact that we don't flag an ambiguity between "abc" and "abd"
   seems strange.

   That is all orthogonal to your patch, of course, but just a
   head-scratcher I noticed while looking at the code.

Re: [PATCH] pretty: find pretty formats case-insensitively

[Brian Lyles]

Hi Peff

Thanks for the review.

On Mon, Mar 25, 2024 at 1:14 AM Jeff King <peff@peff.net> wrote:

> The mention of "recursive" in the function we call made me what wonder
> if we'd need more normalization. And I think we do. Try this
> modification to your test:
> 
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 321e305979..be549b1d4b 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '
>  
>  test_expect_success 'alias user-defined format is matched case-insensitively' '
>  	git log --pretty="format:%h" >expected &&
> -	git config pretty.testalias "format:%h" &&
> -	git log --pretty=testAlias >actual &&
> +	git config pretty.testone "format:%h" &&
> +	git config pretty.testtwo testOne &&
> +	git log --pretty=testTwo >actual &&
>  	test_cmp expected actual
>  '
>  
> 
> which fails because looking up "testOne" in the recursion won't work. So
> I think we'd want to simply match case-insensitively inside the
> function, like:
> 
> diff --git a/pretty.c b/pretty.c
> index 50825c9d25..10f71ee004 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
>  	for (i = 0; i < commit_formats_len; i++) {
>  		size_t match_len;
>  
> -		if (!starts_with(commit_formats[i].name, sought))
> +		if (!istarts_with(commit_formats[i].name, sought))
>  			continue;
>  
>  		match_len = strlen(commit_formats[i].name);
> 
> And then you would not even need to normalize it in
> find_commit_format().

Good catch -- you're absolutely right, and simply switching to
`istarts_with` is a more elegant solution than my initial patch. I'll
switch to this approach in a v2 re-roll.

>> +test_expect_success 'alias user-defined format is matched case-insensitively' '
>> +	git log --pretty="format:%h" >expected &&
>> +	git config pretty.testalias "format:%h" &&
>> +	git log --pretty=testAlias >actual &&
>> +	test_cmp expected actual
>> +'
> 
> Modern style would be to use "test_config" here (or just "git -c"), but
> I see the surrounding tests are too old to do so. So I'd be OK with
> matching them (but cleaning up all of the surrounding ones would be
> nice, too).

Thanks for the tip. Updating the existing tests in this file to use
`test_config` looks to be fairly trivial, so I will start v2 with a
patch that does that as well. I'm opting for `test_config` over `git -c`
for no real reason other than they seem roughly equivalent, but
`test_config` still ends up calling `git config` which seems slightly
more realistic to how pretty formats would be defined normally.

> PS The matching rules in find_commit_format_recursive() seem weird
>    to me. We do a prefix match, and then return the entry whose name is
>    the shortest? And break ties based on which came first? So:
> 
>      git -c pretty.abcd=format:one \
>          -c pretty.abc=format:two \
>          -c pretty.abd=format:three \
> 	 log -1 --format=ab
> 
>    quietly chooses "two". I guess the "shortest wins" is meant to allow
>    "foo" to be chosen over "foobar" if you specify the whole name. But
>    the fact that we don't flag an ambiguity between "abc" and "abd"
>    seems strange.
> 
>    That is all orthogonal to your patch, of course, but just a
>    head-scratcher I noticed while looking at the code.

I agree that this behavior is somewhat odd. I'm not sure what we would
want to do about it at this point -- any change would technically be
breaking, I assume. Regardless, not something I'd scope into this patch,
but good observation.

-- 
Thank you,
Brian Lyles

Re: [PATCH] pretty: find pretty formats case-insensitively

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The mention of "recursive" in the function we call made me what wonder
> if we'd need more normalization. And I think we do. Try this
> modification to your test:
>
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 321e305979..be549b1d4b 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '
>  
>  test_expect_success 'alias user-defined format is matched case-insensitively' '
>  	git log --pretty="format:%h" >expected &&
> -	git config pretty.testalias "format:%h" &&
> -	git log --pretty=testAlias >actual &&
> +	git config pretty.testone "format:%h" &&
> +	git config pretty.testtwo testOne &&
> +	git log --pretty=testTwo >actual &&
>  	test_cmp expected actual
>  '

Very good thinking.  I totally missed the short-cut to another
short-cut while reading the patch.

>> +test_expect_success 'alias user-defined format is matched case-insensitively' '
>> +	git log --pretty="format:%h" >expected &&
>> +	git config pretty.testalias "format:%h" &&
>> +	git log --pretty=testAlias >actual &&
>> +	test_cmp expected actual
>> +'
>
> Modern style would be to use "test_config" here (or just "git -c"), but
> I see the surrounding tests are too old to do so. So I'd be OK with
> matching them (but cleaning up all of the surrounding ones would be
> nice, too).

Yup.  I do not mind seeing it done either way, as a preliminary
clean-up before the main fix, just a fix with more modern style
while leaving the clean-up as #leftoverbits to be done after the
dust settles.

> PS The matching rules in find_commit_format_recursive() seem weird
>    to me. We do a prefix match, and then return the entry whose name is
>    the shortest? And break ties based on which came first? So:
>
>      git -c pretty.abcd=format:one \
>          -c pretty.abc=format:two \
>          -c pretty.abd=format:three \
> 	 log -1 --format=ab
>
>    quietly chooses "two". I guess the "shortest wins" is meant to allow
>    "foo" to be chosen over "foobar" if you specify the whole name. But
>    the fact that we don't flag an ambiguity between "abc" and "abd"
>    seems strange.
>    That is all orthogonal to your patch, of course, but just a
>    head-scratcher I noticed while looking at the code.

I think it is not just strange but outright wrong.  I agree that it
is orthogonal to this fix.

Thanks, both.

[PATCH v2 1/2] pretty: update tests to use `test_config`

[Brian Lyles]

These tests use raw `git config` calls, which is an older style that can
cause config to bleed between tests if not manually unset. `test_config`
ensures that config is unset at the end of each test automatically.

`test_config` is chosen over `git -c` since `test_config` still ends up
calling `git config` which seems slightly more realistic to how pretty
formats would be defined normally.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e3d655e6b8..20bba76c43 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -30,40 +30,38 @@ test_expect_success 'set up basic repos' '
 	>bar &&
 	git add foo &&
 	test_tick &&
-	git config i18n.commitEncoding $test_encoding &&
+	test_config i18n.commitEncoding $test_encoding &&
 	commit_msg $test_encoding | git commit -F - &&
 	git add bar &&
 	test_tick &&
-	git commit -m "add bar" &&
-	git config --unset i18n.commitEncoding
+	git commit -m "add bar"
 '
 
 test_expect_success 'alias builtin format' '
 	git log --pretty=oneline >expected &&
-	git config pretty.test-alias oneline &&
+	test_config pretty.test-alias oneline &&
 	git log --pretty=test-alias >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'alias masking builtin format' '
 	git log --pretty=oneline >expected &&
-	git config pretty.oneline "%H" &&
+	test_config pretty.oneline "%H" &&
 	git log --pretty=oneline >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'alias user-defined format' '
 	git log --pretty="format:%h" >expected &&
-	git config pretty.test-alias "format:%h" &&
+	test_config pretty.test-alias "format:%h" &&
 	git log --pretty=test-alias >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '
-	git config i18n.logOutputEncoding $test_encoding &&
+	test_config i18n.logOutputEncoding $test_encoding &&
 	git log --oneline >expected-s &&
 	git log --pretty="tformat:%h %s" >actual-s &&
-	git config --unset i18n.logOutputEncoding &&
 	test_cmp expected-s actual-s
 '
 
@@ -75,34 +73,34 @@ test_expect_success 'alias user-defined tformat with %s (utf-8 encoding)' '
 
 test_expect_success 'alias user-defined tformat' '
 	git log --pretty="tformat:%h" >expected &&
-	git config pretty.test-alias "tformat:%h" &&
+	test_config pretty.test-alias "tformat:%h" &&
 	git log --pretty=test-alias >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'alias non-existent format' '
-	git config pretty.test-alias format-that-will-never-exist &&
+	test_config pretty.test-alias format-that-will-never-exist &&
 	test_must_fail git log --pretty=test-alias
 '
 
 test_expect_success 'alias of an alias' '
 	git log --pretty="tformat:%h" >expected &&
-	git config pretty.test-foo "tformat:%h" &&
-	git config pretty.test-bar test-foo &&
+	test_config pretty.test-foo "tformat:%h" &&
+	test_config pretty.test-bar test-foo &&
 	git log --pretty=test-bar >actual && test_cmp expected actual
 '
 
 test_expect_success 'alias masking an alias' '
 	git log --pretty=format:"Two %H" >expected &&
-	git config pretty.duplicate "format:One %H" &&
-	git config --add pretty.duplicate "format:Two %H" &&
+	test_config pretty.duplicate "format:One %H" &&
+	test_config pretty.duplicate "format:Two %H" --add &&
 	git log --pretty=duplicate >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'alias loop' '
-	git config pretty.test-foo test-bar &&
-	git config pretty.test-bar test-foo &&
+	test_config pretty.test-foo test-bar &&
+	test_config pretty.test-bar test-foo &&
 	test_must_fail git log --pretty=test-foo
 '
 
-- 
2.43.2

Re: [PATCH v2 1/2] pretty: update tests to use `test_config`

[Jeff King]

On Mon, Mar 25, 2024 at 02:25:12AM -0500, Brian Lyles wrote:

> These tests use raw `git config` calls, which is an older style that can
> cause config to bleed between tests if not manually unset. `test_config`
> ensures that config is unset at the end of each test automatically.
> 
> `test_config` is chosen over `git -c` since `test_config` still ends up
> calling `git config` which seems slightly more realistic to how pretty
> formats would be defined normally.

I think what you have here is fine, and I agree it's more like what
would happen in practice. The nice thing about "-c" is that it involves
two fewer processes (one to make the config and one to clean up). But
that is really the tip of the iceberg for our test suite.

>  t/t4205-log-pretty-formats.sh | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)

The patch looks good to me. The big thing to be careful about here is
tests which actually require the config state to persist between hunks.
But none of these look to be that type.

-Peff

[PATCH v2 2/2] pretty: find pretty formats case-insensitively

[Brian Lyles]

User-defined pretty formats are stored in config, which is meant to use
case-insensitive matching for names as noted in config.txt's 'Syntax'
section:

    All the other lines [...] are recognized as setting variables, in
    the form 'name = value' [...]. The variable names are
    case-insensitive, [...].

When a user specifies one of their format aliases with an uppercase in
it, however, it is not found.

    $ git config pretty.testAlias %h
    $ git config --list | grep pretty
    pretty.testalias=%h
    $ git log --format=testAlias -1
    fatal: invalid --pretty format: testAlias
    $ git log --format=testalias -1
    3c2a3fdc38

This is true whether the name in the config file uses any uppercase
characters or not.

Use case-insensitive comparisons when identifying format aliases.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 pretty.c                      | 2 +-
 t/t4205-log-pretty-formats.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index cf964b060c..8c1092c790 100644
--- a/pretty.c
+++ b/pretty.c
@@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
 	for (i = 0; i < commit_formats_len; i++) {
 		size_t match_len;
 
-		if (!starts_with(commit_formats[i].name, sought))
+		if (!istarts_with(commit_formats[i].name, sought))
 			continue;
 
 		match_len = strlen(commit_formats[i].name);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 20bba76c43..749363ccb8 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -58,6 +58,14 @@ test_expect_success 'alias user-defined format' '
 	test_cmp expected actual
 '
 
+test_expect_success 'alias user-defined format is matched case-insensitively' '
+	git log --pretty="format:%h" >expected &&
+	test_config pretty.testone "format:%h" &&
+	test_config pretty.testtwo testOne &&
+	git log --pretty=testTwo >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '
 	test_config i18n.logOutputEncoding $test_encoding &&
 	git log --oneline >expected-s &&
-- 
2.43.2

Re: [PATCH v2 2/2] pretty: find pretty formats case-insensitively

[Jeff King]

On Mon, Mar 25, 2024 at 02:25:13AM -0500, Brian Lyles wrote:

> Use case-insensitive comparisons when identifying format aliases.
> 
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>

Unsurprisingly, this one looks good to me. I don't know if I deserve a
co-author, but I am happy either way. :)

-Peff

Re: [PATCH v2 2/2] pretty: find pretty formats case-insensitively

[Brian Lyles]

On Mon, Mar 25, 2024 at 4:46 AM Jeff King <peff@peff.net> wrote:

> On Mon, Mar 25, 2024 at 02:25:13AM -0500, Brian Lyles wrote:
> 
>> Use case-insensitive comparisons when identifying format aliases.
>> 
>> Co-authored-by: Jeff King <peff@peff.net>
>> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> 
> Unsurprisingly, this one looks good to me. I don't know if I deserve a
> co-author, but I am happy either way. :)

Let's see, you... *Checks notes* handed me 100% of the fix logic and all
of the non-boilerplate parts of the new test on a silver platter.
Co-authored-by seems pretty accurate to me =).

Thanks for the review!

-- 
Thank you,
Brian Lyles