Re: [PATCH] pretty: find pretty formats case-insensitively
From: Brian Lyles <hidden>
Date: 2024-03-25 07:08:34
Hi Peff Thanks for the review. On Mon, Mar 25, 2024 at 1:14 AM Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to 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 '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.
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).
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