Re: [PATCH] Use the best available exec path only

["Björn Steinbrink" <B.Steinbrink@gmx.de>]

On 2007.11.11 20:50:40 +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 11 Nov 2007, Bj?rn Steinbrink wrote:
> 
> > On 2007.11.11 11:43:02 -0800, Junio C Hamano wrote:
> > > Brian Gernhardt <benji@silverinsanity.com> writes:
> > > 
> > > > I'm sorry, I should have been more clear.  I was referring to the 
> > > > GIT_EXEC_PATH build variable, not the environment variable.  The git 
> > > > wrapper always adds the path determined during build to the front of 
> > > > PATH.  When I was changing my build script, this got set to "/usr/ 
> > > > local/bin" (I usually use /usr/local/stow/git, instead).  Since I 
> > > > have a /usr/local/bin/vim, PATH for git-commit.sh during the test 
> > > > was:
> > > >
> > > > - my git build directory
> > > > - /usr/local/bin (containing a symlink vi -> vim)
> > > > - the t/trash directory, added by the test via `PATH=".:$PATH"`
> > > > (containing the test vi script)
> > > > - my normal path
> > > 
> > > Maybe that is what is broken.  t/test-lib.sh makes the environment 
> > > variable point at the build directory, and that should override the 
> > > path that is compiled in, shouldn't it?
> > 
> > Maybe you prefer this patch then? "make test" survived up to 9101/25, 
> > but that fails with the current master anyway and I didn't bother to run 
> > the remaining tests manually, so it seems to be fine. Might break some 
> > weird setups that rely on being able to set multiple additional paths 
> > though (not that I think that that is a good idea to begin with).
> > 
> > Bj?rn
> > ---
> > Instead of adding all possible exec paths to PATH, only add the best
> > one, following the same rules that --exec-path, without arguments, uses
> > to figure out which path to display.
> > 
> > Signed-off-by: Bj?rn Steinbrink <B.Steinbrink@gmx.de>
> > ---
> 
> For easy application by the maintainer, please make the commit message the 
> first part, then have a single "---", and then the quoted mail.

OK.

> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 2d0a758..9c376ad 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -48,9 +48,7 @@ void setup_path(const char *cmd_path)
> >  
> >  	strbuf_init(&new_path, 0);
> >  
> > -	add_path(&new_path, argv_exec_path);
> > -	add_path(&new_path, getenv(EXEC_PATH_ENVIRONMENT));
> > -	add_path(&new_path, builtin_exec_path);
> > +	add_path(&new_path, git_exec_path());
> >  	add_path(&new_path, cmd_path);
> 
> I wonder why cmd_path is still there, then.  (I'd have expected something 
> like
> 
> 	add_path(&new_path, cmd_path ? cmd_path : git_exec_path());

Wouldn't that break setups where only the "git" wrapper is in $PATH?
cmd_path is just the path to the called executable (argv[0]). As I read
the code, it seems that cmd_path is just a worst case fallback, when the
"real" exec path is messed up somehow. I figured that I'd better keep
it, instead of just dropping it without knowing its intended purpose.
But if you're going to change anything about it, I'd guess that the only
option is to drop it, or change its relative position in PATH.

> In related news, IMO cmd_path should be made absolute if it is not already 
> the case.

add_path() already takes care of that.

Björn