{"thread":{"id":"61196","subject":"[PATCH] pretty: find pretty formats case-insensitively","startedAt":"2024-03-24T21:43:45Z","lastAt":"2024-03-25T18:12:51Z","messageCount":9,"participants":["Brian Lyles","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491378","messageId":"20240324214316.917513-1-brianmlyles@gmail.com","threadId":"61196","inReplyTo":null,"subject":"[PATCH] pretty: find pretty formats case-insensitively","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-24T21:43:09Z","receivedAt":"2024-03-24T21:43:45Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"User-defined pretty formats are stored in config, which is meant to use\ncase-insensitive matching for names as noted in config.txt's 'Syntax'\nsection:\n\n    All the other lines [...] are recognized as setting variables, in\n    the form 'name = value' [...]. The variable names are\n    case-insensitive, [...].\n\nWhen a user specifies one of their format aliases with an uppercase in\nit, however, it is not found.\n\n    $ git config pretty.testAlias %h\n    $ git config --list | grep pretty\n    pretty.testalias=%h\n    $ git log --format=testAlias -1\n    fatal: invalid --pretty format: testAlias\n    $ git log --format=testalias -1\n    3c2a3fdc38\n\nThis is true whether the name in the config file uses any uppercase\ncharacters or not.\n\nNormalize the format name specified via `--format` to lowercase so that\nformat aliases are found case-insensitively. The format aliases loaded\nfrom config against which this name is compared are already normalized\nto lowercase since they are loaded through `git_config()`.\n\n`xstrdup_tolower` is used instead of modifying the string in-place to\nensure that the error shown to the user when the format is not found has\nthe same casing that the user entered. Otherwise, the mismatch may be\nconfusing to the user.\n\nSigned-off-by: Brian Lyles <brianmlyles@gmail.com>\n---\n pretty.c                      | 12 +++++++++++-\n t/t4205-log-pretty-formats.sh |  7 +++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex cf964b060c..78ec7a75ff 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -168,10 +168,20 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n \n static struct cmt_fmt_map *find_commit_format(const char *sought)\n {\n+\tstruct cmt_fmt_map *result;\n+\tchar *sought_lower;\n+\n \tif (!commit_formats)\n \t\tsetup_commit_formats();\n \n-\treturn find_commit_format_recursive(sought, sought, 0);\n+\t/*\n+\t * The sought name will be compared to config names that have already\n+\t * been normalized to lowercase.\n+\t */\n+\tsought_lower = xstrdup_tolower(sought);\n+\tresult = find_commit_format_recursive(sought_lower, sought_lower, 0);\n+\tfree(sought_lower);\n+\treturn result;\n }\n \n void get_commit_format(const char *arg, struct rev_info *rev)\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e3d655e6b8..321e305979 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -59,6 +59,13 @@ test_expect_success 'alias user-defined format' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'alias user-defined format is matched case-insensitively' '\n+\tgit log --pretty=\"format:%h\" >expected &&\n+\tgit config pretty.testalias \"format:%h\" &&\n+\tgit log --pretty=testAlias >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '\n \tgit config i18n.logOutputEncoding $test_encoding &&\n \tgit log --oneline >expected-s &&\n-- \n2.43.2\n\n"},{"id":"491391","messageId":"20240325061452.GA242093@coredump.intra.peff.net","threadId":"61196","inReplyTo":"20240324214316.917513-1-brianmlyles@gmail.com","subject":"Re: [PATCH] pretty: find pretty formats case-insensitively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-25T06:14:52Z","receivedAt":"2024-03-25T06:14:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 24, 2024 at 04:43:09PM -0500, Brian Lyles wrote:\n\n> User-defined pretty formats are stored in config, which is meant to use\n> case-insensitive matching for names as noted in config.txt's 'Syntax'\n> section:\n> \n>     All the other lines [...] are recognized as setting variables, in\n>     the form 'name = value' [...]. The variable names are\n>     case-insensitive, [...].\n> \n> When a user specifies one of their format aliases with an uppercase in\n> it, however, it is not found.\n> \n>     $ git config pretty.testAlias %h\n>     $ git config --list | grep pretty\n>     pretty.testalias=%h\n>     $ git log --format=testAlias -1\n>     fatal: invalid --pretty format: testAlias\n>     $ git log --format=testalias -1\n>     3c2a3fdc38\n\nYeah, I agree that case-insensitive matching makes more sense here due\nto the nature of config keys, especially given this:\n\n> This is true whether the name in the config file uses any uppercase\n> characters or not.\n\nI.e., the config code is going to normalize the variable names already,\nso we must match (even if the user consistently specifies camelCase).\n\nBut...\n\n>  static struct cmt_fmt_map *find_commit_format(const char *sought)\n>  {\n> +\tstruct cmt_fmt_map *result;\n> +\tchar *sought_lower;\n> +\n>  \tif (!commit_formats)\n>  \t\tsetup_commit_formats();\n>  \n> -\treturn find_commit_format_recursive(sought, sought, 0);\n> +\t/*\n> +\t * The sought name will be compared to config names that have already\n> +\t * been normalized to lowercase.\n> +\t */\n> +\tsought_lower = xstrdup_tolower(sought);\n> +\tresult = find_commit_format_recursive(sought_lower, sought_lower, 0);\n> +\tfree(sought_lower);\n> +\treturn result;\n>  }\n\nThe mention of \"recursive\" in the function we call made me what wonder\nif we'd need more normalization. And I think we do. Try this\nmodification to your test:\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 321e305979..be549b1d4b 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '\n \n test_expect_success 'alias user-defined format is matched case-insensitively' '\n \tgit log --pretty=\"format:%h\" >expected &&\n-\tgit config pretty.testalias \"format:%h\" &&\n-\tgit log --pretty=testAlias >actual &&\n+\tgit config pretty.testone \"format:%h\" &&\n+\tgit config pretty.testtwo testOne &&\n+\tgit log --pretty=testTwo >actual &&\n \ttest_cmp expected actual\n '\n \n\nwhich fails because looking up \"testOne\" in the recursion won't work. So\nI think we'd want to simply match case-insensitively inside the\nfunction, like:\n\ndiff --git a/pretty.c b/pretty.c\nindex 50825c9d25..10f71ee004 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n \tfor (i = 0; i < commit_formats_len; i++) {\n \t\tsize_t match_len;\n \n-\t\tif (!starts_with(commit_formats[i].name, sought))\n+\t\tif (!istarts_with(commit_formats[i].name, sought))\n \t\t\tcontinue;\n \n \t\tmatch_len = strlen(commit_formats[i].name);\n\nAnd then you would not even need to normalize it in\nfind_commit_format().\n\n> +test_expect_success 'alias user-defined format is matched case-insensitively' '\n> +\tgit log --pretty=\"format:%h\" >expected &&\n> +\tgit config pretty.testalias \"format:%h\" &&\n> +\tgit log --pretty=testAlias >actual &&\n> +\ttest_cmp expected actual\n> +'\n\nModern style would be to use \"test_config\" here (or just \"git -c\"), but\nI see the surrounding tests are too old to do so. So I'd be OK with\nmatching them (but cleaning up all of the surrounding ones would be\nnice, too).\n\n-Peff\n\nPS The matching rules in find_commit_format_recursive() seem weird\n   to me. We do a prefix match, and then return the entry whose name is\n   the shortest? And break ties based on which came first? So:\n\n     git -c pretty.abcd=format:one \\\n         -c pretty.abc=format:two \\\n         -c pretty.abd=format:three \\\n\t log -1 --format=ab\n\n   quietly chooses \"two\". I guess the \"shortest wins\" is meant to allow\n   \"foo\" to be chosen over \"foobar\" if you specify the whole name. But\n   the fact that we don't flag an ambiguity between \"abc\" and \"abd\"\n   seems strange.\n\n   That is all orthogonal to your patch, of course, but just a\n   head-scratcher I noticed while looking at the code.\n"},{"id":"491393","messageId":"17bff03951f07360.70b1dd9aae081c6e.203dcd72f6563036@zivdesk","threadId":"61196","inReplyTo":"20240325061452.GA242093@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: find pretty formats case-insensitively","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-25T07:08:32Z","receivedAt":"2024-03-25T07:08:34Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"Hi Peff\n\nThanks for the review.\n\nOn Mon, Mar 25, 2024 at 1:14 AM Jeff King <peff@peff.net> wrote:\n\n> The mention of \"recursive\" in the function we call made me what wonder\n> if we'd need more normalization. And I think we do. Try this\n> modification to your test:\n> \n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index 321e305979..be549b1d4b 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '\n>  \n>  test_expect_success 'alias user-defined format is matched case-insensitively' '\n>  \tgit log --pretty=\"format:%h\" >expected &&\n> -\tgit config pretty.testalias \"format:%h\" &&\n> -\tgit log --pretty=testAlias >actual &&\n> +\tgit config pretty.testone \"format:%h\" &&\n> +\tgit config pretty.testtwo testOne &&\n> +\tgit log --pretty=testTwo >actual &&\n>  \ttest_cmp expected actual\n>  '\n>  \n> \n> which fails because looking up \"testOne\" in the recursion won't work. So\n> I think we'd want to simply match case-insensitively inside the\n> function, like:\n> \n> diff --git a/pretty.c b/pretty.c\n> index 50825c9d25..10f71ee004 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n>  \tfor (i = 0; i < commit_formats_len; i++) {\n>  \t\tsize_t match_len;\n>  \n> -\t\tif (!starts_with(commit_formats[i].name, sought))\n> +\t\tif (!istarts_with(commit_formats[i].name, sought))\n>  \t\t\tcontinue;\n>  \n>  \t\tmatch_len = strlen(commit_formats[i].name);\n> \n> And then you would not even need to normalize it in\n> find_commit_format().\n\nGood catch -- you're absolutely right, and simply switching to\n`istarts_with` is a more elegant solution than my initial patch. I'll\nswitch to this approach in a v2 re-roll.\n\n>> +test_expect_success 'alias user-defined format is matched case-insensitively' '\n>> +\tgit log --pretty=\"format:%h\" >expected &&\n>> +\tgit config pretty.testalias \"format:%h\" &&\n>> +\tgit log --pretty=testAlias >actual &&\n>> +\ttest_cmp expected actual\n>> +'\n> \n> Modern style would be to use \"test_config\" here (or just \"git -c\"), but\n> I see the surrounding tests are too old to do so. So I'd be OK with\n> matching them (but cleaning up all of the surrounding ones would be\n> nice, too).\n\nThanks for the tip. Updating the existing tests in this file to use\n`test_config` looks to be fairly trivial, so I will start v2 with a\npatch that does that as well. I'm opting for `test_config` over `git -c`\nfor no real reason other than they seem roughly equivalent, but\n`test_config` still ends up calling `git config` which seems slightly\nmore realistic to how pretty formats would be defined normally.\n\n> PS The matching rules in find_commit_format_recursive() seem weird\n>    to me. We do a prefix match, and then return the entry whose name is\n>    the shortest? And break ties based on which came first? So:\n> \n>      git -c pretty.abcd=format:one \\\n>          -c pretty.abc=format:two \\\n>          -c pretty.abd=format:three \\\n> \t log -1 --format=ab\n> \n>    quietly chooses \"two\". I guess the \"shortest wins\" is meant to allow\n>    \"foo\" to be chosen over \"foobar\" if you specify the whole name. But\n>    the fact that we don't flag an ambiguity between \"abc\" and \"abd\"\n>    seems strange.\n> \n>    That is all orthogonal to your patch, of course, but just a\n>    head-scratcher I noticed while looking at the code.\n\nI agree that this behavior is somewhat odd. I'm not sure what we would\nwant to do about it at this point -- any change would technically be\nbreaking, I assume. Regardless, not something I'd scope into this patch,\nbut good observation.\n\n-- \nThank you,\nBrian Lyles\n"},{"id":"491396","messageId":"20240325072651.947505-1-brianmlyles@gmail.com","threadId":"61196","inReplyTo":"20240324214316.917513-1-brianmlyles@gmail.com","subject":"[PATCH v2 1/2] pretty: update tests to use `test_config`","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-25T07:25:12Z","receivedAt":"2024-03-25T07:27:56Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"These tests use raw `git config` calls, which is an older style that can\ncause config to bleed between tests if not manually unset. `test_config`\nensures that config is unset at the end of each test automatically.\n\n`test_config` is chosen over `git -c` since `test_config` still ends up\ncalling `git config` which seems slightly more realistic to how pretty\nformats would be defined normally.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Brian Lyles <brianmlyles@gmail.com>\n---\n t/t4205-log-pretty-formats.sh | 30 ++++++++++++++----------------\n 1 file changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e3d655e6b8..20bba76c43 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -30,40 +30,38 @@ test_expect_success 'set up basic repos' '\n \t>bar &&\n \tgit add foo &&\n \ttest_tick &&\n-\tgit config i18n.commitEncoding $test_encoding &&\n+\ttest_config i18n.commitEncoding $test_encoding &&\n \tcommit_msg $test_encoding | git commit -F - &&\n \tgit add bar &&\n \ttest_tick &&\n-\tgit commit -m \"add bar\" &&\n-\tgit config --unset i18n.commitEncoding\n+\tgit commit -m \"add bar\"\n '\n \n test_expect_success 'alias builtin format' '\n \tgit log --pretty=oneline >expected &&\n-\tgit config pretty.test-alias oneline &&\n+\ttest_config pretty.test-alias oneline &&\n \tgit log --pretty=test-alias >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'alias masking builtin format' '\n \tgit log --pretty=oneline >expected &&\n-\tgit config pretty.oneline \"%H\" &&\n+\ttest_config pretty.oneline \"%H\" &&\n \tgit log --pretty=oneline >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'alias user-defined format' '\n \tgit log --pretty=\"format:%h\" >expected &&\n-\tgit config pretty.test-alias \"format:%h\" &&\n+\ttest_config pretty.test-alias \"format:%h\" &&\n \tgit log --pretty=test-alias >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '\n-\tgit config i18n.logOutputEncoding $test_encoding &&\n+\ttest_config i18n.logOutputEncoding $test_encoding &&\n \tgit log --oneline >expected-s &&\n \tgit log --pretty=\"tformat:%h %s\" >actual-s &&\n-\tgit config --unset i18n.logOutputEncoding &&\n \ttest_cmp expected-s actual-s\n '\n \n@@ -75,34 +73,34 @@ test_expect_success 'alias user-defined tformat with %s (utf-8 encoding)' '\n \n test_expect_success 'alias user-defined tformat' '\n \tgit log --pretty=\"tformat:%h\" >expected &&\n-\tgit config pretty.test-alias \"tformat:%h\" &&\n+\ttest_config pretty.test-alias \"tformat:%h\" &&\n \tgit log --pretty=test-alias >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'alias non-existent format' '\n-\tgit config pretty.test-alias format-that-will-never-exist &&\n+\ttest_config pretty.test-alias format-that-will-never-exist &&\n \ttest_must_fail git log --pretty=test-alias\n '\n \n test_expect_success 'alias of an alias' '\n \tgit log --pretty=\"tformat:%h\" >expected &&\n-\tgit config pretty.test-foo \"tformat:%h\" &&\n-\tgit config pretty.test-bar test-foo &&\n+\ttest_config pretty.test-foo \"tformat:%h\" &&\n+\ttest_config pretty.test-bar test-foo &&\n \tgit log --pretty=test-bar >actual && test_cmp expected actual\n '\n \n test_expect_success 'alias masking an alias' '\n \tgit log --pretty=format:\"Two %H\" >expected &&\n-\tgit config pretty.duplicate \"format:One %H\" &&\n-\tgit config --add pretty.duplicate \"format:Two %H\" &&\n+\ttest_config pretty.duplicate \"format:One %H\" &&\n+\ttest_config pretty.duplicate \"format:Two %H\" --add &&\n \tgit log --pretty=duplicate >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'alias loop' '\n-\tgit config pretty.test-foo test-bar &&\n-\tgit config pretty.test-bar test-foo &&\n+\ttest_config pretty.test-foo test-bar &&\n+\ttest_config pretty.test-bar test-foo &&\n \ttest_must_fail git log --pretty=test-foo\n '\n \n-- \n2.43.2\n\n"},{"id":"491394","messageId":"20240325072651.947505-2-brianmlyles@gmail.com","threadId":"61196","inReplyTo":"20240324214316.917513-1-brianmlyles@gmail.com","subject":"[PATCH v2 2/2] pretty: find pretty formats case-insensitively","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-25T07:25:13Z","receivedAt":"2024-03-25T07:27:57Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"User-defined pretty formats are stored in config, which is meant to use\ncase-insensitive matching for names as noted in config.txt's 'Syntax'\nsection:\n\n    All the other lines [...] are recognized as setting variables, in\n    the form 'name = value' [...]. The variable names are\n    case-insensitive, [...].\n\nWhen a user specifies one of their format aliases with an uppercase in\nit, however, it is not found.\n\n    $ git config pretty.testAlias %h\n    $ git config --list | grep pretty\n    pretty.testalias=%h\n    $ git log --format=testAlias -1\n    fatal: invalid --pretty format: testAlias\n    $ git log --format=testalias -1\n    3c2a3fdc38\n\nThis is true whether the name in the config file uses any uppercase\ncharacters or not.\n\nUse case-insensitive comparisons when identifying format aliases.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Brian Lyles <brianmlyles@gmail.com>\n---\n pretty.c                      | 2 +-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex cf964b060c..8c1092c790 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -147,7 +147,7 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n \tfor (i = 0; i < commit_formats_len; i++) {\n \t\tsize_t match_len;\n \n-\t\tif (!starts_with(commit_formats[i].name, sought))\n+\t\tif (!istarts_with(commit_formats[i].name, sought))\n \t\t\tcontinue;\n \n \t\tmatch_len = strlen(commit_formats[i].name);\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 20bba76c43..749363ccb8 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -58,6 +58,14 @@ test_expect_success 'alias user-defined format' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'alias user-defined format is matched case-insensitively' '\n+\tgit log --pretty=\"format:%h\" >expected &&\n+\ttest_config pretty.testone \"format:%h\" &&\n+\ttest_config pretty.testtwo testOne &&\n+\tgit log --pretty=testTwo >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'alias user-defined tformat with %s (ISO8859-1 encoding)' '\n \ttest_config i18n.logOutputEncoding $test_encoding &&\n \tgit log --oneline >expected-s &&\n-- \n2.43.2\n\n"},{"id":"491411","messageId":"20240325094444.GA254602@coredump.intra.peff.net","threadId":"61196","inReplyTo":"20240325072651.947505-1-brianmlyles@gmail.com","subject":"Re: [PATCH v2 1/2] pretty: update tests to use `test_config`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-25T09:44:44Z","receivedAt":"2024-03-25T09:44:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2024 at 02:25:12AM -0500, Brian Lyles wrote:\n\n> These tests use raw `git config` calls, which is an older style that can\n> cause config to bleed between tests if not manually unset. `test_config`\n> ensures that config is unset at the end of each test automatically.\n> \n> `test_config` is chosen over `git -c` since `test_config` still ends up\n> calling `git config` which seems slightly more realistic to how pretty\n> formats would be defined normally.\n\nI think what you have here is fine, and I agree it's more like what\nwould happen in practice. The nice thing about \"-c\" is that it involves\ntwo fewer processes (one to make the config and one to clean up). But\nthat is really the tip of the iceberg for our test suite.\n\n>  t/t4205-log-pretty-formats.sh | 30 ++++++++++++++----------------\n>  1 file changed, 14 insertions(+), 16 deletions(-)\n\nThe patch looks good to me. The big thing to be careful about here is\ntests which actually require the config state to persist between hunks.\nBut none of these look to be that type.\n\n-Peff\n"},{"id":"491412","messageId":"20240325094601.GB254602@coredump.intra.peff.net","threadId":"61196","inReplyTo":"20240325072651.947505-2-brianmlyles@gmail.com","subject":"Re: [PATCH v2 2/2] pretty: find pretty formats case-insensitively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-25T09:46:01Z","receivedAt":"2024-03-25T09:46:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2024 at 02:25:13AM -0500, Brian Lyles wrote:\n\n> Use case-insensitive comparisons when identifying format aliases.\n> \n> Co-authored-by: Jeff King <peff@peff.net>\n> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>\n\nUnsurprisingly, this one looks good to me. I don't know if I deserve a\nco-author, but I am happy either way. :)\n\n-Peff\n"},{"id":"491458","messageId":"17c00d29dbbdc86a.70b1dd9aae081c6e.203dcd72f6563036@zivdesk","threadId":"61196","inReplyTo":"20240325094601.GB254602@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] pretty: find pretty formats case-insensitively","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-25T15:58:51Z","receivedAt":"2024-03-25T15:58:53Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"\nOn Mon, Mar 25, 2024 at 4:46 AM Jeff King <peff@peff.net> wrote:\n\n> On Mon, Mar 25, 2024 at 02:25:13AM -0500, Brian Lyles wrote:\n> \n>> Use case-insensitive comparisons when identifying format aliases.\n>> \n>> Co-authored-by: Jeff King <peff@peff.net>\n>> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>\n> \n> Unsurprisingly, this one looks good to me. I don't know if I deserve a\n> co-author, but I am happy either way. :)\n\nLet's see, you... *Checks notes* handed me 100% of the fix logic and all\nof the non-boilerplate parts of the new test on a silver platter.\nCo-authored-by seems pretty accurate to me =).\n\nThanks for the review!\n\n-- \nThank you,\nBrian Lyles\n"},{"id":"491485","messageId":"xmqqmsqmkwo2.fsf@gitster.g","threadId":"61196","inReplyTo":"20240325061452.GA242093@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: find pretty formats case-insensitively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-25T18:12:45Z","receivedAt":"2024-03-25T18:12:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The mention of \"recursive\" in the function we call made me what wonder\n> if we'd need more normalization. And I think we do. Try this\n> modification to your test:\n>\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index 321e305979..be549b1d4b 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -61,8 +61,9 @@ test_expect_success 'alias user-defined format' '\n>  \n>  test_expect_success 'alias user-defined format is matched case-insensitively' '\n>  \tgit log --pretty=\"format:%h\" >expected &&\n> -\tgit config pretty.testalias \"format:%h\" &&\n> -\tgit log --pretty=testAlias >actual &&\n> +\tgit config pretty.testone \"format:%h\" &&\n> +\tgit config pretty.testtwo testOne &&\n> +\tgit log --pretty=testTwo >actual &&\n>  \ttest_cmp expected actual\n>  '\n\nVery good thinking.  I totally missed the short-cut to another\nshort-cut while reading the patch.\n\n>> +test_expect_success 'alias user-defined format is matched case-insensitively' '\n>> +\tgit log --pretty=\"format:%h\" >expected &&\n>> +\tgit config pretty.testalias \"format:%h\" &&\n>> +\tgit log --pretty=testAlias >actual &&\n>> +\ttest_cmp expected actual\n>> +'\n>\n> Modern style would be to use \"test_config\" here (or just \"git -c\"), but\n> I see the surrounding tests are too old to do so. So I'd be OK with\n> matching them (but cleaning up all of the surrounding ones would be\n> nice, too).\n\nYup.  I do not mind seeing it done either way, as a preliminary\nclean-up before the main fix, just a fix with more modern style\nwhile leaving the clean-up as #leftoverbits to be done after the\ndust settles.\n\n> PS The matching rules in find_commit_format_recursive() seem weird\n>    to me. We do a prefix match, and then return the entry whose name is\n>    the shortest? And break ties based on which came first? So:\n>\n>      git -c pretty.abcd=format:one \\\n>          -c pretty.abc=format:two \\\n>          -c pretty.abd=format:three \\\n> \t log -1 --format=ab\n>\n>    quietly chooses \"two\". I guess the \"shortest wins\" is meant to allow\n>    \"foo\" to be chosen over \"foobar\" if you specify the whole name. But\n>    the fact that we don't flag an ambiguity between \"abc\" and \"abd\"\n>    seems strange.\n>    That is all orthogonal to your patch, of course, but just a\n>    head-scratcher I noticed while looking at the code.\n\nI think it is not just strange but outright wrong.  I agree that it\nis orthogonal to this fix.\n\nThanks, both.\n\n\n\n"}]}