From: Kyle J. McKay Date: Mon, 09 Mar 2015 06:32:22 GMT Subject: Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh" Message-ID: In-Reply-To: On Mar 7, 2015, at 23:52, Junio C Hamano wrote: > "Kyle J. McKay" writes: > >> If the user has set SHELL_PATH in the Makefile then we >> should respect that value and use it. >> >> Signed-off-by: Kyle J. McKay >> --- >> builtin/help.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/builtin/help.c b/builtin/help.c >> index 6133fe49..2ae8a1e9 100644 >> --- a/builtin/help.c >> +++ b/builtin/help.c >> @@ -171,7 +171,7 @@ static void exec_man_cmd(const char *cmd, const >> char *page) >> { >> struct strbuf shell_cmd = STRBUF_INIT; >> strbuf_addf(&shell_cmd, "%s %s", cmd, page); >> - execl("/bin/sh", "sh", "-c", shell_cmd.buf, (char *)NULL); >> + execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, (char *)NULL); > > It is a common convention to make the first argument the command > name without its path, and this change breaks that convention. Hmpf. I present these for your consideration: $ sh -c 'echo $0' sh $ /bin/sh -c 'echo $0' /bin/sh $ cd /etc $ ../bin/sh -c 'echo $0' ../bin/sh I always thought it was the actual argument used to invoke the item. If the item is in the PATH and was invoked with a bare word then arg0 would be just the bare word or possibly the actual full pathname as found in PATH. Whereas if it's invoked with a path (relative or absolute) that would passed instead. > Does it matter, or would it break something? I recall that some > implementations of shell (e.g. "bash") change their behaviour > depending on how they are invoked (e.g. "ln -s bash /bin/sh" makes > it run in posix mode) but I do not know if they do so by paying > attention to their argv[0]. Several shells are sensitive to argv[0] in that if it starts with a '-' then they become a login shell. Setting SHELL_PATH to anything that is not an absolute path is likely to break things in other ways though so that doesn't seem like a possibility here. > There might be other fallouts I do not > think of offhand here. > > I do not have an objection to what these patches want to do, though. I also have no objection to changing it to: > - execl("/bin/sh", "sh", "-c", shell_cmd.buf, (char *)NULL); > + execl(SHELL_PATH, basename(SHELL_PATH), "-c", shell_cmd.buf, (char > *)NULL); just to maintain the current behavior. Would you be able to squash that change in or shall I re-roll? -Kyle