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

[Jeff King <peff@peff.net>]

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.