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

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

[Jeff King]

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

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

[Jeff King]

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(+)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Eric Sunshine]

On Mon, Jul 2, 2018 at 5:12 PM Jeff King <peff@peff.net> wrote:
> 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/

> 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>