grep: better support for binary files

PATCH In review René Scharfe 2 participants 10 messages Jun 15, 2016

Share & tools

René Scharfe

author

2016-06-15 22:48 UTC #0 lore raw

cover grep: better support for binary files

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

author

2016-06-15 22:48 UTC #1 lore raw

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

Signed-off-by Rene Scharfe <[email protected]>

---
 t/t7008-grep-binary.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t7008-grep-binary.sh

t/t7008-grep-binary.sh new file +30 -0

new file mode 100755
index 0000000..f9fd5e6
@@ -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

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #2 lore raw

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

In reply to René Scharfe

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 <[email protected]>

---
 grep.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

grep.c +11 -9

index 543b1d5..2a8e879 100644
@@ -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));

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #3 lore raw

PATCH 3/7 grep: --count over binary

In reply to René Scharfe

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 <[email protected]>

---
 grep.c                 |    2 +-
 t/t7008-grep-binary.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

grep.c +1 -1

index 2a8e879..4b6c02e 100644
@@ -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:

t/t7008-grep-binary.sh +6 -0

index f9fd5e6..5449dd9 100755
@@ -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

Signature · 1 line

1.7.1

Dmitry Potapov

2016-06-15 22:48 UTC #4 lore raw

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

Quoted 7 lines · René Scharfe

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

grep.c static int grep_buffer_1(struct grep_opt *opt, const char *name, · line 806

Quoted 9 lines

grep.c +1 -1

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

author

2016-06-15 22:48 UTC #5 lore raw

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

Am 14.05.2010 11:34, schrieb Dmitry Potapov:

Quoted 11 lines · René Scharfe

Quoted 7 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).

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 <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 grep.c                 |    8 ++++----
 t/t7008-grep-binary.sh |   18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

grep.c +4 -4

index 2b2f70e..f292e25 100644
@@ -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

t/t7008-grep-binary.sh +18 -0

index d8fde18..4f5e74f 100755
@@ -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
 '

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #6 lore raw

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

In reply to René Scharfe

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

Signed-off-by Rene Scharfe <[email protected]>

---
 grep.c                 |   16 +++++++++-------
 t/t7008-grep-binary.sh |    4 ++++
 2 files changed, 13 insertions(+), 7 deletions(-)

grep.c +9 -7

index 4b6c02e..4633b63 100644
@@ -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;

t/t7008-grep-binary.sh +4 -0

index 5449dd9..ad97720 100755
@@ -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

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #7 lore raw

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

In reply to René Scharfe

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 <[email protected]>

---
 grep.c                 |   12 +++++++++---
 t/t7008-grep-binary.sh |    4 ++++
 2 files changed, 13 insertions(+), 3 deletions(-)

grep.c +9 -3

index 4633b63..20a02a2 100644
@@ -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) {

t/t7008-grep-binary.sh +4 -0

index ad97720..1143903 100755
@@ -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

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #8 lore raw

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

In reply to René Scharfe

Refactor REG_STARTEND handling into a new helper, regmatch().

Signed-off-by Rene Scharfe <[email protected]>

---
 grep.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

grep.c +13 -9

index 31c0e38..5be72cf 100644
@@ -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)

Signature · 1 line

1.7.1

René Scharfe

author

2016-06-15 22:48 UTC #9 lore raw

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

In reply to René Scharfe

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

Signed-off-by Rene Scharfe <[email protected]>

---
 grep.c                 |    2 +-
 t/t7008-grep-binary.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

grep.c +1 -1

index 5be72cf..9dd2471 100644
@@ -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) ||

t/t7008-grep-binary.sh +10 -0

index 1143903..d8fde18 100755
@@ -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

Signature · 1 line

1.7.1