Re: [PATCH 5/6] grep: allow to use textconv filters

[Jeff King <peff@peff.net>]

On Fri, Apr 19, 2013 at 06:44:48PM +0200, Michael J Gruber wrote:

> From: Jeff King <peff@peff.net>
> 
> 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 <git@drmicha.warpmail.net>
> ---
>  builtin/grep.c         |   2 +
>  grep.c                 | 100 ++++++++++++++++++++++++++++++++++++++++++-------
>  grep.h                 |   1 +
>  t/t7008-grep-binary.sh |  18 +++++++++
>  4 files changed, 107 insertions(+), 14 deletions(-)

This patch, of course, is flawless. :)

Feel free to add:

  Signed-off-by: Jeff King <peff@peff.net>

> +	/*
> +	 * 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");
> +		}
>  	}

Junio mentioned checking the textconv output for binary-ness. Doing that
would involve removing the outer conditional here. But it's not quite so
simple, as we don't load the textconv results until later, and
grep_source_is_binary will lazily load the contents. I think it would be
sufficient to fill_textconv_grep() right before, and then
grep_source_is_binary would rely on the cached buffer.

-Peff