[PATCH 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[PATCH 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[René Scharfe]

This series allows 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.  Its first
half refactors the test script and extends test coverage a bit while
we're at it.  The actual change is in the sixth patch.  It enables the
two following cleanups.  The last two patches are independent simple
cleanups.

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

 diffcore-pickaxe.c     | 142 +++++++++++++++++--------------------------
 t/t4209-log-pickaxe.sh | 159 +++++++++++++++++--------------------------------
 2 files changed, 110 insertions(+), 191 deletions(-)

-- 
1.9.1

[PATCH 01/10] t4209: set up expectations up front

[René Scharfe]

Instead of creating an expect file for each test, build three files with
the possible valid values during setup and use them in the tests.  This
shortens the test code and saves nine calls to git rev-parse.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4209-log-pickaxe.sh | 64 ++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 39 deletions(-)

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 38fb80f..ff668b5 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -4,80 +4,74 @@ test_description='log --grep/--author/--regexp-ignore-case/-S/-G'
 . ./test-lib.sh
 
 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
+	git commit --author="Another Person <another@example.com>" -m second &&
+	git rev-parse --verify HEAD >expect_second
 '
 
 test_expect_success 'log --grep' '
 	git log --grep=initial --format=%H >actual &&
-	git rev-parse --verify HEAD^ >expect &&
-	test_cmp expect actual
+	test_cmp expect_initial 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_cmp expect_initial 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_cmp expect_initial 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_cmp expect_second 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_cmp expect_second actual
 '
 
 test_expect_success 'log -G (nomatch)' '
 	git log -Gpicked --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
+	test_cmp expect_nomatch actual
 '
 
 test_expect_success 'log -G (match)' '
 	git log -GPicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
+	test_cmp expect_second actual
 '
 
 test_expect_success 'log -G --regexp-ignore-case (nomatch)' '
 	git log --regexp-ignore-case -Gpickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
+	test_cmp expect_nomatch actual
 '
 
 test_expect_success 'log -G -i (nomatch)' '
 	git log -i -Gpickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
+	test_cmp expect_nomatch actual
 '
 
 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_cmp expect_second actual
 '
 
 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_cmp expect_second actual
 '
 
 test_expect_success 'log -G --textconv (missing textconv tool)' '
@@ -89,45 +83,38 @@ 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_cmp expect_nomatch actual
 '
 
 test_expect_success 'log -S (match)' '
 	git log -SPicked --format=%H >actual &&
-	git rev-parse --verify HEAD >expect &&
-	test_cmp expect actual
+	test_cmp expect_second 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_cmp expect_second 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_cmp expect_second 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_cmp expect_nomatch actual
 '
 
 test_expect_success 'log -S -i (nomatch)' '
 	git log -i -Spickle --format=%H >actual &&
-	>expect &&
-	test_cmp expect actual
+	test_cmp expect_nomatch actual
 '
 
 test_expect_success 'log -S --textconv (missing textconv tool)' '
@@ -139,8 +126,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
 '
 
-- 
1.9.1

[PATCH 02/10] t4209: factor out helper function test_log()

[René Scharfe]

Twelve tests in t4209 follow the same simple pattern for description,
git log call and checking.  Extract that shared logic into a helper
function named test_log.  Test specifications become a lot more
compact, new tests can be added more easily.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4209-log-pickaxe.sh | 92 +++++++++++++++++++-------------------------------
 1 file changed, 34 insertions(+), 58 deletions(-)

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index ff668b5..9f3bb40 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -3,6 +3,28 @@
 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 $expect in
+	expect_nomatch)
+		match=nomatch
+		;;
+	*)
+		match=match
+		;;
+	esac
+
+	test_expect_success "log $kind${rest:+ $rest} ($match)" "
+		git log $rest $kind $needle --format=%H >actual &&
+		test_cmp $expect actual
+	"
+}
+
 test_expect_success setup '
 	>expect_nomatch &&
 
@@ -44,35 +66,12 @@ test_expect_success 'log --author -i' '
 	test_cmp expect_second actual
 '
 
-test_expect_success 'log -G (nomatch)' '
-	git log -Gpicked --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
-
-test_expect_success 'log -G (match)' '
-	git log -GPicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log -G --regexp-ignore-case (nomatch)' '
-	git log --regexp-ignore-case -Gpickle --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
-
-test_expect_success 'log -G -i (nomatch)' '
-	git log -i -Gpickle --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
-
-test_expect_success 'log -G --regexp-ignore-case (match)' '
-	git log --regexp-ignore-case -Gpicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log -G -i (match)' '
-	git log -i -Gpicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
+test_log expect_nomatch -G picked
+test_log expect_second  -G Picked
+test_log expect_nomatch -G pickle --regexp-ignore-case
+test_log expect_nomatch -G pickle -i
+test_log expect_second  -G picked --regexp-ignore-case
+test_log expect_second  -G picked -i
 
 test_expect_success 'log -G --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
@@ -87,35 +86,12 @@ test_expect_success 'log -G --no-textconv (missing textconv tool)' '
 	rm .gitattributes
 '
 
-test_expect_success 'log -S (nomatch)' '
-	git log -Spicked --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
-
-test_expect_success 'log -S (match)' '
-	git log -SPicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log -S --regexp-ignore-case (match)' '
-	git log --regexp-ignore-case -Spicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log -S -i (match)' '
-	git log -i -Spicked --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log -S --regexp-ignore-case (nomatch)' '
-	git log --regexp-ignore-case -Spickle --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
-
-test_expect_success 'log -S -i (nomatch)' '
-	git log -i -Spickle --format=%H >actual &&
-	test_cmp expect_nomatch actual
-'
+test_log expect_nomatch -S picked
+test_log expect_second  -S Picked
+test_log expect_second  -S picked --regexp-ignore-case
+test_log expect_second  -S picked -i
+test_log expect_nomatch -S pickle --regexp-ignore-case
+test_log expect_nomatch -S pickle -i
 
 test_expect_success 'log -S --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-- 
1.9.1

[PATCH 03/10] t4209: factor out helper function test_log_icase()

[René Scharfe]

Reduce code duplication by introducing test_log_icase() that runs the
same test with both --regexp-ignore-case and -i.  The specification of
the four basic test scenarios (matching/nomatching combined with case
sensitive/insensitive) becomes easier to read and write.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4209-log-pickaxe.sh | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 9f3bb40..dd911c2 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -25,6 +25,11 @@ test_log() {
 	"
 }
 
+test_log_icase() {
+	test_log $@ --regexp-ignore-case
+	test_log $@ -i
+}
+
 test_expect_success setup '
 	>expect_nomatch &&
 
@@ -66,12 +71,10 @@ test_expect_success 'log --author -i' '
 	test_cmp expect_second actual
 '
 
-test_log expect_nomatch -G picked
-test_log expect_second  -G Picked
-test_log expect_nomatch -G pickle --regexp-ignore-case
-test_log expect_nomatch -G pickle -i
-test_log expect_second  -G picked --regexp-ignore-case
-test_log expect_second  -G picked -i
+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 &&
@@ -86,12 +89,10 @@ test_expect_success 'log -G --no-textconv (missing textconv tool)' '
 	rm .gitattributes
 '
 
-test_log expect_nomatch -S picked
-test_log expect_second  -S Picked
-test_log expect_second  -S picked --regexp-ignore-case
-test_log expect_second  -S picked -i
-test_log expect_nomatch -S pickle --regexp-ignore-case
-test_log expect_nomatch -S pickle -i
+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 --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-- 
1.9.1

Re: [PATCH 03/10] t4209: factor out helper function test_log_icase()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Reduce code duplication by introducing test_log_icase() that runs the
> same test with both --regexp-ignore-case and -i.  The specification of
> the four basic test scenarios (matching/nomatching combined with case
> sensitive/insensitive) becomes easier to read and write.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  t/t4209-log-pickaxe.sh | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 9f3bb40..dd911c2 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -25,6 +25,11 @@ test_log() {
>  	"
>  }
>  
> +test_log_icase() {
> +	test_log $@ --regexp-ignore-case
> +	test_log $@ -i

&&-cascade broken?  Will squash in an obvious fix.

> +}

Re: [PATCH 03/10] t4209: factor out helper function test_log_icase()

[Jeff King]

On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:

> > +test_log_icase() {
> > +	test_log $@ --regexp-ignore-case
> > +	test_log $@ -i
> 
> &&-cascade broken?  Will squash in an obvious fix.

I don't think so. This is happening outside of test_expect_success,
which is run by test_log. So adding a && means that if the first test
fails, we do not bother to run the second one at all, which is not what
we want.

-Peff

Re: [PATCH 03/10] t4209: factor out helper function test_log_icase()

[René Scharfe]

Am 24.03.2014 22:10, schrieb Jeff King:
> On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:
>
>>> +test_log_icase() {
>>> +	test_log $@ --regexp-ignore-case
>>> +	test_log $@ -i
>>
>> &&-cascade broken?  Will squash in an obvious fix.
>
> I don't think so. This is happening outside of test_expect_success,
> which is run by test_log. So adding a && means that if the first test
> fails, we do not bother to run the second one at all, which is not what
> we want.

Right; this function runs two independent tests and && is left out 
intentionally.

René

Re: [PATCH 03/10] t4209: factor out helper function test_log_icase()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 24.03.2014 22:10, schrieb Jeff King:
>> On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:
>>
>>>> +test_log_icase() {
>>>> +	test_log $@ --regexp-ignore-case
>>>> +	test_log $@ -i
>>>
>>> &&-cascade broken?  Will squash in an obvious fix.
>>
>> I don't think so. This is happening outside of test_expect_success,
>> which is run by test_log. So adding a && means that if the first test
>> fails, we do not bother to run the second one at all, which is not what
>> we want.
>
> Right; this function runs two independent tests and && is left out
> intentionally.

Ahh, sorry and thanks.

[PATCH 04/10] t4209: use helper functions to test --grep

[René Scharfe]

Also add tests for non-matching cases.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4209-log-pickaxe.sh | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index dd911c2..873a10e 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -46,20 +46,10 @@ test_expect_success setup '
 	git rev-parse --verify HEAD >expect_second
 '
 
-test_expect_success 'log --grep' '
-	git log --grep=initial --format=%H >actual &&
-	test_cmp expect_initial actual
-'
-
-test_expect_success 'log --grep --regexp-ignore-case' '
-	git log --regexp-ignore-case --grep=InItial --format=%H >actual &&
-	test_cmp expect_initial actual
-'
-
-test_expect_success 'log --grep -i' '
-	git log -i --grep=InItial --format=%H >actual &&
-	test_cmp expect_initial 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 --author --regexp-ignore-case' '
 	git log --regexp-ignore-case --author=person --format=%H >actual &&
-- 
1.9.1

Re: [PATCH 04/10] t4209: use helper functions to test --grep

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> -test_expect_success 'log --grep -i' '
> -	git log -i --grep=InItial --format=%H >actual &&
> -	test_cmp expect_initial actual
> -'
> +test_log	expect_initial	--grep initial
> +test_log	expect_nomatch	--grep InItial

This, and the next --author one, assumes that we will never break
"--grep=foo" without breaking "--grep foo".  That should be OK, but
we might want to add separate tests e.g.

	test_log expect_initial --grep=initial

perhaps?  I dunno.

Queued without any tweaks for now.  Thanks.

Re: [PATCH 04/10] t4209: use helper functions to test --grep

[Jeff King]

On Mon, Mar 24, 2014 at 11:22:58AM -0700, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
> > -test_expect_success 'log --grep -i' '
> > -	git log -i --grep=InItial --format=%H >actual &&
> > -	test_cmp expect_initial actual
> > -'
> > +test_log	expect_initial	--grep initial
> > +test_log	expect_nomatch	--grep InItial
> 
> This, and the next --author one, assumes that we will never break
> "--grep=foo" without breaking "--grep foo".  That should be OK, but
> we might want to add separate tests e.g.
> 
> 	test_log expect_initial --grep=initial
> 
> perhaps?  I dunno.

Yeah, I I'd prefer "--grep=" here (and in all scripts).  In general, I
think our attitude should be that "--foo=bar" is guaranteed to work
forever, but "--foo bar" is not. The latter only works if the argument
is non-optional, so that leaves us room to "break" compatibility to make
an argument optional in a future version.

Now, whether the rest of the world and its script-writers are aware of
this fact, I don't know. So it may be too late already (but it does look
like we mention it in gitcli(7)).

-Peff

Re: [PATCH 04/10] t4209: use helper functions to test --grep

[René Scharfe]

Am 24.03.2014 22:14, schrieb Jeff King:
> On Mon, Mar 24, 2014 at 11:22:58AM -0700, Junio C Hamano wrote:
> 
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> -test_expect_success 'log --grep -i' '
>>> -	git log -i --grep=InItial --format=%H >actual &&
>>> -	test_cmp expect_initial actual
>>> -'
>>> +test_log	expect_initial	--grep initial
>>> +test_log	expect_nomatch	--grep InItial
>>
>> This, and the next --author one, assumes that we will never break
>> "--grep=foo" without breaking "--grep foo".  That should be OK, but
>> we might want to add separate tests e.g.
>>
>> 	test_log expect_initial --grep=initial
>>
>> perhaps?  I dunno.
> 
> Yeah, I I'd prefer "--grep=" here (and in all scripts).  In general, I
> think our attitude should be that "--foo=bar" is guaranteed to work
> forever, but "--foo bar" is not. The latter only works if the argument
> is non-optional, so that leaves us room to "break" compatibility to make
> an argument optional in a future version.
> 
> Now, whether the rest of the world and its script-writers are aware of
> this fact, I don't know. So it may be too late already (but it does look
> like we mention it in gitcli(7)).

OK, then the following should be squashed into patch 2 (t4209: factor out
helper function test_log()):

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 9f3bb40..f47231a 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -10,6 +10,14 @@ test_log() {
 	shift 3
 	rest=$@
 
+	case $kind in
+	--*)
+		opt=$kind=$needle
+		;;
+	*)
+		opt=$kind$needle
+		;;
+	esac
 	case $expect in
 	expect_nomatch)
 		match=nomatch
@@ -20,7 +28,7 @@ test_log() {
 	esac
 
 	test_expect_success "log $kind${rest:+ $rest} ($match)" "
-		git log $rest $kind $needle --format=%H >actual &&
+		git log $rest $opt --format=%H >actual &&
 		test_cmp $expect actual
 	"
 }

[PATCH 05/10] t4209: use helper functions to test --author

[René Scharfe]

Also add tests for case sensitive and non-matching cases.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4209-log-pickaxe.sh | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 873a10e..80aabe2 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -51,15 +51,10 @@ test_log	expect_nomatch	--grep InItial
 test_log_icase	expect_initial	--grep InItial
 test_log_icase	expect_nomatch	--grep initail
 
-test_expect_success 'log --author --regexp-ignore-case' '
-	git log --regexp-ignore-case --author=person --format=%H >actual &&
-	test_cmp expect_second actual
-'
-
-test_expect_success 'log --author -i' '
-	git log -i --author=person --format=%H >actual &&
-	test_cmp expect_second 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_log	expect_nomatch	-G picked
 test_log	expect_second	-G Picked
-- 
1.9.1

[PATCH 06/10] pickaxe: honor -i when used with -S and --pickaxe-regex

[René Scharfe]

accccde4 (pickaxe: allow -i to search in patch case-insensitively)
allowed case-insenitive matching for -G and -S, but for the latter
only if fixed string matching is used.  Allow it for -S and regular
expression matching as well to make the support complete.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diffcore-pickaxe.c     | 5 ++++-
 t/t4209-log-pickaxe.sh | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 401eb72..cb75851 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -237,7 +237,10 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 
 	if (opts & DIFF_PICKAXE_REGEX) {
 		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];
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 80aabe2..a31388f 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -79,6 +79,11 @@ test_log	expect_second	-S Picked
 test_log_icase	expect_second	-S picked
 test_log_icase	expect_nomatch	-S pickle
 
+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 &&
 	test_must_fail git -c diff.test.textconv=missing log -Sfoo &&
-- 
1.9.1

[PATCH 07/10] pickaxe: merge diffcore_pickaxe_grep() and diffcore_pickaxe_count() into diffcore_pickaxe()

[René Scharfe]

diffcore_pickaxe_count() initializes the regular expression or kwset for
the search term, calls pickaxe() with the callback has_changes() and
cleans up afterwards.  diffcore_pickaxe_grep() does the same, only it
doesn't support kwset and uses the callback diff_grep() instead.  Merge
the two functions to form the new diffcore_pickaxe() and thus get rid of
the duplicate regex setup and cleanup code.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diffcore-pickaxe.c | 48 +++++++++---------------------------------------
 1 file changed, 9 insertions(+), 39 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cb75851..cfc4262 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -108,29 +108,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;
@@ -227,7 +204,7 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	return ret;
 }
 
-static void diffcore_pickaxe_count(struct diff_options *o)
+void diffcore_pickaxe(struct diff_options *o)
 {
 	const char *needle = o->pickaxe;
 	int opts = o->pickaxe_opts;
@@ -235,7 +212,7 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 	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;
 		int cflags = REG_EXTENDED | REG_NEWLINE;
 		if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
@@ -256,20 +233,13 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 		kwsprep(kws);
 	}
 
-	pickaxe(&diff_queued_diff, o, regexp, kws, has_changes);
-
-	if (opts & DIFF_PICKAXE_REGEX)
-		regfree(&regex);
-	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);
+	pickaxe(&diff_queued_diff, o, regexp, kws,
+		(opts & DIFF_PICKAXE_KIND_G) ? diff_grep : has_changes);
+
+	if (regexp)
+		regfree(regexp);
 	else
-		diffcore_pickaxe_count(o);
+		kwsfree(kws);
+	return;
 }
-- 
1.9.1

[PATCH 08/10] pickaxe: move pickaxe() after pickaxe_match()

[René Scharfe]

pickaxe() calls pickaxe_match(); moving the definition of the former
those after the latter allows us to do without an explicit function
declaration.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diffcore-pickaxe.c | 79 ++++++++++++++++++++++++++----------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cfc4262..827a7d7 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;
@@ -204,6 +163,44 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	return ret;
 }
 
+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;
-- 
1.9.1

Re: [PATCH 08/10] pickaxe: move pickaxe() after pickaxe_match()

[Jeff King]

On Sat, Mar 22, 2014 at 06:15:58PM +0100, René Scharfe wrote:

> pickaxe() calls pickaxe_match(); moving the definition of the former
> those after the latter allows us to do without an explicit function
> declaration.

s/those //

-Peff

[PATCH 09/10] pickaxe: call strlen only when necessary in diffcore_pickaxe_count()

[René Scharfe]

We need to determine the search term's length only when fixed-string
matching is used; regular expression compilation takes a NUL-terminated
string directly.  Only call strlen() in the former case.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diffcore-pickaxe.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 827a7d7..70753d0 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -205,7 +205,6 @@ 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;
 
@@ -226,7 +225,7 @@ void diffcore_pickaxe(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);
 	}
 
-- 
1.9.1

[PATCH 10/10] pickaxe: simplify kwset loop in contains()

[René Scharfe]

Inlining the variable "found" actually makes the code shorter and
easier to read.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diffcore-pickaxe.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 70753d0..185f86b 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -94,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++;
 		}
 	}
-- 
1.9.1

Re: [PATCH 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> This series allows 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.  Its first
> half refactors the test script and extends test coverage a bit while
> we're at it.  The actual change is in the sixth patch.  It enables the
> two following cleanups.  The last two patches are independent simple
> cleanups.

Very nice.  Thanks.

Re: [PATCH 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[Jeff King]

On Sat, Mar 22, 2014 at 06:15:50PM +0100, René Scharfe wrote:

> This series allows 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.  Its first
> half refactors the test script and extends test coverage a bit while
> we're at it.  The actual change is in the sixth patch.  It enables the
> two following cleanups.  The last two patches are independent simple
> cleanups.

This all looks very nice.

Upon reading your cover letter, I wondered if somebody would ever want
case-sensitive "--grep", but case-insensitive pickaxe (or vice versa).
However, you are not adding case-insensitivity to -G/-S, but rather
applying it more consistently. So I think that ship has already sailed
anyway (and even if it did not, I think applying "-i" everywhere makes
the interface simpler for the common cases, so the loss of flexibility
may be worth it).

-Peff