t7005 and vi in GIT_EXEC_PATH

> Brian Gernhardt <benji@silverinsanity.com> writes:
>
>> If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the  
>> real
>> vi is invoked instead of the test vi script.  This is because the git
>> wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy solution to
>> this problem, and thought I should bring it up with the list.
>
> Putting "." at the front of GIT_EXEC_PATH instead of PATH would appear
> to do the trick then, wouldn't it?

>> If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the  
>> real
>> vi is invoked instead of the test vi script.  This is because the git
>> wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy solution to  
>> this
>> problem, and thought I should bring it up with the list.
>
> I don't understand.  GIT_EXEC_PATH should be set to the build  
> directory
> when you are running the tests.  Unless you copy vi _there_, you  
> should
> not have any problem.

> On Nov 11, 2007, at 10:58 AM, Johannes Schindelin wrote:
> 
> > > If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the 
> > > real vi is invoked instead of the test vi script.  This is because 
> > > the git wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy 
> > > solution to this problem, and thought I should bring it up with the 
> > > list.
> > 
> > I don't understand.  GIT_EXEC_PATH should be set to the build 
> > directory when you are running the tests.  Unless you copy vi _there_, 
> > you should not have any problem.
> 
> 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
> 
> The test appeared to hang when running it normally.  When I ran it with 
> -v, I saw that vim was started.

> 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

> 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?

> 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>
> ---

> 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);

> On Sun, 11 Nov 2007, Björn Steinbrink wrote:
>>
>> ---
>> 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.

> 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.

> > 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());

> 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?

> The git wrapper executable always prepends the GIT_EXEC_PATH build
> variable to the current PATH, so prepending "." to the PATH is not
> enough to give precedence to the fake vi executable.
> 
> The --exec-path option allows to prepend a directory to PATH even before
> GIT_EXEC_PATH (which is added anyway), so we can use that instead.

> Hi,
>
> On Sun, 11 Nov 2007, Björn Steinbrink wrote:
>
>> The git wrapper executable always prepends the GIT_EXEC_PATH build
>> variable to the current PATH, so prepending "." to the PATH is not
>> enough to give precedence to the fake vi executable.
>>
>> The --exec-path option allows to prepend a directory to PATH even  
>> before
>> GIT_EXEC_PATH (which is added anyway), so we can use that instead.
>
> Hmm.  This will probably stop working when you do not have git  
> installed,
> because you now tell git to search for git programs in ".", where  
> they are
> not.  Probably git-commit executes your installed write-tree, commit- 
> tree
> and friends, instead of the compiled ones.

> On Nov 11, 2007, at 12:44 PM, Johannes Schindelin wrote:
> 
> > Probably git-commit executes your installed write-tree, commit-tree 
> > and friends, instead of the compiled ones.
> 
> You are wrong there.  From exec_cmd.c:setup_path() (lines 51-54):
> 
>    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, cmd_path);

> Hi,
> 
> On Sun, 11 Nov 2007, Björn Steinbrink wrote:
> 
> > The git wrapper executable always prepends the GIT_EXEC_PATH build
> > variable to the current PATH, so prepending "." to the PATH is not
> > enough to give precedence to the fake vi executable.
> > 
> > The --exec-path option allows to prepend a directory to PATH even before
> > GIT_EXEC_PATH (which is added anyway), so we can use that instead.
> 
> Hmm.  This will probably stop working when you do not have git installed, 
> because you now tell git to search for git programs in ".", where they are 
> not.  Probably git-commit executes your installed write-tree, commit-tree 
> and friends, instead of the compiled ones.