[PATCH] Replace hard-coded path with one configurable at make time

[PATCH] Replace hard-coded path with one configurable at make time

[Chris Webb]

In exec_cmd.c and git-instaweb.sh, git hard-codes a default path of
/usr/local/bin:/usr/bin:/bin. Introduce a make variable allowing this to be
overridden by passing defpath to make.
---
I haven't worked out the capitalisation convention in the makefile, so I'm
not sure if this should be DEFPATH or defpath. (For example, prefix is
lower-case but DESTDIR is all-caps.)

 Makefile        |    6 +++++-
 exec_cmd.c      |    2 +-
 git-instaweb.sh |    4 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 11ec3e2..2f4b39e 100644
--- a/Makefile
+++ b/Makefile
@@ -278,6 +278,7 @@ endif
 lib = lib
 # DESTDIR=
 pathsep = :
+defpath = /usr/local/bin:/usr/bin:/bin
 
 # JavaScript minifier invocation that can function as filter
 JSMIN =
@@ -1426,6 +1427,7 @@ gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_SQ = $(subst ','\'',$(htmldir))
 prefix_SQ = $(subst ','\'',$(prefix))
+defpath_SQ = $(subst ','\'',$(defpath))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
@@ -1588,6 +1590,7 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/gitweb.css gitweb/gitweb.
 	    -e '/@@GITWEB_JS@@/r gitweb/gitweb.js' \
 	    -e '/@@GITWEB_JS@@/d' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
+	    -e 's|@@DEFPATH@@|$(defpath_SQ)|g' \
 	    $@.sh > $@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
@@ -1774,7 +1777,8 @@ endif
 exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
-	'-DPREFIX="$(prefix_SQ)"'
+	'-DPREFIX="$(prefix_SQ)"' \
+	'-DDEFPATH="$(defpath_SQ)"'
 
 builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
diff --git a/exec_cmd.c b/exec_cmd.c
index b2c07c7..39c6a59 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -107,7 +107,7 @@ void setup_path(void)
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		strbuf_addstr(&new_path, DEFPATH);
 
 	setenv("PATH", new_path.buf, 1);
 
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 6a65f25..72d8b12 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -212,7 +212,7 @@ server.errorlog = "$fqgitdir/gitweb/error.log"
 # variable above and uncomment this
 #accesslog.filename = "$fqgitdir/gitweb/access.log"
 
-setenv.add-environment = ( "PATH" => "/usr/local/bin:/usr/bin:/bin" )
+setenv.add-environment = ( "PATH" => "@@DEFPATH@@" )
 
 cgi.assign = ( ".cgi" => "" )
 
@@ -361,7 +361,7 @@ error_log	$fqgitdir/gitweb/error.log
 access_log	$fqgitdir/gitweb/access.log
 
 #cgi setup
-cgi_env		PATH=/usr/local/bin:/usr/bin:/bin,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
+cgi_env		PATH=@@DEFPATH@@,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
 cgi_interp	$PERL
 cgi_ext		cgi,pl
 
-- 
1.7.0.3

Re: [PATCH] Replace hard-coded path with one configurable at make time

[Junio C Hamano]

Chris Webb <chris@arachsys.com> writes:

> In exec_cmd.c and git-instaweb.sh, git hard-codes a default path of
> /usr/local/bin:/usr/bin:/bin. Introduce a make variable allowing this to be
> overridden by passing defpath to make.

A question and an issue.

 * What's the point of making this configurable, other than "because we
   can"?

 * Use of "$(x_SQ)" is about protecting whitespaces and single quotes in
   the literal from make and shell, but does not have anything to do with
   protecting things like $foo in that literal from the location $x is
   eventually embedded in.  As long as paths on DEFPATH do not have double
   quote in it (which would be a sane assumption), the patch to exec_cmd.c
   would work fine, but I don't know if you need an extra quoting when
   DEFPATH is used in shell scripts.  E.g. DEFPATH=$GIT_EXEC_PATH:/usr/bin
   would have GIT_EXEC_PATH expanded in mongoose configuration file, but
   will not be expanded in exec_cmd.c, leading to an inconsistent
   behaviour.

   Does this matter?
  

Re: [PATCH] Replace hard-coded path with one configurable at make time

[Chris Webb]

Junio C Hamano <gitster@pobox.com> writes:

>  * What's the point of making this configurable, other than "because we
>    can"?

I have a local patch against git to fix these paths, as I run it on slightly
unusual systems with a non-standard directory layout (no /usr, but
/local/bin in some cases) and I don't like to see incorrect paths compiled
into my binaries. It occurred to me that if I want to fix it one way
locally, others may well want to vary it too for different reasons, e.g. to
add /opt/bin or /usr/gnu to the default path.

Ultimately, I guess it feels like it should be configurable rather than
needing to be patched in the source for the same reason prefix or gitexecdir
is, but this is definitely for a minority audience!

Were it just exec_cmd.c, I would just have changed it to use _PATH_DEFPATH
from <paths.h> in preference to a make variable, as that should always give
an appropriate value for a correctly put-together system and is a sensible
place to treat as the central definition of 'default path'. However, in this
case it's needed in the shell script too and I don't think I can easily get
at _PATH_DEFPATH from there.

>  * Use of "$(x_SQ)" is about protecting whitespaces and single quotes in
>    the literal from make and shell, but does not have anything to do with
>    protecting things like $foo in that literal from the location $x is
>    eventually embedded in.  As long as paths on DEFPATH do not have double
>    quote in it (which would be a sane assumption), the patch to exec_cmd.c
>    would work fine, but I don't know if you need an extra quoting when
>    DEFPATH is used in shell scripts.  E.g. DEFPATH=$GIT_EXEC_PATH:/usr/bin
>    would have GIT_EXEC_PATH expanded in mongoose configuration file, but
>    will not be expanded in exec_cmd.c, leading to an inconsistent
>    behaviour.

Oh I see, yes; I didn't worry about quoting it correctly in the generated
shell script, assuming it would be reasonable... but if I'm assuming it's
reasonable there's no point in the _SQ to protect the shell invoking sed in
the first place.

I also notice that the makefile makes the assumption that ' might occur in
pathological paths and so needs quoting, but then uses sed 's|x|y|g' for
(say) @@PERL@@ which will break for other pathological paths containing | or
\1 and so on. Tidying that up fully might be entertaining!

Cheers,

Chris.

Re: [PATCH] Replace hard-coded path with one configurable at make time

[Chris Webb]

Chris Webb <chris@arachsys.com> writes:

> Were it just exec_cmd.c, I would just have changed it to use _PATH_DEFPATH
> from <paths.h> in preference to a make variable, as that should always give
> an appropriate value for a correctly put-together system and is a sensible
> place to treat as the central definition of 'default path'. However, in this
> case it's needed in the shell script too and I don't think I can easily get
> at _PATH_DEFPATH from there.

Having looked at this again, I think it's probably better to tackle the two
pieces separately. It would be cleaner to fix exec_cmd.c to use the correct
system-wide _PATH_DEFPATH from <paths.h> if possible, as in the following
patch, rather than introduce yet another make variable.

Similarly, looking more closely at what the path gets used for, I think
git-instaweb.sh is wrong to hard-code a default path anyway: it should
surely pass through the path inherited from the invoking user rather than
silently overriding it. I'll do a separate patch for that.

Cheers,

Chris.

[PATCH] Replace hard-coded path with one from <paths.h>

[Chris Webb]

In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
Get an appropriate value for the system from <paths.h> if possible instead.

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 exec_cmd.c        |    2 +-
 git-compat-util.h |    5 +++++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index b2c07c7..bf22570 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -107,7 +107,7 @@ void setup_path(void)
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		strbuf_addstr(&new_path, _PATH_DEFPATH);
 
 	setenv("PATH", new_path.buf, 1);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 7e62b55..7592be7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -92,6 +92,7 @@
 #include <assert.h>
 #include <regex.h>
 #include <utime.h>
+#include <paths.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
 #include <sys/poll.h>
@@ -164,6 +165,10 @@ extern char *gitbasename(char *);
 #define PATH_SEP ':'
 #endif
 
+#ifndef _PATH_DEFPATH
+#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
+#endif
+
 #ifndef STRIP_EXTENSION
 #define STRIP_EXTENSION ""
 #endif
-- 
1.7.0.3

[PATCH] git-instaweb: pass through invoking user's path to gitweb CGI script

[Chris Webb]

When used with lighttpd or mongoose, git-instaweb previously passed a
hard-coded, default value of PATH to the gitweb CGI script. Use the invoking
user's value for PATH for this instead. (This is already the behaviour for
other web servers supported by git-instaweb.)

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 git-instaweb.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 6a65f25..0e1cb07 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -212,7 +212,7 @@ server.errorlog = "$fqgitdir/gitweb/error.log"
 # variable above and uncomment this
 #accesslog.filename = "$fqgitdir/gitweb/access.log"
 
-setenv.add-environment = ( "PATH" => "/usr/local/bin:/usr/bin:/bin" )
+setenv.add-environment = ( "PATH" => env.PATH )
 
 cgi.assign = ( ".cgi" => "" )
 
@@ -361,7 +361,7 @@ error_log	$fqgitdir/gitweb/error.log
 access_log	$fqgitdir/gitweb/access.log
 
 #cgi setup
-cgi_env		PATH=/usr/local/bin:/usr/bin:/bin,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
+cgi_env		PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
 cgi_interp	$PERL
 cgi_ext		cgi,pl
 
-- 
1.7.0.3

Re: [PATCH] Replace hard-coded path with one from <paths.h>

[Erik Faye-Lund]

On Tue, Apr 6, 2010 at 6:36 PM, Chris Webb <chris@arachsys.com> wrote:
> In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
> Get an appropriate value for the system from <paths.h> if possible instead.
>
> Signed-off-by: Chris Webb <chris@arachsys.com>
> ---
>  exec_cmd.c        |    2 +-
>  git-compat-util.h |    5 +++++
>  2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index b2c07c7..bf22570 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -107,7 +107,7 @@ void setup_path(void)
>        if (old_path)
>                strbuf_addstr(&new_path, old_path);
>        else
> -               strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
> +               strbuf_addstr(&new_path, _PATH_DEFPATH);
>
>        setenv("PATH", new_path.buf, 1);
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 7e62b55..7592be7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -92,6 +92,7 @@
>  #include <assert.h>
>  #include <regex.h>
>  #include <utime.h>
> +#include <paths.h>

This breaks on Windows due to missing paths.h. I guess you need some
guard to detect if the header is present or not.

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Erik Faye-Lund <kusmabite@googlemail.com> writes:

> > +#include <paths.h>
> 
> This breaks on Windows due to missing paths.h. I guess you need some
> guard to detect if the header is present or not.

Is this true of all WIN32, or just __MINGW32__ / __CYGWIN__? Presumably
/usr/local/bin:/usr/bin:/bin is the wrong default PATH on windows too, so
perhaps I should sort that at the same point---what would a canonical
default PATH be for Windows?

Cheers,

Chris.

Re: [PATCH] Replace hard-coded path with one from <paths.h>

[Erik Faye-Lund]

On Thu, Apr 8, 2010 at 12:58 PM, Chris Webb <chris@arachsys.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> > +#include <paths.h>
>>
>> This breaks on Windows due to missing paths.h. I guess you need some
>> guard to detect if the header is present or not.
>
> Is this true of all WIN32, or just __MINGW32__ / __CYGWIN__?

I've only tested it on mingw (msysGit). I don't know about cygwin and msvc.

-- 
Erik "kusma" Faye-Lund

[PATCH v2] Replace hard-coded path with one from <paths.h>

[Chris Webb]

In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
Get an appropriate value for the system from <paths.h> if possible instead.
Do not attempt to #include <paths.h> under Windows.

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 exec_cmd.c        |    2 +-
 git-compat-util.h |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index b2c07c7..bf22570 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -107,7 +107,7 @@ void setup_path(void)
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		strbuf_addstr(&new_path, _PATH_DEFPATH);
 
 	setenv("PATH", new_path.buf, 1);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 7e62b55..9443b9b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -70,6 +70,8 @@
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
 #include <windows.h>
+#else
+#include <paths.h>
 #endif
 
 #include <unistd.h>
@@ -164,6 +166,10 @@ extern char *gitbasename(char *);
 #define PATH_SEP ':'
 #endif
 
+#ifndef _PATH_DEFPATH
+#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
+#endif
+
 #ifndef STRIP_EXTENSION
 #define STRIP_EXTENSION ""
 #endif
-- 
1.7.0.3

Re: [PATCH v2] Replace hard-coded path with one from <paths.h>

[Erik Faye-Lund]

On Thu, Apr 8, 2010 at 1:57 PM, Chris Webb <chris@arachsys.com> wrote:
> In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
> Get an appropriate value for the system from <paths.h> if possible instead.
> Do not attempt to #include <paths.h> under Windows.
>
> Signed-off-by: Chris Webb <chris@arachsys.com>
> ---
>  exec_cmd.c        |    2 +-
>  git-compat-util.h |    6 ++++++
>  2 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index b2c07c7..bf22570 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -107,7 +107,7 @@ void setup_path(void)
>        if (old_path)
>                strbuf_addstr(&new_path, old_path);
>        else
> -               strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
> +               strbuf_addstr(&new_path, _PATH_DEFPATH);
>
>        setenv("PATH", new_path.buf, 1);
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 7e62b55..9443b9b 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -70,6 +70,8 @@
>  #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
>  #include <winsock2.h>
>  #include <windows.h>
> +#else
> +#include <paths.h>
>  #endif
>

Are you sure that all non-Windows platforms have paths.h? It seems
that at least some Open Solaris versions[1] are missing it as well.
Perhaps this should be guarded by a HAVE_PATHS_H define instead?

[1]: http://mail-index.netbsd.org/tech-pkg/2008/11/24/msg002103.html

-- 
Erik "kusma" Faye-Lund

Re: [PATCH v2] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Erik Faye-Lund <kusmabite@googlemail.com> writes:

> Are you sure that all non-Windows platforms have paths.h? It seems
> that at least some Open Solaris versions[1] are missing it as well.
> Perhaps this should be guarded by a HAVE_PATHS_H define instead?

Yes, you're probably right. I'll just set HAVE_PATHS_H for the platforms I'm
sure (or can check) have it for now.

Cheers,

Chris.

Re: [PATCH] Replace hard-coded path with one from <paths.h>

[Tait]

> > > +#include <paths.h>
> >
> > This breaks on Windows due to missing paths.h. I guess you need some
> > guard to detect if the header is present or not.
>
> Is this true of all WIN32, or just __MINGW32__ / __CYGWIN__? Presumably
> /usr/local/bin:/usr/bin:/bin is the wrong default PATH on windows too, so
> perhaps I should sort that at the same point---what would a canonical
> default PATH be for Windows?

Paths.h is not found on my version of mingw/msys.

The "canonical" Windows path is usually the system directory, and system32
and system32\Wbem under the system directory. The system directory could
be anywhere. C:\WINDOWS is common, but the WINDOWS (or even the C:) are
subject to change on any given installation. So for example on my computer,
	C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem

is the path before adding in PowerShell, Resource Kits, GTK, etc. Windows
also assumes '.' is part of your path, even though it's not explicitly
present in %PATH%.

My version of mingw seems to prepend to the above,
	.:/usr/local/bin:/mingw/bin:/bin

besides also using : instead of ; as a separator.

Re: [PATCH v2] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Chris Webb <chris@arachsys.com> writes:

> Erik Faye-Lund <kusmabite@googlemail.com> writes:
> 
> > Are you sure that all non-Windows platforms have paths.h? It seems
> > that at least some Open Solaris versions[1] are missing it as well.
> > Perhaps this should be guarded by a HAVE_PATHS_H define instead?
> 
> Yes, you're probably right. I'll just set HAVE_PATHS_H for the platforms I'm
> sure (or can check) have it for now.

Sorry for the slow follow up. Replacement patch in follow-up that tries this
only on Linux, *BSD and GNU where it's known to work. Should be completely
safe now!

Best wishes,

Chris.

[PATCH v3] Replace hard-coded path with one from <paths.h>

[Chris Webb]

In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
Get an appropriate value for the system from <paths.h> if possible instead. We
only try to include <paths.h> on Linux, FreeBSD, NetBSD, OpenBSD and GNU where
it is known to exist.

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 Makefile          |   10 ++++++++++
 exec_cmd.c        |    2 +-
 git-compat-util.h |    6 ++++++
 3 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 910f471..f4fe941 100644
--- a/Makefile
+++ b/Makefile
@@ -735,10 +735,12 @@ EXTLIBS =
 ifeq ($(uname_S),Linux)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -867,6 +869,7 @@ ifeq ($(uname_S),FreeBSD)
 		NO_STRTOUMAX = YesPlease
 	endif
 	PYTHON_PATH = /usr/local/bin/python
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
@@ -875,6 +878,7 @@ ifeq ($(uname_S),OpenBSD)
 	NEEDS_LIBICONV = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),NetBSD)
 	ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
@@ -884,6 +888,7 @@ ifeq ($(uname_S),NetBSD)
 	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
 	USE_ST_TIMESPEC = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
@@ -904,6 +909,7 @@ ifeq ($(uname_S),GNU)
 	# GNU/Hurd
 	NO_STRLCPY=YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),IRIX)
 	NO_SETENV = YesPlease
@@ -1353,6 +1359,10 @@ else
 	LIB_OBJS += thread-utils.o
 endif
 
+ifdef HAVE_PATHS_H
+	BASIC_CFLAGS += -DHAVE_PATHS_H
+endif
+
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
 	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif
diff --git a/exec_cmd.c b/exec_cmd.c
index b2c07c7..bf22570 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -107,7 +107,7 @@ void setup_path(void)
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		strbuf_addstr(&new_path, _PATH_DEFPATH);
 
 	setenv("PATH", new_path.buf, 1);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 7e62b55..ed1accc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -164,6 +164,12 @@ extern char *gitbasename(char *);
 #define PATH_SEP ':'
 #endif
 
+#ifdef HAVE_PATHS_H
+#include <paths.h>
+#else
+#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
+#endif
+
 #ifndef STRIP_EXTENSION
 #define STRIP_EXTENSION ""
 #endif
-- 
1.7.0.3

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Junio C Hamano]

Chris Webb <chris@arachsys.com> writes:

> In exec_cmd.c, git hard-codes a default path of
> /usr/local/bin:/usr/bin:/bin.  Get an appropriate value for the system
> from <paths.h> if possible instead. We only try to include <paths.h> on
> Linux, FreeBSD, NetBSD, OpenBSD and GNU where it is known to exist.
>
> Signed-off-by: Chris Webb <chris@arachsys.com>
> ---
>  Makefile          |   10 ++++++++++
>  exec_cmd.c        |    2 +-
>  git-compat-util.h |    6 ++++++
>  3 files changed, 17 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 910f471..f4fe941 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -735,10 +735,12 @@ EXTLIBS =
>  ifeq ($(uname_S),Linux)
>  	NO_STRLCPY = YesPlease
>  	NO_MKSTEMPS = YesPlease
> +	HAVE_PATHS_H = YesPlease
>  endif

Ok.  Somebody else may want to add an autoconf support on top of this, but
this is good as-is, I think.

Thanks.

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Junio C Hamano <gitster@pobox.com> writes:

> Ok.  Somebody else may want to add an autoconf support on top of this, but
> this is good as-is, I think.

Thanks; sorry it took me three attempts to get such a simple patch right!

Cheers,

Chris.

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Jakub Narebski]

Chris Webb <chris@arachsys.com> writes:

> In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
> Get an appropriate value for the system from <paths.h> if possible instead. We
> only try to include <paths.h> on Linux, FreeBSD, NetBSD, OpenBSD and GNU where
> it is known to exist.
> 
> Signed-off-by: Chris Webb <chris@arachsys.com>
> ---
>  Makefile          |   10 ++++++++++
>  exec_cmd.c        |    2 +-
>  git-compat-util.h |    6 ++++++
>  3 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 910f471..f4fe941 100644
> --- a/Makefile
> +++ b/Makefile

All other such variables are described at the top of main Makefile,
for example:

  #
  # Define NO_LIBGEN_H if you don't have libgen.h.

I think that HAVE_PATHS_H should also have such one-line description.
By the way it the very first variable with HAVE_* rather than NEEDS_*
or NO_* name.

[...]
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 7e62b55..ed1accc 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -164,6 +164,12 @@ extern char *gitbasename(char *);
>  #define PATH_SEP ':'
>  #endif
>  
> +#ifdef HAVE_PATHS_H
> +#include <paths.h>
> +#else
> +#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
> +#endif
> +
>  #ifndef STRIP_EXTENSION
>  #define STRIP_EXTENSION ""
>  #endif

Why not

  +#ifdef HAVE_PATHS_H
  +#include <paths.h>
  +#endif
  +#ifndef _PATH_DEFPATH
  +#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
  +#endif

This way you are covered if some other header provides _PATH_DEFPATH.
Or is your way better?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> Chris Webb <chris@arachsys.com> writes:
> 
> > In exec_cmd.c, git hard-codes a default path of
> > /usr/local/bin:/usr/bin:/bin.  Get an appropriate value for the system
> > from <paths.h> if possible instead. We only try to include <paths.h> on
> > Linux, FreeBSD, NetBSD, OpenBSD and GNU where it is known to exist.
> >
> > Signed-off-by: Chris Webb <chris@arachsys.com>
> > ---
> >  Makefile          |   10 ++++++++++
> >  exec_cmd.c        |    2 +-
> >  git-compat-util.h |    6 ++++++
> >  3 files changed, 17 insertions(+), 1 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 910f471..f4fe941 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -735,10 +735,12 @@ EXTLIBS =
> >  ifeq ($(uname_S),Linux)
> >  	NO_STRLCPY = YesPlease
> >  	NO_MKSTEMPS = YesPlease
> > +	HAVE_PATHS_H = YesPlease
> >  endif
> 
> Ok.  Somebody else may want to add an autoconf support on top of this, but
> this is good as-is, I think.

Something like that?

-- >8 --
Subject: [PATCH] autoconf: Check if <paths.h> exists

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 config.mak.in |    1 +
 configure.ac  |    6 ++++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index e8d96e8..f0aeb8e 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -31,6 +31,7 @@ NO_OPENSSL=@NO_OPENSSL@
 NO_CURL=@NO_CURL@
 NO_EXPAT=@NO_EXPAT@
 NO_LIBGEN_H=@NO_LIBGEN_H@
+HAVE_PATHS_H=@HAVE_PATHS_H@
 NEEDS_LIBICONV=@NEEDS_LIBICONV@
 NEEDS_SOCKET=@NEEDS_SOCKET@
 NO_SYS_SELECT_H=@NO_SYS_SELECT_H@
diff --git a/configure.ac b/configure.ac
index 108a97f..9dc0320 100644
--- a/configure.ac
+++ b/configure.ac
@@ -633,6 +633,12 @@ AC_CHECK_HEADER([libgen.h],
 [NO_LIBGEN_H=YesPlease])
 AC_SUBST(NO_LIBGEN_H)
 #
+# Define HAVE_PATHS_H if you have paths.h.
+AC_CHECK_HEADER([paths.h],
+[HAVE_PATHS_H=YesPlease],
+[HAVE_PATHS_H=])
+AC_SUBST(HAVE_PATHS_H)
+#
 # Define NO_STRCASESTR if you don't have strcasestr.
 GIT_CHECK_FUNC(strcasestr,
 [NO_STRCASESTR=],
-- 
1.7.0.1

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Jakub Narebski <jnareb@gmail.com> writes:

> All other such variables are described at the top of main Makefile,
> for example:
> 
>   #
>   # Define NO_LIBGEN_H if you don't have libgen.h.
> 
> I think that HAVE_PATHS_H should also have such one-line description.
> By the way it the very first variable with HAVE_* rather than NEEDS_*
> or NO_* name.

To be honest, I used the name suggested to me earlier in the thread without
a great deal of checking to see how consistent it was with existing naming
convention.

Blacklisting OSes with a NO_PATHS_H #define feels like a mistake, as unknown
OSes will fail rather than assuming a safe (if slightly untidy) default. I
got caught out assuming that Windows was sane in this regard, for instance.

To me, NEEDS_PATH_H hints that a system with paths.h would break if it
weren't included, rather than that this is an extra feature available on
this OS. But if NEEDS_* is used elsewhere to enable optional extras on
systems which support them, I agree we should change to NEEDS_PATH_H to be
consistent.

> Why not
> 
>   +#ifdef HAVE_PATHS_H
>   +#include <paths.h>
>   +#endif
>   +#ifndef _PATH_DEFPATH
>   +#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
>   +#endif
> 
> This way you are covered if some other header provides _PATH_DEFPATH.

Yes, makes sense, although I think _PATH_DEFPATH is very unlikely to be
provided outside of <paths.h>.

Best wishes,

Chris.

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Jakub Narebski]

On Thu, 15 April 2010, Chris Webb wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > All other such variables are described at the top of main Makefile,
> > for example:
> > 
> >   #
> >   # Define NO_LIBGEN_H if you don't have libgen.h.
> > 
> > I think that HAVE_PATHS_H should also have such one-line description.
> > By the way it the very first variable with HAVE_* rather than NEEDS_*
> > or NO_* name.
> 
> To be honest, I used the name suggested to me earlier in the thread without
> a great deal of checking to see how consistent it was with existing naming
> convention.

Actually HAVE_STH_H is the convention used in autoconf documentation.
It is Git convention of NO_STH_H (well, the single example of NO_LIBGEN_H)
that is non-standard... but this convention predates [optional] autoconf
support in Git.

> 
> Blacklisting OSes with a NO_PATHS_H #define feels like a mistake, as unknown
> OSes will fail rather than assuming a safe (if slightly untidy) default. I
> got caught out assuming that Windows was sane in this regard, for instance.

Well, there is only one example of checking for _headers_, namely
NO_LIBGEN_H, so it is not that you are against some majority.

In short: if there is no voice against HAVE_PATHS_H, lets have it this
way.

> 
> To me, NEEDS_PATH_H hints that a system with paths.h would break if it
> weren't included, rather than that this is an extra feature available on
> this OS. But if NEEDS_* is used elsewhere to enable optional extras on
> systems which support them, I agree we should change to NEEDS_PATH_H to be
> consistent.

Well, things like NEEDS_LIBGEN or NEEDS_SSL_WITH_CRYPTO are about a few
systems that needs *extra* work.  I don't think NEEDS_PATH_H is a good
variable name.


-- 
Jakub Narebski
Poland

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Chris Webb]

Jakub Narebski <jnareb@gmail.com> writes:

> Well, there is only one example of checking for _headers_, namely
> NO_LIBGEN_H, so it is not that you are against some majority.
> 
> In short: if there is no voice against HAVE_PATHS_H, lets have it this
> way.

Okay, sounds good to me. I'll respin with the style changes you suggested.

Best wishes,

Chris.

[PATCH v4] Replace hard-coded path with one from <paths.h>

[Chris Webb]

In exec_cmd.c, git hard-codes a default path of /usr/local/bin:/usr/bin:/bin.
Get an appropriate value for the system from <paths.h> if possible instead. We
only try to include <paths.h> on Linux, FreeBSD, NetBSD, OpenBSD and GNU where
it is known to exist.

Signed-off-by: Chris Webb <chris@arachsys.com>
---
 Makefile          |   13 +++++++++++++
 exec_cmd.c        |    2 +-
 git-compat-util.h |    7 +++++++
 3 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 910f471..81aa2ba 100644
--- a/Makefile
+++ b/Makefile
@@ -31,6 +31,9 @@ all::
 # Define EXPATDIR=/foo/bar if your expat header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
+# Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
+# it specifies.
+#
 # Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
 #
 # Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks
@@ -735,10 +738,12 @@ EXTLIBS =
 ifeq ($(uname_S),Linux)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -867,6 +872,7 @@ ifeq ($(uname_S),FreeBSD)
 		NO_STRTOUMAX = YesPlease
 	endif
 	PYTHON_PATH = /usr/local/bin/python
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
@@ -875,6 +881,7 @@ ifeq ($(uname_S),OpenBSD)
 	NEEDS_LIBICONV = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),NetBSD)
 	ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
@@ -884,6 +891,7 @@ ifeq ($(uname_S),NetBSD)
 	BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
 	USE_ST_TIMESPEC = YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
@@ -904,6 +912,7 @@ ifeq ($(uname_S),GNU)
 	# GNU/Hurd
 	NO_STRLCPY=YesPlease
 	NO_MKSTEMPS = YesPlease
+	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),IRIX)
 	NO_SETENV = YesPlease
@@ -1353,6 +1362,10 @@ else
 	LIB_OBJS += thread-utils.o
 endif
 
+ifdef HAVE_PATHS_H
+	BASIC_CFLAGS += -DHAVE_PATHS_H
+endif
+
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
 	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif
diff --git a/exec_cmd.c b/exec_cmd.c
index b2c07c7..bf22570 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -107,7 +107,7 @@ void setup_path(void)
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		strbuf_addstr(&new_path, _PATH_DEFPATH);
 
 	setenv("PATH", new_path.buf, 1);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 7e62b55..92c2182 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -164,6 +164,13 @@ extern char *gitbasename(char *);
 #define PATH_SEP ':'
 #endif
 
+#ifdef HAVE_PATHS_H
+#include <paths.h>
+#endif
+#ifndef _PATH_DEFPATH
+#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
+#endif
+
 #ifndef STRIP_EXTENSION
 #define STRIP_EXTENSION ""
 #endif
-- 
1.7.0.3

Re: [PATCH v3] Replace hard-coded path with one from <paths.h>

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> Ok.  Somebody else may want to add an autoconf support on top of this, but
>> this is good as-is, I think.
>
> Something like that?

Looks good.