# Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH) 17 messages from 2007-10-20 to 2007-10-23. Participants: Scott R Parish, Johannes Schindelin, Scott Parish, Shawn O. Pearce, Andreas Ericsson, Johannes Sixt. Thread: https://gitlist.dev/t/10390 ## Scott R Parish, 2007-10-20 08:12 Subject: Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH) Message-ID: <1192867937.v2.fusewebmail-240137@f> URL: https://gitlist.dev/e/1192867937.v2.fusewebmail-240137%40f ``` 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" > 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 > > ``` ## Johannes Schindelin, 2007-10-20 20:25 Subject: Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH) Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710202124050.25221%40racer.site In-Reply-To: <1192867937.v2.fusewebmail-240137@f> ``` 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 ``` ## Scott Parish, 2007-10-20 20:57 Subject: Re: [PATCH] When exec'ing sub-commands, fall back on execvp (thePATH) Message-ID: <20071020205721.GA16291@srparish.net> URL: https://gitlist.dev/e/20071020205721.GA16291%40srparish.net In-Reply-To: <1192867937.v2.fusewebmail-240137@f> ``` 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" > > > 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, 2007-10-20 22:00 Subject: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710202258440.25221%40racer.site In-Reply-To: <20071020205721.GA16291@srparish.net> ``` 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 --- 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 ``` ## Shawn O. Pearce, 2007-10-21 02:36 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <20071021023614.GB14735@spearce.org> URL: https://gitlist.dev/e/20071021023614.GB14735%40spearce.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Scott Parish, 2007-10-21 18:21 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <20071021182150.GG16291@srparish.net> URL: https://gitlist.dev/e/20071021182150.GG16291%40srparish.net In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2007-10-21 21:59 Subject: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710212256270.25221%40racer.site In-Reply-To: <20071021023614.GB14735@spearce.org> ``` 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 --- On Sat, 20 Oct 2007, Shawn O. Pearce wrote: > Johannes Schindelin 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 ``` ## Johannes Schindelin, 2007-10-21 22:02 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710212301490.25221%40racer.site In-Reply-To: <20071021182150.GG16291@srparish.net> ``` 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 ``` ## Shawn O. Pearce, 2007-10-22 04:21 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <20071022042110.GJ14735@spearce.org> URL: https://gitlist.dev/e/20071022042110.GJ14735%40spearce.org In-Reply-To: ``` 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. ... > 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, 2007-10-22 10:35 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710221135100.25221%40racer.site In-Reply-To: <20071022042110.GJ14735@spearce.org> ``` Hi, On Mon, 22 Oct 2007, Shawn O. Pearce wrote: > 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. > ... > > 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 ``` ## Scott Parish, 2007-10-22 14:36 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <20071022143637.GP16291@srparish.net> URL: https://gitlist.dev/e/20071022143637.GP16291%40srparish.net In-Reply-To: ``` 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, 2007-10-22 15:19 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <471CBF88.6020300@op5.se> URL: https://gitlist.dev/e/471CBF88.6020300%40op5.se In-Reply-To: <20071022143637.GP16291@srparish.net> ``` 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 ``` ## Johannes Sixt, 2007-10-22 15:36 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <471CC380.5030603@viscovery.net> URL: https://gitlist.dev/e/471CC380.5030603%40viscovery.net In-Reply-To: <471CBF88.6020300@op5.se> ``` 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 ``` ## Shawn O. Pearce, 2007-10-23 04:34 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <20071023043405.GA14735@spearce.org> URL: https://gitlist.dev/e/20071023043405.GA14735%40spearce.org In-Reply-To: ``` Johannes Schindelin wrote: > On Mon, 22 Oct 2007, Shawn O. Pearce wrote: > > 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. > > ... > > > 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. ``` ## Andreas Ericsson, 2007-10-23 11:12 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <471DD703.70608@op5.se> URL: https://gitlist.dev/e/471DD703.70608%40op5.se In-Reply-To: <471CC380.5030603@viscovery.net> ``` 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 ``` ## Johannes Schindelin, 2007-10-23 11:12 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710231209240.25221%40racer.site In-Reply-To: <20071023043405.GA14735@spearce.org> ``` Hi, On Tue, 23 Oct 2007, Shawn O. Pearce wrote: > Johannes Schindelin wrote: > > On Mon, 22 Oct 2007, Shawn O. Pearce wrote: > > > 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. > > > ... > > > > 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 ``` ## Johannes Sixt, 2007-10-23 15:29 Subject: Re: [PATCH] execv_git_cmd(): also try PATH if everything else fails. Message-ID: <471E136D.4010905@viscovery.net> URL: https://gitlist.dev/e/471E136D.4010905%40viscovery.net In-Reply-To: <471DD703.70608@op5.se> ``` 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 ```