Merge branch 'rs/pickaxe-i'

Allow the options -i/--regexp-ignore-case, --pickaxe-regex, and -S
to be used together and work as expected to perform a pickaxe
search using case-insensitive regular expression matching.

* rs/pickaxe-i:
  pickaxe: simplify kwset loop in contains()
  pickaxe: call strlen only when necessary in diffcore_pickaxe_count()
  pickaxe: move pickaxe() after pickaxe_match()
  pickaxe: merge diffcore_pickaxe_grep() and diffcore_pickaxe_count() into diffcore_pickaxe()
  pickaxe: honor -i when used with -S and --pickaxe-regex
  t4209: use helper functions to test --author
  t4209: use helper functions to test --grep
  t4209: factor out helper function test_log_icase()
  t4209: factor out helper function test_log()
  t4209: set up expectations up front

2 files changed, 116 insertions, 188 deletions

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 401eb72c61..185f86b284 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -12,47 +12,6 @@ typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
 			  regex_t *regexp, kwset_t kws);
 
-static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
-			 regex_t *regexp, kwset_t kws, pickaxe_fn fn);
-
-static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
-		    regex_t *regexp, kwset_t kws, pickaxe_fn fn)
-{
-	int i;
-	struct diff_queue_struct outq;
-
-	DIFF_QUEUE_CLEAR(&outq);
-
-	if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
-		/* Showing the whole changeset if needle exists */
-		for (i = 0; i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (pickaxe_match(p, o, regexp, kws, fn))
-				return; /* do not munge the queue */
-		}
-
-		/*
-		 * Otherwise we will clear the whole queue by copying
-		 * the empty outq at the end of this function, but
-		 * first clear the current entries in the queue.
-		 */
-		for (i = 0; i < q->nr; i++)
-			diff_free_filepair(q->queue[i]);
-	} else {
-		/* Showing only the filepairs that has the needle */
-		for (i = 0; i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (pickaxe_match(p, o, regexp, kws, fn))
-				diff_q(&outq, p);
-			else
-				diff_free_filepair(p);
-		}
-	}
-
-	free(q->queue);
-	*q = outq;
-}
-
 struct diffgrep_cb {
 	regex_t *regexp;
 	int hit;
@@ -108,29 +67,6 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 	return ecbdata.hit;
 }
 
-static void diffcore_pickaxe_grep(struct diff_options *o)
-{
-	int err;
-	regex_t regex;
-	int cflags = REG_EXTENDED | REG_NEWLINE;
-
-	if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
-		cflags |= REG_ICASE;
-
-	err = regcomp(&regex, o->pickaxe, cflags);
-	if (err) {
-		char errbuf[1024];
-		regerror(err, &regex, errbuf, 1024);
-		regfree(&regex);
-		die("invalid regex: %s", errbuf);
-	}
-
-	pickaxe(&diff_queued_diff, o, &regex, NULL, diff_grep);
-
-	regfree(&regex);
-	return;
-}
-
 static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 {
 	unsigned int cnt;
@@ -158,13 +94,10 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 		while (sz) {
 			struct kwsmatch kwsm;
 			size_t offset = kwsexec(kws, data, sz, &kwsm);
-			const char *found;
 			if (offset == -1)
 				break;
-			else
-				found = data + offset;
-			sz -= found - data + kwsm.size[0];
-			data = found + kwsm.size[0];
+			sz -= offset + kwsm.size[0];
+			data += offset + kwsm.size[0];
 			cnt++;
 		}
 	}
@@ -227,17 +160,57 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	return ret;
 }
 
-static void diffcore_pickaxe_count(struct diff_options *o)
+static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
+		    regex_t *regexp, kwset_t kws, pickaxe_fn fn)
+{
+	int i;
+	struct diff_queue_struct outq;
+
+	DIFF_QUEUE_CLEAR(&outq);
+
+	if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
+		/* Showing the whole changeset if needle exists */
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (pickaxe_match(p, o, regexp, kws, fn))
+				return; /* do not munge the queue */
+		}
+
+		/*
+		 * Otherwise we will clear the whole queue by copying
+		 * the empty outq at the end of this function, but
+		 * first clear the current entries in the queue.
+		 */
+		for (i = 0; i < q->nr; i++)
+			diff_free_filepair(q->queue[i]);
+	} else {
+		/* Showing only the filepairs that has the needle */
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (pickaxe_match(p, o, regexp, kws, fn))
+				diff_q(&outq, p);
+			else
+				diff_free_filepair(p);
+		}
+	}
+
+	free(q->queue);
+	*q = outq;
+}
+
+void diffcore_pickaxe(struct diff_options *o)
 {
 	const char *needle = o->pickaxe;
 	int opts = o->pickaxe_opts;
-	unsigned long len = strlen(needle);
 	regex_t regex, *regexp = NULL;
 	kwset_t kws = NULL;
 
-	if (opts & DIFF_PICKAXE_REGEX) {
+	if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
 		int err;
-		err = regcomp(&regex, needle, REG_EXTENDED | REG_NEWLINE);
+		int cflags = REG_EXTENDED | REG_NEWLINE;
+		if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
+			cflags |= REG_ICASE;
+		err = regcomp(&regex, needle, cflags);
 		if (err) {
 			/* The POSIX.2 people are surely sick */
 			char errbuf[1024];
@@ -249,24 +222,17 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 	} else {
 		kws = kwsalloc(DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE)
 			       ? tolower_trans_tbl : NULL);
-		kwsincr(kws, needle, len);
+		kwsincr(kws, needle, strlen(needle));
 		kwsprep(kws);
 	}
 
-	pickaxe(&diff_queued_diff, o, regexp, kws, has_changes);
+	/* Might want to warn when both S and G are on; I don't care... */
+	pickaxe(&diff_queued_diff, o, regexp, kws,
+		(opts & DIFF_PICKAXE_KIND_G) ? diff_grep : has_changes);
 
-	if (opts & DIFF_PICKAXE_REGEX)
-		regfree(&regex);
+	if (regexp)
+		regfree(regexp);
 	else
 		kwsfree(kws);
 	return;
 }
-
-void diffcore_pickaxe(struct diff_options *o)
-{
-	/* Might want to warn when both S and G are on; I don't care... */
-	if (o->pickaxe_opts & DIFF_PICKAXE_KIND_G)
-		diffcore_pickaxe_grep(o);
-	else
-		diffcore_pickaxe_count(o);
-}
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 38fb80f643..844df760f7 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -3,82 +3,72 @@
 test_description='log --grep/--author/--regexp-ignore-case/-S/-G'
 . ./test-lib.sh
 
+test_log () {
+	expect=$1
+	kind=$2
+	needle=$3
+	shift 3
+	rest=$@
+
+	case $kind in
+	--*)
+		opt=$kind=$needle
+		;;
+	*)
+		opt=$kind$needle
+		;;
+	esac
+	case $expect in
+	expect_nomatch)
+		match=nomatch
+		;;
+	*)
+		match=match
+		;;
+	esac
+
+	test_expect_success "log $kind${rest:+ $rest} ($match)" "
+		git log $rest $opt --format=%H >actual &&
+		test_cmp $expect actual
+	"
+}
+
+# test -i and --regexp-ignore-case and expect both to behave the same way
+test_log_icase () {
+	test_log $@ --regexp-ignore-case
+	test_log $@ -i
+}
+
 test_expect_success setup '
+	>expect_nomatch &&
+
 	>file &&
 	git add file &&
 	test_tick &&
 	git commit -m initial &&
+	git rev-parse --verify HEAD >expect_initial &&
 
 	echo Picked >file &&
+	git add file &&
 	test_tick &&
-	git commit -a --author="Another Person <another@example.com>" -m second
-'
-
-test_expect_success 'log --grep' '
-	git log --grep=initial --format=%H >actual &&
-	git rev-parse --verify HEAD^ >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log --grep --regexp-ignore-case' '
-	git log --regexp-ignore-case --grep=InItial --format=%H >actual &&
-	git rev-parse --verify HEAD^ >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log --grep -i' '
-	git log -i --grep=InItial --format=%H >actual &&
-	git rev-parse --verify HEAD^ >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log --author --regexp-ignore-case' '
-	git log --regexp-ignore-case --author=person --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log --author -i' '
-	git log -i --author=person --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -G (nomatch)' '
-	git log -Gpicked --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -G (match)' '
-	git log -GPicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -G --regexp-ignore-case (nomatch)' '
-	git log --regexp-ignore-case -Gpickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
+	git commit --author="Another Person <another@example.com>" -m second &&
+	git rev-parse --verify HEAD >expect_second
 '
 
-test_expect_success 'log -G -i (nomatch)' '
-	git log -i -Gpickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
-'
+test_log	expect_initial	--grep initial
+test_log	expect_nomatch	--grep InItial
+test_log_icase	expect_initial	--grep InItial
+test_log_icase	expect_nomatch	--grep initail
 
-test_expect_success 'log -G --regexp-ignore-case (match)' '
-	git log --regexp-ignore-case -Gpicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
+test_log	expect_second	--author Person
+test_log	expect_nomatch	--author person
+test_log_icase	expect_second	--author person
+test_log_icase	expect_nomatch	--author spreon
 
-test_expect_success 'log -G -i (match)' '
-	git log -i -Gpicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
+test_log	expect_nomatch	-G picked
+test_log	expect_second	-G Picked
+test_log_icase	expect_nomatch	-G pickle
+test_log_icase	expect_second	-G picked
 
 test_expect_success 'log -G --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
@@ -89,46 +79,19 @@ test_expect_success 'log -G --textconv (missing textconv tool)' '
 test_expect_success 'log -G --no-textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
 	git -c diff.test.textconv=missing log -Gfoo --no-textconv >actual &&
-	>expect &&
-	test_cmp expect actual &&
+	test_cmp expect_nomatch actual &&
 	rm .gitattributes
 '
 
-test_expect_success 'log -S (nomatch)' '
-	git log -Spicked --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -S (match)' '
-	git log -SPicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -S --regexp-ignore-case (match)' '
-	git log --regexp-ignore-case -Spicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -S -i (match)' '
-	git log -i -Spicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success 'log -S --regexp-ignore-case (nomatch)' '
-	git log --regexp-ignore-case -Spickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
-'
+test_log	expect_nomatch	-S picked
+test_log	expect_second	-S Picked
+test_log_icase	expect_second	-S picked
+test_log_icase	expect_nomatch	-S pickle
 
-test_expect_success 'log -S -i (nomatch)' '
-	git log -i -Spickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
-'
+test_log	expect_nomatch	-S p.cked --pickaxe-regex
+test_log	expect_second	-S P.cked --pickaxe-regex
+test_log_icase	expect_second	-S p.cked --pickaxe-regex
+test_log_icase	expect_nomatch	-S p.ckle --pickaxe-regex
 
 test_expect_success 'log -S --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
@@ -139,8 +102,7 @@ test_expect_success 'log -S --textconv (missing textconv tool)' '
 test_expect_success 'log -S --no-textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
 	git -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&
-	>expect &&
-	test_cmp expect actual &&
+	test_cmp expect_nomatch actual &&
 	rm .gitattributes
 '