git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Don't search files with an unset "grep" attribute

From
Jeff King <peff@peff.net>
Date
Feb 1, 2012, 08:20 UTC
Message-ID
<20120201082005.GA32348@sigill.intra.peff.net>
In-Reply-To
<7vhazb3rtm.fsf@alter.siamese.dyndns.org>
On Wed, Feb 01, 2012 at 12:01:41AM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> > So if this was all spelled:
> >
> >   $ cat .gitattributes
> >   *.pdf filetype=pdf
> >   $ cat .git/config
> >   [filetype "pdf"]
> >           binary = true
> >           textconv = pdf2txt
> >
> > I think it would be a no-brainer that those type attributes should apply
> > to "git grep".
> 
> I think this discussion has, instead of forking into two equally
> interesting subthreads, veered to a more intellectually stimulating
> tangent and we ended up losing focus.
That's what I'm here for.
Show 8 quoted lines
> Regardless of what to do with "I do not want to grep in these types of
> files" and "I want textconv applied when grepping in these types", which
> would be new attributes to implement two new features, I would like to see
> us first concentrate on fixing the "binary" issue.  When somebody tells us
> "Your autodetection may screw it up, but this file is binary; just show
> 'Binary files differ.' when comparing." with "-diff" (or "binary"), we
> should honor that when "git grep" decides if it should take the 'Binary
> file matches' codepath.  We currently do not, and it clearly is a bug.

Right. It may have been lost in the verbosity of what I wrote in my previous email, but I completely agree. With the caveat that one should also respect "diff=foo" coupled with "diff.foo.binary = true" as making something binary. But that is already handled transparently by the userdiff.[ch] code (which seems like the obvious entry point for grep to use for attribute lookup, and which we already use there for funcname lookup).

The trivial-ish patch is below.
Show 7 quoted lines
> We should have to teach the underlying machinery that matches pathspec
> about negative pathspec entries only once. After we have done so, all the
> callers, not just "git grep", should be able to take advantage of the
> change by just learning to place negative pathspec entries in the "struct
> pathspec" they pass to the machinery.  Doing anything else will lead to
> madness of adding ad-hoc "here we should further filter with the other
> negative 'struct pathspec'" in each and every application.
Yes, I agree.
> But I suspect that it would not materialize anytime soon.  And I also
> suspect that the correct handling of 'Binary file matches', which is a
> pure bugfix, should solve the original issue started these threads 90% in
> practice.

Also agree. Let's fix the bug and then give it some time to see whether people really want more explicit exclusions.

Here's the bug-fix patch. Not quite ready for inclusion, as it obviously needs tests and a commit message. Also, we can cache the result of the userdiff lookup so the funcname code doesn't have to look it up again.

---
diff --git a/grep.c b/grep.c
index b29d09c..d7ab054 100644
--- a/grep.c
+++ b/grep.c
@@ -960,6 +960,15 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
 	fwrite(buf, size, 1, stdout);
 }
 
+static int grep_buffer_is_binary(const char *path,
+				 char *buf, unsigned long size)
+{
+	struct userdiff_driver *drv = userdiff_find_by_path(path);
+	if (drv && drv->binary != -1)
+		return drv->binary;
+	return buffer_is_binary(buf, size);
+}
+
 static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			 char *buf, unsigned long size, int collect_hits)
 {
@@ -994,11 +1003,11 @@ 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 (grep_buffer_is_binary(name, buf, size))
 			binary_match_only = 1;
 		break;
 	case GREP_BINARY_NOMATCH:
-		if (buffer_is_binary(buf, size))
+		if (grep_buffer_is_binary(name, buf, size))
 			return 0; /* Assume unmatch */
 		break;
 	case GREP_BINARY_TEXT:
Previous: Junio C HamanoNext: Jeff King
Message 15 of 43 in “git-grep while excluding files in a blacklist”
  1. Dov GrobgeldJan 17, 2012
  2. Nguyen Thai Ngoc DuyJan 17, 2012
  3. Junio C HamanoJan 17, 2012
  4. Nguyen Thai Ngoc DuyJan 18, 2012
  5. Don't search files with an unset "grep" attributeconrad.irwin@gmail.com, Jan 23, 2012
  6. Junio C HamanoJan 23, 2012
  7. Don't search files with an unset "grep" attributeConrad Irwin, Jan 23, 2012
  8. Junio C HamanoJan 24, 2012
  9. Jeff KingJan 25, 2012
  10. Stephen BashJan 26, 2012
  11. Jeff KingJan 26, 2012
  12. Michael HaggertyJan 26, 2012
  13. Jeff KingJan 27, 2012
  14. Junio C HamanoFeb 1, 2012
  15. Jeff KingFeb 1, 2012
  16. Jeff KingFeb 1, 2012
  17. Conrad IrwinFeb 1, 2012
  18. Jeff KingFeb 1, 2012
  19. Jeff KingFeb 1, 2012
  20. Junio C HamanoFeb 2, 2012
  21. 1/2 grep: let grep_buffer callers specify a binary flagJeff King, Feb 1, 2012
  22. Junio C HamanoFeb 2, 2012
  23. Jeff KingFeb 2, 2012
  24. 0/9 respect binary attribute in grepJeff King, Feb 2, 2012
  25. 1/9 grep: make locking flag globalJeff King, Feb 2, 2012
  26. 2/9 grep: move sha1-reading mutex into low-level codeJeff King, Feb 2, 2012
  27. 3/9 grep: refactor the concept of "grep source" into an objectJeff King, Feb 2, 2012
  28. 4/9 convert git-grep to use grep_source interfaceJeff King, Feb 2, 2012
  29. 5/9 grep: drop grep_buffer's "name" parameterJeff King, Feb 2, 2012
  30. 6/9 grep: cache userdiff_driver in grep_sourceJeff King, Feb 2, 2012
  31. Junio C HamanoFeb 2, 2012
  32. Jeff KingFeb 2, 2012
  33. 7/9 grep: respect diff attributes for binary-nessJeff King, Feb 2, 2012
  34. 8/9 grep: load file data after checking binary-nessJeff King, Feb 2, 2012
  35. 9/9 grep: pre-load userdiff drivers when threadedJeff King, Feb 2, 2012
  36. Jeff KingFeb 2, 2012
  37. Thomas RastFeb 2, 2012
  38. Jeff KingFeb 2, 2012
  39. Junio C HamanoFeb 2, 2012
  40. Pete WyckoffFeb 4, 2012
  41. Jeff KingFeb 4, 2012
  42. 2/2 grep: respect diff attributes for binary-nessJeff King, Feb 1, 2012
  43. Junio C HamanoFeb 1, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.