Hi Peff
Thanks for the review.
On Mon, Mar 25, 2024 at 1:14 AM Jeff King <peff@xxxxxxxx> wrote:
> 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.
>> +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
