Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters

[Michael J Gruber <git@drmicha.warpmail.net>]

Jeff King venit, vidit, dixit 05.02.2013 12:13:
> On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:
> 
>> 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.
> 
> Sounds like a reasonable goal.
> 
>>     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.
> 
>>     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.
> 
>> @@ -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.
> 
>> -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...
> 
>> @@ -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:
> 
>>  	case GREP_BINARY_TEXT:
>> +	case GREP_BINARY_TEXTCONV:
>>  		break;
> 
> We do not call is_binary. :)
> 
>> -static int grep_source_load_sha1(struct grep_source *gs)
>> +static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)
>>  {
>>  	enum object_type type;
>> -
>>  	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.
> 
>> -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();
>> +	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.
> 
> ---
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8025964..915c8ef 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -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 },
> diff --git a/grep.c b/grep.c
> index 4bd1b8b..3880d64 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -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;
> diff --git a/grep.h b/grep.h
> index 8fc854f..94a7ac2 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -106,6 +106,7 @@ struct grep_opt {
>  #define GREP_BINARY_NOMATCH	1
>  #define GREP_BINARY_TEXT	2
>  	int binary;
> +	int allow_textconv;
>  	int extended;
>  	int use_reflog_filter;
>  	int pcre;
> 

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

Michael