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

["Kyle J. McKay" <mackyle@gmail.com>]

On Mar 7, 2015, at 23:52, Junio C Hamano wrote:
> "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.

> 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