patch, 2 partsgit-compat-util.h: move SHELL_PATH default into header

If SHELL_PATH is not defined we use "/bin/sh". However, run-command.c is not the only file that needs to use the default value so move it into a common header.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 git-compat-util.h | 4 ++++
 run-command.c     | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

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(-)

"Kyle J. McKay" <mackyle@gmail.com> writes:

It is a common convention to make the first argument the command name without its path, and this change breaks that convention.

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

Thanks.

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

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.

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.

I also have no objection to changing it to:

just to maintain the current behavior.

Would you be able to squash that change in or shall I re-roll?

-Kyle

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

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

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

Jeff King <peff@peff.net> writes:

OK, then the original under discussion is fine as-is.

Thanks for sanity checking.