Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH)

Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH)

[Scott R Parish]

Yeah, that seems to work fine. The theoretical drawback to this approach
is that it could possibly effect the order in which the paths are tried.
For instance, if a user did "export GIT_EXEC_PATH=", then the
builtin_exec_path wouldn't be tried before the PATH. (i doubt that it
would be a problem, but thought i should note it)

sRp


----- Original Message -----
Subject: Re: [PATCH] When exec'ing sub-commands, fall back on execvp
(thePATH)
Date: Sat, October 20, 2007 0:30
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>

> Hi,
>
> On Fri, 19 Oct 2007, Scott Parish wrote:
>
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 9b74ed2..674c9f3 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -34,15 +34,15 @@ int execv_git_cmd(const char **argv)
> >  {
> >          char git_command[PATH_MAX + 1];
> >          int i;
> > +        int rc;
> >          const char *paths[] = { current_exec_path,
> >                                  getenv(EXEC_PATH_ENVIRONMENT),
> >                                  builtin_exec_path };
> > +        const char *tmp;
> > +        size_t len;
> >
> >          for (i = 0; i < ARRAY_SIZE(paths); ++i) {
> > -                size_t len;
> > -                int rc;
> >                  const char *exec_dir = paths[i];
> > -                const char *tmp;
> >
> >                  if (!exec_dir || !*exec_dir) continue;
> >
> > @@ -106,8 +106,26 @@ int execv_git_cmd(const char **argv)
> >
> >                  argv[0] = tmp;
> >          }
> > -        return -1;
> >
> > +        rc = snprintf(git_command, sizeof(git_command), "git-%s",
argv[0]);
> > +        if (rc < 0 || rc >= sizeof(git_command) - len) {
> > +                fprintf(stderr, "git: command name given is too
long.\n");
> > +                return -1;
> > +        }
> > +
> > +        tmp = argv[0];
> > +        argv[0] = git_command;
> > +
> > +        trace_argv_printf(argv, -1, "trace: exec:");
> > +
> > +        /* execve() can only ever return if it fails */
> > +        execvp(git_command, (char **)argv);
> > +
> > +        trace_printf("trace: exec failed: %s\n", strerror(errno));
> > +
> > +        argv[0] = tmp;
> > +
> > +        return -1;
> >  }
>
> I am not sure that this is elegant enough: Something like this (completely
> untested) might be better:
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..c928f37 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
>          int i;
>          const char *paths[] = { current_exec_path,
>                                  getenv(EXEC_PATH_ENVIRONMENT),
> -                                builtin_exec_path };
> +                                builtin_exec_path,
> +                                "" };
>
>          for (i = 0; i < ARRAY_SIZE(paths); ++i) {
>                  size_t len;
> @@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
>                  const char *exec_dir = paths[i];
>                  const char *tmp;
>
> -                if (!exec_dir || !*exec_dir) continue;
> +                if (!exec_dir) continue;
>
> -                if (*exec_dir != '/') {
> +                if (!*exec_dir)
> +                        /* try PATH */
> +                        *git_command = '\0';
> +                else if (*exec_dir != '/') {
>                          if (!getcwd(git_command, sizeof(git_command))) {
>                                  fprintf(stderr, "git: cannot determine "
>                                          "current directory: %s\n",
> @@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
>
>                  len = strlen(git_command);
>                  rc = snprintf(git_command + len, sizeof(git_command) -
len,
> -                              "/git-%s", argv[0]);
> +                              "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
>                  if (rc < 0 || rc >= sizeof(git_command) - len) {
>                          fprintf(stderr,
>                                  "git: command name given is too long.\n");
>
> Ciao,
> Dscho
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH)

[Johannes Schindelin]

Hi,

[please do not top post. Just delete everything you do not reply to, and 
put your answers below the text you are replying to.  This spares others 
so much time.]

On Sat, 20 Oct 2007, Scott R Parish wrote:

> The theoretical drawback to this approach is that it could possibly 
> effect the order in which the paths are tried. For instance, if a user 
> did "export GIT_EXEC_PATH=", then the builtin_exec_path wouldn't be 
> tried before the PATH. (i doubt that it would be a problem, but thought 
> i should note it)

In that respect, my code does not change anything from your code.

Ciao,
Dscho

Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH)

[Scott Parish]

Actually, i didn't test it right, execve() were using the files in
my cwd. In addition to you patch, you'd need to use execvp() instead
of execve().

sRp

On Sat, Oct 20, 2007 at 03:12:17AM -0500, Scott R Parish wrote:

> Yeah, that seems to work fine. The theoretical drawback to this approach
> is that it could possibly effect the order in which the paths are tried.
> For instance, if a user did "export GIT_EXEC_PATH=", then the
> builtin_exec_path wouldn't be tried before the PATH. (i doubt that it
> would be a problem, but thought i should note it)
> 
> sRp
> 
> 
> ----- Original Message -----
> Subject: Re: [PATCH] When exec'ing sub-commands, fall back on execvp
> (thePATH)
> Date: Sat, October 20, 2007 0:30
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> 
> > Hi,
> >
> > On Fri, 19 Oct 2007, Scott Parish wrote:
> >
> > > diff --git a/exec_cmd.c b/exec_cmd.c
> > > index 9b74ed2..674c9f3 100644
> > > --- a/exec_cmd.c
> > > +++ b/exec_cmd.c
> > > @@ -34,15 +34,15 @@ int execv_git_cmd(const char **argv)
> > >  {
> > >          char git_command[PATH_MAX + 1];
> > >          int i;
> > > +        int rc;
> > >          const char *paths[] = { current_exec_path,
> > >                                  getenv(EXEC_PATH_ENVIRONMENT),
> > >                                  builtin_exec_path };
> > > +        const char *tmp;
> > > +        size_t len;
> > >
> > >          for (i = 0; i < ARRAY_SIZE(paths); ++i) {
> > > -                size_t len;
> > > -                int rc;
> > >                  const char *exec_dir = paths[i];
> > > -                const char *tmp;
> > >
> > >                  if (!exec_dir || !*exec_dir) continue;
> > >
> > > @@ -106,8 +106,26 @@ int execv_git_cmd(const char **argv)
> > >
> > >                  argv[0] = tmp;
> > >          }
> > > -        return -1;
> > >
> > > +        rc = snprintf(git_command, sizeof(git_command), "git-%s",
> argv[0]);
> > > +        if (rc < 0 || rc >= sizeof(git_command) - len) {
> > > +                fprintf(stderr, "git: command name given is too
> long.\n");
> > > +                return -1;
> > > +        }
> > > +
> > > +        tmp = argv[0];
> > > +        argv[0] = git_command;
> > > +
> > > +        trace_argv_printf(argv, -1, "trace: exec:");
> > > +
> > > +        /* execve() can only ever return if it fails */
> > > +        execvp(git_command, (char **)argv);
> > > +
> > > +        trace_printf("trace: exec failed: %s\n", strerror(errno));
> > > +
> > > +        argv[0] = tmp;
> > > +
> > > +        return -1;
> > >  }
> >
> > I am not sure that this is elegant enough: Something like this (completely
> > untested) might be better:
> >
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 9b74ed2..c928f37 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
> >          int i;
> >          const char *paths[] = { current_exec_path,
> >                                  getenv(EXEC_PATH_ENVIRONMENT),
> > -                                builtin_exec_path };
> > +                                builtin_exec_path,
> > +                                "" };
> >
> >          for (i = 0; i < ARRAY_SIZE(paths); ++i) {
> >                  size_t len;
> > @@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
> >                  const char *exec_dir = paths[i];
> >                  const char *tmp;
> >
> > -                if (!exec_dir || !*exec_dir) continue;
> > +                if (!exec_dir) continue;
> >
> > -                if (*exec_dir != '/') {
> > +                if (!*exec_dir)
> > +                        /* try PATH */
> > +                        *git_command = '\0';
> > +                else if (*exec_dir != '/') {
> >                          if (!getcwd(git_command, sizeof(git_command))) {
> >                                  fprintf(stderr, "git: cannot determine "
> >                                          "current directory: %s\n",
> > @@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
> >
> >                  len = strlen(git_command);
> >                  rc = snprintf(git_command + len, sizeof(git_command) -
> len,
> > -                              "/git-%s", argv[0]);
> > +                              "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
> >                  if (rc < 0 || rc >= sizeof(git_command) - len) {
> >                          fprintf(stderr,
> >                                  "git: command name given is too long.\n");
> >
> > Ciao,
> > Dscho
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Scott Parish
http://srparish.net/

[PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Schindelin]

Earlier, we tried to find the git commands in several possible exec
dirs.  Now, if all of these failed, try to find the git command in
PATH.

This allows you to install the git programs somewhere else than
originally specified when building git, as long as you add that location
to the PATH.

Initial implementation by Scott R Parish.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sat, 20 Oct 2007, Scott Parish wrote:

	> Actually, i didn't test it right, execve() were using the files 
	> in my cwd. In addition to you patch, you'd need to use execvp() 
	> instead of execve().

	Ah, right.  I missed that one ;-)

	How about this instead?

 exec_cmd.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..c928f37 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
 	int i;
 	const char *paths[] = { current_exec_path,
 				getenv(EXEC_PATH_ENVIRONMENT),
-				builtin_exec_path };
+				builtin_exec_path,
+				"" };
 
 	for (i = 0; i < ARRAY_SIZE(paths); ++i) {
 		size_t len;
@@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
 		const char *exec_dir = paths[i];
 		const char *tmp;
 
-		if (!exec_dir || !*exec_dir) continue;
+		if (!exec_dir) continue;
 
-		if (*exec_dir != '/') {
+		if (!*exec_dir)
+			/* try PATH */
+			*git_command = '\0';
+		else if (*exec_dir != '/') {
 			if (!getcwd(git_command, sizeof(git_command))) {
 				fprintf(stderr, "git: cannot determine "
 					"current directory: %s\n",
@@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
 
 		len = strlen(git_command);
 		rc = snprintf(git_command + len, sizeof(git_command) - len,
-			      "/git-%s", argv[0]);
+			      "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
 		if (rc < 0 || rc >= sizeof(git_command) - len) {
 			fprintf(stderr,
 				"git: command name given is too long.\n");
-- 
1.5.3.4.1287.g8b31e

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 	On Sat, 20 Oct 2007, Scott Parish wrote:
> 
> 	> Actually, i didn't test it right, execve() were using the files 
> 	> in my cwd. In addition to you patch, you'd need to use execvp() 
> 	> instead of execve().
> 
> 	Ah, right.  I missed that one ;-)
> 
> 	How about this instead?

Uhhh.  Its the same, isn't it?  Still using execve() which means
we will not look at PATH in the final attempt.
 
>  exec_cmd.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..c928f37 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
>  	int i;
>  	const char *paths[] = { current_exec_path,
>  				getenv(EXEC_PATH_ENVIRONMENT),
> -				builtin_exec_path };
> +				builtin_exec_path,
> +				"" };
>  
>  	for (i = 0; i < ARRAY_SIZE(paths); ++i) {
>  		size_t len;
> @@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
>  		const char *exec_dir = paths[i];
>  		const char *tmp;
>  
> -		if (!exec_dir || !*exec_dir) continue;
> +		if (!exec_dir) continue;
>  
> -		if (*exec_dir != '/') {
> +		if (!*exec_dir)
> +			/* try PATH */
> +			*git_command = '\0';
> +		else if (*exec_dir != '/') {
>  			if (!getcwd(git_command, sizeof(git_command))) {
>  				fprintf(stderr, "git: cannot determine "
>  					"current directory: %s\n",
> @@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
>  
>  		len = strlen(git_command);
>  		rc = snprintf(git_command + len, sizeof(git_command) - len,
> -			      "/git-%s", argv[0]);
> +			      "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
>  		if (rc < 0 || rc >= sizeof(git_command) - len) {
>  			fprintf(stderr,
>  				"git: command name given is too long.\n");
> -- 
> 1.5.3.4.1287.g8b31e

-- 
Shawn.

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Scott Parish]

Earlier, we tried to find the git commands in several possible exec
dirs.  Now, if all of these failed, try to find the git command in
PATH.

This allows you to install the git programs somewhere else than
originally specified when building git, as long as you add that location
to the PATH.

Implementation by Johannes Schindelin

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 exec_cmd.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..374ffc9 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
 	int i;
 	const char *paths[] = { current_exec_path,
 				getenv(EXEC_PATH_ENVIRONMENT),
-				builtin_exec_path };
+				builtin_exec_path,
+				"" };
 
 	for (i = 0; i < ARRAY_SIZE(paths); ++i) {
 		size_t len;
@@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
 		const char *exec_dir = paths[i];
 		const char *tmp;
 
-		if (!exec_dir || !*exec_dir) continue;
+		if (!exec_dir) continue;
 
-		if (*exec_dir != '/') {
+		if (!*exec_dir)
+			/* try PATH */
+			*git_command = '\0';
+		else if (*exec_dir != '/') {
 			if (!getcwd(git_command, sizeof(git_command))) {
 				fprintf(stderr, "git: cannot determine "
 					"current directory: %s\n",
@@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
 
 		len = strlen(git_command);
 		rc = snprintf(git_command + len, sizeof(git_command) - len,
-			      "/git-%s", argv[0]);
+			      "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
 		if (rc < 0 || rc >= sizeof(git_command) - len) {
 			fprintf(stderr,
 				"git: command name given is too long.\n");
@@ -99,8 +103,8 @@ int execv_git_cmd(const char **argv)
 
 		trace_argv_printf(argv, -1, "trace: exec:");
 
-		/* execve() can only ever return if it fails */
-		execve(git_command, (char **)argv, environ);
+		/* execvp() can only ever return if it fails */
+		execvp(git_command, (char **)argv);
 
 		trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-- 
1.5.3.4.206.g58ba4-dirty

[PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Schindelin]

Earlier, we tried to find the git commands in several possible exec
dirs.  Now, if all of these failed, try to find the git command in
PATH.

This allows you to install the git programs somewhere else than
originally specified when building git, as long as you add that location
to the PATH.

Initial implementation by Scott R Parish.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sat, 20 Oct 2007, Shawn O. Pearce wrote:

	> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
	> > On Sat, 20 Oct 2007, Scott Parish wrote:
	> > 
	> > > Actually, i didn't test it right, execve() were using the 
	> > > files in my cwd. In addition to you patch, you'd need to use 
	> > > execvp() instead of execve().
	> > 
	> > Ah, right.  I missed that one ;-)
	> > 
	> > How about this instead?
	> 
	> Uhhh.  Its the same, isn't it?  Still using execve() which means 
	> we will not look at PATH in the final attempt.

	Sorry.  I forgot to --amend.

 exec_cmd.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..70b84b0 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
 	int i;
 	const char *paths[] = { current_exec_path,
 				getenv(EXEC_PATH_ENVIRONMENT),
-				builtin_exec_path };
+				builtin_exec_path,
+				"" };
 
 	for (i = 0; i < ARRAY_SIZE(paths); ++i) {
 		size_t len;
@@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
 		const char *exec_dir = paths[i];
 		const char *tmp;
 
-		if (!exec_dir || !*exec_dir) continue;
+		if (!exec_dir) continue;
 
-		if (*exec_dir != '/') {
+		if (!*exec_dir)
+			/* try PATH */
+			*git_command = '\0';
+		else if (*exec_dir != '/') {
 			if (!getcwd(git_command, sizeof(git_command))) {
 				fprintf(stderr, "git: cannot determine "
 					"current directory: %s\n",
@@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
 
 		len = strlen(git_command);
 		rc = snprintf(git_command + len, sizeof(git_command) - len,
-			      "/git-%s", argv[0]);
+			      "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
 		if (rc < 0 || rc >= sizeof(git_command) - len) {
 			fprintf(stderr,
 				"git: command name given is too long.\n");
@@ -100,7 +104,10 @@ int execv_git_cmd(const char **argv)
 		trace_argv_printf(argv, -1, "trace: exec:");
 
 		/* execve() can only ever return if it fails */
-		execve(git_command, (char **)argv, environ);
+		if (*exec_dir)
+			execve(git_command, (char **)argv, environ);
+		else
+			execvp(git_command, (char **)argv);
 
 		trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-- 
1.5.3.4.1289.ga1432

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Schindelin]

Hi,

On Sun, 21 Oct 2007, Scott Parish wrote:

> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..374ffc9 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -99,8 +103,8 @@ int execv_git_cmd(const char **argv)
>  
>  		trace_argv_printf(argv, -1, "trace: exec:");
>  
> -		/* execve() can only ever return if it fails */
> -		execve(git_command, (char **)argv, environ);
> +		/* execvp() can only ever return if it fails */
> +		execvp(git_command, (char **)argv);

I'd rather have it conditional upon *exec_dir (see the brown paper bag 
patch I just sent).

Ciao,
Dscho

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Earlier, we tried to find the git commands in several possible exec
> dirs.  Now, if all of these failed, try to find the git command in
> PATH.
...
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..70b84b0 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
>  	int i;
>  	const char *paths[] = { current_exec_path,
>  				getenv(EXEC_PATH_ENVIRONMENT),
> -				builtin_exec_path };
> +				builtin_exec_path,
> +				"" };

So if the user sets GIT_EXEC_PATH="" and exports it we'll search
$PATH before the builtin exec path that Git was compiled with?
Are we sure we want to do that?

I'm going to throw this into pu tonight just so I don't lose it,
but I have a feeling we want to amend it before merging.

-- 
Shawn.

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Schindelin]

Hi,

On Mon, 22 Oct 2007, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Earlier, we tried to find the git commands in several possible exec
> > dirs.  Now, if all of these failed, try to find the git command in
> > PATH.
> ...
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 9b74ed2..70b84b0 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
> >  	int i;
> >  	const char *paths[] = { current_exec_path,
> >  				getenv(EXEC_PATH_ENVIRONMENT),
> > -				builtin_exec_path };
> > +				builtin_exec_path,
> > +				"" };
> 
> So if the user sets GIT_EXEC_PATH="" and exports it we'll search $PATH 
> before the builtin exec path that Git was compiled with? Are we sure we 
> want to do that?

I thought the proper way to unset EXEC_PATH was to "unset GIT_EXEC_PATH".  
In that case, getenv(EXEC_PATH_ENVIRONMENT) returns NULL and we're fine, 
no?

Ciao,
Dscho

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Scott Parish]

On Sun, Oct 21, 2007 at 10:59:01PM +0100, Johannes Schindelin wrote:

> Earlier, we tried to find the git commands in several possible exec
> dirs.  Now, if all of these failed, try to find the git command in
> PATH.

I'm tempted to try a different approach. What if instead of looping
and building up strings of all the different absolute paths we want
to try we just prepend to PATH with the correct extra precedence,
and then call execvp on the command we want?

sRp

-- 
Scott Parish
http://srparish.net/

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Andreas Ericsson]

Scott Parish wrote:
> On Sun, Oct 21, 2007 at 10:59:01PM +0100, Johannes Schindelin wrote:
> 
>> Earlier, we tried to find the git commands in several possible exec
>> dirs.  Now, if all of these failed, try to find the git command in
>> PATH.
> 
> I'm tempted to try a different approach. What if instead of looping
> and building up strings of all the different absolute paths we want
> to try we just prepend to PATH with the correct extra precedence,
> and then call execvp on the command we want?
> 

That's how the original git --exec-dir feature got implemented. There's 
even a nifty function for it in git.c; prepend_to_path(). It's a 
provably workable solution.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Sixt]

Andreas Ericsson schrieb:
> Scott Parish wrote:
>> I'm tempted to try a different approach. What if instead of looping
>> and building up strings of all the different absolute paths we want
>> to try we just prepend to PATH with the correct extra precedence,
>> and then call execvp on the command we want?
>>
> 
> That's how the original git --exec-dir feature got implemented. There's 
> even a nifty function for it in git.c; prepend_to_path(). It's a 
> provably workable solution.

The reason that this was done is for the sake of shell scripts: They need to 
have the path that was finally decided as exec-path in $PATH.

But I can't think of any negative side effect if *all* exec-path candidates 
are in $PATH. It's important, though, that all paths are absolute because 
the tools chdir every now and then.

-- Hannes

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 22 Oct 2007, Shawn O. Pearce wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > Earlier, we tried to find the git commands in several possible exec
> > > dirs.  Now, if all of these failed, try to find the git command in
> > > PATH.
> > ...
> > > diff --git a/exec_cmd.c b/exec_cmd.c
> > > index 9b74ed2..70b84b0 100644
> > > --- a/exec_cmd.c
> > > +++ b/exec_cmd.c
> > > @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
> > >  	int i;
> > >  	const char *paths[] = { current_exec_path,
> > >  				getenv(EXEC_PATH_ENVIRONMENT),
> > > -				builtin_exec_path };
> > > +				builtin_exec_path,
> > > +				"" };
> > 
> > So if the user sets GIT_EXEC_PATH="" and exports it we'll search $PATH 
> > before the builtin exec path that Git was compiled with? Are we sure we 
> > want to do that?
> 
> I thought the proper way to unset EXEC_PATH was to "unset GIT_EXEC_PATH".  
> In that case, getenv(EXEC_PATH_ENVIRONMENT) returns NULL and we're fine, 
> no?

Sure.  But can't you also export an environment variable that is
set to the empty string?  At least on UNIX.  Windows thinks unset
and empty string are the same thing.

-- 
Shawn.

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Andreas Ericsson]

Johannes Sixt wrote:
> Andreas Ericsson schrieb:
>> Scott Parish wrote:
>>> I'm tempted to try a different approach. What if instead of looping
>>> and building up strings of all the different absolute paths we want
>>> to try we just prepend to PATH with the correct extra precedence,
>>> and then call execvp on the command we want?
>>>
>>
>> That's how the original git --exec-dir feature got implemented. 
>> There's even a nifty function for it in git.c; prepend_to_path(). It's 
>> a provably workable solution.
> 
> The reason that this was done is for the sake of shell scripts: They 
> need to have the path that was finally decided as exec-path in $PATH.
> 
> But I can't think of any negative side effect if *all* exec-path 
> candidates are in $PATH. It's important, though, that all paths are 
> absolute because the tools chdir every now and then.
> 

So long as they're added in "success:failed:failed" order, I don't see
any issues either. Assuming we stop prepending once we find something
that works, that should be a non-issue.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Schindelin]

Hi,

On Tue, 23 Oct 2007, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 22 Oct 2007, Shawn O. Pearce wrote:
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > > Earlier, we tried to find the git commands in several possible exec
> > > > dirs.  Now, if all of these failed, try to find the git command in
> > > > PATH.
> > > ...
> > > > diff --git a/exec_cmd.c b/exec_cmd.c
> > > > index 9b74ed2..70b84b0 100644
> > > > --- a/exec_cmd.c
> > > > +++ b/exec_cmd.c
> > > > @@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
> > > >  	int i;
> > > >  	const char *paths[] = { current_exec_path,
> > > >  				getenv(EXEC_PATH_ENVIRONMENT),
> > > > -				builtin_exec_path };
> > > > +				builtin_exec_path,
> > > > +				"" };
> > > 
> > > So if the user sets GIT_EXEC_PATH="" and exports it we'll search 
> > > $PATH before the builtin exec path that Git was compiled with? Are 
> > > we sure we want to do that?
> > 
> > I thought the proper way to unset EXEC_PATH was to "unset 
> > GIT_EXEC_PATH".  In that case, getenv(EXEC_PATH_ENVIRONMENT) returns 
> > NULL and we're fine, no?
> 
> Sure.  But can't you also export an environment variable that is set to 
> the empty string?  At least on UNIX.  Windows thinks unset and empty 
> string are the same thing.

Not here.  I just tried (with msysGit, of course).

Anyway, I like the other patch Scott sent much more than mine, which 
offloads the work to execvp().

Ciao,
Dscho

Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails.

[Johannes Sixt]

Andreas Ericsson schrieb:
> Johannes Sixt wrote:
>> But I can't think of any negative side effect if *all* exec-path 
>> candidates are in $PATH. It's important, though, that all paths are 
>> absolute because the tools chdir every now and then.
>>
> 
> So long as they're added in "success:failed:failed" order, I don't see
> any issues either. Assuming we stop prepending once we find something
> that works, that should be a non-issue.

No, the point is exactly to let execvp() do all the work and we don't care 
which of the paths is the "success". And I don't think that this has any 
negative side effects.

-- Hannes