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

From: Jeff King <peff@peff.net>
Date: Feb 6, 2013, 22:23 UTC
Message-ID: <20130206222344.GE27507@sigill.intra.peff.net>
In-Reply-To: <da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net>

On Wed, Feb 06, 2013 at 04:08:52PM +0100, Michael J Gruber wrote:

Show 16 quoted lines

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

Show 10 quoted lines

> +	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