From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Brian Lyles <brianmlyles@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] pretty: find pretty formats case-insensitively
Date: Mon, 25 Mar 2024 11:12:45 -0700 [thread overview]
Message-ID: <xmqqmsqmkwo2.fsf@gitster.g> (raw)
In-Reply-To: <20240325061452.GA242093@coredump.intra.peff.net> (Jeff King's message of "Mon, 25 Mar 2024 02:14:52 -0400")
Jeff King <peff@peff.net> writes:
> 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.
>> +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.
next prev parent reply other threads:[~2024-03-25 18:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-24 21:43 [PATCH] pretty: find pretty formats case-insensitively Brian Lyles
2024-03-25 6:14 ` Jeff King
2024-03-25 7:08 ` Brian Lyles
2024-03-25 18:12 ` Junio C Hamano [this message]
2024-03-25 7:25 ` [PATCH v2 1/2] pretty: update tests to use `test_config` Brian Lyles
2024-03-25 9:44 ` Jeff King
2024-03-25 7:25 ` [PATCH v2 2/2] pretty: find pretty formats case-insensitively Brian Lyles
2024-03-25 9:46 ` Jeff King
2024-03-25 15:58 ` Brian Lyles
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqmsqmkwo2.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=brianmlyles@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).