Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

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

 



Excerpts from Jeff King's message of Mon Mar 26 23:29:17 -0400 2012:

Hi Jeff,

> > +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
> > +
> 
> This should be $(SHELL_PATH_SQ), no?

Yes, you're right, it should be.

> > +#ifndef SHELL_PATH
> > +# define SHELL_PATH "sh"
> > +#endif
> 
> Does this default ever kick in? The Makefile defaults SHELL_PATH to
> /bin/sh, so we will always end up with at least that.

Not when using the build system, but as Hannes mentioned, there is
potential for this to be used outside of the default build system, so
I think having the fallback is a good defensive option.  Should it
maybe be set to /bin/sh though to be more consistent with system()?

> The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people
> on less-abled systems, and I do not mind bending the usual
> conventions to make things more convenient on those systems (e.g.,
> by not doing the PATH lookup of the shell name). But it would be
> nice if that bending did not affect people on more mainstream
> systems.

Given the rest of the discussion that happened, I think I understand
that my patch is actually ok with the following caveats:

1. My commit message still needs work.
2. Possibly change the default setting of the SHELL_PATH macro from
   "sh" to "/bin/sh"
3. Use the _SQ variant of SHELL_PATH.

(The tracking of changes for SHELL_PATH is considered too heavy for
now when the other _PATH items aren't tracked the same way.  This
might make a nice separate patch series though, using the idea from
the kernel where individual commands are tracked.)

Did I miss anything else?

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

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