threads / patch / 10390

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

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

## tl;dr

17 messages between Oct 20, 2007 and Oct 23, 2007. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Scott R Parish· Oct 20, 2007, 08:12 UTC · lore

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>
Show 34 quoted lines
> 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");
Show 54 quoted lines
> > +                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,
Show 15 quoted lines
> -                              "/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
>
>
Johannes Schindelin· Oct 20, 2007, 20:25 UTC · re: Scott R Parish · lore
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:
Show 5 quoted lines
> 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

Scott Parish· Oct 20, 2007, 20:57 UTC · re: Scott R Parish · lore

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:
Show 127 quoted lines
> 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/
Johannes Schindelin· Oct 20, 2007, 22:00 UTC · re: Scott Parish · lore

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

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(-)
Show changes to exec_cmd.c +8 −4
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 O. Pearce· Oct 21, 2007, 02:36 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 9 quoted lines
> 	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.
 
Show 43 quoted lines
>  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.
Johannes Schindelin· Oct 21, 2007, 21:59 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to exec_cmd.c +12 −5
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
Shawn O. Pearce· Oct 22, 2007, 04:21 UTC · re: Johannes Schindelin · lore

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

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.
...
Show 11 quoted lines
> 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.
Johannes Schindelin· Oct 22, 2007, 10:35 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Mon, 22 Oct 2007, Shawn O. Pearce wrote:
Show 20 quoted lines
> 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

Shawn O. Pearce· Oct 23, 2007, 04:34 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 25 quoted lines
> 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.
Johannes Schindelin· Oct 23, 2007, 11:12 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Tue, 23 Oct 2007, Shawn O. Pearce wrote:
Show 30 quoted lines
> 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

Scott Parish· Oct 22, 2007, 14:36 UTC · re: Johannes Schindelin · lore

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

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/
Andreas Ericsson· Oct 22, 2007, 15:19 UTC · re: Scott Parish · lore

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

Scott Parish wrote:
Show 11 quoted lines
> 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
Johannes Sixt· Oct 22, 2007, 15:36 UTC · re: Andreas Ericsson · lore

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

Andreas Ericsson schrieb:
Show 10 quoted lines
> 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
Andreas Ericsson· Oct 23, 2007, 11:12 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
Show 19 quoted lines
> 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
Johannes Sixt· Oct 23, 2007, 15:29 UTC · re: Andreas Ericsson · lore

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

Andreas Ericsson schrieb:
Show 9 quoted lines
> 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
Scott Parish· Oct 21, 2007, 18:21 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to exec_cmd.c +10 −6
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
Johannes Schindelin· Oct 21, 2007, 22:02 UTC · re: Scott Parish · lore

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

Hi,
On Sun, 21 Oct 2007, Scott Parish wrote:
Show 12 quoted lines
> 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

← back to recent threads