Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"

From: Kyle J. McKay <mackyle@gmail.com>
Date: Mar 9, 2015, 06:32 UTC
Message-ID: <C611A125-D641-46E6-A5AD-1010D70582F0@gmail.com>
In-Reply-To: <xmqq61acsz7k.fsf@gitster.dls.corp.google.com>

On Mar 7, 2015, at 23:52, Junio C Hamano wrote:

Show 24 quoted lines

> "Kyle J. McKay" <mackyle@gmail.com> 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 <mackyle@gmail.com>
>> ---
>> 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.

Show 5 quoted lines

> 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