From: Junio C Hamano <hidden> Date: 2016-06-15 22:48:33
Chris Webb [off-list ref] 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?
From: Chris Webb <hidden> Date: 2016-06-15 22:48:33
Junio C Hamano [off-list ref] 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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:34
Chris Webb [off-list ref] 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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:34
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 <redacted>
---
exec_cmd.c | 2 +-
git-compat-util.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletions(-)
From: Chris Webb <hidden> Date: 2016-06-15 22:48:34
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 <redacted>
---
git-instaweb.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:34
On Tue, Apr 6, 2010 at 6:36 PM, Chris Webb [off-list ref] wrote:
quoted hunk
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 <redacted>
---
exec_cmd.c | 2 +-
git-compat-util.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletions(-)
From: Chris Webb <hidden> Date: 2016-06-15 22:48:35
Erik Faye-Lund [off-list ref] writes:
quoted
+#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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:35
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 <redacted>
---
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
2 files changed, 7 insertions(+), 1 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:35
On Thu, Apr 8, 2010 at 1:57 PM, Chris Webb [off-list ref] wrote:
quoted hunk
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 <redacted>
---
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
2 files changed, 7 insertions(+), 1 deletions(-)
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
From: Chris Webb <hidden> Date: 2016-06-15 22:48:35
Erik Faye-Lund [off-list ref] 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.
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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:36
Chris Webb [off-list ref] writes:
Erik Faye-Lund [off-list ref] writes:
quoted
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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:36
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 <redacted>
---
Makefile | 10 ++++++++++
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
3 files changed, 17 insertions(+), 1 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:48:37
Chris Webb [off-list ref] writes:
quoted hunk
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 <redacted>
---
Makefile | 10 ++++++++++
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
3 files changed, 17 insertions(+), 1 deletions(-)
From: Jakub Narebski <hidden> Date: 2016-06-15 22:48:37
Chris Webb [off-list ref] writes:
quoted hunk
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 <redacted>
---
Makefile | 10 ++++++++++
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
3 files changed, 17 insertions(+), 1 deletions(-)
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
From: Jakub Narebski <hidden> Date: 2016-06-15 22:48:37
Junio C Hamano [off-list ref] writes:
Chris Webb [off-list ref] writes:
quoted
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 <redacted>
---
Makefile | 10 ++++++++++
exec_cmd.c | 2 +-
git-compat-util.h | 6 ++++++
3 files changed, 17 insertions(+), 1 deletions(-)
@@ -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=],
From: Chris Webb <hidden> Date: 2016-06-15 22:48:37
Jakub Narebski [off-list ref] 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.
From: Jakub Narebski <hidden> Date: 2016-06-15 22:48:37
On Thu, 15 April 2010, Chris Webb wrote:
Jakub Narebski [off-list ref] writes:
quoted
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
From: Chris Webb <hidden> Date: 2016-06-15 22:48:37
Jakub Narebski [off-list ref] 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.
From: Chris Webb <hidden> Date: 2016-06-15 22:48:37
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 <redacted>
---
Makefile | 13 +++++++++++++
exec_cmd.c | 2 +-
git-compat-util.h | 7 +++++++
3 files changed, 21 insertions(+), 1 deletions(-)
@@ -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