{"thread":{"id":"21957","subject":"Giving command line parameter to textconv command?","startedAt":"2009-12-14T22:17:35Z","lastAt":"2010-01-04T16:46:38Z","messageCount":33,"participants":["Nanako Shiraishi","Junio C Hamano","Jeff King","Erik Faye-Lund","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"129913","messageId":"20091215071735.6117@nanako3.lavabit.com","threadId":"21957","inReplyTo":null,"subject":"Giving command line parameter to textconv command?","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-12-14T22:17:35Z","receivedAt":"2009-12-14T22:17:35Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Some text documents in my repository is encoded in EUC-JP \nand I have this line in my .gitattributes file.\n\n\t*.txt\tdiff=eucjp\n\nand these two lines in my .git/config file.\n\n\t[diff \"eucjp\"]\n\t\ttextconv = nkf-w\n\nAnd I have ~/bin/nkf-w script that is executable.\n\n\t#!/bin/sh\n\tnkf -w \"$@\"\n\nThe command takes a (Japanese) text file and converts it \ninto UTF-8 (it guesses the input encoding).\n\nI need this extra script because setting 'nkf -w' for\ntextconv like this\n\n\t[diff \"eucjp\"]\n\t\ttextconv = nkf -w\n\ngives an error.\n\n\t% diff --git a/hello.txt b/hello.txt\n\tindex 696acd7..f07aa1a 100644\n\terror: cannot run nkf -w: No such file or directory\n\terror: error running textconv command 'nkf -w'\n\tfatal: unable to read files to diff\n\nCould you fix textconv so that it can be given parameters?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"129917","messageId":"7vvdg9ceud.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"20091215071735.6117@nanako3.lavabit.com","subject":"Re: Giving command line parameter to textconv command?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-14T23:31:38Z","receivedAt":"2009-12-14T23:31:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi <nanako3@lavabit.com> writes:\n\n> I need this extra script because setting 'nkf -w' for\n> textconv like this\n>\n> \t[diff \"eucjp\"]\n> \t\ttextconv = nkf -w\n>\n> gives an error.\n>\n> \t% diff --git a/hello.txt b/hello.txt\n> \tindex 696acd7..f07aa1a 100644\n> \terror: cannot run nkf -w: No such file or directory\n> \terror: error running textconv command 'nkf -w'\n> \tfatal: unable to read files to diff\n>\n> Could you fix textconv so that it can be given parameters?\n\nThe change to do so looks like this; it has a few side effects:\n\n - If somebody else were relying on the fact that 'nkf -w' names the\n   entire command, it now will run 'nkf' command with '-w' as an argument,\n   and it will break such a set-up.  IOW, command that has an IFS white\n   space in its path will now need to be quoted from the shell.\n\n   You can see the fallout from this in the damage made to t/ hierarchy in\n   the attached patch.\n\n - You can now use $HOME and other environment variables your shell\n   expands when defining your textconv command.\n\nOverall I think it is a good direction to go, but we need to be careful\nabout how we transition the existing repositories that use the old\nsemantics.\n\nWe might need to introduce diff.*.xtextconv or something.\n\n diff.c                         |    9 ++++-----\n t/t4030-diff-textconv.sh       |    2 +-\n t/t4031-diff-rewrite-binary.sh |    2 +-\n 3 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 08bbd3e..64a1486 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3760,15 +3760,14 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \t\tsize_t *outsize)\n {\n \tstruct diff_tempfile *temp;\n-\tconst char *argv[3];\n-\tconst char **arg = argv;\n+\tconst char *argv[4] = { \"sh\", \"-c\", NULL, NULL };\n \tstruct child_process child;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf cmd = STRBUF_INIT;\n \n \ttemp = prepare_temp_file(spec->path, spec);\n-\t*arg++ = pgm;\n-\t*arg++ = temp->name;\n-\t*arg = NULL;\n+\tstrbuf_addf(&cmd, \"%s %s\", pgm, temp->name);\n+\targv[2] = strbuf_detach(&cmd, NULL);\n \n \tmemset(&child, 0, sizeof(child));\n \tchild.argv = argv;\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex a3f0897..3468f77 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -48,7 +48,7 @@ test_expect_success 'file is considered binary by plumbing' '\n \n test_expect_success 'setup textconv filters' '\n \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"$PWD\"/hexdump &&\n+\tgit config diff.foo.textconv \\\"\"$PWD\"/hexdump\\\" &&\n \tgit config diff.fail.textconv false\n '\n \ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex a894c60..e6cb30e 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -54,7 +54,7 @@ chmod +x dump\n \n test_expect_success 'setup textconv' '\n \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"$PWD\"/dump\n+\tgit config diff.foo.textconv \\\"\"$PWD\"/dump\\\"\n '\n \n test_expect_success 'rewrite diff respects textconv' '\n"},{"id":"129927","messageId":"20091215121110.6117@nanako3.lavabit.com","threadId":"21957","inReplyTo":"7vvdg9ceud.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-12-15T03:11:10Z","receivedAt":"2009-12-15T03:11:10Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano <gitster@pobox.com>\n\n> Nanako Shiraishi <nanako3@lavabit.com> writes:\n>\n>> I need this extra script because setting 'nkf -w' for\n>> textconv like this\n>>\n>> \t[diff \"eucjp\"]\n>> \t\ttextconv = nkf -w\n>>\n>> gives an error.\n>>\n>> \t% diff --git a/hello.txt b/hello.txt\n>> \tindex 696acd7..f07aa1a 100644\n>> \terror: cannot run nkf -w: No such file or directory\n>> \terror: error running textconv command 'nkf -w'\n>> \tfatal: unable to read files to diff\n>>\n>> Could you fix textconv so that it can be given parameters?\n>\n> The change to do so looks like this; it has a few side effects:\n>\n>  - If somebody else were relying on the fact that 'nkf -w' names the\n>    entire command, it now will run 'nkf' command with '-w' as an argument,\n>    and it will break such a set-up.  IOW, command that has an IFS white\n>    space in its path will now need to be quoted from the shell.\n>\n>    You can see the fallout from this in the damage made to t/ hierarchy in\n>    the attached patch.\n>\n>  - You can now use $HOME and other environment variables your shell\n>    expands when defining your textconv command.\n>\n> Overall I think it is a good direction to go, but we need to be careful\n> about how we transition the existing repositories that use the old\n> semantics.\n>\n> We might need to introduce diff.*.xtextconv or something.\n\nI experimented with other variables (eg. smudge and clean) and \nthey honor their command line arguments. If textconv is the only \nsetting that doesn't, the change may be easier to justify.\n\nBy the way, there should be a better description of the filters \nin the gitattributes documentation, similar to how [diff \"name\"] \nsections in the .git/config file are described.\n\n-- >8 --\nSubject: Illustrate \"filter\" attribute with an example\n\nThe example was taken from aa4ed402c9721170fde2e9e43c3825562070e65e\n(Add 'filter' attribute and external filter driver definition).\n\nSigned-off-by: Nanako Shiraishi <nanako3@lavabit.com>\n---\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 1f472ce..5a45e51 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -197,6 +197,25 @@ intent is that if someone unsets the filter driver definition,\n or does not have the appropriate filter program, the project\n should still be usable.\n \n+For example, in .gitattributes, you would assign the `filter`\n+attribute for paths.\n+\n+------------------------\n+*.c\tfilter=indent\n+------------------------\n+\n+Then you would define a \"filter.indent.clean\" and \"filter.indent.smudge\"\n+configuration in your .git/config to specify a pair of commands to\n+modify the contents of C programs when the source files are checked\n+in (\"clean\" is run) and checked out (no change is made because the\n+command is \"cat\").\n+\n+------------------------\n+[filter \"indent\"]\n+\tclean = indent\n+\tsmudge = cat\n+------------------------\n+\n \n Interaction between checkin/checkout attributes\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n\n\n\n\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"129934","messageId":"7vfx7c3hmb.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"20091215121110.6117@nanako3.lavabit.com","subject":"Re: Giving command line parameter to textconv command?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-15T05:56:28Z","receivedAt":"2009-12-15T05:56:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi <nanako3@lavabit.com> writes:\n\n> I experimented with other variables (eg. smudge and clean) and \n> they honor their command line arguments. If textconv is the only \n> setting that doesn't, the change may be easier to justify.\n\nYes, as you found out, convert.c::apply_filter() is aware of the command\nline arguments.\n\nLet's try to do a bit more work to make the coverage complete.  After\nscanning \"git grep -e start_async -e run_command\" output, here is what I\ncame up with:\n\n - editor.c::launch_editor() that allows a custom editor named via\n   GIT_EDITOR does seem to honor your command line arguments.\n\n - pager.c::setup_pager() is used for GIT_PAGER and it does honor your\n   command line arguments.\n\n - ll-merge.c::ll_ext_merge() that is used to handle custom merge drivers\n   lets the user specify command line via templating to replace %O %A %B\n   and naturally it needs to be aware of the command line arguments.\n\n - diff.c::run_external_diff() that runs GIT_EXTERNAL_DIFF defines that\n   the command has to take 7 parameters in a fixed order, and is not\n   designed to permute its arguments like ll_ext_merge() does, but these\n   days people don't use it directly (they use it indirectly via\n   \"difftool\" wrapper), so it probably is not an issue.\n\n - merge-index.c::merge_entry() also defines a strict order and semantics\n   to its parameters, but similar to GIT_EXTERNAL_DIFF, it is not\n   something you would throw a ready-made program (like an editor or an\n   pager) and expect it to work, so it wouldn't be an issue either.\n\nHooks do not even take arbitrary command line arguments, so we don't have\nto worry about them.\n\nSo it does look like that textconv is the only odd-man out.\n"},{"id":"129950","messageId":"20091215164959.GA21322@coredump.intra.peff.net","threadId":"21957","inReplyTo":"7vfx7c3hmb.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-15T16:49:59Z","receivedAt":"2009-12-15T16:49:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 14, 2009 at 09:56:28PM -0800, Junio C Hamano wrote:\n\n> Let's try to do a bit more work to make the coverage complete.  After\n> scanning \"git grep -e start_async -e run_command\" output, here is what I\n> came up with:\n> \n>  - editor.c::launch_editor() that allows a custom editor named via\n>    GIT_EDITOR does seem to honor your command line arguments.\n> \n>  - pager.c::setup_pager() is used for GIT_PAGER and it does honor your\n>    command line arguments.\n> \n>  - ll-merge.c::ll_ext_merge() that is used to handle custom merge drivers\n>    lets the user specify command line via templating to replace %O %A %B\n>    and naturally it needs to be aware of the command line arguments.\n\nI think it is important to note in user-facing documentation (like\nrelease notes describing the proposed textconv change) that these are\nnot just \"honor your command line arguments\" but \"execute your command\nwith /bin/sh\". Maybe that is obviously how it is implemented, but I\nthink it should be made clear that you have the power to use (and\nresponsibility to protect yourself from) arbitrary shell code.\n\n>  - diff.c::run_external_diff() that runs GIT_EXTERNAL_DIFF defines that\n>    the command has to take 7 parameters in a fixed order, and is not\n>    designed to permute its arguments like ll_ext_merge() does, but these\n>    days people don't use it directly (they use it indirectly via\n>    \"difftool\" wrapper), so it probably is not an issue.\n\nThere is also diff.*.command, which I think people _do_ set manually (I\nused to, until I wrote textconv. :) ). It does not use the shell\ncurrently. I agree that people almost certainly have to write a\nshell-script wrapper anyway. But I wonder if we should pass it through\nthe shell, just for the sake of consistency with the other variables (in\nparticular, if textconv changes, I think diff.*.command is closely\nrelated).\n\n-Peff\n"},{"id":"129951","messageId":"20091215170321.GB21322@coredump.intra.peff.net","threadId":"21957","inReplyTo":"7vvdg9ceud.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-15T17:03:21Z","receivedAt":"2009-12-15T17:03:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 14, 2009 at 03:31:38PM -0800, Junio C Hamano wrote:\n\n> The change to do so looks like this; it has a few side effects:\n> \n>  - If somebody else were relying on the fact that 'nkf -w' names the\n>    entire command, it now will run 'nkf' command with '-w' as an argument,\n>    and it will break such a set-up.  IOW, command that has an IFS white\n>    space in its path will now need to be quoted from the shell.\n> \n>    You can see the fallout from this in the damage made to t/ hierarchy in\n>    the attached patch.\n> \n>  - You can now use $HOME and other environment variables your shell\n>    expands when defining your textconv command.\n> \n> Overall I think it is a good direction to go, but we need to be careful\n> about how we transition the existing repositories that use the old\n> semantics.\n\nI agree that this is a good change. My only reservation would be that\nspawning a shell would be slightly slower (think \"git log -p\"). However:\n\n  1. textconv is dog-slow already, as it has to dump each blob to a\n     tempfile[1].\n\n  2. There is an obvious optimization, which is that you can skip the\n     shell if there are no metacharacters (in fact, we seem to be doing\n     that already in launch_editor).\n\n> We might need to introduce diff.*.xtextconv or something.\n\nI am torn on this. I don't like introducing behavior changes that might\nhurt people (and really, I think we are just talking about people with\ntextconv pointing to a program by its full path that has a space in it).\nBut I also hate carrying around baggage crap like xtextconv forever. It\nmakes git harder to explain why there are two variables that do almost\nthe same thing, and the one they want to use is probably the one with\nthe crappier, unlikely-to-be-guessed name.\n\nSo I am somewhat inclined to call it a bug fix, but I don't feel very\nstrongly.\n\n-Peff\n\n[1] The current textconv interface is really nice for things like just\nusing \"antiword\" out of the box. But I wrote a new interface which can\nbe much faster: it calls the textconv filter with the blob name, and\nthen the filter is responsible for using cat-file to get at the blob.\nThis means the filter can look at only part of a blob (e.g., if we are\ninterested in the metadata tags at the beginning of a large media file),\nand it can cache answers as it sees fit, avoiding access to the blob\nentirely.\n\nI need to polish the code a bit and submit it. Obviously this is not\nmeant to replace the existing textconv, but rather to supplement it, for\nwhen \"fast and inconvenient\" is better than \"slow and simple\". What is\nthe best way to configure this? I can imagine \"diff.*.textconvType =\nfast\", or also \"diff.*.fastTextconv\".\n\n-Peff\n"},{"id":"129953","messageId":"7vy6l4179h.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"20091215170321.GB21322@coredump.intra.peff.net","subject":"Re: Giving command line parameter to textconv command?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-15T17:23:06Z","receivedAt":"2009-12-15T17:23:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [1] The current textconv interface is really nice for things like just\n> using \"antiword\" out of the box. But I wrote a new interface which can\n> be much faster: it calls the textconv filter with the blob name, and\n> then the filter is responsible for using cat-file to get at the blob.\n> This means the filter can look at only part of a blob (e.g., if we are\n> interested in the metadata tags at the beginning of a large media file),\n> and it can cache answers as it sees fit, avoiding access to the blob\n> entirely.\n>\n> I need to polish the code a bit and submit it. Obviously this is not\n> meant to replace the existing textconv, but rather to supplement it, for\n> when \"fast and inconvenient\" is better than \"slow and simple\". What is\n> the best way to configure this? I can imagine \"diff.*.textconvType =\n> fast\", or also \"diff.*.fastTextconv\".\n\n\"diff.*.blobfilter\"?\n"},{"id":"129972","messageId":"7vljh3wwwf.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"20091215164959.GA21322@coredump.intra.peff.net","subject":"Re: Giving command line parameter to textconv command?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-16T01:05:52Z","receivedAt":"2009-12-16T01:05:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>>  - diff.c::run_external_diff() that runs GIT_EXTERNAL_DIFF defines that\n>>    the command has to take 7 parameters in a fixed order, and is not\n>>    designed to permute its arguments like ll_ext_merge() does, but these\n>>    days people don't use it directly (they use it indirectly via\n>>    \"difftool\" wrapper), so it probably is not an issue.\n>\n> There is also diff.*.command, which I think people _do_ set manually (I\n> used to, until I wrote textconv. :) ).\n\nI had to spend fair amount of time to find where \"diff.*.command\" is\ndescribed.  We may want to update the documentation.\n\n> .... I agree that people almost certainly have to write a shell-script\n> wrapper anyway. But I wonder if we should pass it through the shell,\n> just for the sake of consistency with the other variables (in\n> particular, if textconv changes,\n\nThis covers GIT_EXTERNAL_DIFF, diff.external, and diff.<driver>.command\ntrio, if I am not mistaken.\n\nIf we changed run_external_diff(), in practice nobody would notice, except\nfor people who have installed the difftool helper in a directory with IFS\nin the path.  That's one downside but I don't offhand see a use case where\nthe change would make somebody vastly happier.\n\nBut maybe people will find good uses and we'll never know until we try.\nCare to roll a patch for that as well, to be queued for 1.7.0 (which will\nbe the one after 1.6.6)?\n"},{"id":"129973","messageId":"20091216011330.GA28523@coredump.intra.peff.net","threadId":"21957","inReplyTo":"7vljh3wwwf.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-16T01:13:31Z","receivedAt":"2009-12-16T01:13:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2009 at 05:05:52PM -0800, Junio C Hamano wrote:\n\n> > There is also diff.*.command, which I think people _do_ set manually (I\n> > used to, until I wrote textconv. :) ).\n> \n> I had to spend fair amount of time to find where \"diff.*.command\" is\n> described.  We may want to update the documentation.\n\nYeah, I think textconv is similarly hard to find. We should probably\nhave a pointer in \"git-diff.txt\" to the attributes documentation.\n\nI also think it would be much more obvious as \"diff.*.external\", but it\nis probably not worth changing at this point.\n\n> > .... I agree that people almost certainly have to write a shell-script\n> > wrapper anyway. But I wonder if we should pass it through the shell,\n> > just for the sake of consistency with the other variables (in\n> > particular, if textconv changes,\n> \n> This covers GIT_EXTERNAL_DIFF, diff.external, and diff.<driver>.command\n> trio, if I am not mistaken.\n\nIt does cover all three. We could do them separately, I guess, but I\nthink that is just making things confusingly more inconsistent.\n\n> If we changed run_external_diff(), in practice nobody would notice, except\n> for people who have installed the difftool helper in a directory with IFS\n> in the path.  That's one downside but I don't offhand see a use case where\n> the change would make somebody vastly happier.\n\nYeah, the only upside I can see is consistency. Which I do value, but it\nwill be a hard sell to somebody whose setup has been broken. ;P\n\n> But maybe people will find good uses and we'll never know until we try.\n> Care to roll a patch for that as well, to be queued for 1.7.0 (which will\n> be the one after 1.6.6)?\n\nWill do, but it will probably be a few days as I'm about to do some\nholiday traveling. I'll do the textconv change with it as a series.\n\n-Peff\n"},{"id":"130512","messageId":"20091230121325.6117@nanako3.lavabit.com","threadId":"21957","inReplyTo":"7vvdg9ceud.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-12-30T03:13:25Z","receivedAt":"2009-12-30T03:13:25Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Junio, could you tell us what happened to this thread?\n\nI use this patch myself, and it works really well.\n"},{"id":"130525","messageId":"7vr5qc51kv.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"20091230121325.6117@nanako3.lavabit.com","subject":"Re: Giving command line parameter to textconv command?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-30T08:05:20Z","receivedAt":"2009-12-30T08:05:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi <nanako3@lavabit.com> writes:\n\n> Junio, could you tell us what happened to this thread?\n>\n> I use this patch myself, and it works really well.\n\nI stopped promoting the patch after Peff mentioned he was planning a\nrework of textconv, but now I re-read it, I think his rework would be\northogonal to the patch.\n\nAlso Peff gives a good hint about borrowing how launch_editor() calls out\nto the shell.  I think the codepath we fork&exec user-defined commands\n(not hooks, but filters like smudge/clean/textconv and EDITOR/PAGER that\ntake a command line) need to be first enumerated, then we need to see if\nwe can refactor what launch_editor() does into a common helper function.\nI felt it was unclear what we would want to do with GIT_EXTERNAL_DIFF,\ndiff.external, and diff.<driver>.command trio, but I tend to agree that we\nshould run things the same way, honoring $IFS and shell everywhere.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/135250/focus=135312\n"},{"id":"130533","messageId":"20091230095634.GA16349@coredump.intra.peff.net","threadId":"21957","inReplyTo":"7vr5qc51kv.fsf@alter.siamese.dyndns.org","subject":"Re: Giving command line parameter to textconv command?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T09:56:34Z","receivedAt":"2009-12-30T09:56:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 30, 2009 at 12:05:20AM -0800, Junio C Hamano wrote:\n\n> I stopped promoting the patch after Peff mentioned he was planning a\n> rework of textconv, but now I re-read it, I think his rework would be\n> orthogonal to the patch.\n\nActually, I thought I volunteered to produce a patch series converting\ntextconv and diff.\n\n> Also Peff gives a good hint about borrowing how launch_editor() calls out\n> to the shell.  I think the codepath we fork&exec user-defined commands\n> (not hooks, but filters like smudge/clean/textconv and EDITOR/PAGER that\n> take a command line) need to be first enumerated, then we need to see if\n> we can refactor what launch_editor() does into a common helper function.\n\nI think the helper function is not too hard. Whereas the patch you\nposted earlier actually runs { \"sh\", \"-c\", \"%s %s\" } for textconv, it is\na bit simpler to do { \"sh\", \"-c\", \"%s \\\"$@\\\"\", ... } as launch_editor\ndoes. And then we don't have to worry about quoting the arguments (which\nyour patch gets wrong).\n\n> I felt it was unclear what we would want to do with GIT_EXTERNAL_DIFF,\n> diff.external, and diff.<driver>.command trio, but I tend to agree that we\n> should run things the same way, honoring $IFS and shell everywhere.\n\nI thought it was left at \"maybe for 1.7.0\", but perhaps we don't have\ntime now to issue sufficient warning.\n\nAnyway, I'm working on a series which will hopefully be done in a few\nminutes.\n\n-Peff\n"},{"id":"130538","messageId":"20091230105316.GA22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 1/6] run-command: add \"use shell\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T10:53:16Z","receivedAt":"2009-12-30T10:53:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many callsites run \"sh -c $CMD\" to run $CMD. We can make it\na little simpler for them by factoring out the munging of\nargv.\n\nFor simple cases with no arguments, this doesn't help much, but:\n\n  1. For cases with arguments, we save the caller from\n     having to build the appropriate shell snippet.\n\n  2. We can later optimize to avoid the shell when\n     there are no metacharacters in the program.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI made the matching tweak to the Windows half of run-command, but I\ndon't actually have a box to test it on.\n\nI modeled this after execv_git_cmd. Like that function, I try to release\nthe allocated argv on error. However, we do actually leak the strbuf\nmemory in one case. I'm not sure how much we care. On unix, this will\nalways happen in a forked process which will either exec or die. On\nWindows, we seem to already be leaking the prepared argv for the git_cmd\ncase (and now we leak the shell_cmd case, too).\n\n run-command.c |   45 +++++++++++++++++++++++++++++++++++++++++++++\n run-command.h |    2 ++\n 2 files changed, 47 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..dc65903 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,46 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+static const char **prepare_shell_cmd(const char **argv)\n+{\n+\tint argc, nargc = 0;\n+\tconst char **nargv;\n+\n+\tfor (argc = 0; argv[argc]; argc++)\n+\t\t; /* just counting */\n+\t/* +1 for NULL, +3 for \"sh -c\" plus extra $0 */\n+\tnargv = xmalloc(sizeof(*nargv) * (argc + 1 + 3));\n+\n+\tif (argc < 1)\n+\t\tdie(\"BUG: shell command is empty\");\n+\n+\tnargv[nargc++] = \"sh\";\n+\tnargv[nargc++] = \"-c\";\n+\n+\tif (argc < 2)\n+\t\tnargv[nargc++] = argv[0];\n+\telse {\n+\t\tstruct strbuf arg0 = STRBUF_INIT;\n+\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", argv[0]);\n+\t\tnargv[nargc++] = strbuf_detach(&arg0, NULL);\n+\t}\n+\n+\tfor (argc = 0; argv[argc]; argc++)\n+\t\tnargv[nargc++] = argv[argc];\n+\tnargv[nargc] = NULL;\n+\n+\treturn nargv;\n+}\n+\n+static int execv_shell_cmd(const char **argv)\n+{\n+\tconst char **nargv = prepare_shell_cmd(argv);\n+\ttrace_argv_printf(nargv, \"trace: exec:\");\n+\texecvp(nargv[0], (char **)nargv);\n+\tfree(nargv);\n+\treturn -1;\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -123,6 +163,8 @@ fail_pipe:\n \t\t\tcmd->preexec_cb();\n \t\tif (cmd->git_cmd) {\n \t\t\texecv_git_cmd(cmd->argv);\n+\t\t} else if (cmd->use_shell) {\n+\t\t\texecv_shell_cmd(cmd->argv);\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n@@ -179,6 +221,8 @@ fail_pipe:\n \n \tif (cmd->git_cmd) {\n \t\tcmd->argv = prepare_git_cmd(cmd->argv);\n+\t} else if (cmd->use_shell) {\n+\t\tcmd->argv = prepare_shell_cmd(cmd->argv);\n \t}\n \n \tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);\n@@ -297,6 +341,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,\n \tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n \tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n+\tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n }\n \n int run_command_v_opt(const char **argv, int opt)\ndiff --git a/run-command.h b/run-command.h\nindex fb34209..967ba8c 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -33,6 +33,7 @@ struct child_process {\n \tunsigned git_cmd:1; /* if this is to be git sub-command */\n \tunsigned silent_exec_failure:1;\n \tunsigned stdout_to_stderr:1;\n+\tunsigned use_shell:1;\n \tvoid (*preexec_cb)(void);\n };\n \n@@ -46,6 +47,7 @@ extern int run_hook(const char *index_file, const char *name, ...);\n #define RUN_GIT_CMD\t     2\t/*If this is to be git sub-command */\n #define RUN_COMMAND_STDOUT_TO_STDERR 4\n #define RUN_SILENT_EXEC_FAILURE 8\n+#define RUN_USING_SHELL 16\n int run_command_v_opt(const char **argv, int opt);\n \n /*\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130539","messageId":"20091230105357.GB22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 2/6] run-command: convert simple callsites to use_shell","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T10:53:57Z","receivedAt":"2009-12-30T10:53:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that we have the use_shell feature, these callsites can\nall be converted with small changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThese should be non-controversial.\n\n convert.c   |    3 ++-\n imap-send.c |    8 ++------\n ll-merge.c  |    6 +++---\n pager.c     |    5 +++--\n 4 files changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 491e714..950b1f9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -249,10 +249,11 @@ static int filter_buffer(int fd, void *data)\n \tstruct child_process child_process;\n \tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n-\tconst char *argv[] = { \"sh\", \"-c\", params->cmd, NULL };\n+\tconst char *argv[] = { params->cmd, NULL };\n \n \tmemset(&child_process, 0, sizeof(child_process));\n \tchild_process.argv = argv;\n+\tchild_process.use_shell = 1;\n \tchild_process.in = -1;\n \tchild_process.out = fd;\n \ndiff --git a/imap-send.c b/imap-send.c\nindex de8114b..51f371b 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -965,17 +965,13 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n \t/* open connection to IMAP server */\n \n \tif (srvc->tunnel) {\n-\t\tconst char *argv[4];\n+\t\tconst char *argv[] = { srvc->tunnel, NULL };\n \t\tstruct child_process tunnel = {0};\n \n \t\timap_info(\"Starting tunnel '%s'... \", srvc->tunnel);\n \n-\t\targv[0] = \"sh\";\n-\t\targv[1] = \"-c\";\n-\t\targv[2] = srvc->tunnel;\n-\t\targv[3] = NULL;\n-\n \t\ttunnel.argv = argv;\n+\t\ttunnel.use_shell = 1;\n \t\ttunnel.in = -1;\n \t\ttunnel.out = -1;\n \t\tif (start_command(&tunnel))\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 2d6b6d6..18511e2 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -175,7 +175,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t{ \"B\", temp[2] },\n \t\t{ NULL }\n \t};\n-\tconst char *args[] = { \"sh\", \"-c\", NULL, NULL };\n+\tconst char *args[] = { NULL, NULL };\n \tint status, fd, i;\n \tstruct stat st;\n \n@@ -190,8 +190,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n \n-\targs[2] = cmd.buf;\n-\tstatus = run_command_v_opt(args, 0);\n+\targs[0] = cmd.buf;\n+\tstatus = run_command_v_opt(args, RUN_USING_SHELL);\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\ndiff --git a/pager.c b/pager.c\nindex 92c03f6..2c7e8ec 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -28,7 +28,7 @@ static void pager_preexec(void)\n }\n #endif\n \n-static const char *pager_argv[] = { \"sh\", \"-c\", NULL, NULL };\n+static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process;\n \n static void wait_for_pager(void)\n@@ -81,7 +81,8 @@ void setup_pager(void)\n \tspawned_pager = 1; /* means we are emitting to terminal */\n \n \t/* spawn the pager */\n-\tpager_argv[2] = pager;\n+\tpager_argv[0] = pager;\n+\tpager_process.use_shell = 1;\n \tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n \tif (!getenv(\"LESS\")) {\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130540","messageId":"20091230105536.GC22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T10:55:36Z","receivedAt":"2009-12-30T10:55:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If there are no metacharacters in the program to be run, we\ncan just skip running the shell entirely and directly exec\nthe program.\n\nThe metacharacter test is pulled verbatim from\nlaunch_editor, which already implements this optimization.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSomething inside me feels wrong with a catch-known-metacharacter test\ninstead of an allow-known-good check. But this is the same test we have\nbeen using with launch_editor for some time, so I decided not to mess\nwith it.\n\n run-command.c |   18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex dc65903..22e2777 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -28,15 +28,17 @@ static const char **prepare_shell_cmd(const char **argv)\n \tif (argc < 1)\n \t\tdie(\"BUG: shell command is empty\");\n \n-\tnargv[nargc++] = \"sh\";\n-\tnargv[nargc++] = \"-c\";\n-\n-\tif (argc < 2)\n-\t\tnargv[nargc++] = argv[0];\n-\telse {\n-\t\tstruct strbuf arg0 = STRBUF_INIT;\n-\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", argv[0]);\n-\t\tnargv[nargc++] = strbuf_detach(&arg0, NULL);\n+\tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n+\t\tnargv[nargc++] = \"sh\";\n+\t\tnargv[nargc++] = \"-c\";\n+\n+\t\tif (argc < 2)\n+\t\t\tnargv[nargc++] = argv[0];\n+\t\telse {\n+\t\t\tstruct strbuf arg0 = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", argv[0]);\n+\t\t\tnargv[nargc++] = strbuf_detach(&arg0, NULL);\n+\t\t}\n \t}\n \n \tfor (argc = 0; argv[argc]; argc++)\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130541","messageId":"20091230105616.GD22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 4/6] editor: use run_command's shell feature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T10:56:16Z","receivedAt":"2009-12-30T10:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that run_command implements the same code in a more\ngeneral form, we can make use of it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nShould also be non-controversial, and the diffstat shows that this is\nthe payoff for all of the added code earlier in the series. :)\n\n editor.c |   21 ++-------------------\n 1 files changed, 2 insertions(+), 19 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex 615f575..d834003 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -36,26 +36,9 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n \tif (strcmp(editor, \":\")) {\n-\t\tsize_t len = strlen(editor);\n-\t\tint i = 0;\n-\t\tint failed;\n-\t\tconst char *args[6];\n-\t\tstruct strbuf arg0 = STRBUF_INIT;\n+\t\tconst char *args[] = { editor, path, NULL };\n \n-\t\tif (strcspn(editor, \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != len) {\n-\t\t\t/* there are specials */\n-\t\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", editor);\n-\t\t\targs[i++] = \"sh\";\n-\t\t\targs[i++] = \"-c\";\n-\t\t\targs[i++] = arg0.buf;\n-\t\t}\n-\t\targs[i++] = editor;\n-\t\targs[i++] = path;\n-\t\targs[i] = NULL;\n-\n-\t\tfailed = run_command_v_opt_cd_env(args, 0, NULL, env);\n-\t\tstrbuf_release(&arg0);\n-\t\tif (failed)\n+\t\tif (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))\n \t\t\treturn error(\"There was a problem with the editor '%s'.\",\n \t\t\t\t\teditor);\n \t}\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130542","messageId":"20091230110109.GE22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 5/6] textconv: use shell to run helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T11:01:09Z","receivedAt":"2009-12-30T11:01:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently textconv helpers are run directly. Running through\nthe shell is useful because the user can provide a program\nwith command line arguments, like \"antiword -f\".\n\nIt also makes textconv more consistent with other parts of\ngit, most of which run their helpers using the shell.\n\nThe downside is that textconv helpers with shell\nmetacharacters (like space) in the filename will be broken.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the first actual change in behavior. This patch and 6/6 should\nperhaps be held back with a warning period. I am inclined to consider it\na bug that they didn't use the shell, and this is fixing it.\n\n diff.c                         |    1 +\n t/t4030-diff-textconv.sh       |    2 +-\n t/t4031-diff-rewrite-binary.sh |    2 +-\n 3 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d14a575..2794238 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3818,6 +3818,7 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \t*arg = NULL;\n \n \tmemset(&child, 0, sizeof(child));\n+\tchild.use_shell = 1;\n \tchild.argv = argv;\n \tchild.out = -1;\n \tif (start_command(&child) != 0 ||\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex a3f0897..c16d538 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -48,7 +48,7 @@ test_expect_success 'file is considered binary by plumbing' '\n \n test_expect_success 'setup textconv filters' '\n \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"$PWD\"/hexdump &&\n+\tgit config diff.foo.textconv \"\\\"$PWD\\\"\"/hexdump &&\n \tgit config diff.fail.textconv false\n '\n \ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex a894c60..27fb31b 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -54,7 +54,7 @@ chmod +x dump\n \n test_expect_success 'setup textconv' '\n \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"$PWD\"/dump\n+\tgit config diff.foo.textconv \"\\\"$PWD\\\"\"/dump\n '\n \n test_expect_success 'rewrite diff respects textconv' '\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130543","messageId":"20091230110335.GF22959@coredump.intra.peff.net","threadId":"21957","inReplyTo":"20091230095634.GA16349@coredump.intra.peff.net","subject":"[PATCH 6/6] diff: run external diff helper with shell","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-30T11:03:35Z","receivedAt":"2009-12-30T11:03:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is mostly to make it more consistent with the rest of\ngit, which uses the shell to exec helpers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nConsistency is the main advantage here. Though I think you can actually\ndo craziness like setting\n\n  GIT_EXTERNAL_DIFF='my-command $2 $3 && true' git diff\n\nto pick out some subset of the parameters, I can't imagine it is\nactually a good idea.\n\n diff.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2794238..9bb40bc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2294,7 +2294,7 @@ static void run_external_diff(const char *pgm,\n \t}\n \t*arg = NULL;\n \tfflush(NULL);\n-\tretval = run_command_v_opt(spawn_arg, 0);\n+\tretval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);\n \tremove_tempfile();\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n-- \n1.6.6.65.g050d2.dirty\n"},{"id":"130555","messageId":"40aa078e0912300555x5beb1f0ejc27fd544856f6da1@mail.gmail.com","threadId":"21957","inReplyTo":"20091230105316.GA22959@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] run-command: add \"use shell\" option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-30T13:55:43Z","receivedAt":"2009-12-30T13:55:43Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 30, 2009 at 11:53 AM, Jeff King <peff@peff.net> wrote:\n> Many callsites run \"sh -c $CMD\" to run $CMD. We can make it\n> a little simpler for them by factoring out the munging of\n> argv.\n>\n> For simple cases with no arguments, this doesn't help much, but:\n>\n>  1. For cases with arguments, we save the caller from\n>     having to build the appropriate shell snippet.\n>\n>  2. We can later optimize to avoid the shell when\n>     there are no metacharacters in the program.\n>\n\nNice. This could be helpful if we ever decide not to depend on a\nsh-compatible shell on Windows (since one isn't included by default no\nWindows, and installing msys/cygwin has some compatibility-issues with\nprevious installations).\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"130614","messageId":"4B3CD74D.7020605@kdbg.org","threadId":"21957","inReplyTo":"20091230105536.GC22959@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T16:54:37Z","receivedAt":"2009-12-31T16:54:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> If there are no metacharacters in the program to be run, we\n> can just skip running the shell entirely and directly exec\n> the program.\n> \n> The metacharacter test is pulled verbatim from\n> launch_editor, which already implements this optimization.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Something inside me feels wrong with a catch-known-metacharacter test\n> instead of an allow-known-good check. But this is the same test we have\n> been using with launch_editor for some time, so I decided not to mess\n> with it.\n\nThe git version that msysgit ships (and the one that I use on Windows) has \nthis sequence in pager.c:\n\nstatic const char *pager_argv[] = { \"sh\", \"-c\", NULL, NULL };\n...\n\tpager_process.argv = &pager_argv[2];\n\tpager_process.in = -1;\n\tif (start_command(&pager_process)) {\n\t\tpager_process.argv = pager_argv;\n\t\tpager_process.in = -1;\n\t\tif (start_command(&pager_process))\n\t\t\treturn;\n\t}\n\nto help people set\n\n  PAGER=C:\\Program Files\\cygwin\\bin\\less\n\nThat is, we first try to run the program without the shell, then retry \nwrapped in sh -c.\n\nWouldn't it be possible to do the same here, assuming that we don't have \nprograms such as \"editor -f\" in the path?\n\nIt does assume that we are able to detect execvp failure due to ENOENT \nwhich is currently proposed elsewhere by Ilari Liusvaara (and which is \nalready possible on Windows).\n\n-- Hannes\n"},{"id":"130625","messageId":"7v4on7x6w1.fsf@alter.siamese.dyndns.org","threadId":"21957","inReplyTo":"4B3CD74D.7020605@kdbg.org","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-31T19:47:42Z","receivedAt":"2009-12-31T19:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> to help people set\n>\n>  PAGER=C:\\Program Files\\cygwin\\bin\\less\n>\n> That is, we first try to run the program without the shell, then retry\n> wrapped in sh -c.\n>\n> Wouldn't it be possible to do the same here, assuming that we don't\n> have programs such as \"editor -f\" in the path?\n\nIt is a cute idea that covers 70-80% of the cases, as you also have to\nassume that you don't have to specify your own pager on a path with IFS\n(e.g. \"Program files\" in your example) and give your parameter to the\npager at the same time, e.g.\n\n    PAGER=\"C:\\Program Files\\cygwin\\bin\\less -FRSX\"\n\nBecause it has its own LESS environment to set FRSX and you can get away\nwith:\n\n    PAGER=\"C:\\Program Files\\cygwin\\bin\\less\"\n    LESS=FRSX\n\n\"less\" is not a representative example for this issue.  In real life I\nsuspect that custom programs that we don't ship with git (or you don't\nship with msysgit) would lack such a workaround, (and that is why I didn't\nsay \"98% of the cases\").\n"},{"id":"130634","messageId":"20091231214134.GA31399@coredump.intra.peff.net","threadId":"21957","inReplyTo":"4B3CD74D.7020605@kdbg.org","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-31T21:41:35Z","receivedAt":"2009-12-31T21:41:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 31, 2009 at 05:54:37PM +0100, Johannes Sixt wrote:\n\n> The git version that msysgit ships (and the one that I use on\n> Windows) has this sequence in pager.c:\n> \n> static const char *pager_argv[] = { \"sh\", \"-c\", NULL, NULL };\n> ...\n> \tpager_process.argv = &pager_argv[2];\n> \tpager_process.in = -1;\n> \tif (start_command(&pager_process)) {\n> \t\tpager_process.argv = pager_argv;\n> \t\tpager_process.in = -1;\n> \t\tif (start_command(&pager_process))\n> \t\t\treturn;\n> \t}\n\nAs a side note to what you are saying, my patch 2/6 will break this. The\n\"new\" way to do it would be:\n\n  /* do not set pager_argv[2] specially */\n  pager_process.in = -1;\n  if (start_command(&pager_process)) {\n          pager_process.use_shell = 1;\n          pager_process.in = -1;\n          if (start_command(&pager_process))\n                  return;\n  }\n\nthough I am happy to also just revert the pager.c changes and leave it\nto handle the shell itself.\n\nBut of course if we use your trick internally in run-command, then your\npager-specific change can just go away.\n\n> to help people set\n> \n>  PAGER=C:\\Program Files\\cygwin\\bin\\less\n> \n> That is, we first try to run the program without the shell, then\n> retry wrapped in sh -c.\n> \n> Wouldn't it be possible to do the same here, assuming that we don't\n> have programs such as \"editor -f\" in the path?\n\nHmm. That is somewhat clever. And it would actually solve the\nbackward-compatibility problem for helpers moving to shell execution. If\nyou have a textconv of \"/path with space/foo\", it will continue to\nwork transparently, but now \"/path_without_space/foo --option\" will also\nJust Work.\n\nThe two downsides I see are:\n\n  1. You want to run \"foo\" with \"-bar\" in your path but you have \"foo\n     -bar\" in your path (your \"editor -f\" example). This just seems\n     insane to me.\n\n  2. There is a little bit of an interface inconsistency. You can do\n     PAGER=\"/path with space/foo\", but once you want to add an option,\n     PAGER=\"/path with space/foo -bar\" does not do what you mean. You\n     have to understand the magic that is going on, and that you now\n     need to quote the first part.\n\n     I'm not sure that is not a feature, though. You are paying that\n     cost in the transition, but getting the DWYM magic for the common\n     case.\n\n> It does assume that we are able to detect execvp failure due to\n> ENOENT which is currently proposed elsewhere by Ilari Liusvaara (and\n> which is already possible on Windows).\n\nWe could also simply do the path lookup ourselves, decide whether to use\nthe shell, and then exec. Path lookup is not that hard, and I think we\nalready have mingw compat routines for it anyway.\n\n-Peff\n"},{"id":"130635","messageId":"200912312241.37895.j6t@kdbg.org","threadId":"21957","inReplyTo":"7v4on7x6w1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T21:41:37Z","receivedAt":"2009-12-31T21:41:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. Dezember 2009, Junio C Hamano wrote:\n> It is a cute idea that covers 70-80% of the cases, as you also have to\n> assume that you don't have to specify your own pager on a path with IFS\n> (e.g. \"Program files\" in your example) and give your parameter to the\n> pager at the same time, e.g.\n>\n>     PAGER=\"C:\\Program Files\\cygwin\\bin\\less -FRSX\"\n>\n> Because it has its own LESS environment to set FRSX and you can get away\n> with:\n>\n>     PAGER=\"C:\\Program Files\\cygwin\\bin\\less\"\n>     LESS=FRSX\n>\n> \"less\" is not a representative example for this issue.  In real life I\n> suspect that custom programs that we don't ship with git (or you don't\n> ship with msysgit) would lack such a workaround, (and that is why I didn't\n> say \"98% of the cases\").\n\nOTOH, once you see that you would have to set\n\n\tPAGER: C:\\Program Files\\cygwin\\bin\\less -FRSX\n\n(I'm not using shell syntax here; think of a dialog that has name and value in \nseparate edit boxes) then it is rather obvious that this cannot work. If you \nare clever (and you probably are - after all, you are modifying something \nesoteric: the environment!), then you will have heard about the magic \ndouble-quotes, and you will write this as\n\n\tPAGER: \"C:\\Program Files\\cygwin\\bin\\less\" -FRSX\n\ninstead, and it will work as intended.\n\nGranted, \"less\" is not representative.\n\n\tGIT_EDITOR: \"C:\\Program Files\\Notepad++\\notepad++\" -multiInst\n\nis probably more realistic (but I didn't test it).\n\n-- Hannes\n"},{"id":"130636","messageId":"200912312316.47925.j6t@kdbg.org","threadId":"21957","inReplyTo":"20091231214134.GA31399@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T22:16:47Z","receivedAt":"2009-12-31T22:16:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. Dezember 2009, Jeff King wrote:\n> But of course if we use your trick internally in run-command, then your\n> pager-specific change can just go away.\n\nThis is what I had in mind.\n\n> > It does assume that we are able to detect execvp failure due to\n> > ENOENT which is currently proposed elsewhere by Ilari Liusvaara (and\n> > which is already possible on Windows).\n>\n> We could also simply do the path lookup ourselves, decide whether to use\n> the shell, and then exec.\n\nI tried to convince Ilari that this is the way to go, but...\n\n-- Hannes\n"},{"id":"130654","messageId":"20100101045017.GA20769@coredump.intra.peff.net","threadId":"21957","inReplyTo":"200912312316.47925.j6t@kdbg.org","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-01T04:50:18Z","receivedAt":"2010-01-01T04:50:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 31, 2009 at 11:16:47PM +0100, Johannes Sixt wrote:\n\n> > > It does assume that we are able to detect execvp failure due to\n> > > ENOENT which is currently proposed elsewhere by Ilari Liusvaara (and\n> > > which is already possible on Windows).\n> >\n> > We could also simply do the path lookup ourselves, decide whether to use\n> > the shell, and then exec.\n> \n> I tried to convince Ilari that this is the way to go, but...\n\nHow should we proceed, then? The \"DWIM with spaces\" magic seems like\nsomething that can come later, so I am tempted to recommend taking my\nseries now, fixing up msysgit as mentioned earlier (or just dropping the\npager.c portion of my 2/6), and then implementing DWIM once Ilari's\ntopic matures.\n\nWe might want to hold my 5/6 and 6/6 back from master until the DWIM\n(which would make both totally safe, I think).\n\n-Peff\n"},{"id":"130664","messageId":"4B3DC996.1060801@kdbg.org","threadId":"21957","inReplyTo":"20100101045017.GA20769@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] run-command: optimize out useless shell calls","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-01T10:08:22Z","receivedAt":"2010-01-01T10:08:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> How should we proceed, then? The \"DWIM with spaces\" magic seems like\n> something that can come later, so I am tempted to recommend taking my\n> series now, fixing up msysgit as mentioned earlier (or just dropping the\n> pager.c portion of my 2/6), and then implementing DWIM once Ilari's\n> topic matures.\n\nI think this procedure is fine. msysgit can resolve the conflict in \npager.c as suggested earlier.\n\n> We might want to hold my 5/6 and 6/6 back from master until the DWIM\n> (which would make both totally safe, I think).\n\nWould make sense, too.\n\n-- Hannes\n"},{"id":"130669","messageId":"4B3E7354.209@kdbg.org","threadId":"21957","inReplyTo":"20091230105316.GA22959@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] run-command: add \"use shell\" option","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-01T22:12:36Z","receivedAt":"2010-01-01T22:12:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> Many callsites run \"sh -c $CMD\" to run $CMD. We can make it\n> a little simpler for them by factoring out the munging of\n> argv.\n> \n> For simple cases with no arguments, this doesn't help much, but:\n> \n>   1. For cases with arguments, we save the caller from\n>      having to build the appropriate shell snippet.\n> \n>   2. We can later optimize to avoid the shell when\n>      there are no metacharacters in the program.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I made the matching tweak to the Windows half of run-command, but I\n> don't actually have a box to test it on.\n> \n> I modeled this after execv_git_cmd. Like that function, I try to release\n> the allocated argv on error. However, we do actually leak the strbuf\n> memory in one case. I'm not sure how much we care. On unix, this will\n> always happen in a forked process which will either exec or die. On\n> Windows, we seem to already be leaking the prepared argv for the git_cmd\n> case (and now we leak the shell_cmd case, too).\n\nThat is OK. We can fix this when we find a work-load where this is\na problem.\n\nBut would you please squash this in to avoid a warning about an unused\nstatic function on Windows.\n\n---\n  run-command.c |    2 ++\n  1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 22e2777..47ced57 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -48,6 +48,7 @@ static const char **prepare_shell_cmd(const char **argv)\n  \treturn nargv;\n  }\n\n+#ifndef WIN32\n  static int execv_shell_cmd(const char **argv)\n  {\n  \tconst char **nargv = prepare_shell_cmd(argv);\n@@ -56,6 +57,7 @@ static int execv_shell_cmd(const char **argv)\n  \tfree(nargv);\n  \treturn -1;\n  }\n+#endif\n\n  int start_command(struct child_process *cmd)\n  {\n-- \n1.6.6.1073.gd853b.dirty\n"},{"id":"130671","messageId":"4B3E73AE.6050003@kdbg.org","threadId":"21957","inReplyTo":"20091230110335.GF22959@coredump.intra.peff.net","subject":"[PATCH 7/6] t0021: use $SHELL_PATH for the filter script","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-01T22:14:06Z","receivedAt":"2010-01-01T22:14:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Windows, we need the shbang line to correctly invoke shell scripts via\na POSIX shell, except when the script is invoked via 'sh -c' because\nsh (a bash) does \"the right thing\". Since nowadays the clean and smudge\nfilters are not always invoked via 'sh -c' anymore, we have to mark the\nthe one in t0021-conversion with #!$SHELL_PATH.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n  t/t0021-conversion.sh    |    3 ++-\n  t/t4030-diff-textconv.sh |    6 ++++--\n  2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 8fc39d7..6cb8d60 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -4,7 +4,8 @@ test_description='blob conversion via gitattributes'\n\n  . ./test-lib.sh\n\n-cat <<\\EOF >rot13.sh\n+cat <<EOF >rot13.sh\n+#!$SHELL_PATH\n  tr \\\n    'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \\\n    'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex c16d538..3cb7e63 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -19,10 +19,12 @@ cat >expect.text <<'EOF'\n  +1\n  EOF\n\n-cat >hexdump <<'EOF'\n-#!/bin/sh\n+{\n+\techo \"#!$SHELL_PATH\"\n+\tcat <<'EOF'\n  perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n  EOF\n+} >hexdump\n  chmod +x hexdump\n\n  test_expect_success 'setup binary file with history' '\n-- \n1.6.6.1073.gd853b.dirty\n"},{"id":"130670","messageId":"4B3E73F6.3040402@kdbg.org","threadId":"21957","inReplyTo":"4B3E73AE.6050003@kdbg.org","subject":"[PATCH 8/6] t4030, t4031: work around bogus MSYS bash path conversion","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-01T22:15:18Z","receivedAt":"2010-01-01T22:15:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Recall that MSYS bash converts POSIX style absolute paths to Windows style\nabsolute paths. Unfortunately, it converts a program argument that begins\nwith a double-quote and otherwise looks like an absolute POSIX path, but\nin doing so, it strips everything past the second double-quote[*]. This\ncase is triggered in the two test scripts. The work-around is to place the\nWindows style path between the quotes to avoid the path conversion.\n\n[*] It is already bogus that a conversion is even considered when a program\nargument begins with a double-quote because it cannot be an absolute POSIX\npath.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n  t/t4030-diff-textconv.sh       |    2 +-\n  t/t4031-diff-rewrite-binary.sh |    2 +-\n  2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 3cb7e63..9b94647 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -50,7 +50,7 @@ test_expect_success 'file is considered binary by plumbing' '\n\n  test_expect_success 'setup textconv filters' '\n  \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"\\\"$PWD\\\"\"/hexdump &&\n+\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/hexdump &&\n  \tgit config diff.fail.textconv false\n  '\n\ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex 27fb31b..7e7b307 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -54,7 +54,7 @@ chmod +x dump\n\n  test_expect_success 'setup textconv' '\n  \techo file diff=foo >.gitattributes &&\n-\tgit config diff.foo.textconv \"\\\"$PWD\\\"\"/dump\n+\tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/dump\n  '\n\n  test_expect_success 'rewrite diff respects textconv' '\n-- \n1.6.6.1073.gd853b.dirty\n"},{"id":"130721","messageId":"20100103072408.GA23031@sigill.intra.peff.net","threadId":"21957","inReplyTo":"4B3E73AE.6050003@kdbg.org","subject":"Re: [PATCH 7/6] t0021: use $SHELL_PATH for the filter script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-03T07:24:08Z","receivedAt":"2010-01-03T07:24:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 01, 2010 at 11:14:06PM +0100, Johannes Sixt wrote:\n\n> On Windows, we need the shbang line to correctly invoke shell scripts via\n> a POSIX shell, except when the script is invoked via 'sh -c' because\n> sh (a bash) does \"the right thing\". Since nowadays the clean and smudge\n> filters are not always invoked via 'sh -c' anymore, we have to mark the\n> the one in t0021-conversion with #!$SHELL_PATH.\n\nHrm. This does mean we might be breaking users who have helper scripts\nin a similar state to those in the test suite (of course, so does your\npager hack, or anything which might optimize out a shell call).  But\nperhaps given that scripts without a shebang generally don't work on\nWindows, they are not too common and we don't need to worry about it.\n\n-Peff\n"},{"id":"130771","messageId":"4B420E4F.1040706@kdbg.org","threadId":"21957","inReplyTo":"20100103072408.GA23031@sigill.intra.peff.net","subject":"Re: [PATCH 7/6] t0021: use $SHELL_PATH for the filter script","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-04T15:50:39Z","receivedAt":"2010-01-04T15:50:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Fri, Jan 01, 2010 at 11:14:06PM +0100, Johannes Sixt wrote:\n> \n>> On Windows, we need the shbang line to correctly invoke shell scripts via\n>> a POSIX shell, except when the script is invoked via 'sh -c' because\n>> sh (a bash) does \"the right thing\". Since nowadays the clean and smudge\n>> filters are not always invoked via 'sh -c' anymore, we have to mark the\n>> the one in t0021-conversion with #!$SHELL_PATH.\n> \n> Hrm. This does mean we might be breaking users who have helper scripts\n> in a similar state to those in the test suite...\n\nNot helper scripts in general, but only clean and smudge filters, because \nthese have been invoked with \"sh -c\" so far. Everything else, that was not \nrun via \"sh -c\", but now is, is safe.\n\n-- Hannes\n"},{"id":"130776","messageId":"20100104160317.GB9136@coredump.intra.peff.net","threadId":"21957","inReplyTo":"4B420E4F.1040706@kdbg.org","subject":"Re: [PATCH 7/6] t0021: use $SHELL_PATH for the filter script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T16:03:17Z","receivedAt":"2010-01-04T16:03:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2010 at 04:50:39PM +0100, Johannes Sixt wrote:\n\n> >>On Windows, we need the shbang line to correctly invoke shell scripts via\n> >>a POSIX shell, except when the script is invoked via 'sh -c' because\n> >>sh (a bash) does \"the right thing\". Since nowadays the clean and smudge\n> >>filters are not always invoked via 'sh -c' anymore, we have to mark the\n> >>the one in t0021-conversion with #!$SHELL_PATH.\n> >\n> >Hrm. This does mean we might be breaking users who have helper scripts\n> >in a similar state to those in the test suite...\n> \n> Not helper scripts in general, but only clean and smudge filters,\n> because these have been invoked with \"sh -c\" so far. Everything else,\n> that was not run via \"sh -c\", but now is, is safe.\n\nI converted more than that; see my 2/6. It is also the pager, the\nimap-send tunnel helper, and external merge helpers. Not the editor,\nsince it already had the no-metacharacters optimization (though it, too,\ncould be affected if we implement your DWIM trick instead of the\nmetacharacter thing).\n\nSo I think we need to make a conscious decision that this is an\nacceptable change of behavior (and I am totally fine with the change\nhappening -- I just want to be clear about the extent of what is being\nchanged).\n\n-Peff\n"},{"id":"130783","messageId":"4B421B6E.3070501@kdbg.org","threadId":"21957","inReplyTo":"20100104160317.GB9136@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] t0021: use $SHELL_PATH for the filter script","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-04T16:46:38Z","receivedAt":"2010-01-04T16:46:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> I converted more than that; see my 2/6. It is also the pager, the\n> imap-send tunnel helper, and external merge helpers. Not the editor,\n> since it already had the no-metacharacters optimization (though it, too,\n> could be affected if we implement your DWIM trick instead of the\n> metacharacter thing).\n> \n> So I think we need to make a conscious decision that this is an\n> acceptable change of behavior (and I am totally fine with the change\n> happening -- I just want to be clear about the extent of what is being\n> changed).\n\nHm, ok, I see.\n\n- The clean and smudge filters are probably the most important cases.\n\n- I *did* write my own merge script (to merge PNGs!), but I made sure to \nbegin it with #!/bin/bash, and I don't think anybody else is crazy enough \nto write a custom merge script ;)\n\n- imap-send on Windows is so new that I don't think anyone is already \nusing it with a custom tunneling script.\n\n- The change in pager.c is unimportant because all versions shipped so far \n(via msysgit) have the conflicting change that tried without \"sh -c\" first.\n\nI think that these can be handled with an entry in the release notes.\n\n-- Hannes\n"}]}