From: Jakub Narebski <jnareb@gmail.com>
Date: Thu, 15 Apr 2010 12:25:14 GMT
Subject: Re: [PATCH v3] Replace hard-coded path with one from <paths.h>
Message-ID: <m3hbncgaxu.fsf@localhost.localdomain>
In-Reply-To: <20100413090713.GC770@arachsys.com>

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