Re: [PATCH] use only the PATH for exec'ing git commands

Re: [PATCH] use only the PATH for exec'ing git commands

[Scott R Parish]

> Alex Riesen, Mon, Oct 22, 2007 12:01
> Scott R Parish, Mon, Oct 22, 2007 19:01:48 +0200:
> > +                strbuf_addch(out, ':');
>
> Shouldn't it break MingW32 native port?

What can i do here to better accommodate MingW32? You're
right, just because the original code did it this way
isn't a good excuse for me not to do it better.

sRp

Re: [PATCH] use only the PATH for exec'ing git commands

[Alex Riesen]

Scott R Parish, Tue, Oct 23, 2007 00:12:02 +0200:
> > Alex Riesen, Mon, Oct 22, 2007 12:01
> > Scott R Parish, Mon, Oct 22, 2007 19:01:48 +0200:
> > > +                strbuf_addch(out, ':');
> >
> > Shouldn't it break MingW32 native port?
> 
> What can i do here to better accommodate MingW32? You're
> right, just because the original code did it this way
> isn't a good excuse for me not to do it better.

someone here mentioned "higher abstractions". Maybe he meant moving
the routines operating on path names and path lists out of the generic
code into platform specific (like compat/). Maybe he was even going to
do that himself, but I could be mistaken

Re: [PATCH] use only the PATH for exec'ing git commands

[Johannes Schindelin]

Hi,

On Tue, 23 Oct 2007, Alex Riesen wrote:

> Scott R Parish, Tue, Oct 23, 2007 00:12:02 +0200:
> > > Alex Riesen, Mon, Oct 22, 2007 12:01
> > > Scott R Parish, Mon, Oct 22, 2007 19:01:48 +0200:
> > > > +                strbuf_addch(out, ':');
> > >
> > > Shouldn't it break MingW32 native port?
> > 
> > What can i do here to better accommodate MingW32? You're right, just 
> > because the original code did it this way isn't a good excuse for me 
> > not to do it better.
> 
> someone here mentioned "higher abstractions".

That someone may well have mentioned that term, but what he meant was 
special case those code paths which have to be treated differently for 
Windows, and Windows alone.

(Yeah, I know, a response to that was that Nokia phones have drive 
letters, too, but that was an invalid objection to that particular 
statement, since Nokia phones expect the full path at any time, so I did 
not even bother answering to that mail.  Well, in a way, I guess I just 
did.)

All this boils down to adding code dedicated to Windows.  And I am a huge 
fan of the principle that you should cross bridges when you get to them, 
and not before (i.e. avoid over-engineering).

So my vote is: get it in as-is, us msysGit lot will cope with it in one 
way or another.  (We already have at least one other place where ":" is 
used, which we have to cope with, in shell.c:do_cvs_command().)

Ciao,
Dscho

Re: [PATCH] use only the PATH for exec'ing git commands

[Johannes Sixt]

Scott R Parish schrieb:
>> Alex Riesen, Mon, Oct 22, 2007 12:01
>> Scott R Parish, Mon, Oct 22, 2007 19:01:48 +0200:
>>> +                strbuf_addch(out, ':');
>> Shouldn't it break MingW32 native port?
> 
> What can i do here to better accommodate MingW32? You're
> right, just because the original code did it this way
> isn't a good excuse for me not to do it better.

Don't bother with it right now. GIT currently does not have MinGW specific 
code, yet.

-- Hannes

Re: [PATCH] use only the PATH for exec'ing git commands

[Scott R Parish]

> Alex Reisen, Mon, Oct 22, 2007 12:01:
> > +                strbuf_addch(out, ':');
>
> Shouldn't it break MingW32 native port?
>

<snip>

> > +        if (old_path)
> > +                strbuf_addstr(&amp;new_path, old_path);
> > +        else
> > +                strbuf_addstr(&amp;new_path,
"/usr/local/bin:/usr/bin:/bin");
>
> the default PATH is platform-dependent. Git is multi-platform.
> You should consider putting the path list somewhere in Makefile,
> config.mak or configure.
>

The original code was already doing both of these things (see git.c's
prepend_to_path())

sRp

Re: [PATCH] use only the PATH for exec'ing git commands

[Scott R Parish]

> Alex Reisen, Mon, Oct 22, 2007 12:01:
> > +                strbuf_addch(out, ':');
>
> Shouldn't it break MingW32 native port?
>

<snip>

> > +        if (old_path)
> > +                strbuf_addstr(&amp;new_path, old_path);
> > +        else
> > +                strbuf_addstr(&amp;new_path,
"/usr/local/bin:/usr/bin:/bin");
>
> the default PATH is platform-dependent. Git is multi-platform.
> You should consider putting the path list somewhere in Makefile,
> config.mak or configure.
>

The original code was already doing both of these things (see git.c's
prepend_to_path())

sRp

Re: [PATCH] use only the PATH for exec'ing git commands

[Alex Riesen]

Scott R Parish, Mon, Oct 22, 2007 21:59:01 +0200:
> > > +        if (old_path)
> > > +                strbuf_addstr(&new_path, old_path);
> > > +        else
> > > +                strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
> >
> > the default PATH is platform-dependent. Git is multi-platform.
> > You should consider putting the path list somewhere in Makefile,
> > config.mak or configure.
> >
> 
> The original code was already doing both of these things (see git.c's
> prepend_to_path())

Well, would be nice if your code was better in this respect.

Anyway, I suspect the mingw people will trash the code anyway sometime
(or not, which is just as well - it is a rare case).

[PATCH] use only the PATH for exec'ing git commands

[Scott R Parish]

We need to correctly set up PATH for non-c based git commands. Since we
already do this, we can just use that PATH and execvp, instead of looping
over the paths with execve.

This patch adds a setup_path() function to exec_cmd.c, which sets
the PATH order correctly for our search order. execv_git_cmd() is
stripped down to setting up argv and calling execvp(). git.c's main()
only only needs to call setup_path().

Signed-off-by: Scott R Parish <srp@srparish.net>
---
 exec_cmd.c |  122 ++++++++++++++++++++++++++----------------------------------
 exec_cmd.h |    1 +
 git.c      |   43 +++------------------
 3 files changed, 61 insertions(+), 105 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 8b681d0..b154c24 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -29,85 +29,69 @@ const char *git_exec_path(void)
 	return builtin_exec_path;
 }
 
+static void add_path(struct strbuf *out, const char *path)
+{
+	if (path && strlen(path)) {
+		if (is_absolute_path(path))
+			strbuf_addstr(out, path);
+		else
+			strbuf_addstr(out, make_absolute_path(path));
+
+		strbuf_addch(out, ':');
+	}
+}
+
+void setup_path(const char *cmd_path)
+{
+	const char *old_path = getenv("PATH");
+	struct strbuf new_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, cmd_path);
+		
+	if (old_path)
+		strbuf_addstr(&new_path, old_path);
+	else 
+		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+
+	setenv("PATH", new_path.buf, 1);
+
+	strbuf_release(&new_path);
+}
+
 
 int execv_git_cmd(const char **argv)
 {
-	char git_command[PATH_MAX + 1];
-	int i;
-	const char *paths[] = { argv_exec_path,
-				getenv(EXEC_PATH_ENVIRONMENT),
-				builtin_exec_path };
-
-	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;
-
-		if (*exec_dir != '/') {
-			if (!getcwd(git_command, sizeof(git_command))) {
-				fprintf(stderr, "git: cannot determine "
-					"current directory: %s\n",
-					strerror(errno));
-				break;
-			}
-			len = strlen(git_command);
-
-			/* Trivial cleanup */
-			while (!prefixcmp(exec_dir, "./")) {
-				exec_dir += 2;
-				while (*exec_dir == '/')
-					exec_dir++;
-			}
-
-			rc = snprintf(git_command + len,
-				      sizeof(git_command) - len, "/%s",
-				      exec_dir);
-			if (rc < 0 || rc >= sizeof(git_command) - len) {
-				fprintf(stderr, "git: command name given "
-					"is too long.\n");
-				break;
-			}
-		} else {
-			if (strlen(exec_dir) + 1 > sizeof(git_command)) {
-				fprintf(stderr, "git: command name given "
-					"is too long.\n");
-				break;
-			}
-			strcpy(git_command, exec_dir);
-		}
-
-		len = strlen(git_command);
-		rc = snprintf(git_command + len, sizeof(git_command) - len,
-			      "/git-%s", argv[0]);
-		if (rc < 0 || rc >= sizeof(git_command) - len) {
-			fprintf(stderr,
-				"git: command name given is too long.\n");
-			break;
-		}
+	struct strbuf cmd;
+	const char *tmp;
 
-		/* argv[0] must be the git command, but the argv array
-		 * belongs to the caller, and my be reused in
-		 * subsequent loop iterations. Save argv[0] and
-		 * restore it on error.
-		 */
+	strbuf_init(&cmd, 0);
+	strbuf_addf(&cmd, "git-%s", argv[0]);
 
-		tmp = argv[0];
-		argv[0] = git_command;
+	/* argv[0] must be the git command, but the argv array
+	 * belongs to the caller, and my be reused in
+	 * subsequent loop iterations. Save argv[0] and
+	 * restore it on error.
+	 */
+	tmp = argv[0];
+	argv[0] = cmd.buf;
 
-		trace_argv_printf(argv, -1, "trace: exec:");
+	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(cmd.buf, (char **)argv);
 
-		trace_printf("trace: exec failed: %s\n", strerror(errno));
+	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-		argv[0] = tmp;
-	}
-	return -1;
+	argv[0] = tmp;
 
+	strbuf_release(&cmd);
+
+	return -1;
 }
 
 
diff --git a/exec_cmd.h b/exec_cmd.h
index da99287..a892355 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,6 +3,7 @@
 
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char* git_exec_path(void);
+extern void setup_path(const char *);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern int execl_git_cmd(const char *cmd, ...);
 
diff --git a/git.c b/git.c
index f659338..a639e42 100644
--- a/git.c
+++ b/git.c
@@ -6,28 +6,6 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
-static void prepend_to_path(const char *dir, int len)
-{
-	const char *old_path = getenv("PATH");
-	char *path;
-	int path_len = len;
-
-	if (!old_path)
-		old_path = "/usr/local/bin:/usr/bin:/bin";
-
-	path_len = len + strlen(old_path) + 1;
-
-	path = xmalloc(path_len + 1);
-
-	memcpy(path, dir, len);
-	path[len] = ':';
-	memcpy(path + len + 1, old_path, path_len - len);
-
-	setenv("PATH", path, 1);
-
-	free(path);
-}
-
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -403,7 +381,7 @@ int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
 	char *slash = strrchr(cmd, '/');
-	const char *exec_path = NULL;
+	const char *cmd_path = NULL;
 	int done_alias = 0;
 
 	/*
@@ -413,10 +391,7 @@ int main(int argc, const char **argv)
 	 */
 	if (slash) {
 		*slash++ = 0;
-		if (*cmd == '/')
-			exec_path = cmd;
-		else
-			exec_path = xstrdup(make_absolute_path(cmd));
+		cmd_path = cmd;
 		cmd = slash;
 	}
 
@@ -451,16 +426,12 @@ int main(int argc, const char **argv)
 	cmd = argv[0];
 
 	/*
-	 * We execute external git command via execv_git_cmd(),
-	 * which looks at "--exec-path" option, GIT_EXEC_PATH
-	 * environment, and $(gitexecdir) in Makefile while built,
-	 * in this order.  For scripted commands, we prepend
-	 * the value of the exec_path variable to the PATH.
+	 * We use PATH to find git commands, but we prepend some higher
+	 * precidence paths: the "--exec-path" option, the GIT_EXEC_PATH
+	 * environment, and the $(gitexecdir) from the Makefile at build
+	 * time.
 	 */
-	if (exec_path)
-		prepend_to_path(exec_path, strlen(exec_path));
-	exec_path = git_exec_path();
-	prepend_to_path(exec_path, strlen(exec_path));
+	setup_path(cmd_path);
 
 	while (1) {
 		/* See if it's an internal command */
-- 
gitgui.0.8.4.11176.gd9205-dirty

Re: [PATCH] use only the PATH for exec'ing git commands

[Johannes Schindelin]

Hi,

On Mon, 22 Oct 2007, Scott R Parish wrote:

>  3 files changed, 61 insertions(+), 105 deletions(-)

Nice.

Ciao,
Dscho

Re: [PATCH] use only the PATH for exec'ing git commands

[Alex Riesen]

Scott R Parish, Mon, Oct 22, 2007 19:01:48 +0200:
> +static void add_path(struct strbuf *out, const char *path)
> +{
> +	if (path && strlen(path)) {
> +		if (is_absolute_path(path))
> +			strbuf_addstr(out, path);
> +		else
> +			strbuf_addstr(out, make_absolute_path(path));
> +
> +		strbuf_addch(out, ':');

Shouldn't it break MingW32 native port?

> +	}
> +}
> +
> +void setup_path(const char *cmd_path)
> +{
> +	const char *old_path = getenv("PATH");
> +	struct strbuf new_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, cmd_path);
> +		

trailing space

> +	if (old_path)
> +		strbuf_addstr(&new_path, old_path);
> +	else 
> +		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");

the default PATH is platform-dependent. Git is multi-platform.
You should consider putting the path list somewhere in Makefile,
config.mak or configure.

> +
> +	setenv("PATH", new_path.buf, 1);

Re: [PATCH] use only the PATH for exec'ing git commands

[Eric Merritt]

> the default PATH is platform-dependent. Git is multi-platform.
> You should consider putting the path list somewhere in Makefile,
> config.mak or configure.

This static configuration that you describe is one of the things this
patch is designed to strip out. Compile time configuration breaks down
completly if you don't deploy to the path defined when the system was
compiled. Thats a problem.