threads / patch / 48818

patch, 3 partsun-break for-each-ref --ignore-case

Subject: [PATCH 0/3] un-break for-each-ref --ignore-case

## tl;dr

5 messages between Jul 2, 2018 and Jul 2, 2018. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Jul 2, 2018, 21:11 UTC · lore
This is a follow-up to this patch last September:
  https://public-inbox.org/git/79c946a2-532e-1c9b-7bf2-1f1ccbff721e@gmail.com/
The patch was the right thing to do, but:
 1. It needed a commit message.
 2. It needed a signoff.
 3. It needed a test, which then showed that there was _another_ bug. :)

This fixes 1 and 3, and the extra bug. I've added my signoff to Aleksandr's patch, as I think it is a trivial enough change to fall under the DCO part b. But Aleksandr, if you can give your explicit sign-off to include the patch in the project, that would be better still.

  [1/3]: t6300: add a test for --ignore-case
  [2/3]: for-each-ref: consistently pass WM_IGNORECASE flag
  [3/3]: ref-filter: avoid backend filtering with --ignore-case
 ref-filter.c            | 11 ++++++++++-
 t/t6300-for-each-ref.sh | 11 +++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
-Peff
Jeff King· Jul 2, 2018, 21:11 UTC · re: Jeff King · lore

[PATCH 1/3] t6300: add a test for --ignore-case

The --ignore-case option was added by 3bb16a8bf2 (tag, branch, for-each-ref: add --ignore-case for sorting and filtering, 2016-12-04), but it was never tested. And indeed, it does not work due to multiple bugs (which will be fixed in subsequent patches).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6300-for-each-ref.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to t/t6300-for-each-ref.sh +11 −0
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 48379aa0ee..b2bc81103f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -795,4 +795,15 @@ test_expect_success ':remotename and :remoteref' '
 	)
 '
 
+test_expect_failure 'for-each-ref --ignore-case ignores case' '
+	>expect &&
+	git for-each-ref --format="%(refname)" refs/heads/MASTER >actual &&
+	test_cmp expect actual &&
+
+	echo refs/heads/master >expect &&
+	git for-each-ref --format="%(refname)" --ignore-case \
+		refs/heads/MASTER >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.359.ge51c883f96
Jeff King· Jul 2, 2018, 21:11 UTC · re: Jeff King · lore

[PATCH 2/3] for-each-ref: consistently pass WM_IGNORECASE flag

From: Aleksandr Makarov <aleksandr.o.makarov@gmail.com>

The match_name_as_path() function learned to set WM_IGNORECASE in the "flags" field when the user passed --ignore-case. But it forgot to actually pass the flags to wildmatch()!

As a result, the --ignore-case feature has been broken since it was added in 3bb16a8bf2 (tag, branch, for-each-ref: add --ignore-case for sorting and filtering, 2016-12-04). We didn't notice because we added tests only for git-branch and git-tag. Whereas git-for-each-ref has slightly different matching rules, and thus uses a different function (the related function match_pattern() does it correctly).

Incidentally, this also caused clang's scan-build to complain about the code; the assignment to "flags" was dead code.

Note that we can't flip the test in t6300 to expect_success yet. There's another bug, which will be dealt with in the next patch.

Commit-message-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ref-filter.c +1 −1
diff --git a/ref-filter.c b/ref-filter.c
index fa3685d91f..5c0cbde52b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1813,7 +1813,7 @@ static int match_name_as_path(const struct ref_filter *filter, const char *refna
 		     refname[plen] == '/' ||
 		     p[plen-1] == '/'))
 			return 1;
-		if (!wildmatch(p, refname, WM_PATHNAME))
+		if (!wildmatch(p, refname, flags))
 			return 1;
 	}
 	return 0;
-- 
2.18.0.359.ge51c883f96
Jeff King· Jul 2, 2018, 21:12 UTC · re: Jeff King · lore

[PATCH 3/3] ref-filter: avoid backend filtering with --ignore-case

When for-each-ref is used with --ignore-case, we expect match_name_as_path() to do a case-insensitive match. But there's an extra layer of filtering that happens before we even get there. Since commit cfe004a5a9 (ref-filter: limit traversal to prefix, 2017-05-22), we feed the prefix to the ref backend so that it can optimize the ref iteration.

There's no mechanism for us to tell the backend we're matching case-insensitively. And nor is there likely to be one anytime soon, since the packed backend relies on binary-searching the sorted list of refs. Let's just punt on this case. The extra filtering is an optimization that we simply can't do. We'll still give the correct answer via the filtering in match_name_as_path().

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c            | 9 +++++++++
 t/t6300-for-each-ref.sh | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
Show changes to 2 files +10 −1

ref-filter.c, t/t6300-for-each-ref.sh

diff --git a/ref-filter.c b/ref-filter.c
index 5c0cbde52b..52704c7be6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1868,6 +1868,15 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 		return for_each_fullref_in("", cb, cb_data, broken);
 	}
 
+	if (filter->ignore_case) {
+		/*
+		 * we can't handle case-insensitive comparisons,
+		 * so just return everything and let the caller
+		 * sort it out.
+		 */
+		return for_each_fullref_in("", cb, cb_data, broken);
+	}
+
 	if (!filter->name_patterns[0]) {
 		/* no patterns; we have to look at everything */
 		return for_each_fullref_in("", cb, cb_data, broken);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index b2bc81103f..e0496da812 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -795,7 +795,7 @@ test_expect_success ':remotename and :remoteref' '
 	)
 '
 
-test_expect_failure 'for-each-ref --ignore-case ignores case' '
+test_expect_success 'for-each-ref --ignore-case ignores case' '
 	>expect &&
 	git for-each-ref --format="%(refname)" refs/heads/MASTER >actual &&
 	test_cmp expect actual &&
-- 
2.18.0.359.ge51c883f96
Eric Sunshine· Jul 2, 2018, 21:30 UTC · re: Jeff King · lore

Re: [PATCH 3/3] ref-filter: avoid backend filtering with --ignore-case

On Mon, Jul 2, 2018 at 5:12 PM Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> When for-each-ref is used with --ignore-case, we expect
> match_name_as_path() to do a case-insensitive match. But
> there's an extra layer of filtering that happens before we
> even get there. Since commit cfe004a5a9 (ref-filter: limit
> traversal to prefix, 2017-05-22), we feed the prefix to the
> ref backend so that it can optimize the ref iteration.
>
> There's no mechanism for us to tell the backend we're
> matching case-insensitively. And nor is there likely to be
Perhaps: s/And nor/nor/
Show 7 quoted lines
> one anytime soon, since the packed backend relies on
> binary-searching the sorted list of refs. Let's just punt on
> this case. The extra filtering is an optimization that we
> simply can't do. We'll still give the correct answer via the
> filtering in match_name_as_path().
>
> Signed-off-by: Jeff King <peff@peff.net>

← back to recent threads