t7005 and vi in GIT_EXEC_PATH

t7005 and vi in GIT_EXEC_PATH

[Brian Gernhardt]

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.

~~ Brian

Re: t7005 and vi in GIT_EXEC_PATH

[David Kastrup]

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?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: t7005 and vi in GIT_EXEC_PATH

[Brian Gernhardt]

On Nov 10, 2007, at 5:09 PM, David Kastrup wrote:

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

The GIT_EXEC_PATH I was referring to is the one set in the Makefile  
and compiled into git.  The GIT_EXEC_PATH environment variable is set  
to the git repository.  PATH ends up looking like this (paraphrased):   
"git.git:install directory:.:normal PATH".  And since the install  
directory contains vi, the test fails (actually appears to hang  
because vi is waiting for input while it's output is being sent to / 
dev/null).

~~ Brian

Re: t7005 and vi in GIT_EXEC_PATH

[Johannes Schindelin]

Hi,

On Sat, 10 Nov 2007, Brian Gernhardt 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.

Ciao,
Dscho

Re: t7005 and vi in GIT_EXEC_PATH

[Brian Gernhardt]

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.

~~ Brian

Re: t7005 and vi in GIT_EXEC_PATH

[Johannes Schindelin]

Hi,

On Sun, 11 Nov 2007, Brian Gernhardt wrote:

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

The obvious solution would be to copy "vi" into the git build directory 
for the test, or skip the test if that copy could not be performed.

Ciao,
Dscho

Re: t7005 and vi in GIT_EXEC_PATH

[Junio C Hamano]

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?

[PATCH] Use the best available exec path only

[Björn Steinbrink]

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

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

[Johannes Schindelin]

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

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

Ciao,
Dscho

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

[Jakub Narebski]

Johannes Schindelin wrote:
> 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.

There are two conventions, for slightly different cases:

1. Commit message, then diffstat / single "---", then comments,
   then patch

2. Email (for example long discussion), then separator BUT NOT "---";
   use for example "-- >8 --" (cut here scissors), then commit message,
   then diffstat and patch, beginning with "---".

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Björn Steinbrink]

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

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

[Johannes Schindelin]

Hi,

On Sun, 11 Nov 2007, Bj?rn Steinbrink wrote:

> > In related news, IMO cmd_path should be made absolute if it is not 
> > already the case.
> 
> add_path() already takes care of that.

Yeah, and cmd_path is not used otherwise.  Sorry for the noise.

Ciao,
Dscho

Re: t7005 and vi in GIT_EXEC_PATH

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Ah, nevermind.  I did not see the other messages in the thread.

[PATCH] t7005-editor.sh: Don't invoke real vi when it is in GIT_EXEC_PATH

[Björn Steinbrink]

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.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
 t/t7005-editor.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 01cc0c0..0b36ee1 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -61,7 +61,7 @@ do
 		;;
 	esac
 	test_expect_success "Using $i" '
-		git commit --amend &&
+		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&
 		diff actual expect
@@ -83,7 +83,7 @@ do
 		;;
 	esac
 	test_expect_success "Using $i (override)" '
-		git commit --amend &&
+		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&
 		diff actual expect
-- 
1.5.3.5.622.g6fd7a

Re: [PATCH] t7005-editor.sh: Don't invoke real vi when it is in GIT_EXEC_PATH

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 675 bytes --]

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.

Ciao,
Dscho

Re: [PATCH] t7005-editor.sh: Don't invoke real vi when it is in GIT_EXEC_PATH

[Brian Gernhardt]

On Nov 11, 2007, at 12:44 PM, Johannes Schindelin wrote:

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

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

So the path with this patch will still include the build directory  
before the install location.

~~ Brian

Re: [PATCH] t7005-editor.sh: Don't invoke real vi when it is in GIT_EXEC_PATH

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 648 bytes --]

Hi Brian, hi Björn,

On Sun, 11 Nov 2007, Brian Gernhardt wrote:

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

Ah, I forgot that --exec-path=<path> did not override GIT_EXEC_PATH.  
Thanks for clarifying!  Your patch is obviously good, then.

Ciao,
Dscho

Re: [PATCH] t7005-editor.sh: Don't invoke real vi when it is in GIT_EXEC_PATH

[Björn Steinbrink]

On 2007.11.11 17:44:14 +0000, Johannes Schindelin wrote:
> 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.

The . is prepended to PATH in _addition_ to the usual paths (as I wrote,
see setup_path() in exec_cmd.c). It does not replace anything AFAICT.

$ echo $PATH
/home/doener/bin:/usr/local/bin:/usr/bin:/bin:/usr/games

$ GIT_EXEC_PATH=.. GIT_EDITOR="env;" ../git  commit | grep ^PATH=
PATH=/home/doener/src/git:/home/doener/bin:/home/doener/src/git:/home/doener/bin:/usr/local/bin:/usr/bin:/bin:/usr/games

$ GIT_EXEC_PATH=.. GIT_EDITOR="env;" ../git --exec-path=. commit | grep
^PATH=
PATH=/home/doener/src/git/t:/home/doener/src/git:/home/doener/bin:/home/doener/src/git:/home/doener/bin:/usr/local/bin:/usr/bin:/bin:/usr/games


Looks good to me...

Björn