Re: [PATCH] convert filter: supply path to external driver

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2010, 21:59 UTC
Message-ID: <7vzks1e84p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101219212925.GA7393@arf.padd.com>

Pete Wyckoff <pw@padd.com> writes:

Show 28 quoted lines

> Filtering to support keyword expansion may need the name of
> the file being filtered.  In particular, to support p4 keywords
> like
>
>     $File: //depot/product/dir/script.sh $
>
> the smudge filter needs to know the name of the file it is
> smudging.
>
> Add a "%s" conversion specifier to the gitattribute for filter.
> It will be expanded with the path name to the file when invoking
> the external filter command.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---
>
> pw@padd.com wrote on Sat, 18 Dec 2010 17:38 -0500:
>> I'm using git-p4 to import and work with upstream p4
>> repositories.  Some of the files are ktext, meaning they expect
>> expansion of $Id$ and similar identifiers.
>> 
>> Using the filter driver for this file, I can do the "clean" part
>> easily, but to calculate the "smudge" correctly, I need to know
>> the filename inside the filter driver.
>
> This works fine for me.  It is backward compatible, and leaves
> open the possibility of adding other % modifiers if we find
> a need later.

This is not backward compatible for people who wanted to use '%' literal on their filter command line for whatever reason, so please do not advertise as such. A fair argument you could make is "Even though this is not strictly backward compatible, it is very unlikely that people passed a literal % to their filter command line, and the benefit of being able to give the pathname information would outweigh the downside of not being compatible", and people can agree or disagree.

I am personally moderately negative about $any expansion$ (I don't use it myself, and I don't think sane people use it either). As far as I can tell, this should has no impact on the correctness and very little impact on the performance for people who do not use $any expansion$, so I am Ok with the patch.

Modulo one worry. Don't we have, or don't we at least plant to allow us to have, a facility to cache expensive blob conversion result, similar to the textconv caching? How would this change interact with two blobs that live in different paths?