Giving command line parameter to textconv command?

Some text documents in my repository is encoded in EUC-JP and I have this line in my .gitattributes file.

	*.txt	diff=eucjp

and these two lines in my .git/config file.

	[diff "eucjp"]
		textconv = nkf-w

And I have ~/bin/nkf-w script that is executable.

	#!/bin/sh
	nkf -w "$@"

The command takes a (Japanese) text file and converts it into UTF-8 (it guesses the input encoding).

I need this extra script because setting 'nkf -w' for textconv like this

	[diff "eucjp"]
		textconv = nkf -w

gives an error.

	% diff --git a/hello.txt b/hello.txt
	index 696acd7..f07aa1a 100644
	error: cannot run nkf -w: No such file or directory
	error: error running textconv command 'nkf -w'
	fatal: unable to read files to diff

Could you fix textconv so that it can be given parameters?

Nanako Shiraishi <nanako3@lavabit.com> writes:

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.

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

 diff.c                         |    9 ++++-----
 t/t4030-diff-textconv.sh       |    2 +-
 t/t4031-diff-rewrite-binary.sh |    2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

Quoting Junio C Hamano <gitster@pobox.com>

I experimented with other variables (eg. smudge and clean) and they honor their command line arguments. If textconv is the only setting that doesn't, the change may be easier to justify.

By the way, there should be a better description of the filters in the gitattributes documentation, similar to how [diff "name"] sections in the .git/config file are described.

-- >8 --
Subject: Illustrate "filter" attribute with an example

The example was taken from aa4ed402c9721170fde2e9e43c3825562070e65e (Add 'filter' attribute and external filter driver definition).

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---

Nanako Shiraishi <nanako3@lavabit.com> writes:

Yes, as you found out, convert.c::apply_filter() is aware of the command line arguments.

Let's try to do a bit more work to make the coverage complete. After scanning "git grep -e start_async -e run_command" output, here is what I came up with:

 - editor.c::launch_editor() that allows a custom editor named via
   GIT_EDITOR does seem to honor your command line arguments.

 - pager.c::setup_pager() is used for GIT_PAGER and it does honor your
   command line arguments.

 - ll-merge.c::ll_ext_merge() that is used to handle custom merge drivers
   lets the user specify command line via templating to replace %O %A %B
   and naturally it needs to be aware of the command line arguments.

 - diff.c::run_external_diff() that runs GIT_EXTERNAL_DIFF defines that
   the command has to take 7 parameters in a fixed order, and is not
   designed to permute its arguments like ll_ext_merge() does, but these
   days people don't use it directly (they use it indirectly via
   "difftool" wrapper), so it probably is not an issue.

 - merge-index.c::merge_entry() also defines a strict order and semantics
   to its parameters, but similar to GIT_EXTERNAL_DIFF, it is not
   something you would throw a ready-made program (like an editor or an
   pager) and expect it to work, so it wouldn't be an issue either.

Hooks do not even take arbitrary command line arguments, so we don't have to worry about them.

So it does look like that textconv is the only odd-man out.

On Mon, Dec 14, 2009 at 09:56:28PM -0800, Junio C Hamano wrote:

I think it is important to note in user-facing documentation (like release notes describing the proposed textconv change) that these are not just "honor your command line arguments" but "execute your command with /bin/sh". Maybe that is obviously how it is implemented, but I think it should be made clear that you have the power to use (and responsibility to protect yourself from) arbitrary shell code.

There is also diff.*.command, which I think people _do_ set manually (I used to, until I wrote textconv. :) ). It does not use the shell currently. I agree that people almost certainly have to write a shell-script wrapper anyway. But I wonder if we should pass it through the shell, just for the sake of consistency with the other variables (in particular, if textconv changes, I think diff.*.command is closely related).

-Peff

Jeff King <peff@peff.net> writes:

I had to spend fair amount of time to find where "diff.*.command" is described. We may want to update the documentation.

This covers GIT_EXTERNAL_DIFF, diff.external, and diff.<driver>.command trio, if I am not mistaken.

If we changed run_external_diff(), in practice nobody would notice, except for people who have installed the difftool helper in a directory with IFS in the path. That's one downside but I don't offhand see a use case where the change would make somebody vastly happier.

But maybe people will find good uses and we'll never know until we try. Care to roll a patch for that as well, to be queued for 1.7.0 (which will be the one after 1.6.6)?

On Tue, Dec 15, 2009 at 05:05:52PM -0800, Junio C Hamano wrote:

Yeah, I think textconv is similarly hard to find. We should probably have a pointer in "git-diff.txt" to the attributes documentation.

I also think it would be much more obvious as "diff.*.external", but it is probably not worth changing at this point.

It does cover all three. We could do them separately, I guess, but I think that is just making things confusingly more inconsistent.

Yeah, the only upside I can see is consistency. Which I do value, but it will be a hard sell to somebody whose setup has been broken. ;P

Will do, but it will probably be a few days as I'm about to do some holiday traveling. I'll do the textconv change with it as a series.

-Peff

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

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).

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

Jeff King <peff@peff.net> writes:

"diff.*.blobfilter"?

Junio, could you tell us what happened to this thread?

I use this patch myself, and it works really well.

Nanako Shiraishi <nanako3@lavabit.com> writes:

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.

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

http://thread.gmane.org/gmane.comp.version-control.git/135250/focus=135312

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

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

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

Many callsites run "sh -c $CMD" to run $CMD. We can make it a little simpler for them by factoring out the munging of argv.

For simple cases with no arguments, this doesn't help much, but:

  1. For cases with arguments, we save the caller from
     having to build the appropriate shell snippet.

  2. We can later optimize to avoid the shell when
     there are no metacharacters in the program.

Signed-off-by: Jeff King <peff@peff.net>
---
I made the matching tweak to the Windows half of run-command, but I
don't actually have a box to test it on.

I modeled this after execv_git_cmd. Like that function, I try to release the allocated argv on error. However, we do actually leak the strbuf memory in one case. I'm not sure how much we care. On unix, this will always happen in a forked process which will either exec or die. On Windows, we seem to already be leaking the prepared argv for the git_cmd case (and now we leak the shell_cmd case, too).

 run-command.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 run-command.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)

On Wed, Dec 30, 2009 at 11:53 AM, Jeff King <peff@peff.net> wrote:

Nice. This could be helpful if we ever decide not to depend on a sh-compatible shell on Windows (since one isn't included by default no Windows, and installing msys/cygwin has some compatibility-issues with previous installations).

Jeff King schrieb:

That is OK. We can fix this when we find a work-load where this is a problem.

But would you please squash this in to avoid a warning about an unused static function on Windows.

---
  run-command.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

Now that we have the use_shell feature, these callsites can all be converted with small changes.

Signed-off-by: Jeff King <peff@peff.net>
---
These should be non-controversial.

 convert.c   |    3 ++-
 imap-send.c |    8 ++------
 ll-merge.c  |    6 +++---
 pager.c     |    5 +++--
 4 files changed, 10 insertions(+), 12 deletions(-)

If there are no metacharacters in the program to be run, we can just skip running the shell entirely and directly exec the program.

The metacharacter test is pulled verbatim from launch_editor, which already implements this optimization.

Signed-off-by: Jeff King <peff@peff.net>
---
Something inside me feels wrong with a catch-known-metacharacter test
instead of an allow-known-good check. But this is the same test we have
been using with launch_editor for some time, so I decided not to mess
with it.

 run-command.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

Jeff King schrieb:

The git version that msysgit ships (and the one that I use on Windows) has this sequence in pager.c:

static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
...
	pager_process.argv = &pager_argv[2];
	pager_process.in = -1;
	if (start_command(&pager_process)) {
		pager_process.argv = pager_argv;
		pager_process.in = -1;
		if (start_command(&pager_process))
			return;
	}

to help people set

  PAGER=C:\Program Files\cygwin\bin\less

That is, we first try to run the program without the shell, then retry wrapped in sh -c.

Wouldn't it be possible to do the same here, assuming that we don't have programs such as "editor -f" in the path?

It does assume that we are able to detect execvp failure due to ENOENT which is currently proposed elsewhere by Ilari Liusvaara (and which is already possible on Windows).

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

It is a cute idea that covers 70-80% of the cases, as you also have to assume that you don't have to specify your own pager on a path with IFS (e.g. "Program files" in your example) and give your parameter to the pager at the same time, e.g.

    PAGER="C:\Program Files\cygwin\bin\less -FRSX"

Because it has its own LESS environment to set FRSX and you can get away with:

    PAGER="C:\Program Files\cygwin\bin\less"
    LESS=FRSX

"less" is not a representative example for this issue. In real life I suspect that custom programs that we don't ship with git (or you don't ship with msysgit) would lack such a workaround, (and that is why I didn't say "98% of the cases").

On Donnerstag, 31. Dezember 2009, Junio C Hamano wrote:

OTOH, once you see that you would have to set

	PAGER: C:\Program Files\cygwin\bin\less -FRSX

(I'm not using shell syntax here; think of a dialog that has name and value in separate edit boxes) then it is rather obvious that this cannot work. If you are clever (and you probably are - after all, you are modifying something esoteric: the environment!), then you will have heard about the magic double-quotes, and you will write this as

	PAGER: "C:\Program Files\cygwin\bin\less" -FRSX

instead, and it will work as intended.

Granted, "less" is not representative.

	GIT_EDITOR: "C:\Program Files\Notepad++\notepad++" -multiInst

is probably more realistic (but I didn't test it).

-- Hannes

On Thu, Dec 31, 2009 at 05:54:37PM +0100, Johannes Sixt wrote:

As a side note to what you are saying, my patch 2/6 will break this. The "new" way to do it would be:

  /* do not set pager_argv[2] specially */
  pager_process.in = -1;
  if (start_command(&pager_process)) {
          pager_process.use_shell = 1;
          pager_process.in = -1;
          if (start_command(&pager_process))
                  return;
  }

though I am happy to also just revert the pager.c changes and leave it to handle the shell itself.

But of course if we use your trick internally in run-command, then your pager-specific change can just go away.

Hmm. That is somewhat clever. And it would actually solve the backward-compatibility problem for helpers moving to shell execution. If you have a textconv of "/path with space/foo", it will continue to work transparently, but now "/path_without_space/foo --option" will also Just Work.

The two downsides I see are:

  1. You want to run "foo" with "-bar" in your path but you have "foo
     -bar" in your path (your "editor -f" example). This just seems
     insane to me.

  2. There is a little bit of an interface inconsistency. You can do
     PAGER="/path with space/foo", but once you want to add an option,
     PAGER="/path with space/foo -bar" does not do what you mean. You
     have to understand the magic that is going on, and that you now
     need to quote the first part.

     I'm not sure that is not a feature, though. You are paying that
     cost in the transition, but getting the DWYM magic for the common
     case.

We could also simply do the path lookup ourselves, decide whether to use the shell, and then exec. Path lookup is not that hard, and I think we already have mingw compat routines for it anyway.

-Peff

On Donnerstag, 31. Dezember 2009, Jeff King wrote:

This is what I had in mind.

I tried to convince Ilari that this is the way to go, but...

-- Hannes

On Thu, Dec 31, 2009 at 11:16:47PM +0100, Johannes Sixt wrote:

How should we proceed, then? The "DWIM with spaces" magic seems like something that can come later, so I am tempted to recommend taking my series now, fixing up msysgit as mentioned earlier (or just dropping the pager.c portion of my 2/6), and then implementing DWIM once Ilari's topic matures.

We might want to hold my 5/6 and 6/6 back from master until the DWIM (which would make both totally safe, I think).

-Peff

Jeff King schrieb:

I think this procedure is fine. msysgit can resolve the conflict in pager.c as suggested earlier.

Would make sense, too.

-- Hannes

Now that run_command implements the same code in a more general form, we can make use of it.

Signed-off-by: Jeff King <peff@peff.net>
---
Should also be non-controversial, and the diffstat shows that this is
the payoff for all of the added code earlier in the series. :)

 editor.c |   21 ++-------------------
 1 files changed, 2 insertions(+), 19 deletions(-)

Currently textconv helpers are run directly. Running through the shell is useful because the user can provide a program with command line arguments, like "antiword -f".

It also makes textconv more consistent with other parts of git, most of which run their helpers using the shell.

The downside is that textconv helpers with shell metacharacters (like space) in the filename will be broken.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the first actual change in behavior. This patch and 6/6 should
perhaps be held back with a warning period. I am inclined to consider it
a bug that they didn't use the shell, and this is fixing it.

 diff.c                         |    1 +
 t/t4030-diff-textconv.sh       |    2 +-
 t/t4031-diff-rewrite-binary.sh |    2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

This is mostly to make it more consistent with the rest of git, which uses the shell to exec helpers.

Signed-off-by: Jeff King <peff@peff.net>
---
Consistency is the main advantage here. Though I think you can actually
do craziness like setting

  GIT_EXTERNAL_DIFF='my-command $2 $3 && true' git diff

to pick out some subset of the parameters, I can't imagine it is actually a good idea.

 diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Windows, we need the shbang line to correctly invoke shell scripts via a POSIX shell, except when the script is invoked via 'sh -c' because sh (a bash) does "the right thing". Since nowadays the clean and smudge filters are not always invoked via 'sh -c' anymore, we have to mark the the one in t0021-conversion with #!$SHELL_PATH.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
  t/t0021-conversion.sh    |    3 ++-
  t/t4030-diff-textconv.sh |    6 ++++--
  2 files changed, 6 insertions(+), 3 deletions(-)

Recall that MSYS bash converts POSIX style absolute paths to Windows style absolute paths. Unfortunately, it converts a program argument that begins with a double-quote and otherwise looks like an absolute POSIX path, but in doing so, it strips everything past the second double-quote[*]. This case is triggered in the two test scripts. The work-around is to place the Windows style path between the quotes to avoid the path conversion.

[*] It is already bogus that a conversion is even considered when a program argument begins with a double-quote because it cannot be an absolute POSIX path.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
  t/t4030-diff-textconv.sh       |    2 +-
  t/t4031-diff-rewrite-binary.sh |    2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

On Fri, Jan 01, 2010 at 11:14:06PM +0100, Johannes Sixt wrote:

Hrm. This does mean we might be breaking users who have helper scripts in a similar state to those in the test suite (of course, so does your pager hack, or anything which might optimize out a shell call). But perhaps given that scripts without a shebang generally don't work on Windows, they are not too common and we don't need to worry about it.

-Peff

Jeff King schrieb:

Not helper scripts in general, but only clean and smudge filters, because these have been invoked with "sh -c" so far. Everything else, that was not run via "sh -c", but now is, is safe.

-- Hannes

On Mon, Jan 04, 2010 at 04:50:39PM +0100, Johannes Sixt wrote:

I converted more than that; see my 2/6. It is also the pager, the imap-send tunnel helper, and external merge helpers. Not the editor, since it already had the no-metacharacters optimization (though it, too, could be affected if we implement your DWIM trick instead of the metacharacter thing).

So I think we need to make a conscious decision that this is an acceptable change of behavior (and I am totally fine with the change happening -- I just want to be clear about the extent of what is being changed).

-Peff

Jeff King schrieb:

Hm, ok, I see.

- The clean and smudge filters are probably the most important cases.

- I *did* write my own merge script (to merge PNGs!), but I made sure to 
begin it with #!/bin/bash, and I don't think anybody else is crazy enough 
to write a custom merge script ;)

- imap-send on Windows is so new that I don't think anyone is already 
using it with a custom tunneling script.

- The change in pager.c is unimportant because all versions shipped so far 
(via msysgit) have the conflicting change that tried without "sh -c" first.

I think that these can be handled with an entry in the release notes.

-- Hannes