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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2024, 18:12 UTC
Message-ID: <xmqqmsqmkwo2.fsf@gitster.g>
In-Reply-To: <20240325061452.GA242093@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 19 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
>  '

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

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

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.

Show 15 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 think it is not just strange but outright wrong. I agree that it is orthogonal to this fix.

Thanks, both.