{"thread":{"id":"48818","subject":"[PATCH 0/3] un-break for-each-ref --ignore-case","startedAt":"2018-07-02T21:11:05Z","lastAt":"2018-07-02T21:30:21Z","messageCount":5,"participants":["Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"351561","messageId":"20180702211100.GA20885@sigill.intra.peff.net","threadId":"48818","inReplyTo":null,"subject":"[PATCH 0/3] un-break for-each-ref --ignore-case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-02T21:11:00Z","receivedAt":"2018-07-02T21:11:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a follow-up to this patch last September:\n\n https://public-inbox.org/git/79c946a2-532e-1c9b-7bf2-1f1ccbff721e@gmail.com/\n\nThe patch was the right thing to do, but:\n\n 1. It needed a commit message.\n\n 2. It needed a signoff.\n\n 3. It needed a test, which then showed that there was _another_ bug. :)\n\nThis fixes 1 and 3, and the extra bug. I've added my signoff to\nAleksandr's patch, as I think it is a trivial enough change to fall\nunder the DCO part b. But Aleksandr, if you can give your explicit\nsign-off to include the patch in the project, that would be better\nstill.\n\n [1/3]: t6300: add a test for --ignore-case\n [2/3]: for-each-ref: consistently pass WM_IGNORECASE flag\n [3/3]: ref-filter: avoid backend filtering with --ignore-case\n\n ref-filter.c | 11 ++++++++++-\n t/t6300-for-each-ref.sh | 11 +++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"351562","messageId":"20180702211122.GA23324@sigill.intra.peff.net","threadId":"48818","inReplyTo":"20180702211100.GA20885@sigill.intra.peff.net","subject":"[PATCH 1/3] t6300: add a test for --ignore-case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-02T21:11:23Z","receivedAt":"2018-07-02T21:11:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --ignore-case option was added by 3bb16a8bf2 (tag,\nbranch, for-each-ref: add --ignore-case for sorting and\nfiltering, 2016-12-04), but it was never tested. And indeed,\nit does not work due to multiple bugs (which will be fixed\nin subsequent patches).\n\nSigned-off-by: Jeff King \n---\n t/t6300-for-each-ref.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 48379aa0ee..b2bc81103f 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -795,4 +795,15 @@ test_expect_success ':remotename and :remoteref' '\n \t)\n '\n \n+test_expect_failure 'for-each-ref --ignore-case ignores case' '\n+\t>expect &&\n+\tgit for-each-ref --format=\"%(refname)\" refs/heads/MASTER >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo refs/heads/master >expect &&\n+\tgit for-each-ref --format=\"%(refname)\" --ignore-case \\\n+\t\trefs/heads/MASTER >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.18.0.359.ge51c883f96\n\n"},{"id":"351563","messageId":"20180702211159.GB23324@sigill.intra.peff.net","threadId":"48818","inReplyTo":"20180702211100.GA20885@sigill.intra.peff.net","subject":"[PATCH 2/3] for-each-ref: consistently pass WM_IGNORECASE flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-02T21:11:59Z","receivedAt":"2018-07-02T21:12:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Aleksandr Makarov \n\nThe match_name_as_path() function learned to set\nWM_IGNORECASE in the \"flags\" field when the user passed\n--ignore-case. But it forgot to actually pass the flags to\nwildmatch()!\n\nAs a result, the --ignore-case feature has been broken since\nit was added in 3bb16a8bf2 (tag, branch, for-each-ref: add\n--ignore-case for sorting and filtering, 2016-12-04). We\ndidn't notice because we added tests only for git-branch and\ngit-tag. Whereas git-for-each-ref has slightly different\nmatching rules, and thus uses a different function (the\nrelated function match_pattern() does it correctly).\n\nIncidentally, this also caused clang's scan-build to\ncomplain about the code; the assignment to \"flags\" was dead\ncode.\n\nNote that we can't flip the test in t6300 to expect_success\nyet. There's another bug, which will be dealt with in the\nnext patch.\n\nCommit-message-by: Jeff King \nSigned-off-by: Jeff King \n---\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fa3685d91f..5c0cbde52b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1813,7 +1813,7 @@ static int match_name_as_path(const struct ref_filter *filter, const char *refna\n \t\t refname[plen] == '/' ||\n \t\t p[plen-1] == '/'))\n \t\t\treturn 1;\n-\t\tif (!wildmatch(p, refname, WM_PATHNAME))\n+\t\tif (!wildmatch(p, refname, flags))\n \t\t\treturn 1;\n \t}\n \treturn 0;\n-- \n2.18.0.359.ge51c883f96\n\n"},{"id":"351564","messageId":"20180702211242.GC23324@sigill.intra.peff.net","threadId":"48818","inReplyTo":"20180702211100.GA20885@sigill.intra.peff.net","subject":"[PATCH 3/3] ref-filter: avoid backend filtering with --ignore-case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-02T21:12:42Z","receivedAt":"2018-07-02T21:12:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When for-each-ref is used with --ignore-case, we expect\nmatch_name_as_path() to do a case-insensitive match. But\nthere's an extra layer of filtering that happens before we\neven get there. Since commit cfe004a5a9 (ref-filter: limit\ntraversal to prefix, 2017-05-22), we feed the prefix to the\nref backend so that it can optimize the ref iteration.\n\nThere's no mechanism for us to tell the backend we're\nmatching case-insensitively. And nor is there likely to be\none anytime soon, since the packed backend relies on\nbinary-searching the sorted list of refs. Let's just punt on\nthis case. The extra filtering is an optimization that we\nsimply can't do. We'll still give the correct answer via the\nfiltering in match_name_as_path().\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 9 +++++++++\n t/t6300-for-each-ref.sh | 2 +-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 5c0cbde52b..52704c7be6 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1868,6 +1868,15 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n \t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n \t}\n \n+\tif (filter->ignore_case) {\n+\t\t/*\n+\t\t * we can't handle case-insensitive comparisons,\n+\t\t * so just return everything and let the caller\n+\t\t * sort it out.\n+\t\t */\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t}\n+\n \tif (!filter->name_patterns[0]) {\n \t\t/* no patterns; we have to look at everything */\n \t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex b2bc81103f..e0496da812 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -795,7 +795,7 @@ test_expect_success ':remotename and :remoteref' '\n \t)\n '\n \n-test_expect_failure 'for-each-ref --ignore-case ignores case' '\n+test_expect_success 'for-each-ref --ignore-case ignores case' '\n \t>expect &&\n \tgit for-each-ref --format=\"%(refname)\" refs/heads/MASTER >actual &&\n \ttest_cmp expect actual &&\n-- \n2.18.0.359.ge51c883f96\n"},{"id":"351568","messageId":"CAPig+cSJrZi+rVgJp3Yhbc=F2yyooGw7a046e-KKiE++3CiP3w@mail.gmail.com","threadId":"48818","inReplyTo":"20180702211242.GC23324@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] ref-filter: avoid backend filtering with --ignore-case","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-02T21:30:05Z","receivedAt":"2018-07-02T21:30:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 2, 2018 at 5:12 PM Jeff King wrote:\n> When for-each-ref is used with --ignore-case, we expect\n> match_name_as_path() to do a case-insensitive match. But\n> there's an extra layer of filtering that happens before we\n> even get there. Since commit cfe004a5a9 (ref-filter: limit\n> traversal to prefix, 2017-05-22), we feed the prefix to the\n> ref backend so that it can optimize the ref iteration.\n>\n> There's no mechanism for us to tell the backend we're\n> matching case-insensitively. And nor is there likely to be\n\nPerhaps: s/And nor/nor/\n\n> one anytime soon, since the packed backend relies on\n> binary-searching the sorted list of refs. Let's just punt on\n> this case. The extra filtering is an optimization that we\n> simply can't do. We'll still give the correct answer via the\n> filtering in match_name_as_path().\n>\n> Signed-off-by: Jeff King \n"}]}