Re: Giving command line parameter to textconv command?

From: Jeff King <peff@peff.net>
Date: Dec 15, 2009, 17:03 UTC
Message-ID: <20091215170321.GB21322@coredump.intra.peff.net>
In-Reply-To: <7vvdg9ceud.fsf@alter.siamese.dyndns.org>

On Mon, Dec 14, 2009 at 03:31:38PM -0800, Junio C Hamano wrote:

Show 16 quoted lines

> The change to do so looks like this; it has a few side effects:
> 
>  - If somebody else were relying on the fact that 'nkf -w' names the
>    entire command, it now will run 'nkf' command with '-w' as an argument,
>    and it will break such a set-up.  IOW, command that has an IFS white
>    space in its path will now need to be quoted from the shell.
> 
>    You can see the fallout from this in the damage made to t/ hierarchy in
>    the attached patch.
> 
>  - You can now use $HOME and other environment variables your shell
>    expands when defining your textconv command.
> 
> Overall I think it is a good direction to go, but we need to be careful
> about how we transition the existing repositories that use the old
> semantics.

I agree that this is a good change. My only reservation would be that spawning a shell would be slightly slower (think "git log -p"). However:

  1. textconv is dog-slow already, as it has to dump each blob to a
     tempfile[1].

  2. There is an obvious optimization, which is that you can skip the
     shell if there are no metacharacters (in fact, we seem to be doing
     that already in launch_editor).

> We might need to introduce diff.*.xtextconv or something.

I am torn on this. I don't like introducing behavior changes that might hurt people (and really, I think we are just talking about people with textconv pointing to a program by its full path that has a space in it). But I also hate carrying around baggage crap like xtextconv forever. It makes git harder to explain why there are two variables that do almost the same thing, and the one they want to use is probably the one with the crappier, unlikely-to-be-guessed name.

So I am somewhat inclined to call it a bug fix, but I don't feel very strongly.

-Peff

[1] The current textconv interface is really nice for things like just using "antiword" out of the box. But I wrote a new interface which can be much faster: it calls the textconv filter with the blob name, and then the filter is responsible for using cat-file to get at the blob. This means the filter can look at only part of a blob (e.g., if we are interested in the metadata tags at the beginning of a large media file), and it can cache answers as it sees fit, avoiding access to the blob entirely.

I need to polish the code a bit and submit it. Obviously this is not meant to replace the existing textconv, but rather to supplement it, for when "fast and inconvenient" is better than "slow and simple". What is the best way to configure this? I can imagine "diff.*.textconvType = fast", or also "diff.*.fastTextconv".

-Peff