[PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header

[PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header

[Kyle J. McKay]

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

diff --git a/git-compat-util.h b/git-compat-util.h
index a3095be9..fbfd10da 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -876,4 +876,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define USE_PARENS_AROUND_GETTEXT_N 1
 #endif
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "/bin/sh"
+#endif
+
 #endif
diff --git a/run-command.c b/run-command.c
index 0b432cc9..3afb124c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,10 +4,6 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
-#ifndef SHELL_PATH
-# define SHELL_PATH "/bin/sh"
-#endif
-
 void child_process_init(struct child_process *child)
 {
 	memset(child, 0, sizeof(*child));
---

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

[Kyle J. McKay]

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);
 	warning(_("failed to exec '%s': %s"), cmd, strerror(errno));
 }
 
---

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

[Junio C Hamano]

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

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.

>  	warning(_("failed to exec '%s': %s"), cmd, strerror(errno));
>  }
>  
> ---

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

[Kyle J. McKay]

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

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

[Jeff King]

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

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Thanks for sanity checking.