From: Jeff King <peff@peff.net>
Date: Wed, 30 Dec 2009 09:56:34 GMT
Subject: Re: Giving command line parameter to textconv command?
Message-ID: <20091230095634.GA16349@coredump.intra.peff.net>
In-Reply-To: <7vr5qc51kv.fsf@alter.siamese.dyndns.org>

On Wed, Dec 30, 2009 at 12:05:20AM -0800, Junio C Hamano wrote:

> I stopped promoting the patch after Peff mentioned he was planning a
> rework of textconv, but now I re-read it, I think his rework would be
> orthogonal to the patch.

Actually, I thought I volunteered to produce a patch series converting
textconv and diff.

> Also Peff gives a good hint about borrowing how launch_editor() calls out
> to the shell.  I think the codepath we fork&exec user-defined commands
> (not hooks, but filters like smudge/clean/textconv and EDITOR/PAGER that
> take a command line) need to be first enumerated, then we need to see if
> we can refactor what launch_editor() does into a common helper function.

I think the helper function is not too hard. Whereas the patch you
posted earlier actually runs { "sh", "-c", "%s %s" } for textconv, it is
a bit simpler to do { "sh", "-c", "%s \"$@\"", ... } as launch_editor
does. And then we don't have to worry about quoting the arguments (which
your patch gets wrong).

> I felt it was unclear what we would want to do with GIT_EXTERNAL_DIFF,
> diff.external, and diff.<driver>.command trio, but I tend to agree that we
> should run things the same way, honoring $IFS and shell everywhere.

I thought it was left at "maybe for 1.7.0", but perhaps we don't have
time now to issue sufficient warning.

Anyway, I'm working on a series which will hopefully be done in a few
minutes.

-Peff