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

From: Brian Lyles <brianmlyles@gmail.com>
Date: Mar 25, 2024, 07:08 UTC
Message-ID: <17bff03951f07360.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
In-Reply-To: <20240325061452.GA242093@coredump.intra.peff.net>

Hi Peff

Thanks for the review.

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

Show 41 quoted lines

> 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.

Show 11 quoted lines

>> +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.

Show 16 quoted lines

> 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