Replace hard-coded path with one configurable at make time

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

index 11ec3e2..2f4b39e 100644
@@ -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' \
 	    [email protected] > $@+ && \
 	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)"'

index b2c07c7..39c6a59 100644
@@ -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);
 

index 6a65f25..72d8b12 100755
@@ -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

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?

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.

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.

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.

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.

---
 exec_cmd.c        |    2 +-
 git-compat-util.h |    5 +++++
 2 files changed, 6 insertions(+), 1 deletions(-)

index b2c07c7..bf22570 100644
@@ -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);
 

index 7e62b55..7592be7 100644
@@ -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

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.

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.

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

Erik "kusma" Faye-Lund

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.

---
 exec_cmd.c        |    2 +-
 git-compat-util.h |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

index b2c07c7..bf22570 100644
@@ -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);
 

index 7e62b55..9443b9b 100644
@@ -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

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.

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.

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.

---
 Makefile          |   10 ++++++++++
 exec_cmd.c        |    2 +-
 git-compat-util.h |    6 ++++++
 3 files changed, 17 insertions(+), 1 deletions(-)

index 910f471..f4fe941 100644
@@ -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

index b2c07c7..bf22570 100644
@@ -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);
 

index 7e62b55..ed1accc 100644
@@ -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

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

Cheers,

Chris.

Something like that?

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

---
 config.mak.in |    1 +
 configure.ac  |    6 ++++++
 2 files changed, 7 insertions(+), 0 deletions(-)

index e8d96e8..f0aeb8e 100644
@@ -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@

index 108a97f..9dc0320 100644
@@ -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

Looks good.

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.

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

Best wishes,

Chris.

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.

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.

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

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

Best wishes,

Chris.

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.

---
 Makefile          |   13 +++++++++++++
 exec_cmd.c        |    2 +-
 git-compat-util.h |    7 +++++++
 3 files changed, 21 insertions(+), 1 deletions(-)

index 910f471..81aa2ba 100644
@@ -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

index b2c07c7..bf22570 100644
@@ -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);
 

index 7e62b55..92c2182 100644
@@ -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

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

---
 git-instaweb.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 6a65f25..0e1cb07 100755
@@ -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