{"thread":{"id":"61783","subject":"Can dependency on /bin/sh be removed?","startedAt":"2024-07-15T18:41:57Z","lastAt":"2024-07-17T05:53:36Z","messageCount":12,"participants":["Scott Moser","Junio C Hamano","brian m. carlson","Jeff King","Andreas Schwab","Paul Smith"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498747","messageId":"CADaTQqDZ_6wORXOFc2CE90aizgHJ116NDHZhNeY4Nx7NH8DHJw@mail.gmail.com","threadId":"61783","inReplyTo":null,"subject":"Can dependency on /bin/sh be removed?","fromName":"Scott Moser","fromEmail":"scott.moser@chainguard.dev","sentAt":"2024-07-15T18:41:43Z","receivedAt":"2024-07-15T18:41:57Z","isPatch":false,"sender":{"key":"scott.moser@chainguard.dev","avatar":null},"body":"Hi,\n\nI'm looking at putting together minimal images to run git. In order\nto make gitcredentials to work (git config credential.helper) the\nimage needs a /bin/sh.\n\nI realize there are many workflows that are going to need a shell, but\nit does not seem like it should be required in order to handle a\ngitconfig like:\n\n [credential-helper]\n helper = /bin/myhelper\n\nIn that case, the shell is only being used to tokenize 'myhelper get'.\n\nIs there a solution that I'm missing here?\n\nWould upstream be open to some modifier on the helper value that would\nindicate \"do not pass to shell\" ? Like a '@' to indicate \"direct\ninvoke\" rather than letting shell handle?\n\n [credential-helper]\n helper = @/bin/myhelper\n\nScott\n"},{"id":"498755","messageId":"xmqq8qy21k9f.fsf@gitster.g","threadId":"61783","inReplyTo":"CADaTQqDZ_6wORXOFc2CE90aizgHJ116NDHZhNeY4Nx7NH8DHJw@mail.gmail.com","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-15T20:18:52Z","receivedAt":"2024-07-15T20:18:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Scott Moser writes:\n\n> [credential-helper]\n> helper = /bin/myhelper\n>\n> In that case, the shell is only being used to tokenize 'myhelper get'.\n>\n> Is there a solution that I'm missing here?\n\nIsn't the above example faulty? Shouldn't it be more like\n\n\t[credential]\n\t\thelper = /bin/myhelper\n\n> Would upstream be open to some modifier on the helper value that would\n> indicate \"do not pass to shell\" ? Like a '@' to indicate \"direct\n> invoke\" rather than letting shell handle?\n\nAbsolutely not.\n\nMy first gut reaction regarding what needs to happen was that\nsomebody needs to teach credential.c:run_credential_helper() the\nsame trick as run-command.c::prepare_shell_cmd().\n\nIn the latter codepath, run_command() calls start_command() which in\nturn calls prepare_cmd(). The prepare_cmd() then dispatches between\nprepare_shell_cmd() and strvec_pushv(), but even when .use_shell is\nset and prepare_shell_cmd() is called, prepare_shell_cmd() knows\nthat it can bypass the shell altogether if the command is simple\nenough (and /bin/myhelper is indeed simple enough). And when that\nsimplification is taken, shell is not involved at all to run that\nsimple command.\n\nEven though the code path starting from start_command() is what\nrun_credential_helper() does use, what is run is NOT a simple\ncommand \"/bin/myhelper\". It will receive arguments, like\n\n\t/bin/myhelper erase\n\t/bin/myhelper get\n\t/bin/myhelper store\n\netc., because the caller appends these operation verbs to the value\nof the configuration variable. And as you found out, to tokenize them\ninto two, we need shell.\n\nWe may be able to teach credential.c:credential_do() not to paste\nthe operation verb to the command line so early. Instead you could\nteach the function to send the command line and operation verb\nseparately down to run_credential_helper() though. That way, we\nmight be able to avoid the shell in this particular case. That is,\nif we can \n\n * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()\n codepath to take the usual route _without_ the operation verb\n tucked to the command line, we would get cmd->args.v[] that does\n not rely on the shell;\n\n * Then before the prepared command is executed, if we can somehow\n _append_ to cmd->args.v[] the operation verb (after all, that\n wants to become the argv[1] to the spawned command) before\n start_command() exec's it\n\nthen we are done.\n\nHaving said that, I do not think you can avoid /bin/sh if your goal\nis \"minimal image *to run git*\", as there are many things we run,\nstarting from the editor and the pager and end-user hooks. The\ncredential helper is probably the least of your problems. What's a\nminimum /bin/dash image cost these days?\n\n\n"},{"id":"498760","messageId":"ZpWYmZwcfZMeKpfe@tapette.crustytoothpaste.net","threadId":"61783","inReplyTo":"xmqq8qy21k9f.fsf@gitster.g","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-15T21:46:01Z","receivedAt":"2024-07-15T21:46:09Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-15 at 20:18:52, Junio C Hamano wrote:\n> Having said that, I do not think you can avoid /bin/sh if your goal\n> is \"minimal image *to run git*\", as there are many things we run,\n> starting from the editor and the pager and end-user hooks. The\n> credential helper is probably the least of your problems. What's a\n> minimum /bin/dash image cost these days?\n\nDebian provides busybox-static, a statically linked, multi-call busybox\nbinary including sh, other POSIX utilities, and quite a lot of other\nfeatures, all in a 1.9 MiB binary. If you don't need, say, httpd or\ndpkg emulation, you can build a much more stripped down version for less\ndisk usage. Alpine Linux is based off of busybox (dynamically linked at\n808 KiB), so we can assume that that configuration works just fine.\n\nWe know that Git for Windows also ships a configuration using busybox\nsuccessfully (although with a much larger size).\n\nI am very interested to know what kind of restricted environment needs\nGit (especially with a credential helper) but doesn't ship with a POSIX\nsh. I suppose a restricted Windows environment could qualify.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"498767","messageId":"20240715235212.GA628996@coredump.intra.peff.net","threadId":"61783","inReplyTo":"xmqq8qy21k9f.fsf@gitster.g","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-15T23:52:12Z","receivedAt":"2024-07-15T23:52:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote:\n\n> Even though the code path starting from start_command() is what\n> run_credential_helper() does use, what is run is NOT a simple\n> command \"/bin/myhelper\". It will receive arguments, like\n> \n> \t/bin/myhelper erase\n> \t/bin/myhelper get\n> \t/bin/myhelper store\n> \n> etc., because the caller appends these operation verbs to the value\n> of the configuration variable. And as you found out, to tokenize them\n> into two, we need shell.\n\nIt is also usually \"git myhelper get\", and so on (though you can\nconfigure a shell command).\n\n> We may be able to teach credential.c:credential_do() not to paste\n> the operation verb to the command line so early. Instead you could\n> teach the function to send the command line and operation verb\n> separately down to run_credential_helper() though. That way, we\n> might be able to avoid the shell in this particular case. That is,\n> if we can \n> \n> * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()\n> codepath to take the usual route _without_ the operation verb\n> tucked to the command line, we would get cmd->args.v[] that does\n> not rely on the shell;\n> \n> * Then before the prepared command is executed, if we can somehow\n> _append_ to cmd->args.v[] the operation verb (after all, that\n> wants to become the argv[1] to the spawned command) before\n> start_command() exec's it\n> \n> then we are done.\n\nYes, I think this is reasonable. You'd also perhaps want to have it set\nchild->git_cmd as appropriate (though really, I do not think that does\nanything except stick \"git\" into child.args[0], so we could just do that\nourselves).\n\nI'm actually a little surprised it was not written this way in the first\nplace. In the non-!, non-absolute-path case we are pasting together a\nstring that will be passed to the shell, and it includes the \"helper\"\nargument without further quoting. I don't think you could smuggle a\nsemicolon into there (due to our protocol restrictions), but it does\nseem like a possible shell injection route.\n\nI think it probably goes all the way back to my abca927dbe (introduce\ncredentials API, 2011-12-10).\n\n> Having said that, I do not think you can avoid /bin/sh if your goal\n> is \"minimal image *to run git*\", as there are many things we run,\n> starting from the editor and the pager and end-user hooks. The\n> credential helper is probably the least of your problems. What's a\n> minimum /bin/dash image cost these days?\n\nRight. The bigger issue to me is that the credential helper \"!\" syntax\nis defined to the end user as running a shell (and ditto for all of\nthose other spots). So any platform where we can't run a shell would not\nbe fully compatible with git on other platforms.\n\nThat may be an OK limitation as long as it is advertised clearly, but\nthe use of a shell here is not just an implementation detail, but an\nintentional user-facing design.\n\n-Peff\n"},{"id":"498789","messageId":"CADaTQqB4wm5qzRzgRw7wz1L=Lju=X9iKtktLgdN2MfKf0kg3jA@mail.gmail.com","threadId":"61783","inReplyTo":"20240715235212.GA628996@coredump.intra.peff.net","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Scott Moser","fromEmail":"scott.moser@chainguard.dev","sentAt":"2024-07-16T15:23:15Z","receivedAt":"2024-07-16T15:23:28Z","isPatch":false,"sender":{"key":"scott.moser@chainguard.dev","avatar":null},"body":"On Mon, Jul 15, 2024 at 7:52 PM Jeff King wrote:\n>\n> On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote:\n\n> Yes, I think this is reasonable. You'd also perhaps want to have it set\n> child->git_cmd as appropriate (though really, I do not think that does\n> anything except stick \"git\" into child.args[0], so we could just do that\n> ourselves).\n>\n> I'm actually a little surprised it was not written this way in the first\n> place.\n\nI was too. It seems odd to combine the arguments into a single string\nearly. I was also surprised / didn't realize that 'use_shell' might be\nignored.\n\n> > Having said that, I do not think you can avoid /bin/sh if your goal\n> > is \"minimal image *to run git*\", as there are many things we run,\n> > starting from the editor and the pager and end-user hooks. The\n> > credential helper is probably the least of your problems. What's a\n> > minimum /bin/dash image cost these days?\n\nAdding dash is ultimately what we're going to do at least for now.\nThat won't get us a pager or an editor. That's fine. The image will be\nused in non-interactive environments such as c-i.\n\nNot being able to utilize your custom hook that runs awk and sed\nis one thing. Not being able to build your private repo from github\nis another.\n\nThanks for your input.\n\nOn Mon, Jul 15, 2024 at 7:52 PM Jeff King wrote:\n>\n> On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote:\n>\n> > Even though the code path starting from start_command() is what\n> > run_credential_helper() does use, what is run is NOT a simple\n> > command \"/bin/myhelper\". It will receive arguments, like\n> >\n> > /bin/myhelper erase\n> > /bin/myhelper get\n> > /bin/myhelper store\n> >\n> > etc., because the caller appends these operation verbs to the value\n> > of the configuration variable. And as you found out, to tokenize them\n> > into two, we need shell.\n>\n> It is also usually \"git myhelper get\", and so on (though you can\n> configure a shell command).\n>\n> > We may be able to teach credential.c:credential_do() not to paste\n> > the operation verb to the command line so early. Instead you could\n> > teach the function to send the command line and operation verb\n> > separately down to run_credential_helper() though. That way, we\n> > might be able to avoid the shell in this particular case. That is,\n> > if we can\n> >\n> > * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()\n> > codepath to take the usual route _without_ the operation verb\n> > tucked to the command line, we would get cmd->args.v[] that does\n> > not rely on the shell;\n> >\n> > * Then before the prepared command is executed, if we can somehow\n> > _append_ to cmd->args.v[] the operation verb (after all, that\n> > wants to become the argv[1] to the spawned command) before\n> > start_command() exec's it\n> >\n> > then we are done.\n>\n> Yes, I think this is reasonable. You'd also perhaps want to have it set\n> child->git_cmd as appropriate (though really, I do not think that does\n> anything except stick \"git\" into child.args[0], so we could just do that\n> ourselves).\n>\n> I'm actually a little surprised it was not written this way in the first\n> place. In the non-!, non-absolute-path case we are pasting together a\n> string that will be passed to the shell, and it includes the \"helper\"\n> argument without further quoting. I don't think you could smuggle a\n> semicolon into there (due to our protocol restrictions), but it does\n> seem like a possible shell injection route.\n>\n> I think it probably goes all the way back to my abca927dbe (introduce\n> credentials API, 2011-12-10).\n>\n> > Having said that, I do not think you can avoid /bin/sh if your goal\n> > is \"minimal image *to run git*\", as there are many things we run,\n> > starting from the editor and the pager and end-user hooks. The\n> > credential helper is probably the least of your problems. What's a\n> > minimum /bin/dash image cost these days?\n>\n> Right. The bigger issue to me is that the credential helper \"!\" syntax\n> is defined to the end user as running a shell (and ditto for all of\n> those other spots). So any platform where we can't run a shell would not\n> be fully compatible with git on other platforms.\n>\n> That may be an OK limitation as long as it is advertised clearly, but\n> the use of a shell here is not just an implementation detail, but an\n> intentional user-facing design.\n>\n> -Peff\n"},{"id":"498793","messageId":"xmqqfrs99u1q.fsf@gitster.g","threadId":"61783","inReplyTo":"CADaTQqB4wm5qzRzgRw7wz1L=Lju=X9iKtktLgdN2MfKf0kg3jA@mail.gmail.com","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T16:32:33Z","receivedAt":"2024-07-16T16:32:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Scott Moser writes:\n\n> I was too. It seems odd to combine the arguments into a single string\n> early. I was also surprised / didn't realize that 'use_shell' might be\n> ignored.\n\nCall it \"optimized away\" ;-)\n"},{"id":"498798","messageId":"20240716192307.GA12536@coredump.intra.peff.net","threadId":"61783","inReplyTo":"20240715235212.GA628996@coredump.intra.peff.net","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-16T19:23:07Z","receivedAt":"2024-07-16T19:29:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 15, 2024 at 07:52:12PM -0400, Jeff King wrote:\n\n> > We may be able to teach credential.c:credential_do() not to paste\n> > the operation verb to the command line so early. Instead you could\n> > teach the function to send the command line and operation verb\n> > separately down to run_credential_helper() though. That way, we\n> > might be able to avoid the shell in this particular case. That is,\n> > if we can \n> > \n> > * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()\n> > codepath to take the usual route _without_ the operation verb\n> > tucked to the command line, we would get cmd->args.v[] that does\n> > not rely on the shell;\n> > \n> > * Then before the prepared command is executed, if we can somehow\n> > _append_ to cmd->args.v[] the operation verb (after all, that\n> > wants to become the argv[1] to the spawned command) before\n> > start_command() exec's it\n> > \n> > then we are done.\n> \n> Yes, I think this is reasonable. You'd also perhaps want to have it set\n> child->git_cmd as appropriate (though really, I do not think that does\n> anything except stick \"git\" into child.args[0], so we could just do that\n> ourselves).\n> \n> I'm actually a little surprised it was not written this way in the first\n> place. In the non-!, non-absolute-path case we are pasting together a\n> string that will be passed to the shell, and it includes the \"helper\"\n> argument without further quoting. I don't think you could smuggle a\n> semicolon into there (due to our protocol restrictions), but it does\n> seem like a possible shell injection route.\n> \n> I think it probably goes all the way back to my abca927dbe (introduce\n> credentials API, 2011-12-10).\n\nAh, having tried to refactor it, I see now why it is written as it is.\nEven for a regular helper without \"!\", it is important that we construct\na string and pass it to the shell, since it is legal (and even\nencouraged) to do things like:\n\n [credential]\n helper = cache --socket=/path/to/socket --timeout=123\n\nArguably we could have gotten away with word-splitting ourselves,\nsticking the result in child_process.args, and avoided the shell. But\nthe use of the shell is documented in gitcredentials(7):\n\n helper\n The name of an external credential helper, and any associated\n options. If the helper name is not an absolute path, then the string\n git credential- is prepended. The resulting string is executed by\n the shell (so, for example, setting this to foo --option=bar will\n execute git credential-foo --option=bar via the shell. See the\n manual of specific helpers for examples of their use.\n\nSo users may be depending on that to do \"--socket=$HOME/.foo\", or even\nmore exotic shell constructs.\n\nAgain, it's possible that we could detect that no shell metacharacters\nare in play and do the word-splitting ourselves. But at that point I\nthink it should go into run-command's prepare_shell_cmd(). That is, I I\nthink it could take space out of the list of metachars that force us to\ninvoke the shell, and do the word-splitting there. But not having\nthought very hard about it, there are probably corner cases where that\noptimization is detectable by the user (presumably unusual IFS, but\nmaybe more?).\n\n-Peff\n"},{"id":"498803","messageId":"xmqqo76x6r69.fsf@gitster.g","threadId":"61783","inReplyTo":"20240716192307.GA12536@coredump.intra.peff.net","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-16T20:02:54Z","receivedAt":"2024-07-16T20:03:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [credential]\n> helper = cache --socket=/path/to/socket --timeout=123\n>\n> Arguably we could have gotten away with word-splitting ourselves,\n> sticking the result in child_process.args, and avoided the shell. But\n> the use of the shell is documented in gitcredentials(7):\n>\n> helper\n> The name of an external credential helper, and any associated\n> options. If the helper name is not an absolute path, then the string\n> git credential- is prepended. The resulting string is executed by\n> the shell (so, for example, setting this to foo --option=bar will\n> execute git credential-foo --option=bar via the shell. See the\n> manual of specific helpers for examples of their use.\n>\n> So users may be depending on that to do \"--socket=$HOME/.foo\", or even\n> more exotic shell constructs.\n>\n> Again, it's possible that we could detect that no shell metacharacters\n> are in play and do the word-splitting ourselves. But at that point I\n> think it should go into run-command's prepare_shell_cmd(). That is, I I\n> think it could take space out of the list of metachars that force us to\n> invoke the shell, and do the word-splitting there. But not having\n> thought very hard about it, there are probably corner cases where that\n> optimization is detectable by the user (presumably unusual IFS, but\n> maybe more?).\n\nWell, I strongly object to an approach for us to \"parse\" anything.\nBut even then it would be sensible to formulate:\n\n\targv[0] = sh\n\targv[1] = -c\n\targv[2] = git-credential-cache --socket=/path/\t--timeout=123 \"$@\"\n\targv[3] = -\n\targv[4] = NULL\n\nand if there is an argument say \"get\", extend it to\n\n\targv[0] = sh\n\targv[1] = -c\n\targv[2] = git-credential-cache --socket=/path/\t--timeout=123 \"$@\"\n\targv[3] = -\n\targv[4] = get\n\targv[5] = NULL\n\nbefore passing the array to execv(), no?\n\nAnd with the metacharacter optimization to drop .use_shell we\nalready have, a single-token /bin/myhelper case would then become\n\n\targv[0] = /bin/myhelper\n\targv[1] = get\n\targv[2] = NULL\n\nnaturally.\n"},{"id":"498806","messageId":"87jzhlf2i4.fsf@igel.home","threadId":"61783","inReplyTo":"20240716192307.GA12536@coredump.intra.peff.net","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2024-07-16T21:30:59Z","receivedAt":"2024-07-16T21:31:09Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jul 16 2024, Jeff King wrote:\n\n> Again, it's possible that we could detect that no shell metacharacters\n> are in play and do the word-splitting ourselves. But at that point I\n> think it should go into run-command's prepare_shell_cmd().\n\nThis is what GNU make does (see construct_command_argv_internal), for\nperformance reason. But run_command is probably not performance\ncritical.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"498807","messageId":"d98f82b8b6434c47fc2d9a4ecc870fef336a9e5a.camel@gnu.org","threadId":"61783","inReplyTo":"87jzhlf2i4.fsf@igel.home","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Paul Smith","fromEmail":"psmith@gnu.org","sentAt":"2024-07-16T21:40:33Z","receivedAt":"2024-07-16T21:40:37Z","isPatch":false,"sender":{"key":"psmith@gnu.org","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Tue, 2024-07-16 at 23:30 +0200, Andreas Schwab wrote:\n> On Jul 16 2024, Jeff King wrote:\n> \n> > Again, it's possible that we could detect that no shell\n> > metacharacters are in play and do the word-splitting ourselves. But\n> > at that point I think it should go into run-command's\n> > prepare_shell_cmd().\n> \n> This is what GNU make does (see construct_command_argv_internal), for\n> performance reason.  But run_command is probably not performance\n> critical.\n\nAlso I would definitely not recommend anyone look at this part of the\nGNU Make code for inspiration. It's an unholy mess.\n\nThe concept is very good though.\n"},{"id":"498812","messageId":"20240717055203.GB547635@coredump.intra.peff.net","threadId":"61783","inReplyTo":"xmqqo76x6r69.fsf@gitster.g","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-17T05:52:03Z","receivedAt":"2024-07-17T05:52:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2024 at 01:02:54PM -0700, Junio C Hamano wrote:\n\n> > Again, it's possible that we could detect that no shell metacharacters\n> > are in play and do the word-splitting ourselves. But at that point I\n> > think it should go into run-command's prepare_shell_cmd(). That is, I I\n> > think it could take space out of the list of metachars that force us to\n> > invoke the shell, and do the word-splitting there. But not having\n> > thought very hard about it, there are probably corner cases where that\n> > optimization is detectable by the user (presumably unusual IFS, but\n> > maybe more?).\n> \n> Well, I strongly object to an approach for us to \"parse\" anything.\n\nOK, it makes me nervous, too, so I am happy to leave it be (though it is\ninteresting to hear that \"make\" does a similar optimization).\n\n> But even then it would be sensible to formulate:\n> \n> \targv[0] = sh\n> \targv[1] = -c\n> \targv[2] = git-credential-cache --socket=/path/\t--timeout=123 \"$@\"\n> \targv[3] = -\n> \targv[4] = NULL\n> \n> and if there is an argument say \"get\", extend it to\n> \n> \targv[0] = sh\n> \targv[1] = -c\n> \targv[2] = git-credential-cache --socket=/path/\t--timeout=123 \"$@\"\n> \targv[3] = -\n> \targv[4] = get\n> \targv[5] = NULL\n> \n> before passing the array to execv(), no?\n\nHmm. I think that is OK. It is a little funny to have some arguments\npasted in and some via \"$@\", but I think in the end it should be\nindistinguishable to the user.\n\nIn your example it is \"git-credential-cache\", whereas we do \"git\ncredential-cache\" now. I _think_ the two should be equivalent at this\npoint (since the parent process running this code would already have set\nup GIT_EXEC_PATH to find dashed forms), but it does give me pause.\n\nWe could keep saying \"git credential-cache\", but then in most cases we\nwould never trigger the metacharacter optimization, because of the\nspace.\n\nAnd of course:\n\nSplitting it out like this:\n\n argv[0] = sh\n argv[1] = -c\n argv[2] = git \"$@\"\n argv[3] = -\n argv[4] = credential-cache --socket=/path/ --timeout=123\n argv[5] = get\n argv[6] = NULL\n\nis wrong, because argv[4] is really a shell snippet, not a single\nargument (and we cannot split it ourselves without doing shell-like\nparsing).\n\n> And with the metacharacter optimization to drop .use_shell we\n> already have, a single-token /bin/myhelper case would then become\n> \n> \targv[0] = /bin/myhelper\n> \targv[1] = get\n> \targv[2] = NULL\n> \n> naturally.\n\nYes, I think that is the one case that would benefit. I do wonder how\noften people point to an absolute path, though, rather than\ngit-credential-* or a more complex shell invocation.\n\n-Peff\n"},{"id":"498813","messageId":"20240717055335.GC547635@coredump.intra.peff.net","threadId":"61783","inReplyTo":"87jzhlf2i4.fsf@igel.home","subject":"Re: Can dependency on /bin/sh be removed?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-17T05:53:35Z","receivedAt":"2024-07-17T05:53:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2024 at 11:30:59PM +0200, Andreas Schwab wrote:\n\n> On Jul 16 2024, Jeff King wrote:\n> \n> > Again, it's possible that we could detect that no shell metacharacters\n> > are in play and do the word-splitting ourselves. But at that point I\n> > think it should go into run-command's prepare_shell_cmd().\n> \n> This is what GNU make does (see construct_command_argv_internal), for\n> performance reason. But run_command is probably not performance\n> critical.\n\nThanks, that's interesting to hear. I agree that run_command is not\nusually performance critical. Generally if we find ourselves spawning a\nlot of processes, the right solution is to accomplish the same thing\nwith fewer processes (or even in-process), not micro-optimize out the\nintermediate shell.\n\n-Peff\n"}]}