{"thread":{"id":"45971","subject":"git rebase regression: cannot pass a shell expression directly to --exec","startedAt":"2017-05-15T18:08:45Z","lastAt":"2017-05-16T21:11:54Z","messageCount":29,"participants":["Eric Rannaud","Jeff King","Junio C Hamano","Johannes Schindelin","Jonathan Nieder","Kevin Daudt","Brandon Williams","Linus Torvalds","Eric Wong"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"319808","messageId":"CA+zRj8X3OoejQVhUHD9wvv60jpTEZy06qa0y7TtodfBa1q5bnA@mail.gmail.com","threadId":"45971","inReplyTo":null,"subject":"git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-15T18:08:39Z","receivedAt":"2017-05-15T18:08:45Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"Hi all,\n\nIt used to be possible to run a sequence like:\n\n foo() { echo X; }\n export -f foo\n git rebase --exec foo HEAD~10\n\nSince upgrading to 2.13.0, I had to update my scripts to run:\n\n git rebase --exec \"bash -c foo\" HEAD~10\n\nI'm not sure if this was an intended change. Bisecting with the\nfollowing script:\n\n #!/usr/bin/env bash\n\n make -j8 || exit 3\n\n function foo() {\n echo OK\n }\n export -f foo\n\n pushd tmp\n ../git --exec-path=.. rebase --exec foo HEAD^^\n ret=$?\n # Cleanup if failure\n ../git --exec-path=.. rebase --abort &> /dev/null\n popd\n exit $ret\n\nIt points to this commit:\n\ncommit 18633e1a22a68bbe8e6311a1039d13ebbf6fd041 (refs/bisect/bad)\nAuthor: Johannes Schindelin \nDate: Thu Feb 9 23:23:11 2017 +0100\n\n rebase -i: use the rebase--helper builtin\n\n Now that the sequencer learned to process a \"normal\" interactive rebase,\n we use it. The original shell script is still used for \"non-normal\"\n interactive rebases, i.e. when --root or --preserve-merges was passed.\n\n Please note that the --root option (via the $squash_onto variable) needs\n special handling only for the very first command, hence it is still okay\n to use the helper upon continue/skip.\n\n Also please note that the --no-ff setting is volatile, i.e. when the\n interactive rebase is interrupted at any stage, there is no record of\n it. Therefore, we have to pass it from the shell script to the\n rebase--helper.\n\n Note: the test t3404 had to be adjusted because the the error messages\n produced by the sequencer comply with our current convention to start with\n a lower-case letter.\n\n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n\n\nThanks,\nEric\n"},{"id":"319869","messageId":"20170516032503.bzkxmtqpmppxgi75@sigill.intra.peff.net","threadId":"45971","inReplyTo":"CA+zRj8X3OoejQVhUHD9wvv60jpTEZy06qa0y7TtodfBa1q5bnA@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T03:25:04Z","receivedAt":"2017-05-16T03:25:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 11:08:39AM -0700, Eric Rannaud wrote:\n\n> It used to be possible to run a sequence like:\n> \n> foo() { echo X; }\n> export -f foo\n> git rebase --exec foo HEAD~10\n> \n> Since upgrading to 2.13.0, I had to update my scripts to run:\n> \n> git rebase --exec \"bash -c foo\" HEAD~10\n\nInteresting. The \"exec\" string is still run with a shell. E.g.:\n\n $ git rebase --exec 'for i in 1 2 3; do echo >&2 $i; done' HEAD~5\n Executing: for i in 1 2 3; do echo >&2 $i; done\n 1\n 2\n 3\n Executing: for i in 1 2 3; do echo >&2 $i; done\n 1\n 2\n 3\n [...and so on...]\n\nI wonder if this is falling afoul of the optimization in run-command's\nprepare_shell_cmd() to skip shell invocations for \"simple\" commands.\n\nE.g., if I do:\n\n strace -fe execve git rebase -x foo HEAD^ 2>&1 | grep foo\n\nI see:\n\n ...\n Executing: foo\n [pid 21820] execve(\"foo\", [\"foo\"], [/* 57 vars */]) = -1 ENOENT (No such file or directory)\n fatal: cannot run foo: No such file or directory\n\nBut if I were to add a shell meta-character, like this:\n\n strace -fe execve git rebase -x 'foo;' HEAD^ 2>&1 | grep foo\n\nthen I see:\n\n ...\n Executing: foo;\n [pid 22569] execve(\"/bin/sh\", [\"/bin/sh\", \"-c\", \"foo;\", \"foo;\"], [/* 57 vars */]) = 0\n foo;: 1: foo;: foo: not found\n\nSo I suspect if you added an extraneous semi-colon, your case would work\nagain (and that would confirm for us that this is indeed the problem).\n\n> I'm not sure if this was an intended change. Bisecting with the\n> following script:\n> [...]\n> It points to this commit:\n>\n> commit 18633e1a22a68bbe8e6311a1039d13ebbf6fd041 (refs/bisect/bad)\n> Author: Johannes Schindelin \n> Date: Thu Feb 9 23:23:11 2017 +0100\n>\n> rebase -i: use the rebase--helper builtin\n\nThe optimization in run-command is very old, but the switch to\nrebase--helper presumably moved us from doing that exec in the actual\nshell script to doing it via the C code.\n\nWhich means your exported-function technique has been broken for _most_\nof Git all along, but it just now affected this particular spot.\n\nI'm not sure how to feel about it. In the face of exported functions, we\ncan never do the shell-skipping optimization, because we don't know how\nthe shell is going to interpret even a simple-looking command. And it is\nkind of a neat trick. But I also hate to add extra useless shell\ninvocations for the predominantly common case that people aren't using\nthis trick (or aren't even using a shell that supports function\nexports).\n\nOne hack would be to look for BASH_FUNC_* in the environment and disable\nthe optimization in that case. I think that would make your case Just\nWork. It doesn't help other oddball cases, like:\n\n - you're trying to run a shell builtin that behaves differently than\n its exec-able counterpart\n\n - your shell has some other mechanism for defining commands that we\n would not find via exec. I don't know of one offhand. Obviously $ENV\n could point to a file which defines some, but for most shells would\n not read any startup files for a non-interactive \"sh -c\" invocation.\n\n-Peff\n"},{"id":"319872","messageId":"20170516033736.23cfkouus3p67vhc@sigill.intra.peff.net","threadId":"45971","inReplyTo":"20170516032503.bzkxmtqpmppxgi75@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T03:37:36Z","receivedAt":"2017-05-16T03:37:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 11:25:03PM -0400, Jeff King wrote:\n\n> One hack would be to look for BASH_FUNC_* in the environment and disable\n> the optimization in that case. I think that would make your case Just\n> Work. It doesn't help other oddball cases, like:\n> \n> - you're trying to run a shell builtin that behaves differently than\n> its exec-able counterpart\n> \n> - your shell has some other mechanism for defining commands that we\n> would not find via exec. I don't know of one offhand. Obviously $ENV\n> could point to a file which defines some, but for most shells would\n> not read any startup files for a non-interactive \"sh -c\" invocation.\n\nSo I was thinking something like the patch below, though I guess\ntechnically you could look for BASH_FUNC_$argv[0]%%, which seems to be\nbash's magic variable name. I hate to get too intimate with those\ndetails, though.\n\nAnother option is to speculatively run \"foo\" without the shell, and if\nexecve fails to find it, then fall back to running the shell. That would\ncatch any number of cases where the shell \"somehow\" finds a command that\nwe can't.\n\nYou'd still have confusing behavior if your shell builtin behaved\ndifferently than the exec-able version, though (because we'd quietly use\nthe exec-able one), but I would imagine that's exceedingly rare.\n\nI dunno. Maybe the whole thing is a fool's errand.\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c02bfb2e..8328d27fb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -240,12 +240,24 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n+static int env_has_bash_functions(void)\n+{\n+\textern char **environ;\n+\tchar **key;\n+\n+\tfor (key = environ; *key; key++)\n+\t\tif (starts_with(*key, \"BASH_FUNC_\"))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)\n {\n \tif (!argv[0])\n \t\tdie(\"BUG: shell command is empty\");\n \n-\tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n+\tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0]) ||\n+\t env_has_bash_functions()) {\n #ifndef GIT_WINDOWS_NATIVE\n \t\targv_array_push(out, SHELL_PATH);\n #else\n"},{"id":"319873","messageId":"xmqqr2zp31vg.fsf@gitster.mtv.corp.google.com","threadId":"45971","inReplyTo":"20170516032503.bzkxmtqpmppxgi75@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-16T03:40:51Z","receivedAt":"2017-05-16T03:40:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Interesting. The \"exec\" string is still run with a shell. E.g.:\n> ...\n> I wonder if this is falling afoul of the optimization in run-command's\n> prepare_shell_cmd() to skip shell invocations for \"simple\" commands.\n> ...\n> So I suspect if you added an extraneous semi-colon, your case would work\n> again (and that would confirm for us that this is indeed the problem).\n\nWow, that's a brilliant analysis.\n\n> The optimization in run-command is very old, but the switch to\n> rebase--helper presumably moved us from doing that exec in the actual\n> shell script to doing it via the C code.\n>\n> Which means your exported-function technique has been broken for _most_\n> of Git all along, but it just now affected this particular spot.\n>\n> I'm not sure how to feel about it. In the face of exported functions, we\n> can never do the shell-skipping optimization, because we don't know how\n> the shell is going to interpret even a simple-looking command. And it is\n> kind of a neat trick. But I also hate to add extra useless shell\n> invocations for the predominantly common case that people aren't using\n> this trick (or aren't even using a shell that supports function\n> exports).\n\nI was about to write this off as \"an unfortunate regression, a\nfallout that will likely left unfixed, due to lack of a good\npractical workaround.\"\n\nThe point of rewriting things in C and using run_command() interface\nwas to avoid shell overhead. We are doing an exec already, but\nadding a shell invocation in the middle will double the number of\nexec (and probably add an extra fork as well), which probably is\nmeasurable on systems with slow fork/exec.\n\nThe \"semicolon\" trick is way too obscure, but perhaps can be\ndocumented as an escape hatch?\n\n"},{"id":"319876","messageId":"20170516035357.2wutcd2tyax4tjyn@sigill.intra.peff.net","threadId":"45971","inReplyTo":"xmqqr2zp31vg.fsf@gitster.mtv.corp.google.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T03:53:58Z","receivedAt":"2017-05-16T03:54:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 12:40:51PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Interesting. The \"exec\" string is still run with a shell. E.g.:\n> > ...\n> > I wonder if this is falling afoul of the optimization in run-command's\n> > prepare_shell_cmd() to skip shell invocations for \"simple\" commands.\n> > ...\n> > So I suspect if you added an extraneous semi-colon, your case would work\n> > again (and that would confirm for us that this is indeed the problem).\n> \n> Wow, that's a brilliant analysis.\n\nIf it's right. :) It's all theory at this point.\n\nMy /bin/sh isn't bash, but I should be able to build with\nSHELL_PATH=/bin/bash to reproduce. But I can't:\n\n $ bash\n $ foo() { echo >&2 \"in foo\"; }\n $ export -f foo\n $ bash -c foo\n in foo\n\n $ strace -f 2>&1 git.compile rebase -x 'foo;' HEAD^ | grep foo\n Executing: foo;\n [pid 1788] execve(\"/bin/bash\", [\"/bin/bash\", \"-c\", \"foo;\", \"foo;\"], [/* 60 vars */] \n foo;: foo: command not found\n\nSo I'm not sure why the direct \"bash -c\" can find it, but somehow the\nvariable doesn't make it through to the \"bash -c\" at the lower level.\nReplacing \"foo;\" with \"env\" shows the environment, but BASH_FUNC_foo\nisn't set in it. I'm not sure where it's getting eaten, though.\n\n> The \"semicolon\" trick is way too obscure, but perhaps can be\n> documented as an escape hatch?\n\nYeah, I agree this should be documented if it can't be fixed. I wasn't\nsure if we were giving up just yet.\n\nEither way, though, it wouldn't hurt to mention optimizing out \"maybe\nshell\" optimization, because it can occasionally produce user-visible\neffects. Where would be a good place? In git(1), I guess?\n\nIt's almost something that could go in gitcli(7), but it's not really\nabout the CLI in particular. In most cases the shell-exec'd commands are\nfrom config, but not always (as this case shows). So git-config(1)\nprobably isn't the right place.\n\nAFAIK, we don't talk about this behavior at all in the existing\ndocumentation. And I really don't know where we'd put it that somebody\nwould find it without having to read the documentation exhaustively.\n\n-Peff\n"},{"id":"319883","messageId":"20170516040859.pzkm2ewbmri47rcp@sigill.intra.peff.net","threadId":"45971","inReplyTo":"20170516035357.2wutcd2tyax4tjyn@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T04:08:59Z","receivedAt":"2017-05-16T04:09:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 11:53:57PM -0400, Jeff King wrote:\n\n> My /bin/sh isn't bash, but I should be able to build with\n> SHELL_PATH=/bin/bash to reproduce. But I can't:\n> \n> $ bash\n> $ foo() { echo >&2 \"in foo\"; }\n> $ export -f foo\n> $ bash -c foo\n> in foo\n> \n> $ strace -f 2>&1 git.compile rebase -x 'foo;' HEAD^ | grep foo\n> Executing: foo;\n> [pid 1788] execve(\"/bin/bash\", [\"/bin/bash\", \"-c\", \"foo;\", \"foo;\"], [/* 60 vars */] \n> foo;: foo: command not found\n> \n> So I'm not sure why the direct \"bash -c\" can find it, but somehow the\n> variable doesn't make it through to the \"bash -c\" at the lower level.\n> Replacing \"foo;\" with \"env\" shows the environment, but BASH_FUNC_foo\n> isn't set in it. I'm not sure where it's getting eaten, though.\n\nHmph. It looks like \"dash\" eats it. My \"git.compile\" above is a symlink\nto /path/to/git/bin-wrappers/git. But it looks like our Makefile isn't\nsmart enough to rebuild bin-wrappers when you switch SHELL_PATH, so it\nwill had \"/bin/sh\" in it. Which points to dash on my machine. And\nindeed, dash seems to wipe the environment:\n\n $ foo() { echo >&2 \"in foo\"; }\n $ export -f foo\n $ bash -c foo\n in foo\n $ dash -c \"bash -c foo\"\n bash: foo: command not found\n\nI wonder if it's related to ShellShock protections, or if dash just\nrejects variable names with the \"%\" in them or something. Anyway. That\nexplains why I had trouble reproducing. Using bash consistently as my\nshell lets me reproduce Eric's results. And doing the semicolon trick\ndoes make it work again.\n\nSo I feel confident that I've analyzed the problem correctly, at least.\n\n-Peff\n"},{"id":"319912","messageId":"alpine.DEB.2.21.1.1705161220230.3610@virtualbox","threadId":"45971","inReplyTo":"CA+zRj8X3OoejQVhUHD9wvv60jpTEZy06qa0y7TtodfBa1q5bnA@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-16T10:23:02Z","receivedAt":"2017-05-16T10:23:11Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 15 May 2017, Eric Rannaud wrote:\n\n> It used to be possible to run a sequence like:\n> \n> foo() { echo X; }\n> export -f foo\n> git rebase --exec foo HEAD~10\n\nIt would appear to me that you used a side effect of an implementation\ndetail: that `git rebase -i` was implemented entirely as a shell script.\n\nIn my mind, this was a fragile assumption, and that is why...\n\n> commit 18633e1a22a68bbe8e6311a1039d13ebbf6fd041 (refs/bisect/bad)\n> Author: Johannes Schindelin \n> Date: Thu Feb 9 23:23:11 2017 +0100\n> \n> rebase -i: use the rebase--helper builtin\n\n... this commit, which is a long-overdue start of cleaning up our act,\nbroke your code's assumption.\n\nI am afraid that your code placed too much of a dependency on an\nimplementation detail that changed.\n\nIn short: I think that your fix to your script is correct.\n\nCiao,\nJohannes\n"},{"id":"319914","messageId":"alpine.DEB.2.21.1.1705161227060.3610@virtualbox","threadId":"45971","inReplyTo":"xmqqr2zp31vg.fsf@gitster.mtv.corp.google.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-16T10:46:49Z","receivedAt":"2017-05-16T10:47:20Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 16 May 2017, Junio C Hamano wrote:\n\n> The point of rewriting things in C and using run_command() interface was\n> to avoid shell overhead.\n\nThat statement is missing the point.\n\nIt is true that converting shell scripts to proper builtins avoids the\nshell overhead.\n\nIt is even more true that that results in a nice speed-up that can even be\nfelt on Linux (I feel I have to stress this, as you seem to be mostly\nfocused on Linux usage, and I do not want you to get the impression that\nyou are only doing \"the Windows folks\" a favor when you gracefully accept\nthe patches to convert scripts to proper builtins).\n\nBut the real truth is: shell scripting is not portable.\n\nShell scripting is never only shell scripting, of course. A quite\nundocumented set of utilities is expected to be present for our scripts to\nrun, too: sed, awk, tr, cat, expr, just to name a few.\n\nIt does not end there. For example, sed is not equal to sed. BSD sed has\ndifferent semantics than GNU sed, and we jump through hoops to try to\nensure that our shell scripts run with both versions. Which must make many\na contributor feel a lot less positive about their contributions e.g. when\na reviewer points out that their \\t in their sed statement would break Git\non MacOSX. Sad!\n\nWe place a burden on maintainers targeting platforms that are not Linux.\nIf I remember correctly, we need to override the default shell on one\nUnix, even, because we simply failed to make our shell scripts run with\nit.\n\nAnd then, of course, there are all the problems inflicted on users on that\nmost prevalent Operating System of them all: Windows. Yes, I know, you\n\"non-Windows folk\" would like to happily ignore that little tiny problem,\nbut that's really doing Git a disservice. So stop doing it.\n\nWhat hurts me most about this is that Subversion, a software that us \"Git\nfolk\" pride ourselves in making obsolete, never had those problems. It was\ndesigned with an eye toward portability from the start.\n\nIt is a good thing to support users who want to automate their workflows\nusing scripts, of course. Oh, and please, do not forget to remember that\nthere are tons of scripting languages out there, e.g. AppleScript,\nJavascript, Tcl, Python, Ruby; just because you prefer Bash does not mean\nthe majority of developers do.\n\nSo let's continue to broaden our scripting support. And at the same time,\nlet's also change our mindset so that we can reduce the portability\nproblems of Git.\n\nCiao,\nDscho\n"},{"id":"319925","messageId":"20170516161858.stp4ylwfmbgc4oid@sigill.intra.peff.net","threadId":"45971","inReplyTo":"alpine.DEB.2.21.1.1705161220230.3610@virtualbox","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T16:18:59Z","receivedAt":"2017-05-16T16:19:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 12:23:02PM +0200, Johannes Schindelin wrote:\n\n> On Mon, 15 May 2017, Eric Rannaud wrote:\n> \n> > It used to be possible to run a sequence like:\n> > \n> > foo() { echo X; }\n> > export -f foo\n> > git rebase --exec foo HEAD~10\n> \n> It would appear to me that you used a side effect of an implementation\n> detail: that `git rebase -i` was implemented entirely as a shell script.\n\nI don't think that's true at all. He expected the user-provided \"--exec\"\ncommand to be run by a shell, which seems like a reasonable thing for\nGit to promise (and we already make a similar promise for most\nuser-provided commands that we run). What happens in between, be it\nshell or C code, doesn't matter, and the conversion away from a shell\nscript in this case only tickled an existing bad interaction between\n\"export -f\" and Git's run-command code.\n\nSee my other replies for the full story.\n\nI don't think this has anything in particular to do with git-rebase,\nthough. Our solutions are either:\n\n - declare \"export -f\" as too tricky for our optimization, and teach\n people about the \";\" trick\n\n - figure out some workaround/fallback to disable the shell-skipping\n optimization in this case\n\n-Peff\n"},{"id":"319927","messageId":"20170516164124.GO27400@aiede.svl.corp.google.com","threadId":"45971","inReplyTo":"20170516033736.23cfkouus3p67vhc@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-16T16:41:24Z","receivedAt":"2017-05-16T16:41:37Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+Brandon Williams, who may have more context for execvp-related things)\nHi,\n\nJeff King wrote:\n> On Mon, May 15, 2017 at 11:25:03PM -0400, Jeff King wrote:\n\n>> One hack would be to look for BASH_FUNC_* in the environment and disable\n>> the optimization in that case. I think that would make your case Just\n>> Work. It doesn't help other oddball cases, like:\n>>\n>> - you're trying to run a shell builtin that behaves differently than\n>> its exec-able counterpart\n>>\n>> - your shell has some other mechanism for defining commands that we\n>> would not find via exec. I don't know of one offhand. Obviously $ENV\n>> could point to a file which defines some, but for most shells would\n>> not read any startup files for a non-interactive \"sh -c\" invocation.\n>\n> So I was thinking something like the patch below, though I guess\n> technically you could look for BASH_FUNC_$argv[0]%%, which seems to be\n> bash's magic variable name. I hate to get too intimate with those\n> details, though.\n>\n> Another option is to speculatively run \"foo\" without the shell, and if\n> execve fails to find it, then fall back to running the shell. That would\n> catch any number of cases where the shell \"somehow\" finds a command that\n> we can't.\n\nHm. execvp explicitly does this when it hits ENOEXEC, but not for\nENOENT. Do you know why that is?\n\nI think we want to behave consistently for shell builtins and for\nexported functions --- they are different sides of the same problem,\nand behaving differently between the two feels confusing.\n\nThanks,\nJonathan\n"},{"id":"319929","messageId":"CA+zRj8V94H4E1DPage1-YAVGAxHC_8oYiSoFrwHkg0QbUsJu0Q@mail.gmail.com","threadId":"45971","inReplyTo":"20170516035357.2wutcd2tyax4tjyn@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-16T16:45:38Z","receivedAt":"2017-05-16T16:45:43Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"On Mon, May 15, 2017 at 8:53 PM, Jeff King wrote:\n>> > So I suspect if you added an extraneous semi-colon, your case would work\n>> > again (and that would confirm for us that this is indeed the problem).\n>>\n>> Wow, that's a brilliant analysis.\n>\n> If it's right. :) It's all theory at this point.\n>\n> My /bin/sh isn't bash, but I should be able to build with\n> SHELL_PATH=/bin/bash to reproduce. But I can't:\n\nJust to clarify if there was any doubt, the semicolon trick does\nindeed work fine when bash is your shell.\n\nIt would be nice if it were consistent for all git commands that take\na argument:\n\n foo() { echo foo; }\n export -f foo\n git bisect start master master^^\n git bisect run foo # works\n git bisect run 'foo;' # doesn't work\nrunning foo;\n/usr/lib/git-core/git-bisect: line 493: foo;: command not found\n\nAlso, what's a \"simple command\", exactly?\n\n foo() { echo foo; }\n export -f foo\n git rebase --exec foo master^^ # fails\n git rebase --exec 'foo;' master^^ # OK\n git rebase --exec 'foo 1' master^^ # OK\n\nNot sure if this can be made easy to understand in the manpage.\n\nThanks,\nEric\n"},{"id":"319930","messageId":"20170516164750.3tw6xlbcbyuu5t72@sigill.intra.peff.net","threadId":"45971","inReplyTo":"20170516164124.GO27400@aiede.svl.corp.google.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T16:47:50Z","receivedAt":"2017-05-16T16:48:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 09:41:24AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Mon, May 15, 2017 at 11:25:03PM -0400, Jeff King wrote:\n> \n> >> One hack would be to look for BASH_FUNC_* in the environment and disable\n> >> the optimization in that case. I think that would make your case Just\n> >> Work. It doesn't help other oddball cases, like:\n> >>\n> >> - you're trying to run a shell builtin that behaves differently than\n> >> its exec-able counterpart\n> >>\n> >> - your shell has some other mechanism for defining commands that we\n> >> would not find via exec. I don't know of one offhand. Obviously $ENV\n> >> could point to a file which defines some, but for most shells would\n> >> not read any startup files for a non-interactive \"sh -c\" invocation.\n> >\n> > So I was thinking something like the patch below, though I guess\n> > technically you could look for BASH_FUNC_$argv[0]%%, which seems to be\n> > bash's magic variable name. I hate to get too intimate with those\n> > details, though.\n> >\n> > Another option is to speculatively run \"foo\" without the shell, and if\n> > execve fails to find it, then fall back to running the shell. That would\n> > catch any number of cases where the shell \"somehow\" finds a command that\n> > we can't.\n> \n> Hm. execvp explicitly does this when it hits ENOEXEC, but not for\n> ENOENT. Do you know why that is?\n\nWhen execvp(foo) falls back on ENOEXEC, it is not running \"sh -c foo\".\nIt is actually running \"sh foo\" to run the script contents. So it's\nabout letting you do:\n\n echo \"no shebang line\" >script\n chmod +x script\n ./script\n\nAnd nothing to do with shell builtins.\n\n> I think we want to behave consistently for shell builtins and for\n> exported functions --- they are different sides of the same problem,\n> and behaving differently between the two feels confusing.\n\nYes, I think ideally we'd handle it all transparently. Although I'd also\npoint out that Git the behavior under discussion dates back to 2009 and\nthis is (as far as I can recall) the first report. So documenting the\ncurrent behavior might not be that bad an option.\n\n-Peff\n"},{"id":"319931","messageId":"CA+zRj8Uxh1SGdVW=zz29Q4m4OnDcs665T0XtV6FiZ_qZSiG-8g@mail.gmail.com","threadId":"45971","inReplyTo":"20170516161858.stp4ylwfmbgc4oid@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-16T16:59:03Z","receivedAt":"2017-05-16T16:59:09Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"On Tue, May 16, 2017 at 9:18 AM, Jeff King wrote:\n> On Tue, May 16, 2017 at 12:23:02PM +0200, Johannes Schindelin wrote:\n>> It would appear to me that you used a side effect of an implementation\n>> detail: that `git rebase -i` was implemented entirely as a shell script.\n>\n> I don't think that's true at all. He expected the user-provided \"--exec\"\n> command to be run by a shell, which seems like a reasonable thing for\n> Git to promise (and we already make a similar promise for most\n> user-provided commands that we run). What happens in between, be it\n\nAs a \"user\", my expectation was simply that the command would be run\nnot just in \"a shell\", but in *my* shell (or the shell that calls git,\nmaybe). So I don't see any portability question with respect to Git.\nMy script that uses git rebase --exec may not be portable, but that's\nmy problem.\n\nWhen I use \"git rebase --exec \" I'm basically writing a \"foreach\ncommit in range { }\" in my shell. Same idea with git bisect run.\n\nA transparent optimization that tries execve() then falls back to the\nuser's shell sounds like a good idea.\n"},{"id":"319932","messageId":"CA+zRj8VjNDaX0vgWX5pRMMVnFHzb_Hgx-WhHgVEe74JaRawO0g@mail.gmail.com","threadId":"45971","inReplyTo":"20170516164750.3tw6xlbcbyuu5t72@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-16T17:11:05Z","receivedAt":"2017-05-16T17:11:11Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"On Tue, May 16, 2017 at 9:47 AM, Jeff King wrote:\n>> I think we want to behave consistently for shell builtins and for\n>> exported functions --- they are different sides of the same problem,\n>> and behaving differently between the two feels confusing.\n>\n> Yes, I think ideally we'd handle it all transparently. Although I'd also\n> point out that Git the behavior under discussion dates back to 2009 and\n> this is (as far as I can recall) the first report. So documenting the\n> current behavior might not be that bad an option.\n\nThis is on Arch Linux which is usually pretty aggressive with their\nGit upgrades and I first saw the problem this week. The script that\nhits this case runs constantly on my machine so my guess is that if\nanyone relies on the old behavior, they're just starting to see a\nnew-enough Git to have a problem.\n"},{"id":"319933","messageId":"20170516171414.GC2169@alpha.vpn.ikke.info","threadId":"45971","inReplyTo":"CA+zRj8Uxh1SGdVW=zz29Q4m4OnDcs665T0XtV6FiZ_qZSiG-8g@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-05-16T17:14:14Z","receivedAt":"2017-05-16T17:14:20Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, May 16, 2017 at 09:59:03AM -0700, Eric Rannaud wrote:\n> On Tue, May 16, 2017 at 9:18 AM, Jeff King wrote:\n> > On Tue, May 16, 2017 at 12:23:02PM +0200, Johannes Schindelin wrote:\n> >> It would appear to me that you used a side effect of an implementation\n> >> detail: that `git rebase -i` was implemented entirely as a shell script.\n> >\n> > I don't think that's true at all. He expected the user-provided \"--exec\"\n> > command to be run by a shell, which seems like a reasonable thing for\n> > Git to promise (and we already make a similar promise for most\n> > user-provided commands that we run). What happens in between, be it\n> \n> As a \"user\", my expectation was simply that the command would be run\n> not just in \"a shell\", but in *my* shell (or the shell that calls git,\n> maybe). So I don't see any portability question with respect to Git.\n> My script that uses git rebase --exec may not be portable, but that's\n> my problem.\n> \n> When I use \"git rebase --exec \" I'm basically writing a \"foreach\n> commit in range { }\" in my shell. Same idea with git bisect run.\n> \n> A transparent optimization that tries execve() then falls back to the\n> user's shell sounds like a good idea.\n\nIt does not really work that way. Git runs in a separate process that\ndoes not have access to your current shell. That's why you need to do\n'export -f foo'.\n\nIf you want git to be able to ecute the foo shell function, git needs to\nstart a _new_ shell process, which reads the environment, recognize the\nexported function and run that.\n\nThis is not the same as git executing the command in your shell. Not\nexported variables would not be available in this function (as it would\nbe in your equivalent).\n"},{"id":"319934","messageId":"20170516171540.GG79147@google.com","threadId":"45971","inReplyTo":"20170516164750.3tw6xlbcbyuu5t72@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-16T17:15:40Z","receivedAt":"2017-05-16T17:15:47Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/16, Jeff King wrote:\n> On Tue, May 16, 2017 at 09:41:24AM -0700, Jonathan Nieder wrote:\n> \n> > Jeff King wrote:\n> > > On Mon, May 15, 2017 at 11:25:03PM -0400, Jeff King wrote:\n> > \n> > >> One hack would be to look for BASH_FUNC_* in the environment and disable\n> > >> the optimization in that case. I think that would make your case Just\n> > >> Work. It doesn't help other oddball cases, like:\n> > >>\n> > >> - you're trying to run a shell builtin that behaves differently than\n> > >> its exec-able counterpart\n> > >>\n> > >> - your shell has some other mechanism for defining commands that we\n> > >> would not find via exec. I don't know of one offhand. Obviously $ENV\n> > >> could point to a file which defines some, but for most shells would\n> > >> not read any startup files for a non-interactive \"sh -c\" invocation.\n> > >\n> > > So I was thinking something like the patch below, though I guess\n> > > technically you could look for BASH_FUNC_$argv[0]%%, which seems to be\n> > > bash's magic variable name. I hate to get too intimate with those\n> > > details, though.\n\nOne concern with that is what about all other shells that are not BASH?\nI'm sure they use a different env var for storing functions so we may\nneed to handle other shell's too? That is assuming we want to keep the\nold behavior.\n\n> > >\n> > > Another option is to speculatively run \"foo\" without the shell, and if\n> > > execve fails to find it, then fall back to running the shell. That would\n> > > catch any number of cases where the shell \"somehow\" finds a command that\n> > > we can't.\n> > \n> > Hm. execvp explicitly does this when it hits ENOEXEC, but not for\n> > ENOENT. Do you know why that is?\n> \n> When execvp(foo) falls back on ENOEXEC, it is not running \"sh -c foo\".\n> It is actually running \"sh foo\" to run the script contents. So it's\n> about letting you do:\n> \n> echo \"no shebang line\" >script\n> chmod +x script\n> ./script\n> \n> And nothing to do with shell builtins.\n\nThat's correct, and is the exact behavior I was trying to mimic with the\nchanges to run_command.\n 1. try executing the command.\n 2. if it fails with ENOEXEC then attempt to execute it with a shell\n\n> \n> > I think we want to behave consistently for shell builtins and for\n> > exported functions --- they are different sides of the same problem,\n> > and behaving differently between the two feels confusing.\n> \n> Yes, I think ideally we'd handle it all transparently. Although I'd also\n> point out that Git the behavior under discussion dates back to 2009 and\n> this is (as far as I can recall) the first report. So documenting the\n> current behavior might not be that bad an option.\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"319937","messageId":"CA+zRj8XLQiTmiVbsJZ7XjdW4bQvMSJpCtKW8xx5JvMKOhBDmLQ@mail.gmail.com","threadId":"45971","inReplyTo":"CA+zRj8Uxh1SGdVW=zz29Q4m4OnDcs665T0XtV6FiZ_qZSiG-8g@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-16T17:21:51Z","receivedAt":"2017-05-16T17:21:59Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"On Tue, May 16, 2017 at 9:59 AM, Eric Rannaud wrote:\n> When I use \"git rebase --exec \" I'm basically writing a \"foreach\n> commit in range { }\" in my shell. Same idea with git bisect run.\n>\n> A transparent optimization that tries execve() then falls back to the\n> user's shell sounds like a good idea.\n\nOne issue with the execve-else-shell optimization is that sometimes a\nbinary exists that will shadow an exported function or a shell\nbuiltin:\n\n git rebase --exec true master^^ # OK but in fact this runs /usr/bin/true\n\nIn this case it doesn't really matter. If the optimization is only\napplied to \"simple commands\" (i.e. no arguments), then it's probably\nOK. I can't think of problematic cases. Except weird things like:\n\n $ git rebase --exec time master^\n Executing: time\n Usage: time [-apvV] [-f format] [-o file] [--append] [--verbose]\n [--portability] [--format=format] [--output=file] [--version]\n [--help] command [arg...]\n warning: execution failed: time\n\n/usr/bin/time requires an argument. Even though the bash builtin time\nruns fine without argument.\n\n $ time\n\n real 0m0.000s\n user 0m0.000s\n sys 0m0.000s\n\nBut if the optimization is applied to more complex commands, then we\nwill have problems. For instance, the builtin echo supports \\E, but\n/usr/bin/echo doesn't support it.\n\nIn any case, the manpage says --exec and \" will be\ninterpreted as one or more shell commands.\", it doesn't say \"--exec\n\".\n"},{"id":"319938","messageId":"20170516172307.36hyshwypomlsubx@sigill.intra.peff.net","threadId":"45971","inReplyTo":"20170516171540.GG79147@google.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T17:23:07Z","receivedAt":"2017-05-16T17:23:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 10:15:40AM -0700, Brandon Williams wrote:\n\n> > > > So I was thinking something like the patch below, though I guess\n> > > > technically you could look for BASH_FUNC_$argv[0]%%, which seems to be\n> > > > bash's magic variable name. I hate to get too intimate with those\n> > > > details, though.\n> \n> One concern with that is what about all other shells that are not BASH?\n> I'm sure they use a different env var for storing functions so we may\n> need to handle other shell's too? That is assuming we want to keep the\n> old behavior.\n\nMost other shells don't do function-exporting at all. Certainly dash and\nmost traditional bourne shells don't. I wouldn't be surprised if zsh\ndoes. But yeah, we'd have to support them one by one (and possibly\nvariants across different versions of each shell). Workable, but gross.\n\n> > When execvp(foo) falls back on ENOEXEC, it is not running \"sh -c foo\".\n> > It is actually running \"sh foo\" to run the script contents. So it's\n> > about letting you do:\n> > \n> > echo \"no shebang line\" >script\n> > chmod +x script\n> > ./script\n> > \n> > And nothing to do with shell builtins.\n> \n> That's correct, and is the exact behavior I was trying to mimic with the\n> changes to run_command.\n> 1. try executing the command.\n> 2. if it fails with ENOEXEC then attempt to execute it with a shell\n\nI think the logic here would be more like:\n\n 1. During prepare_shell_cmd(), even if we optimize out the shell call,\n still prepare a fallback argv (since we can't allocate memory\n post-fork).\n\n 2. In the forked child, if we get ENOENT from exec and cmd->use_shell\n is set, then exec the fallback shell argv instead. Propagate its\n results, even if it's 127.\n\nThat still means we'd prefer a $PATH copy of a command to its shell\nbuiltin variant, but that can't be helped (and I kind of doubt anybody\nwould care too much).\n\n-Peff\n"},{"id":"319939","messageId":"CA+zRj8WFuqaNV_6U5=VUrEmeQ_fNaRs8mKxdSd=Z0QMyGk4pHw@mail.gmail.com","threadId":"45971","inReplyTo":"20170516171414.GC2169@alpha.vpn.ikke.info","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Eric Rannaud","fromEmail":"eric.rannaud@gmail.com","sentAt":"2017-05-16T17:29:31Z","receivedAt":"2017-05-16T17:29:37Z","isPatch":false,"sender":{"key":"eric.rannaud@gmail.com","avatar":null},"body":"On Tue, May 16, 2017 at 10:14 AM, Kevin Daudt wrote:\n>> A transparent optimization that tries execve() then falls back to the\n>> user's shell sounds like a good idea.\n>\n> It does not really work that way. Git runs in a separate process that\n> does not have access to your current shell. That's why you need to do\n> 'export -f foo'.\n>\n> If you want git to be able to ecute the foo shell function, git needs to\n> start a _new_ shell process, which reads the environment, recognize the\n> exported function and run that.\n>\n> This is not the same as git executing the command in your shell. Not\n> exported variables would not be available in this function (as it would\n> be in your equivalent).\n\nI'm sorry, I didn't mean (or say) \"my shell process\". Indeed, it\ndoesn't work that way. And to be clear, there is no problem with\nhaving to \"export -f foo\". The only question is how should git run the\n passed to --exec: should it run directly or using a shell?\n"},{"id":"319940","messageId":"20170516173041.GH79147@google.com","threadId":"45971","inReplyTo":"20170516172307.36hyshwypomlsubx@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-16T17:30:41Z","receivedAt":"2017-05-16T17:30:55Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/16, Jeff King wrote:\n> On Tue, May 16, 2017 at 10:15:40AM -0700, Brandon Williams wrote:\n> \n> > > > > So I was thinking something like the patch below, though I guess\n> > > > > technically you could look for BASH_FUNC_$argv[0]%%, which seems to be\n> > > > > bash's magic variable name. I hate to get too intimate with those\n> > > > > details, though.\n> > \n> > One concern with that is what about all other shells that are not BASH?\n> > I'm sure they use a different env var for storing functions so we may\n> > need to handle other shell's too? That is assuming we want to keep the\n> > old behavior.\n> \n> Most other shells don't do function-exporting at all. Certainly dash and\n> most traditional bourne shells don't. I wouldn't be surprised if zsh\n> does. But yeah, we'd have to support them one by one (and possibly\n> variants across different versions of each shell). Workable, but gross.\n> \n> > > When execvp(foo) falls back on ENOEXEC, it is not running \"sh -c foo\".\n> > > It is actually running \"sh foo\" to run the script contents. So it's\n> > > about letting you do:\n> > > \n> > > echo \"no shebang line\" >script\n> > > chmod +x script\n> > > ./script\n> > > \n> > > And nothing to do with shell builtins.\n> > \n> > That's correct, and is the exact behavior I was trying to mimic with the\n> > changes to run_command.\n> > 1. try executing the command.\n> > 2. if it fails with ENOEXEC then attempt to execute it with a shell\n> \n> I think the logic here would be more like:\n\nOh yes, I was just saying what I did not taking into account how we\nwould solve this issue.\n\n> \n> 1. During prepare_shell_cmd(), even if we optimize out the shell call,\n> still prepare a fallback argv (since we can't allocate memory\n> post-fork).\n> \n> 2. In the forked child, if we get ENOENT from exec and cmd->use_shell\n> is set, then exec the fallback shell argv instead. Propagate its\n> results, even if it's 127.\n> \n> That still means we'd prefer a $PATH copy of a command to its shell\n> builtin variant, but that can't be helped (and I kind of doubt anybody\n> would care too much).\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"319941","messageId":"20170516173707.GP27400@aiede.svl.corp.google.com","threadId":"45971","inReplyTo":"20170516164750.3tw6xlbcbyuu5t72@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-16T17:37:07Z","receivedAt":"2017-05-16T17:37:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n>>> On Mon, May 15, 2017 at 11:25:03PM -0400, Jeff King wrote:\n\n>>>> One hack would be to look for BASH_FUNC_* in the environment and disable\n>>>> the optimization in that case. I think that would make your case Just\n>>>> Work. It doesn't help other oddball cases, like:\n[...]\n>> Hm. execvp explicitly does this when it hits ENOEXEC, but not for\n>> ENOENT. Do you know why that is?\n>\n> When execvp(foo) falls back on ENOEXEC, it is not running \"sh -c foo\".\n> It is actually running \"sh foo\" to run the script contents.\n\nOh --- now I get it. This is about the optimization to bypass the shell\nin prepare_shell_command. It has nothing to do with execvp --- our\nexecvp emulation already does the right thing, and Brandon's patches\ndid not make this problem any worse or better.\n\nUnconditionally falling back to sh -c when we get ENOENT in the\nRUN_USING_SHELL case makes sense to me.\n\nSorry for the confusion,\nJonathan\n"},{"id":"319942","messageId":"20170516173727.5zstcg2g5bradhcf@sigill.intra.peff.net","threadId":"45971","inReplyTo":"CA+zRj8XLQiTmiVbsJZ7XjdW4bQvMSJpCtKW8xx5JvMKOhBDmLQ@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T17:37:27Z","receivedAt":"2017-05-16T17:37:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 10:21:51AM -0700, Eric Rannaud wrote:\n\n> On Tue, May 16, 2017 at 9:59 AM, Eric Rannaud wrote:\n> > When I use \"git rebase --exec \" I'm basically writing a \"foreach\n> > commit in range { }\" in my shell. Same idea with git bisect run.\n> >\n> > A transparent optimization that tries execve() then falls back to the\n> > user's shell sounds like a good idea.\n> \n> One issue with the execve-else-shell optimization is that sometimes a\n> binary exists that will shadow an exported function or a shell\n> builtin:\n> \n> git rebase --exec true master^^ # OK but in fact this runs /usr/bin/true\n\nYeah, this is the builtin thing I mentioned elsewhere. I think it's\npretty rare to run a builtin with no arguments and care about the\nbehavior differences.\n\n> /usr/bin/time requires an argument. Even though the bash builtin time\n> runs fine without argument.\n> \n> $ time\n> \n> real 0m0.000s\n> user 0m0.000s\n> sys 0m0.000s\n\nI've run into the \"time\" distinction even when running things from the\nshell, because the time builtin is special. E.g.:\n\n $ time true\n real\t0m0.000s\n user\t0m0.000s\n sys\t0m0.000s\n\n $ (echo foo | time true) 2>&1\n 0.00user 0.00system 0:00.00elapsed 0%CPU (0avgtext+0avgdata 1136maxresident)k\n 0inputs+0outputs (0major+61minor)pagefaults 0swaps\n\nSo to some degree, depending on builtins versus external commands\n(especially when you're round-tripping through another program running a\nsecond shell) is going to have some surprises.\n\n> But if the optimization is applied to more complex commands, then we\n> will have problems. For instance, the builtin echo supports \\E, but\n> /usr/bin/echo doesn't support it.\n\nNo, it shouldn't. If any of\n\n |&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\n\nappear in the string, we always run the shell.\n\n> In any case, the manpage says --exec and \" will be\n> interpreted as one or more shell commands.\", it doesn't say \"--exec\n> \".\n\nRight, it's clearly supposed to use the shell, or behave as if the shell\nwere invoked (within reason).\n\n-Peff\n"},{"id":"319943","messageId":"20170516174106.l2kmlprrimxku5vr@sigill.intra.peff.net","threadId":"45971","inReplyTo":"CA+zRj8WFuqaNV_6U5=VUrEmeQ_fNaRs8mKxdSd=Z0QMyGk4pHw@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T17:41:07Z","receivedAt":"2017-05-16T17:41:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 10:29:31AM -0700, Eric Rannaud wrote:\n\n> > It does not really work that way. Git runs in a separate process that\n> > does not have access to your current shell. That's why you need to do\n> > 'export -f foo'.\n> >\n> > If you want git to be able to ecute the foo shell function, git needs to\n> > start a _new_ shell process, which reads the environment, recognize the\n> > exported function and run that.\n> >\n> > This is not the same as git executing the command in your shell. Not\n> > exported variables would not be available in this function (as it would\n> > be in your equivalent).\n> \n> I'm sorry, I didn't mean (or say) \"my shell process\". Indeed, it\n> doesn't work that way. And to be clear, there is no problem with\n> having to \"export -f foo\". The only question is how should git run the\n> passed to --exec: should it run directly or using a shell?\n\nIt's definitely \"using a shell\". Most things Git runs on your behalf\nbehave the same, but there are some exceptions due to historical warts\n(that would break backwards compatibility if we switched them). E.g.,\n$GIT_SSH does not use the shell, but we introduced GIT_SSH_COMMAND as an\nalternative which does use the shell.\n\nBut note that \"a shell\" may not be necessarily be your login shell. We\nalways execute \"/bin/sh -c\" (and you can override the shell path at\nbuild time). So your \"export -f\" trick only works because your /bin/sh\nis a symlink to bash (or because you specifically built with the\nSHELL_PATH knob pointed at bash).\n\n-Peff\n"},{"id":"319954","messageId":"CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com","threadId":"45971","inReplyTo":"20170516172307.36hyshwypomlsubx@sigill.intra.peff.net","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-05-16T19:14:38Z","receivedAt":"2017-05-16T19:14:44Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, May 16, 2017 at 10:23 AM, Jeff King wrote:\n>\n> I think the logic here would be more like:\n>\n> 1. During prepare_shell_cmd(), even if we optimize out the shell call,\n> still prepare a fallback argv (since we can't allocate memory\n> post-fork).\n>\n> 2. In the forked child, if we get ENOENT from exec and cmd->use_shell\n> is set, then exec the fallback shell argv instead. Propagate its\n> results, even if it's 127.\n>\n> That still means we'd prefer a $PATH copy of a command to its shell\n> builtin variant, but that can't be helped (and I kind of doubt anybody\n> would care too much).\n\nI think it would be better to just\n\n (a) get rid of the magic strcspn() entirely\n\n (b) make the 'can we optimize this' test be simply just looking up\n'argv[0]' in $PATH\n\nHmm? If you have executables with special characters in them in your\nPATH, you have bigger issues.\n\nAlso, if people really want to optimize the code that executes an\nexternal program (whether in shell or directly), I think it might be\nworth it to look at replacing the \"fork()\" with a \"vfork()\".\n\nSomething like this\n\n- cmd->pid = fork();\n+ cmd->pid = (cmd->git_cmd || cmd->env) ? fork() : vfork();\n\nmight work (the native git_cmd case needs a real fork, and if we\nchange the environment variables we need it too, but the other cases\nlook like they might work with vfork()).\n\nUsing vfork() can be hugely more efficient, because you don't have the\nextra page table copies and teardown, but also avoid a lot of possible\ncopy-on-write faults.\n\n Linus\n"},{"id":"319956","messageId":"20170516193557.GA14257@dcvr","threadId":"45971","inReplyTo":"CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com","subject":"[TANGENT] run-command: use vfork instead of fork","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-05-16T19:35:57Z","receivedAt":"2017-05-16T19:36:04Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Linus Torvalds wrote:\n> Also, if people really want to optimize the code that executes an\n> external program (whether in shell or directly), I think it might be\n> worth it to look at replacing the \"fork()\" with a \"vfork()\".\n> \n> Something like this\n> \n> - cmd->pid = fork();\n> + cmd->pid = (cmd->git_cmd || cmd->env) ? fork() : vfork();\n> \n> might work (the native git_cmd case needs a real fork, and if we\n> change the environment variables we need it too, but the other cases\n> look like they might work with vfork()).\n> \n> Using vfork() can be hugely more efficient, because you don't have the\n> extra page table copies and teardown, but also avoid a lot of possible\n> copy-on-write faults.\n\nFwiw, most of the vfork preparation was already done by Brandon\nand myself a few weeks ago, and cooking in pu.\n\nI think only the patch below would be needed to enable vfork\n(along with any build-time detection)\n\nHowever, I haven't noticed enough forking in git to make a\ndifference (but maybe others do). I think it would make a\nbigger difference if such changes were made to bash, dash,\nmake, and perl5.\n\n--------8<------------\nSubject: [PATCH] run-command: use vfork instead of fork\n\nTo enable vfork, we merely have to avoid modifying memory we\nshare with the parent, so the guard functions\n`child_(error|warn|die)_fn` can now be disabled.\n\nFIXME: still missing autoconf + Makefile portability tweaks.\n\nSigned-off-by: Eric Wong \n---\n run-command.c | 28 +---------------------------\n 1 file changed, 1 insertion(+), 27 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 9e36151bf9..0292dd94b6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -324,25 +324,6 @@ static void fake_fatal(const char *err, va_list params)\n \tvreportf(\"fatal: \", err, params);\n }\n \n-static void child_error_fn(const char *err, va_list params)\n-{\n-\tconst char msg[] = \"error() should not be called in child\\n\";\n-\txwrite(2, msg, sizeof(msg) - 1);\n-}\n-\n-static void child_warn_fn(const char *err, va_list params)\n-{\n-\tconst char msg[] = \"warn() should not be called in child\\n\";\n-\txwrite(2, msg, sizeof(msg) - 1);\n-}\n-\n-static void NORETURN child_die_fn(const char *err, va_list params)\n-{\n-\tconst char msg[] = \"die() should not be called in child\\n\";\n-\txwrite(2, msg, sizeof(msg) - 1);\n-\t_exit(2);\n-}\n-\n /* this runs in the parent process */\n static void child_err_spew(struct child_process *cmd, struct child_err *cerr)\n {\n@@ -658,17 +639,10 @@ int start_command(struct child_process *cmd)\n \t * never be released in the child process. This means only\n \t * Async-Signal-Safe functions are permitted in the child.\n \t */\n-\tcmd->pid = fork();\n+\tcmd->pid = vfork();\n \tfailed_errno = errno;\n \tif (!cmd->pid) {\n \t\tint sig;\n-\t\t/*\n-\t\t * Ensure the default die/error/warn routines do not get\n-\t\t * called, they can take stdio locks and malloc.\n-\t\t */\n-\t\tset_die_routine(child_die_fn);\n-\t\tset_error_routine(child_error_fn);\n-\t\tset_warn_routine(child_warn_fn);\n \n \t\tclose(notify_pipe[0]);\n \t\tset_cloexec(notify_pipe[1]);\n-- \nAlso fetchable: git://bogomips.org/git-svn vfork-test\ncommit 5f88d79182aaabc5ea467d1d29e13e45bd2b99bf\n"},{"id":"319959","messageId":"alpine.DEB.2.21.1.1705162211320.3610@virtualbox","threadId":"45971","inReplyTo":"CA+55aFwB-MWASj7dZWkXWhgd4gvEfoOhL6Fo7kXeJSm9dht4Jg@mail.gmail.com","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-16T20:12:10Z","receivedAt":"2017-05-16T20:12:32Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Linus,\n\nOn Tue, 16 May 2017, Linus Torvalds wrote:\n\n> On Tue, May 16, 2017 at 10:23 AM, Jeff King wrote:\n> >\n> > I think the logic here would be more like:\n> >\n> > 1. During prepare_shell_cmd(), even if we optimize out the shell call,\n> > still prepare a fallback argv (since we can't allocate memory\n> > post-fork).\n> >\n> > 2. In the forked child, if we get ENOENT from exec and cmd->use_shell\n> > is set, then exec the fallback shell argv instead. Propagate its\n> > results, even if it's 127.\n> >\n> > That still means we'd prefer a $PATH copy of a command to its shell\n> > builtin variant, but that can't be helped (and I kind of doubt anybody\n> > would care too much).\n> \n> I think it would be better to just\n> \n> (a) get rid of the magic strcspn() entirely\n> \n> (b) make the 'can we optimize this' test be simply just looking up\n> 'argv[0]' in $PATH\n\nWhat about\n\n\tABC=1 my-executable my-arg\n\nCiao,\nDscho\n"},{"id":"319961","messageId":"CA+55aFwX2RDwT=g4w55bbLV38ggaVpdcLpMcCvqMcKRziUSesQ@mail.gmail.com","threadId":"45971","inReplyTo":"alpine.DEB.2.21.1.1705162211320.3610@virtualbox","subject":"Re: git rebase regression: cannot pass a shell expression directly to --exec","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-05-16T20:27:42Z","receivedAt":"2017-05-16T20:27:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, May 16, 2017 at 1:12 PM, Johannes Schindelin\n wrote:\n>>\n>> I think it would be better to just\n>>\n>> (a) get rid of the magic strcspn() entirely\n>>\n>> (b) make the 'can we optimize this' test be simply just looking up\n>> 'argv[0]' in $PATH\n>\n> What about\n>\n> ABC=1 my-executable my-arg\n\nWhat about it?\n\nDo you have a binary like\n\n '/usr/bin/ABC=1 my-executable my-arg'\n\nor something? If so, it gets executed. If not, it would get passed off\nto \"/bin/sh\".\n\nThat sounds a lot more obvious than \"some random set of characters are magical\".\n\n Linus\n"},{"id":"319963","messageId":"CA+55aFygKMJH3BRitQftmtCTx6es93CTR4_y1ULGGE2rSsMyog@mail.gmail.com","threadId":"45971","inReplyTo":"20170516193557.GA14257@dcvr","subject":"Re: [TANGENT] run-command: use vfork instead of fork","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-05-16T20:47:41Z","receivedAt":"2017-05-16T20:47:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, May 16, 2017 at 12:35 PM, Eric Wong wrote:\n>\n> Fwiw, most of the vfork preparation was already done by Brandon\n> and myself a few weeks ago, and cooking in pu.\n\nOh, interesting. Was that done for vfork(), or is it for something\nelse? Some of the changes seem almost overly careful. Is this for\nprep-work porting to some odd environment that doesn't really have a\nMMU at all? There's nothing fundamentally wrong with allocating memory\nafter fork().\n\nBut yes, it looks like it helps the vfork case.\n\n Linus\n"},{"id":"319965","messageId":"20170516211125.GA74302@google.com","threadId":"45971","inReplyTo":"CA+55aFygKMJH3BRitQftmtCTx6es93CTR4_y1ULGGE2rSsMyog@mail.gmail.com","subject":"Re: [TANGENT] run-command: use vfork instead of fork","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-16T21:11:25Z","receivedAt":"2017-05-16T21:11:54Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/16, Linus Torvalds wrote:\n> On Tue, May 16, 2017 at 12:35 PM, Eric Wong wrote:\n> >\n> > Fwiw, most of the vfork preparation was already done by Brandon\n> > and myself a few weeks ago, and cooking in pu.\n> \n> Oh, interesting. Was that done for vfork(), or is it for something\n> else? Some of the changes seem almost overly careful. Is this for\n> prep-work porting to some odd environment that doesn't really have a\n> MMU at all? There's nothing fundamentally wrong with allocating memory\n> after fork().\n> \n> But yes, it looks like it helps the vfork case.\n> \n> Linus\n\nI started working on the run-command code when I ran into a deadlock in\n'git grep --recurse-submodules'. When I added support for submodules to\ngrep I just assumed that launching a process (which atm is unfortunately\nthe only way to work on a submodule) would work in a multi-threaded\nenvironment. I was naive and wrong!\n\nThe deadlock was due to a malloc lock being held by thread 'A' while\nthread 'b' tried to launch a process. Since that lock was in a\nlocked-state at the time of forking, it remained in a locked-state with\nno hope of ever being released. So when the child process that thread\n'b' spawned tried to malloc a chunk of memory after forking, it\ndeadlocked.\n\nI didn't catch this in initial testing because gclib registers\natfork_handelers in order to prevent this sort of thing, while libraries\nlike tcmalloc don't do this.\n\nSo to account for this, I worked to make run-command safe to use in the\npresence of threads, which had the benefit of also preparing it to be\nvfork() ready.\n\nUltimately I'd like to drop the requirement to spawn a child process to\nwork on a submodule, but that's going to take a lot more effort.\n\n-- \nBrandon Williams\n"}]}