threads / patch / 23332

patchReplace hard-coded path with one configurable at make time

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

## tl;dr

24 messages between Apr 4, 2010 and Apr 15, 2010. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Chris Webb· Apr 4, 2010, 20:48 UTC · lore

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(-)
Show changes to 3 files +8 −4

Makefile, exec_cmd.c, git-instaweb.sh

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
Junio C Hamano· Apr 4, 2010, 21:38 UTC · re: Chris Webb · lore

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

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?
Chris Webb· Apr 4, 2010, 22:28 UTC · re: Junio C Hamano · lore

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

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.

Show 10 quoted lines
>  * 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.
Chris Webb· Apr 6, 2010, 16:35 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 6 quoted lines
> 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.
Chris Webb· Apr 6, 2010, 16:36 UTC · re: Chris Webb · lore

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

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(-)
Show changes to 2 files +6 −1

exec_cmd.c, git-compat-util.h

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
Erik Faye-Lund· Apr 7, 2010, 10:57 UTC · re: Chris Webb · lore

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

On Tue, Apr 6, 2010 at 6:36 PM, Chris Webb <chris@arachsys.com> wrote:
Show 31 quoted lines
> 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
Chris Webb· Apr 8, 2010, 10:58 UTC · re: Erik Faye-Lund · lore

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

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.
Erik Faye-Lund· Apr 8, 2010, 11:26 UTC · re: Chris Webb · lore

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

On Thu, Apr 8, 2010 at 12:58 PM, Chris Webb <chris@arachsys.com> wrote:
Show 8 quoted lines
> 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
Chris Webb· Apr 8, 2010, 11:57 UTC · re: Erik Faye-Lund · lore

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

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(-)
Show changes to 2 files +7 −1

exec_cmd.c, git-compat-util.h

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
Erik Faye-Lund· Apr 8, 2010, 12:08 UTC · re: Chris Webb · lore

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

On Thu, Apr 8, 2010 at 1:57 PM, Chris Webb <chris@arachsys.com> wrote:
Show 35 quoted lines
> 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
Chris Webb· Apr 9, 2010, 05:45 UTC · re: Erik Faye-Lund · lore

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

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.
Chris Webb· Apr 13, 2010, 09:06 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 8 quoted lines
> 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.
Chris Webb· Apr 13, 2010, 09:07 UTC · re: Chris Webb · lore

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

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(-)
Show changes to 3 files +17 −1

Makefile, exec_cmd.c, git-compat-util.h

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
Junio C Hamano· Apr 13, 2010, 20:01 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 22 quoted lines
> 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.
Chris Webb· Apr 14, 2010, 07:22 UTC · re: Junio C Hamano · lore

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

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.
Jakub Narebski· Apr 15, 2010, 12:27 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> 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(-)
Show changes to 2 files +7 −0

config.mak.in, configure.ac

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
Junio C Hamano· Apr 15, 2010, 19:15 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 6 quoted lines
> 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.
Jakub Narebski· Apr 15, 2010, 12:25 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 16 quoted lines
> 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.

[...]
Show 17 quoted lines
> 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
Chris Webb· Apr 15, 2010, 12:40 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 9 quoted lines
> 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.

Show 10 quoted lines
> 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.
Jakub Narebski· Apr 15, 2010, 12:57 UTC · re: Chris Webb · lore

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

On Thu, 15 April 2010, Chris Webb wrote:
Show 15 quoted lines
> 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.

Show 6 quoted lines
> 
> 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
Chris Webb· Apr 15, 2010, 13:01 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> 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.
Chris Webb· Apr 15, 2010, 13:21 UTC · re: Chris Webb · lore

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

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(-)
Show changes to 3 files +21 −1

Makefile, exec_cmd.c, git-compat-util.h

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
Tait· Apr 9, 2010, 23:54 UTC · re: Chris Webb · lore

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

Show 9 quoted lines
> > > +#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.
Chris Webb· Apr 6, 2010, 16:57 UTC · re: Chris Webb · lore

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

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(-)
Show changes to git-instaweb.sh +2 −2
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

← back to recent threads