Re: [PATCH] pretty: find pretty formats case-insensitively
From: Junio C Hamano <hidden>
Date: 2024-03-25 18:12:51
Jeff King [off-list ref] writes:
quoted hunk
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.
quoted
+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.