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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mar 7, 2015, at 23:52, Junio C Hamano wrote:
"Kyle J. McKay" <mackyle@xxxxxxxxx> 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@xxxxxxxxx>
---
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]