threads / patch / 23800

patch, 7 partsgrep: better support for binary files

Subject: [PATCH 0/7] grep: better support for binary files

## tl;dr

10 messages between May 13, 2010 and May 16, 2010. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

René Scharfe· May 13, 2010, 20:33 UTC · lore

This series improves support of git grep for binary files. It tries to pick the low hanging fruits; at the end you can search _in_ files that contain NUL characters, but you can't search _for_ NULs, yet.

[PATCH 1/7] grep: add test script for binary file handling
	This patch adds a simple test script documenting what git grep
	can do with binary files.

[PATCH 2/7] grep: refactor handling of binary mode options [PATCH 3/7] grep: --count over binary

	These two makes git grep handle counting in binary files like
	GNU grep does.

[PATCH 4/7] grep: use memmem() for fixed string search [PATCH 5/7] grep: continue case insensitive fixed string search after NUL chars

	These two patches make git grep -F work on binary files.

[PATCH 6/7] grep: add regmatch(), a wrapper for REG_STARTEND handling [PATCH 7/7] grep: use regmatch() for line matching

	The final patches make git grep work on binary files if the
	platform's regexec() supports the flag REG_STARTEND.  Our own
	version in compat/ doesn't, unfortunately.
 grep.c                 |   70 ++++++++++++++++++++++++++++-------------------
 t/t7008-grep-binary.sh |   54 +++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 28 deletions(-)
René Scharfe· May 13, 2010, 20:34 UTC · re: René Scharfe · lore

[PATCH 1/7] grep: add test script for binary file handling

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t7008-grep-binary.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t7008-grep-binary.sh
Show changes to t/t7008-grep-binary.sh +30 −0
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
new file mode 100755
index 0000000..f9fd5e6
--- /dev/null
+++ b/t/t7008-grep-binary.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='git grep in binary files'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' "
+	printf 'binary\000file\n' >a &&
+	git add a &&
+	git commit -m.
+"
+
+test_expect_success 'git grep ina a' '
+	echo Binary file a matches >expect &&
+	git grep ina a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git grep -ah ina a' '
+	git grep -ah ina a >actual &&
+	test_cmp a actual
+'
+
+test_expect_success 'git grep -I ina a' '
+	: >expect &&
+	test_must_fail git grep -I ina a >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.1
René Scharfe· May 13, 2010, 20:36 UTC · re: René Scharfe · lore

[PATCH 2/7] grep: refactor handling of binary mode options

Turn the switch inside-out and add labels for each possible value of ->binary. This makes the code easier to read and avoids calling buffer_is_binary() if the option -a was given.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
Show changes to grep.c +11 −9
diff --git a/grep.c b/grep.c
index 543b1d5..2a8e879 100644
--- a/grep.c
+++ b/grep.c
@@ -800,17 +800,19 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 		opt->show_hunk_mark = 1;
 	opt->last_shown = 0;
 
-	if (buffer_is_binary(buf, size)) {
-		switch (opt->binary) {
-		case GREP_BINARY_DEFAULT:
+	switch (opt->binary) {
+	case GREP_BINARY_DEFAULT:
+		if (buffer_is_binary(buf, size))
 			binary_match_only = 1;
-			break;
-		case GREP_BINARY_NOMATCH:
+		break;
+	case GREP_BINARY_NOMATCH:
+		if (buffer_is_binary(buf, size))
 			return 0; /* Assume unmatch */
-			break;
-		default:
-			break;
-		}
+		break;
+	case GREP_BINARY_TEXT:
+		break;
+	default:
+		die("bug: unknown binary handling mode");
 	}
 
 	memset(&xecfg, 0, sizeof(xecfg));
-- 
1.7.1
René Scharfe· May 13, 2010, 20:37 UTC · re: René Scharfe · lore

[PATCH 3/7] grep: --count over binary

The intent of showing the message "Binary file xyz matches" for binary files is to avoid annoying users by potentially messing up their terminals by printing control characters. In --count mode, this precaution isn't necessary.

Display counts of matches if -c/--count was specified, even if -a was not given. GNU grep does the same.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c                 |    2 +-
 t/t7008-grep-binary.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)
Show changes to 2 files +7 −1

grep.c, t/t7008-grep-binary.sh

diff --git a/grep.c b/grep.c
index 2a8e879..4b6c02e 100644
--- a/grep.c
+++ b/grep.c
@@ -802,7 +802,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 
 	switch (opt->binary) {
 	case GREP_BINARY_DEFAULT:
-		if (buffer_is_binary(buf, size))
+		if (!opt->count && buffer_is_binary(buf, size))
 			binary_match_only = 1;
 		break;
 	case GREP_BINARY_NOMATCH:
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index f9fd5e6..5449dd9 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -27,4 +27,10 @@ test_expect_success 'git grep -I ina a' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git grep -c ina a' '
+	echo a:1 >expect &&
+	git grep -c ina a >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.1
Dmitry Potapov· May 14, 2010, 09:34 UTC · re: René Scharfe · lore

Re: [PATCH 3/7] grep: --count over binary

On Thu, May 13, 2010 at 10:37:10PM +0200, René Scharfe wrote:
Show 7 quoted lines
> The intent of showing the message "Binary file xyz matches" for
> binary files is to avoid annoying users by potentially messing up
> their terminals by printing control characters.  In --count mode,
> this precaution isn't necessary.
> 
> Display counts of matches if -c/--count was specified, even if -a
> was not given.  GNU grep does the same.

It is also not necessary with '-l' and '-L' options. (At least, if we follow GNU grep).

Show 9 quoted lines
> --- a/grep.c
> +++ b/grep.c
> @@ -802,7 +802,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
>  
>  	switch (opt->binary) {
>  	case GREP_BINARY_DEFAULT:
> -		if (buffer_is_binary(buf, size))
> +		if (!opt->count && buffer_is_binary(buf, size))
>  			binary_match_only = 1;
So, I believe it should be:
		if (!opt->count && !opt->name_only && !opt->unmatch_name_only &&
				buffer_is_binary(buf, size))
Dmitry
René Scharfe· May 16, 2010, 17:45 UTC · re: Dmitry Potapov · lore

Re: [PATCH 3/7] grep: --count over binary

Am 14.05.2010 11:34, schrieb Dmitry Potapov:
Show 11 quoted lines
> On Thu, May 13, 2010 at 10:37:10PM +0200, René Scharfe wrote:
>> The intent of showing the message "Binary file xyz matches" for
>> binary files is to avoid annoying users by potentially messing up
>> their terminals by printing control characters.  In --count mode,
>> this precaution isn't necessary.
>>
>> Display counts of matches if -c/--count was specified, even if -a
>> was not given.  GNU grep does the same.
> 
> It is also not necessary with '-l' and '-L' options. (At least, if
> we follow GNU grep).

Good point. The same is true for -q, too. ->unmatch_name_only (-L) and ->status_only (-q) are already handled correctly because they are checked before binary_match_only. We can do the same for ->name_only.

-- >8 --
Subject: grep: --name-only over binary

As with the option -c/--count, git grep with the option -l/--name-only should work the same with binary files as with text files because there is no danger of messing up the terminal with control characters from the contents of matching files. GNU grep does the same.

Move the check for ->name_only before the one for binary_match_only, thus making the latter irrelevant for git grep -l. Also add a simple test for each of -l, -L and -q. The latter two options were already handled before binary_match_only, so no code changes were needed to make them pass.

Reported-by: Dmitry Potapov <dpotapov@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c                 |    8 ++++----
 t/t7008-grep-binary.sh |   18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)
Show changes to 2 files +22 −4

grep.c, t/t7008-grep-binary.sh

diff --git a/grep.c b/grep.c
index 2b2f70e..f292e25 100644
--- a/grep.c
+++ b/grep.c
@@ -885,6 +885,10 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			count++;
 			if (opt->status_only)
 				return 1;
+			if (opt->name_only) {
+				show_name(opt, name);
+				return 1;
+			}
 			if (binary_match_only) {
 				opt->output(opt, "Binary file ", 12);
 				output_color(opt, name, strlen(name),
@@ -892,10 +896,6 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 				opt->output(opt, " matches\n", 9);
 				return 1;
 			}
-			if (opt->name_only) {
-				show_name(opt, name);
-				return 1;
-			}
 			/* Hit at this line.  If we haven't shown the
 			 * pre-context lines, we would need to show them.
 			 * When asked to do "count", this still show
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index d8fde18..4f5e74f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -33,6 +33,24 @@ test_expect_success 'git grep -c ina a' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git grep -l ina a' '
+	echo a >expect &&
+	git grep -l ina a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git grep -L bar a' '
+	echo a >expect &&
+	git grep -L bar a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git grep -q ina a' '
+	: >expect &&
+	git grep -q ina a >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git grep -F ile a' '
 	git grep -F ile a
 '
-- 
1.7.1
René Scharfe· May 13, 2010, 20:38 UTC · re: René Scharfe · lore

[PATCH 4/7] grep: use memmem() for fixed string search

Allow searching beyond NUL characters by using memmem() instead of strstr().

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c                 |   16 +++++++++-------
 t/t7008-grep-binary.sh |    4 ++++
 2 files changed, 13 insertions(+), 7 deletions(-)
Show changes to 2 files +13 −7

grep.c, t/t7008-grep-binary.sh

diff --git a/grep.c b/grep.c
index 4b6c02e..4633b63 100644
--- a/grep.c
+++ b/grep.c
@@ -329,14 +329,15 @@ static void show_name(struct grep_opt *opt, const char *name)
 	opt->output(opt, opt->null_following_name ? "\0" : "\n", 1);
 }
 
-
-static int fixmatch(const char *pattern, char *line, int ignore_case, regmatch_t *match)
+static int fixmatch(const char *pattern, char *line, char *eol,
+		    int ignore_case, regmatch_t *match)
 {
 	char *hit;
+
 	if (ignore_case)
 		hit = strcasestr(line, pattern);
 	else
-		hit = strstr(line, pattern);
+		hit = memmem(line, eol - line, pattern, strlen(pattern));
 
 	if (!hit) {
 		match->rm_so = match->rm_eo = -1;
@@ -399,7 +400,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 
  again:
 	if (p->fixed)
-		hit = !fixmatch(p->pattern, bol, p->ignore_case, pmatch);
+		hit = !fixmatch(p->pattern, bol, eol, p->ignore_case, pmatch);
 	else
 		hit = !regexec(&p->regexp, bol, 1, pmatch, eflags);
 
@@ -725,9 +726,10 @@ static int look_ahead(struct grep_opt *opt,
 		int hit;
 		regmatch_t m;
 
-		if (p->fixed)
-			hit = !fixmatch(p->pattern, bol, p->ignore_case, &m);
-		else {
+		if (p->fixed) {
+			hit = !fixmatch(p->pattern, bol, bol + *left_p,
+					p->ignore_case, &m);
+		} else {
 #ifdef REG_STARTEND
 			m.rm_so = 0;
 			m.rm_eo = *left_p;
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index 5449dd9..ad97720 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -33,4 +33,8 @@ test_expect_success 'git grep -c ina a' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git grep -F ile a' '
+	git grep -F ile a
+'
+
 test_done
-- 
1.7.1
René Scharfe· May 13, 2010, 20:39 UTC · re: René Scharfe · lore

[PATCH 5/7] grep: continue case insensitive fixed string search after NUL chars

Functions for C strings, like strcasestr(), can't see beyond NUL characters. Check if there is such an obstacle on the line and try again behind it.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c                 |   12 +++++++++---
 t/t7008-grep-binary.sh |    4 ++++
 2 files changed, 13 insertions(+), 3 deletions(-)
Show changes to 2 files +13 −3

grep.c, t/t7008-grep-binary.sh

diff --git a/grep.c b/grep.c
index 4633b63..20a02a2 100644
--- a/grep.c
+++ b/grep.c
@@ -334,9 +334,15 @@ static int fixmatch(const char *pattern, char *line, char *eol,
 {
 	char *hit;
 
-	if (ignore_case)
-		hit = strcasestr(line, pattern);
-	else
+	if (ignore_case) {
+		char *s = line;
+		do {
+			hit = strcasestr(s, pattern);
+			if (hit)
+				break;
+			s += strlen(s) + 1;
+		} while (s < eol);
+	} else
 		hit = memmem(line, eol - line, pattern, strlen(pattern));
 
 	if (!hit) {
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index ad97720..1143903 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -37,4 +37,8 @@ test_expect_success 'git grep -F ile a' '
 	git grep -F ile a
 '
 
+test_expect_success 'git grep -Fi iLE a' '
+	git grep -Fi iLE a
+'
+
 test_done
-- 
1.7.1
René Scharfe· May 13, 2010, 20:40 UTC · re: René Scharfe · lore

[PATCH 6/7] grep: add regmatch(), a wrapper for REG_STARTEND handling

Refactor REG_STARTEND handling into a new helper, regmatch().
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)
Show changes to grep.c +13 −9
diff --git a/grep.c b/grep.c
index 31c0e38..5be72cf 100644
--- a/grep.c
+++ b/grep.c
@@ -359,6 +359,17 @@ static int fixmatch(const char *pattern, char *line, char *eol,
 	}
 }
 
+static int regmatch(const regex_t *preg, char *line, char *eol,
+		    regmatch_t *match, int eflags)
+{
+#ifdef REG_STARTEND
+	match->rm_so = 0;
+	match->rm_eo = eol - line;
+	eflags |= REG_STARTEND;
+#endif
+	return regexec(preg, line, 1, match, eflags);
+}
+
 static int strip_timestamp(char *bol, char **eol_p)
 {
 	char *eol = *eol_p;
@@ -738,15 +749,8 @@ static int look_ahead(struct grep_opt *opt,
 		if (p->fixed) {
 			hit = !fixmatch(p->pattern, bol, bol + *left_p,
 					p->ignore_case, &m);
-		} else {
-#ifdef REG_STARTEND
-			m.rm_so = 0;
-			m.rm_eo = *left_p;
-			hit = !regexec(&p->regexp, bol, 1, &m, REG_STARTEND);
-#else
-			hit = !regexec(&p->regexp, bol, 1, &m, 0);
-#endif
-		}
+		} else
+			hit = !regmatch(&p->regexp, bol, bol + *left_p, &m, 0);
 		if (!hit || m.rm_so < 0 || m.rm_eo < 0)
 			continue;
 		if (earliest < 0 || m.rm_so < earliest)
-- 
1.7.1
René Scharfe· May 13, 2010, 20:41 UTC · re: René Scharfe · lore

[PATCH 7/7] grep: use regmatch() for line matching

Use regmatch() in match_one_pattern(), allowing regex matching beyond NUL characters if regexec() supports the flag REG_STARTEND.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c                 |    2 +-
 t/t7008-grep-binary.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)
Show changes to 2 files +11 −1

grep.c, t/t7008-grep-binary.sh

diff --git a/grep.c b/grep.c
index 5be72cf..9dd2471 100644
--- a/grep.c
+++ b/grep.c
@@ -422,7 +422,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 	if (p->fixed)
 		hit = !fixmatch(p->pattern, bol, eol, p->ignore_case, pmatch);
 	else
-		hit = !regexec(&p->regexp, bol, 1, pmatch, eflags);
+		hit = !regmatch(&p->regexp, bol, eol, pmatch, eflags);
 
 	if (hit && p->word_regexp) {
 		if ((pmatch[0].rm_so < 0) ||
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index 1143903..d8fde18 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -41,4 +41,14 @@ test_expect_success 'git grep -Fi iLE a' '
 	git grep -Fi iLE a
 '
 
+# This test actually passes on platforms where regexec() supports the
+# flag REG_STARTEND.
+test_expect_failure 'git grep ile a' '
+	git grep ile a
+'
+
+test_expect_failure 'git grep .fi a' '
+	git grep .fi a
+'
+
 test_done
-- 
1.7.1

← back to recent threads