* [PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header
@ 2015-03-08 5:07 Kyle J. McKay
2015-03-08 5:08 ` [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh" Kyle J. McKay
0 siblings, 1 reply; 6+ messages in thread
From: Kyle J. McKay @ 2015-03-08 5:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
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));
---
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"
2015-03-08 5:07 [PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header Kyle J. McKay
@ 2015-03-08 5:08 ` Kyle J. McKay
2015-03-08 7:52 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Kyle J. McKay @ 2015-03-08 5:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
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));
}
---
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"
2015-03-08 5:08 ` [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh" Kyle J. McKay
@ 2015-03-08 7:52 ` Junio C Hamano
2015-03-09 6:32 ` Kyle J. McKay
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2015-03-08 7:52 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Git mailing list
"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));
> }
>
> ---
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"
2015-03-08 7:52 ` Junio C Hamano
@ 2015-03-09 6:32 ` Kyle J. McKay
2015-03-09 7:20 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Kyle J. McKay @ 2015-03-09 6:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"
2015-03-09 6:32 ` Kyle J. McKay
@ 2015-03-09 7:20 ` Jeff King
2015-03-10 2:21 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2015-03-09 7:20 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Junio C Hamano, Git mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh"
2015-03-09 7:20 ` Jeff King
@ 2015-03-10 2:21 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2015-03-10 2:21 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle J. McKay, Git mailing list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-10 2:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-08 5:07 [PATCH 1/2] git-compat-util.h: move SHELL_PATH default into header Kyle J. McKay
2015-03-08 5:08 ` [PATCH 2/2] help.c: use SHELL_PATH instead of hard-coded "/bin/sh" Kyle J. McKay
2015-03-08 7:52 ` Junio C Hamano
2015-03-09 6:32 ` Kyle J. McKay
2015-03-09 7:20 ` Jeff King
2015-03-10 2:21 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.