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

From: Jeff King <peff@peff.net>
Date: Mar 9, 2015, 07:20 UTC
Message-ID: <20150309072040.GA28148@peff.net>
In-Reply-To: <C611A125-D641-46E6-A5AD-1010D70582F0@gmail.com>

On Sun, Mar 08, 2015 at 11:32:22PM -0700, Kyle J. McKay wrote:

Show 18 quoted lines

> >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.

Yes, you are correct. When there is a full path, that typically gets passed instead (unless you are trying to convey something specific to the program, like telling bash "pretend to be POSIX sh"; that's usually done with a symlink, but the caller might want to override it).

If we were starting from scratch, I would say that SHELL_PATH is supposed to be a replacement POSIX shell, and so we should call:

  execl(SHELL_PATH, "sh", "-c", ...);

to tell shells like bash to operate in POSIX mode.

However, that is _not_ what we currently do with run-command's use_shell directive. There we put SHELL_PATH as argv[0], and run:

  execv(argv[0], argv);

I doubt it matters much in practice (after all, these are just "-c" snippets, not whole scripts). But it's possible that by passing "-c" we would introduce bugs (e.g., if somebody has a really complicated inline alias, and sets SHELL_PATH to /path/to/bash, they'll get full-on bash with the current code).

Show 7 quoted lines

> 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.

If we want to maintain consistency with the rest of our uses of run-command, it would be just your original:

  execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, NULL);

That makes the most sense to me, unless we are changing run-command's behavior, too.

There's no point in calling basename(). Shells like bash which behave differently when called as "sh" are smart enough to check the basename themselves (this would matter, e.g., if you set SHELL_PATH to "/path/to/my/sh" and that was actually a symlink to bash).

-Peff