grep: allow to use textconv filters

Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same:

ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)

"git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping.

Introduce a binary mode "--textconv" (in addition to "--text" and "-I") which makes git grep use any configured textconv filters for grepping and output purposes.

The difficulty is in getting the different cases (blob/sha1 vs. worktree) right, and in making the changes minimally invasive. It seems that some more refactoring could help: "git show --textconv" does not use textconv filters when used on blobs either. (It does for diffs, of course.) Most existing helper functions are tailored for diffs.

Nota bene: --textconv does not affect "diff --stat" either...

(quoted diff) +1 -1

-static int grep_source_load_sha1(struct grep_source *gs)
+static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)
 {

(quoted diff) +0 -0

 	enum object_type type;

(quoted diff) +1 -2

-void grep_source_load_driver(struct grep_source *gs)
+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)
 {
 	if (gs->driver)
 		return;
 
-	grep_attr_lock();

Quoted 8 lines

Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same:

ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)

Quoted 6 lines

"git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping.

Introduce a binary mode "--textconv" (in addition to "--text" and "-I") which makes git grep use any configured textconv filters for grepping and output purposes.

Sounds like a reasonable goal.

Quoted 5 lines

The difficulty is in getting the different cases (blob/sha1 vs. worktree) right, and in making the changes minimally invasive. It seems that some more refactoring could help: "git show --textconv" does not use textconv filters when used on blobs either. (It does for diffs, of course.) Most existing helper functions are tailored for diffs.

I think "git show" with blobs originally did not because we have no filename with which to look up the attributes. IIRC, the patches to support "cat-file --textconv" taught get_sha1_with_context to report the path at which we found a blob. I suspect it is mostly a matter of plumbing that information from the revision parser through to show_blob_object.

Quoted 1 line

Nota bene: --textconv does not affect "diff --stat" either...

Yeah, though I wonder if it should be on by default. The diffstat for a binary file, unlike the diff, is already useful. The diffstat of the textconv'd data may _also_ be useful, of course.

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 664

Quoted 7 lines

builtin/grep.c +3 -0

@@ -659,6 +659,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('I', NULL, &opt.binary,
 			N_("don't match patterns in binary files"),
 			GREP_BINARY_NOMATCH),
+		OPT_SET_INT(0, "textconv", &opt.binary,
+			N_("process binary files with textconv filters"),
+			GREP_BINARY_TEXTCONV),

Is this really a new form of GREP_BINARY_*? What happens when a file does not have a textconv filter?

I would expect this to be more like diff's "--textconv" flag, which is really "allow textconv to be respected". Then you could do:

  git grep -I --textconv foo

to grep in the text version of files which support it, and ignore the rest.

Review int cmd_grep(int argc, const char **argv, const char *prefix) · line 666

Quoted 4 lines

(quoted diff) +2 -2

-static int grep_source_load(struct grep_source *gs);
-static int grep_source_is_binary(struct grep_source *gs);
+static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);
+static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);

Hmm. grep_source_load is more or less the analogue of diff_populate_filespec, which does not know about textconv at all. So I feel like this might be going in at the wrong layer...

Review static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle · line 1365

Quoted 13 lines

(quoted diff) +2 -2

@@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 
 	switch (opt->binary) {
 	case GREP_BINARY_DEFAULT:
-		if (grep_source_is_binary(gs))
+		if (grep_source_is_binary(gs, opt))
 			binary_match_only = 1;
 		break;
 	case GREP_BINARY_NOMATCH:
-		if (grep_source_is_binary(gs))
+		if (grep_source_is_binary(gs, opt))
 			return 0; /* Assume unmatch */
 		break;

The is_binary function learned about "opt" so that it could pass it to grep_source_load, which might do the textconv for us. But we _know_ that we will not here, because we see that we have other GREP_BINARY flags.

And when we do have _TEXTCONV:

Review static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle · line 1368

Quoted 3 lines

(quoted diff) +1 -0

 	case GREP_BINARY_TEXT:
+	case GREP_BINARY_TEXTCONV:
 		break;

We do not call is_binary. :)

Quoted 4 lines

(quoted diff) +1 -1

-static int grep_source_load_sha1(struct grep_source *gs)
+static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)
 {

(quoted diff) +0 -0

 	enum object_type type;

Review

Quoted 10 lines

(quoted diff) +7 -2

-
 	grep_read_lock();
-	gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
+	if (opt->binary == GREP_BINARY_TEXTCONV) {
+		struct diff_filespec *df = alloc_filespec(gs->name);
+		gs->size = fill_textconv(gs->driver, df, &gs->buf);
+		free_filespec(df);
+	} else {
+		gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
+	}

So here we do the textconv for the sha1 case. But what about file sources?

This is why I think the layer is wrong; you want the fill_textconv function to call your abstract _load function (in the diff_filespec world, it is diff_filespec_populate, but it is the moral equivalent). And you want it to hold off as long as possible in case we can pull the value from cache, or feed the working tree version of a file straight to the filter.

Quoted 7 lines

(quoted diff) +1 -2

-void grep_source_load_driver(struct grep_source *gs)
+void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)
 {
 	if (gs->driver)
 		return;
 
-	grep_attr_lock();

Review

Quoted 10 lines

(quoted diff) +4 -0

+	grep_attr_lock(); //TODO
+	printf("Looking up userdiff driver for: %s", gs->path);
 	if (gs->path)
 		gs->driver = userdiff_find_by_path(gs->path);
 	if (!gs->driver)
 		gs->driver = userdiff_find_by_name("default");
+	if (opt->binary == GREP_BINARY_TEXTCONV)
+		gs->driver = userdiff_get_textconv(gs->driver);
 	grep_attr_unlock();
 }

This is wrong. The point of userdiff_get_textconv is that it will return NULL when we are not doing textconv for this path. So you can use it like:

  struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);

  if (textconv) {
          /* ok, we are doing textconv. Call our fill_textconv
           * equivalent. */
  }
  else {
          /* nope, plain old file. */
  }

But by assigning it on top of gs->driver, you're going to end up with a NULL driver sometimes. And the post-condition of the load_driver function is that gs->driver always points to a valid driver (even if it is the default one). I wouldn't be surprised if this causes segfaults.

So I would do it more like the patch below. Only lightly tested by me. There are some refactoring opportunities if you want to bring grep_source and diff_filespec closer together.

---

builtin/grep.c +2 -0

index 8025964..915c8ef 100644
@@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('I', NULL, &opt.binary,
 			N_("don't match patterns in binary files"),
 			GREP_BINARY_NOMATCH),
+		OPT_BOOL(0, "textconv", &opt.allow_textconv,
+			 N_("process binary files with textconv filters")),
 		{ OPTION_INTEGER, 0, "max-depth", &opt.max_depth, N_("depth"),
 			N_("descend at most <depth> levels"), PARSE_OPT_NONEG,
 			NULL, 1 },

grep.c +86 -14

index 4bd1b8b..3880d64 100644
@@ -2,6 +2,8 @@
 #include "grep.h"
 #include "userdiff.h"
 #include "xdiff-interface.h"
+#include "diff.h"
+#include "diffcore.h"
 
 static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs);

@@ -1321,6 +1323,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
 	fwrite(buf, size, 1, stdout);
 }
 
+static int fill_textconv_grep(struct userdiff_driver *driver,
+			      struct grep_source *gs)
+{
+	struct diff_filespec *df;
+	char *buf;
+	size_t size;
+
+	if (!driver || !driver->textconv)
+		return grep_source_load(gs);
+
+	/*
+	 * The textconv interface is intimately tied to diff_filespecs, so we
+	 * have to pretend to be one. If we could unify the grep_source
+	 * and diff_filespec structs, this mess could just go away.
+	 */
+	df = alloc_filespec(gs->path);
+	switch (gs->type) {
+	case GREP_SOURCE_SHA1:
+		fill_filespec(df, gs->identifier, 1, 0100644);
+		break;
+	case GREP_SOURCE_FILE:
+		fill_filespec(df, null_sha1, 0, 0100644);
+		break;
+	default:
+		die("BUG: attempt to textconv something without a path?");
+	}
+
+	/*
+	 * fill_textconv is not remotely thread-safe; it may load objects
+	 * behind the scenes, and it modifies the global diff tempfile
+	 * structure.
+	 */
+	grep_read_lock();
+	size = fill_textconv(driver, df, &buf);
+	grep_read_unlock();
+	free_filespec(df);
+
+	/*
+	 * The normal fill_textconv usage by the diff machinery would just keep
+	 * the textconv'd buf separate from the diff_filespec. But much of the
+	 * grep code passes around a grep_source and assumes that its "buf"
+	 * pointer is the beginning of the thing we are searching. So let's
+	 * install our textconv'd version into the grep_source, taking care not
+	 * to leak any existing buffer.
+	 */
+	grep_source_clear_data(gs);
+	gs->buf = buf;
+	gs->size = size;
+
+	return 0;
+}
+
 static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)
 {
 	char *bol;

@@ -1331,6 +1385,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	unsigned count = 0;
 	int try_lookahead = 0;
 	int show_function = 0;
+	struct userdiff_driver *textconv = NULL;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 

@@ -1352,19 +1407,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	}
 	opt->last_shown = 0;
 
-	switch (opt->binary) {
-	case GREP_BINARY_DEFAULT:
-		if (grep_source_is_binary(gs))
-			binary_match_only = 1;
-		break;
-	case GREP_BINARY_NOMATCH:
-		if (grep_source_is_binary(gs))
-			return 0; /* Assume unmatch */
-		break;
-	case GREP_BINARY_TEXT:
-		break;
-	default:
-		die("bug: unknown binary handling mode");
+	if (opt->allow_textconv) {
+		grep_source_load_driver(gs);
+		/*
+		 * We might set up the shared textconv cache data here, which
+		 * is not thread-safe.
+		 */
+		grep_attr_lock();
+		textconv = userdiff_get_textconv(gs->driver);
+		grep_attr_unlock();
+	}
+
+	/*
+	 * We know the result of a textconv is text, so we only have to care
+	 * about binary handling if we are not using it.
+	 */
+	if (!textconv) {
+		switch (opt->binary) {
+		case GREP_BINARY_DEFAULT:
+			if (grep_source_is_binary(gs))
+				binary_match_only = 1;
+			break;
+		case GREP_BINARY_NOMATCH:
+			if (grep_source_is_binary(gs))
+				return 0; /* Assume unmatch */
+			break;
+		case GREP_BINARY_TEXT:
+			break;
+		default:
+			die("bug: unknown binary handling mode");
+		}
 	}
 
 	memset(&xecfg, 0, sizeof(xecfg));

@@ -1372,7 +1444,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 
 	try_lookahead = should_lookahead(opt);
 
-	if (grep_source_load(gs) < 0)
+	if (fill_textconv_grep(textconv, gs) < 0)
 		return 0;
 
 	bol = gs->buf;

grep.h +0 -0

index 8fc854f..94a7ac2 100644

Thanks Jeff, that helps a lot! It covers "grep expr" and "grep expr rev -- path" just fine. I'll look into "grep expr rev:path" which does not work yet because of an empty driver.

I also have "show --textconv" covered and a suggestion for "cat-file --textconv" (to work without a textconv filter).

Expect a mini-series soon :)

Quoted 9 lines

builtin/log.c +0 -0

index 8f0b2e8..f83870d 100644
@@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	strbuf_release(&out);
 }
 

builtin/log.c static void show_tagger(char *buf, int len, struct rev_info *rev) · line 405

Quoted 2 lines

builtin/log.c +1 -1

-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)

Should this maybe just take the whole object_array_entry as a cleanup?

Quoted 9 lines

builtin/log.c +6 -1

 {
+	unsigned char sha1c[20];
+	struct object_context obj_context;
+	char *buf;
+	unsigned long size;
+
 	fflush(stdout);
-	return stream_blob_to_fd(1, sha1, NULL, 0);
+	if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))

builtin/log.c static void show_tagger(char *buf, int len, struct rev_info *rev) · line 417

Quoted 4 lines

builtin/log.c +4 -0

+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+		die("Not a valid object name %s", obj_name);

It seems a little hacky that we have to look up the sha1 again. What should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to a race with a simultaneous update of a ref?

Would it be better if object_array_entry replaced its "mode" member with an object_context?

The only downside I see is that we might waste a
significant amount of memory (each context has a PATH_MAX buffer in it).

Quoted 2 lines

Would it be better if object_array_entry replaced its "mode" member with an object_context?

Do all callers/users want to deal with object_context?

I'm wondering why o_c has a mode at all, since it is mostly used in conjunction with an object, isn't it?

Quoted 5 lines

Quoted 3 lines

Would it be better if object_array_entry replaced its "mode" member with an object_context?

Do all callers/users want to deal with object_context?

Wouldn't it just mean replacing "entry->mode" with "entry->oc.mode" at each user?

Quoted 2 lines

I'm wondering why o_c has a mode at all, since it is mostly used in conjunction with an object, isn't it?

Just as we record the path from the surrounding tree, we record the mode. It's that mode which gets put into the pending object list by the revision parser (see the very end of handle_revision_arg). Storing an object_context instead of the mode would be a strict superset of what we store now (right now we just throw the rest away).

Quoted 5 lines

Just as we record the path from the surrounding tree, we record the mode. It's that mode which gets put into the pending object list by the revision parser (see the very end of handle_revision_arg). Storing an object_context instead of the mode would be a strict superset of what we store now (right now we just throw the rest away).

Sure. But why does object_context have a mode member at all? Maybe it is not alway used together with another struct which has the mode already, then that's a reason.

Currently, "diff" and "cat-file" for blobs obey "--textconv" options (with the former defaulting to "--textconv" and the latter to "--no-textconv") whereas "show" does not obey this option, even though it takes diff options.

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by default and "--no-textconv" when given.

Signed-off-by Michael J Gruber <[email protected]>

---
 builtin/log.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

builtin/log.c +21 -3

index 8f0b2e8..f83870d 100644
@@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	strbuf_release(&out);
 }
 
-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
 {
+	unsigned char sha1c[20];
+	struct object_context obj_context;
+	char *buf;
+	unsigned long size;
+
 	fflush(stdout);
-	return stream_blob_to_fd(1, sha1, NULL, 0);
+	if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+		die("Not a valid object name %s", obj_name);
+	if (!obj_context.path[0] ||
+	    !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))
+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (!buf)
+		die("git show %s: bad file", obj_name);
+
+	write_or_die(1, buf, size);
+	return 0;
 }
 
 static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)

@@ -491,7 +509,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		const char *name = objects[i].name;
 		switch (o->type) {
 		case OBJ_BLOB:
-			ret = show_blob_object(o->sha1, NULL);
+			ret = show_blob_object(o->sha1, &rev, name);
 			break;
 		case OBJ_TAG: {
 			struct tag *t = (struct tag *)o;

Quoted 7 lines · Michael J Gruber

Currently, "diff" and "cat-file" for blobs obey "--textconv" options (with the former defaulting to "--textconv" and the latter to "--no-textconv") whereas "show" does not obey this option, even though it takes diff options.

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by default and "--no-textconv" when given.

What does "log -p" do currently, and what should it do? Does/should it also use --textconv?

The --textconv is a natural extension of what --ext-diff provides us, so I think it should trigger the same way as how --ext-diff triggers.

We apply "--ext-diff" for "diff" by default but not for "log -p" and "show"; I suspect this may have been for a good reason but I do not recall the discussion that led to the current behaviour offhand.

And stating it that way makes it clear that there may be other missing steps (3 and up) to apply other gitattributes. For example, should "git show $blob" respect crlf attributes? Smudge filters?

Quoted 3 lines · Jeff King

And stating it that way makes it clear that there may be other missing steps (3 and up) to apply other gitattributes. For example, should "git show $blob" respect crlf attributes? Smudge filters?

Yeah, I think applying these when path is availble may make sense.

Are we going to teach cat-file to honor them with "--textconv" and possibly other new options?

Or should the "--textconv" imply application of these other filters? I am tempted to say yes, but I haven't thought things through...

git-show should probably have the same option. I doubt it should be on by default, but I can see it being useful for:

  git show --to-filesystem HEAD:Makefile >Makefile

or whatever. I don't think those features necessarily need to come as part of Michael's series.

Quoted 5 lines · Michael J Gruber

Currently, "diff" and "cat-file" for blobs obey "--textconv" options (with the former defaulting to "--textconv" and the latter to "--no-textconv") whereas "show" does not obey this option, even though it takes diff options.

Quoted 2 lines

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by default and "--no-textconv" when given.

What does "log -p" do currently, and what should it do? Does/should it also use --textconv?

The --textconv is a natural extension of what --ext-diff provides us, so I think it should trigger the same way as how --ext-diff triggers.

We apply "--ext-diff" for "diff" by default but not for "log -p" and "show"; I suspect this may have been for a good reason but I do not recall the discussion that led to the current behaviour offhand.

Quoted 49 lines

Signed-off-by Michael J Gruber <[email protected]>

---
 builtin/log.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

builtin/log.c +21 -3

index 8f0b2e8..f83870d 100644
@@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	strbuf_release(&out);
 }
 
-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
+static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
 {
+	unsigned char sha1c[20];
+	struct object_context obj_context;
+	char *buf;
+	unsigned long size;
+
 	fflush(stdout);
-	return stream_blob_to_fd(1, sha1, NULL, 0);
+	if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+		die("Not a valid object name %s", obj_name);
+	if (!obj_context.path[0] ||
+	    !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))
+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (!buf)
+		die("git show %s: bad file", obj_name);
+
+	write_or_die(1, buf, size);
+	return 0;
 }
 
 static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)

@@ -491,7 +509,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		const char *name = objects[i].name;
 		switch (o->type) {
 		case OBJ_BLOB:
-			ret = show_blob_object(o->sha1, NULL);
+			ret = show_blob_object(o->sha1, &rev, name);
 			break;
 		case OBJ_TAG: {
 			struct tag *t = (struct tag *)o;

When a command is supposed to use textconv filters (by default or with "--textconv") and none are configured then the blob is output without conversion; the only exception to this rule is "cat-file --textconv".

Make it behave like the rest of textconv aware commands.

Which made me wonder: what happens with:

  git cat-file --textconv HEAD

It looks like we die just before textconv-ing, because we have no obj_context.path. But that is also unlike all of the other --textconv switches, which mean "turn on textconv if you are showing a blob that supports it" and not "the specific operation is --textconv, apply it to this blob". I don't know if that is worth changing or not.

Quoted 9 lines · Jeff King

Which made me wonder: what happens with:

  git cat-file --textconv HEAD

It looks like we die just before textconv-ing, because we have no obj_context.path. But that is also unlike all of the other --textconv switches, which mean "turn on textconv if you are showing a blob that supports it" and not "the specific operation is --textconv, apply it to this blob". I don't know if that is worth changing or not.

OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should die with "Hey, that is not a blob", in other words, Michael's patch does what we want without further tweaks, right?

By the way are we sure textconv_object() barfs and dies if fed a non blob? Otherwise the above does not hold, and the semantics become "turn on textconv if the object you are showing supports it, otherwise it has to be a blob.", I think.

When a command is supposed to use textconv filters (by default or with "--textconv") and none are configured then the blob is output without conversion; the only exception to this rule is "cat-file --textconv".

Make it behave like the rest of textconv aware commands.

Signed-off-by Michael J Gruber <[email protected]>

---
 builtin/cat-file.c           |  9 +++++----
 t/t8007-cat-file-textconv.sh | 20 +++++---------------
 2 files changed, 10 insertions(+), 19 deletions(-)

builtin/cat-file.c +0 -0

index 00528dd..6912dc2 100644
@@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
 			    obj_name);

From: Jeff King <[email protected]>

Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same:

ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)

"git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping.

Introduce an option "--textconv" which makes git grep use any configured textconv filters for grepping and output purposes. It is off by default.

Signed-off-by Michael J Gruber <[email protected]>

Introduce an option "--textconv" which makes git grep use any configured textconv filters for grepping and output purposes. It is off by default.

Signed-off-by Michael J Gruber <[email protected]>

---
 builtin/grep.c |   2 ++
 grep.c         | 100 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 grep.h         |   1 +

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 831

Quoted 3 lines

builtin/grep.c +1 -1

-			add_object_array(object, arg, &list);
+			add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));

If we go this route, this new _with_context variant should be used in patch 1, too.

object.c void add_object_array_with_mode(struct object *obj, const char *name, struct obj · line 265

Quoted 7 lines

object.c +1 -0

@@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
 	objects[nr].item = obj;
 	objects[nr].name = name;
 	objects[nr].mode = mode;
+	objects[nr].context = context;
 	array->nr = ++nr;
 }

This seems a little gross. Who is responsible for allocating the context? Who frees it? It looks like we duplicate it in cmd_grep. Which

I think is OK, but it means all of this context infrastructure in object.[ch] is just bolted-on junk waiting for somebody to use it wrong or get confused. It does not get set, for example, by the regular setup_revisions code path.

It would be nice if we could just always have the context available, then setup_revisions could set it up by default (and replace the "mode" parameter entirely). But we'd need to do something to avoid the PATH_MAX-sized buffer for each entry, as some code paths may have a large number of pending objects.

Quoted 10 lines

object.c void add_object_array_with_mode(struct object *obj, const char *name, struct obj · line 265

Quoted 7 lines

object.c +1 -0

@@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
 	objects[nr].item = obj;
 	objects[nr].name = name;
 	objects[nr].mode = mode;
+	objects[nr].context = context;
 	array->nr = ++nr;
 }

This seems a little gross. Who is responsible for allocating the context? Who frees it? It looks like we duplicate it in cmd_grep. Which

Well, who is responsible for allocating and freeing name and item? I didn't want to introduce a new member which is a struct when all other complex members are pointers. Wouldn't that be confusing?

Quoted 12 lines

Quoted 11 lines

object.c void add_object_array_with_mode(struct object *obj, const char *name, struct obj · line 265

Quoted 8 lines

object.c +1 -0

@@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
 	objects[nr].item = obj;
 	objects[nr].name = name;
 	objects[nr].mode = mode;
+	objects[nr].context = context;
 	array->nr = ++nr;
 }

This seems a little gross. Who is responsible for allocating the context? Who frees it? It looks like we duplicate it in cmd_grep. Which

Quoted 3 lines

Well, who is responsible for allocating and freeing name and item? I didn't want to introduce a new member which is a struct when all other complex members are pointers. Wouldn't that be confusing?

We cheat on those two. "item" is always a pointer to a "struct object", which lasts forever and never gets freed. When "name" is set by setup_revisions, it comes from the argv list, which is assumed to last forever (and when we add pending blobs for a "--objects" traversal, it is the empty string (literal).

I'd be OK if we had an exterior object_context that could be handled in the same way. But how do we tell setup_revisions that we are interested in seeing the object_context from each parsed item, where does the allocation come from (is it malloc'd by setup_revisions?), and who is responsible for freeing it when we pop pending objects in get_revisions and similar?

I don't think it's as clear cut.

I wonder, though...what we really care about here is just the pathname. But if it is a pending object that comes from a blob revision argument, won't it always be of the form "treeish:path"? Could we not even resolve the sha1 again, but instead just parse out the ":path" bit?

That is sort of like what the repeated call to get_sha1_with_context does in your first patch. Except that we do not actually want to lookup the sha1, and it is harmful to do so (e.g., if the ref had moved on to a new tree that does not have that path, get_sha1 would fail, but we do not even care what is in the tree; we only want the parsing side effects of get_sha1).

Hmm.

-Peff

PS By the way, while looking at the object_array code (which I have not
really used much before), I noticed that add_pending_commit_list sets the "name" field to the result of sha1_to_hex. Which means that it is likely to be completely bogus by the time you read it. I'm not even sure where it gets read or if this matters. And obviously it's completely unrelated to what we were discussing; just something I noticed.

Quoted 6 lines

I'd be OK if we had an exterior object_context that could be handled in the same way. But how do we tell setup_revisions that we are interested in seeing the object_context from each parsed item, where does the allocation come from (is it malloc'd by setup_revisions?), and who is responsible for freeing it when we pop pending objects in get_revisions and similar?

Do we really need all of tree, path and mode in object_context (I mean not just here, but other users), or only the path? I'd try and resurrect the virtual path name objects then, they would be just like "item" storage-wise.

Quoted 6 lines

I don't think it's as clear cut.

I wonder, though...what we really care about here is just the pathname. But if it is a pending object that comes from a blob revision argument, won't it always be of the form "treeish:path"? Could we not even resolve the sha1 again, but instead just parse out the ":path" bit?

Do we have that, and in what form (e.g. magic expanded etc.)?

Another thing I noted is that our path mangling at least for grep has some issues:

(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Quoted 8 lines

Quoted 7 lines

I'd be OK if we had an exterior object_context that could be handled in the same way. But how do we tell setup_revisions that we are interested in seeing the object_context from each parsed item, where does the allocation come from (is it malloc'd by setup_revisions?), and who is responsible for freeing it when we pop pending objects in get_revisions and similar?

Quoted 4 lines

Do we really need all of tree, path and mode in object_context (I mean not just here, but other users), or only the path? I'd try and resurrect the virtual path name objects then, they would be just like "item" storage-wise.

We need at least mode, since that is how the mode parameter of object_array_entry gets set. I do not know off-hand who uses "tree". I suspect the intent was to do .gitattributes lookups inside that tree, but I do not think we actually do in-tree lookups currently.

Quoted 8 lines

Quoted 6 lines

I don't think it's as clear cut.

I wonder, though...what we really care about here is just the pathname. But if it is a pending object that comes from a blob revision argument, won't it always be of the form "treeish:path"? Could we not even resolve the sha1 again, but instead just parse out the ":path" bit?

Do we have that, and in what form (e.g. magic expanded etc.)?

Ah, I should have mentioned that. :) We should have the original rev name in the object_array_entry's name field, shouldn't we? It's just a matter of re-parsing it.

Quoted 5 lines

Another thing I noted is that our path mangling at least for grep has some issues:

(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Yuck.

Quoted 4 lines

Quoted 2 lines

(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Yuck.

And even more yuck:

(cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)
HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Someone does not expect a "rev:" to be in there, it seems ;)

Quoted 7 lines · Michael J Gruber

Quoted 4 lines

Quoted 2 lines

(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Yuck.

And even more yuck:

Quoted 4 lines

(cd t && git grep --full-name GET_SHA1_QUIETLY HEAD:../cache.h)
HEAD:../cache.h:#define GET_SHA1_QUIETLY 01

Someone does not expect a "rev:" to be in there, it seems ;)

I think stepping outside of $(cwd) is an afterthought the code does not anticipate.

Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same:

ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)

"git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping.

Introduce a binary mode "--textconv" (in addition to "--text" and "-I") which makes git grep use any configured textconv filters for grepping and output purposes.

Signed-off-by Michael J Gruber <[email protected]>

---

Notes:
I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out in request for help. I'm sure there are people for whom it's a breeze to get this right.

The difficulty is in getting the different cases (blob/sha1 vs. worktree) right, and in making the changes minimally invasive.

Quoted 17 lines · Michael J Gruber

Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same:

ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)

"git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping.

Introduce a binary mode "--textconv" (in addition to "--text" and "-I") which makes git grep use any configured textconv filters for grepping and output purposes.

Signed-off-by Michael J Gruber <[email protected]>

---

Quoted 4 lines

Notes:
I'm somehow stuck in textconv/filespec/... hell, so I'm sending this out in request for help. I'm sure there are people for whom it's a breeze to get this right.

Looks like the patch touches the right places in the codepaths that need to be updated from a quick read.

Quoted 2 lines

The difficulty is in getting the different cases (blob/sha1 vs. worktree) right, and in making the changes minimally invasive.

I think grep_source abstraction was intended to be a way for that, and if what it gives you is not sufficient for your needs, extending it should not be seen as "invasive" at all.