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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2010, 17:59 UTC
Message-ID: <7v8vzkcol8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101220160911.GA32136@honk.padd.com>

Pete Wyckoff <pw@padd.com> writes:

> This version of the patch handles quoting of shell
> meta-characters, as pointed out by Hannes.  I decided to invoke
> sq_quote_buf directly on the path before expanding %s, rather
> than writing a dict entry to understand %'s.

Ahh, that's much more preferable than my horrible suggestion ;-) If the user wants to have concatenation with other parameters, that can be left to the shell that is invoked, e.g. "--clean foo/%s" for "hello world.c" would expand to "--clean foo/'hello world.c'" and does what we want.

Another nitpick is if 's' is the right letter to use for the pathname information. I think you took 's' after "string", but if this is to be extensible, you should anticipate that later there will be other kinds of information you may want to throw at the filters, and expect that some of them also will be of type "string", and you will have painted the person who wants to add that new information into a tight corner as you already took the valuable 's'.

Which would suggest that you shouldn't be naming the placeholder after the type but after what the placeholder means, no? Perhaps %f (for filename) would be a better choice?