Re: [RFC/PATCH 3/4] grep: allow to use textconv filters

[Jeff King <peff@peff.net>]

On Wed, Feb 06, 2013 at 04:08:52PM +0100, 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>

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

I'd really love to see the refactoring I talked about in my earlier
message. But as I'm not willing to devote the time to do it right now,
and I do not think this patch has any particular bugs, I think it is OK
as it gets the job done, and does not make the later refactoring any
harder.

The one ugliness that still remains is:

> +	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 lock/unlock the grep_attr_lock twice here: once in
grep_source_load_driver, and then immediately again to call
userdiff_get_textconv. I don't know if it is worth doing the two under
the same lock or not (I guess it should not increase lock contention,
since we do the same amount of work, so it is really just the extra lock
instructions).

-Peff