{"thread":{"id":"38749","subject":"[PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header","startedAt":"2015-03-08T05:07:59Z","lastAt":"2015-03-10T02:21:45Z","messageCount":6,"participants":["Kyle J. McKay","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"257262","messageId":"38be9195b966a027cb050e5a1b47526@74d39fa044aa309eaea14b9f57fe79c","threadId":"38749","inReplyTo":null,"subject":"[PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-08T05:07:59Z","receivedAt":"2015-03-08T05:07:59Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"If SHELL_PATH is not defined we use \"/bin/sh\". However,\nrun-command.c is not the only file that needs to use\nthe default value so move it into a common header.\n\nSigned-off-by: Kyle J. McKay \n---\n git-compat-util.h | 4 ++++\n run-command.c | 4 ----\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a3095be9..fbfd10da 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -876,4 +876,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define USE_PARENS_AROUND_GETTEXT_N 1\n #endif\n \n+#ifndef SHELL_PATH\n+# define SHELL_PATH \"/bin/sh\"\n+#endif\n+\n #endif\ndiff --git a/run-command.c b/run-command.c\nindex 0b432cc9..3afb124c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -4,10 +4,6 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n \n-#ifndef SHELL_PATH\n-# define SHELL_PATH \"/bin/sh\"\n-#endif\n-\n void child_process_init(struct child_process *child)\n {\n \tmemset(child, 0, sizeof(*child));\n---\n"},{"id":"257263","messageId":"0ebc0373b21c75fa88adb5aefd098e9@74d39fa044aa309eaea14b9f57fe79c","threadId":"38749","inReplyTo":"38be9195b966a027cb050e5a1b47526@74d39fa044aa309eaea14b9f57fe79c","subject":"[PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded \"/bin/sh\"","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-08T05:08:00Z","receivedAt":"2015-03-08T05:08:00Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"If the user has set SHELL_PATH in the Makefile then we\nshould respect that value and use it.\n\nSigned-off-by: Kyle J. McKay \n---\n builtin/help.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 6133fe49..2ae8a1e9 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const char *page)\n {\n \tstruct strbuf shell_cmd = STRBUF_INIT;\n \tstrbuf_addf(&shell_cmd, \"%s %s\", cmd, page);\n-\texecl(\"/bin/sh\", \"sh\", \"-c\", shell_cmd.buf, (char *)NULL);\n+\texecl(SHELL_PATH, SHELL_PATH, \"-c\", shell_cmd.buf, (char *)NULL);\n \twarning(_(\"failed to exec '%s': %s\"), cmd, strerror(errno));\n }\n \n---\n"},{"id":"257271","messageId":"xmqq61acsz7k.fsf@gitster.dls.corp.google.com","threadId":"38749","inReplyTo":"0ebc0373b21c75fa88adb5aefd098e9@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded \"/bin/sh\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T07:52:31Z","receivedAt":"2015-03-08T07:52:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> If the user has set SHELL_PATH in the Makefile then we\n> should respect that value and use it.\n>\n> Signed-off-by: Kyle J. McKay \n> ---\n> builtin/help.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/help.c b/builtin/help.c\n> index 6133fe49..2ae8a1e9 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const char *page)\n> {\n> \tstruct strbuf shell_cmd = STRBUF_INIT;\n> \tstrbuf_addf(&shell_cmd, \"%s %s\", cmd, page);\n> -\texecl(\"/bin/sh\", \"sh\", \"-c\", shell_cmd.buf, (char *)NULL);\n> +\texecl(SHELL_PATH, SHELL_PATH, \"-c\", shell_cmd.buf, (char *)NULL);\n\nIt is a common convention to make the first argument the command\nname without its path, and this change breaks that convention.\n\nDoes it matter, or would it break something? I recall that some\nimplementations of shell (e.g. \"bash\") change their behaviour\ndepending on how they are invoked (e.g. \"ln -s bash /bin/sh\" makes\nit run in posix mode) but I do not know if they do so by paying\nattention to their argv[0]. There might be other fallouts I do not\nthink of offhand here.\n\nI do not have an objection to what these patches want to do, though.\n\nThanks.\n\n> \twarning(_(\"failed to exec '%s': %s\"), cmd, strerror(errno));\n> }\n> \n> ---\n"},{"id":"257363","messageId":"C611A125-D641-46E6-A5AD-1010D70582F0@gmail.com","threadId":"38749","inReplyTo":"xmqq61acsz7k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded \"/bin/sh\"","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-09T06:32:22Z","receivedAt":"2015-03-09T06:32:22Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 7, 2015, at 23:52, Junio C Hamano wrote:\n> \"Kyle J. McKay\" writes:\n>\n>> If the user has set SHELL_PATH in the Makefile then we\n>> should respect that value and use it.\n>>\n>> Signed-off-by: Kyle J. McKay \n>> ---\n>> builtin/help.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/help.c b/builtin/help.c\n>> index 6133fe49..2ae8a1e9 100644\n>> --- a/builtin/help.c\n>> +++ b/builtin/help.c\n>> @@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const \n>> char *page)\n>> {\n>> \tstruct strbuf shell_cmd = STRBUF_INIT;\n>> \tstrbuf_addf(&shell_cmd, \"%s %s\", cmd, page);\n>> -\texecl(\"/bin/sh\", \"sh\", \"-c\", shell_cmd.buf, (char *)NULL);\n>> +\texecl(SHELL_PATH, SHELL_PATH, \"-c\", shell_cmd.buf, (char *)NULL);\n>\n> It is a common convention to make the first argument the command\n> name without its path, and this change breaks that convention.\n\nHmpf. I present these for your consideration:\n\n$ sh -c 'echo $0'\nsh\n$ /bin/sh -c 'echo $0'\n/bin/sh\n$ cd /etc\n$ ../bin/sh -c 'echo $0'\n../bin/sh\n\nI always thought it was the actual argument used to invoke the item. \nIf the item is in the PATH and was invoked with a bare word then arg0 \nwould be just the bare word or possibly the actual full pathname as \nfound in PATH. Whereas if it's invoked with a path (relative or \nabsolute) that would passed instead.\n\n> Does it matter, or would it break something? I recall that some\n> implementations of shell (e.g. \"bash\") change their behaviour\n> depending on how they are invoked (e.g. \"ln -s bash /bin/sh\" makes\n> it run in posix mode) but I do not know if they do so by paying\n> attention to their argv[0].\n\nSeveral shells are sensitive to argv[0] in that if it starts with a \n'-' then they become a login shell. Setting SHELL_PATH to anything \nthat is not an absolute path is likely to break things in other ways \nthough so that doesn't seem like a possibility here.\n\n> There might be other fallouts I do not\n> think of offhand here.\n>\n> I do not have an objection to what these patches want to do, though.\n\nI also have no objection to changing it to:\n\n> -\texecl(\"/bin/sh\", \"sh\", \"-c\", shell_cmd.buf, (char *)NULL);\n> +\texecl(SHELL_PATH, basename(SHELL_PATH), \"-c\", shell_cmd.buf, (char \n> *)NULL);\n\njust to maintain the current behavior.\n\nWould you be able to squash that change in or shall I re-roll?\n\n-Kyle\n"},{"id":"257366","messageId":"20150309072040.GA28148@peff.net","threadId":"38749","inReplyTo":"C611A125-D641-46E6-A5AD-1010D70582F0@gmail.com","subject":"Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded \"/bin/sh\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-09T07:20:40Z","receivedAt":"2015-03-09T07:20:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2015 at 11:32:22PM -0700, Kyle J. McKay wrote:\n\n> >It is a common convention to make the first argument the command\n> >name without its path, and this change breaks that convention.\n> \n> Hmpf. I present these for your consideration:\n> \n> $ sh -c 'echo $0'\n> sh\n> $ /bin/sh -c 'echo $0'\n> /bin/sh\n> $ cd /etc\n> $ ../bin/sh -c 'echo $0'\n> ../bin/sh\n> \n> I always thought it was the actual argument used to invoke the item. If the\n> item is in the PATH and was invoked with a bare word then arg0 would be just\n> the bare word or possibly the actual full pathname as found in PATH.\n> Whereas if it's invoked with a path (relative or absolute) that would passed\n> instead.\n\nYes, you are correct. When there is a full path, that typically gets\npassed instead (unless you are trying to convey something specific to\nthe program, like telling bash \"pretend to be POSIX sh\"; that's usually\ndone with a symlink, but the caller might want to override it).\n\nIf we were starting from scratch, I would say that SHELL_PATH is\nsupposed to be a replacement POSIX shell, and so we should call:\n\n execl(SHELL_PATH, \"sh\", \"-c\", ...);\n\nto tell shells like bash to operate in POSIX mode.\n\nHowever, that is _not_ what we currently do with run-command's\nuse_shell directive. There we put SHELL_PATH as argv[0], and run:\n\n execv(argv[0], argv);\n\nI doubt it matters much in practice (after all, these are just \"-c\"\nsnippets, not whole scripts). But it's possible that by passing \"-c\" we\nwould introduce bugs (e.g., if somebody has a really complicated inline\nalias, and sets SHELL_PATH to /path/to/bash, they'll get full-on bash\nwith the current code).\n\n> I also have no objection to changing it to:\n> \n> >-\texecl(\"/bin/sh\", \"sh\", \"-c\", shell_cmd.buf, (char *)NULL);\n> >+\texecl(SHELL_PATH, basename(SHELL_PATH), \"-c\", shell_cmd.buf, (char\n> >*)NULL);\n> \n> just to maintain the current behavior.\n\nIf we want to maintain consistency with the rest of our uses of\nrun-command, it would be just your original:\n\n execl(SHELL_PATH, SHELL_PATH, \"-c\", shell_cmd.buf, NULL);\n\nThat makes the most sense to me, unless we are changing run-command's\nbehavior, too. \n\nThere's no point in calling basename(). Shells like bash which\nbehave differently when called as \"sh\" are smart enough to check the\nbasename themselves (this would matter, e.g., if you set SHELL_PATH to\n\"/path/to/my/sh\" and that was actually a symlink to bash).\n\n-Peff\n"},{"id":"257440","messageId":"xmqq7fup618m.fsf@gitster.dls.corp.google.com","threadId":"38749","inReplyTo":"20150309072040.GA28148@peff.net","subject":"Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded \"/bin/sh\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T02:21:45Z","receivedAt":"2015-03-10T02:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> However, that is _not_ what we currently do with run-command's\n> use_shell directive. There we put SHELL_PATH as argv[0], and run:\n>\n> execv(argv[0], argv);\n> ...\n> If we want to maintain consistency with the rest of our uses of\n> run-command, it would be just your original:\n>\n> execl(SHELL_PATH, SHELL_PATH, \"-c\", shell_cmd.buf, NULL);\n>\n> That makes the most sense to me, unless we are changing run-command's\n> behavior, too. \n\nOK, then the original under discussion is fine as-is.\n\nThanks for sanity checking.\n"}]}