login
Search
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); ...
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 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. 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 --
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 --
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. --
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. 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 --
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 --
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. 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 ...
Ok. Somebody else may want to add an autoconf support on top of this, but this is good as-is, I think. Thanks. --
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 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 --
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. 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 --
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 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 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 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. 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 ...
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. --
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 --
