Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s

Michal Ostrowski [off-list ref] writes:

Calls to git_setup_exec_path() are inserted on paths that will execute
other git programs. git_setup_exec_path() will ensure that the git
installation directories are in the path.

About fetch-clone.c (which is shared by fetch-pack and
clone-pack), it runs "git-index-pack" from finish_pack and
"git-unpack-objects" from unpack_pack, so spelling these exec
with execlp("git", "git", "index-pack", ...) might be cleaner,
since "git" is required to be in users' PATH even though git-*
may be moved out of the PATH in later versions of git.  I
dunno...

In send-pack.c, I wonder why you didn't do a setup_exec_path()
at the beginning of main() instead of having two calls close to
exec*() call site.

The same comment applies for run-command.c; you do it once for
each child, but calling it once at the beginning of receive-pack
would be good enough.  The same thing for daemon.c.

I suspect you are trying to limit the extent of damage, but I do
not think of a downside if we just call setup_exec_path() once
at the beginning of main().  $GIT_EXEC_PATH _could_ have a
private copy of broken "diff" to confuse diff-* family, but you
cannot say "git diff" in such a setup anyway because "git" does
the PATH prefixing already, so it would be a moot point.

Here is the list my "nm | grep ' exec[vlpe]*\($\|@@\)'" found
that use some variant of exec* family (except "git-diff-*"):

	clone-pack
        daemon
        fetch-pack
        merge-index
        peek-remote
        receive-pack
        send-pack
        shell
        ssh-fetch/ssh-pull
        ssh-upload/ssh-push
        upload-pack

I do not care too much about ssh-* commit walkers (users can say
e.g. GIT_SSH_PUSH themselves).

Anyway, thanks for starting this.  I need a bit more thought and
a bit of list discussion to convince myself this is a good
change.

On Mon, 2006-01-09 at 18:52 -0800, Junio C Hamano wrote:

Michal Ostrowski [off-list ref] writes:

quoted

Calls to git_setup_exec_path() are inserted on paths that will execute
other git programs. git_setup_exec_path() will ensure that the git
installation directories are in the path.

About fetch-clone.c (which is shared by fetch-pack and
clone-pack), it runs "git-index-pack" from finish_pack and
"git-unpack-objects" from unpack_pack, so spelling these exec
with execlp("git", "git", "index-pack", ...) might be cleaner,
since "git" is required to be in users' PATH even though git-*
may be moved out of the PATH in later versions of git.  I
dunno...

In send-pack.c, I wonder why you didn't do a setup_exec_path()
at the beginning of main() instead of having two calls close to
exec*() call site.

The same comment applies for run-command.c; you do it once for
each child, but calling it once at the beginning of receive-pack
would be good enough.  The same thing for daemon.c.

I suspect you are trying to limit the extent of damage, but I do
not think of a downside if we just call setup_exec_path() once
at the beginning of main().  $GIT_EXEC_PATH _could_ have a
private copy of broken "diff" to confuse diff-* family, but you
cannot say "git diff" in such a setup anyway because "git" does
the PATH prefixing already, so it would be a moot point.

I'm not actually happy with the idea of mucking around with PATH, even
within git.c.  Hence I tried to only change PATH if the code had already
committed to an exec.

An approach that I think is better is to require all exec's of git
programs from within git programs to use a specific git interface,
rather than letting each one set up it's own exec parameters.

Once you have that implemented, we can have a separate discussion of how
the executable is to be found; 
 - should we use PATH?
 - should we change PATH?
 - should we always exec using an absolute file name? (my preference)
 
If a user invokes /home/user/bin/git-foo, and git-foo wants to call
git-bar, is it legitimate for git-foo to call /usr/local/bin/git-bar, or
should it require /home/user/bin/git-bar?

Should the same rules be applied to the shell scripts? (In which case
we'd want to do something like s:git-:$(GIT_EXEC_PATH)/git-:g.)

Anyways, it may just be easier to show this in C (patch below).


-- 
Michal Ostrowski [off-list ref]

diff --git a/Makefile b/Makefile
index c9c15b5..db57858 100644
--- a/Makefile
+++ b/Makefile

@@ -173,7 +173,7 @@ DIFF_OBJS = \
 
 LIB_OBJS = \
 	blob.o commit.o connect.o count-delta.o csum-file.o \
-	date.o diff-delta.o entry.o ident.o index.o \
+	date.o diff-delta.o entry.o exec_cmd.o ident.o index.o \
 	object.o pack-check.o patch-delta.o path.o pkt-line.o \
 	quote.o read-cache.o refs.o run-command.o \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \

@@ -184,6 +184,16 @@ LIB_OBJS = \
 LIBS = $(LIB_FILE)
 LIBS += -lz
 
+
+# .exec_cmd.bindir stores $(bindir) used to compile exec_cmd.o
+# If it has changed, store the new value and force exec_cmd.o to be

rebuilt
+ifneq ($(shell cat .exec_cmd.bindir 2>/dev/null),$(bindir))
+.PHONY: exec_cmd.c
+$(shell echo $(bindir) > .exec_cmd.bindir)
+endif
+
+exec_cmd.o: CFLAGS+=-DGIT_EXEC_PATH=\"$(bindir)\"
+
 # Shell quote;
 # Result of this needs to be placed inside ''
 shq = $(subst ','\'',$(1))

diff --git a/daemon.c b/daemon.c
index 3bd1426..ab793bd 100644
--- a/daemon.c
+++ b/daemon.c

@@ -9,6 +9,7 @@
 #include <syslog.h>
 #include "pkt-line.h"
 #include "cache.h"
+#include "exec_cmd.h"
 
 static int log_syslog;
 static int verbose;

@@ -227,7 +228,7 @@ static int upload(char *dir)
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
 
 	/* git-upload-pack only ever reads stuff, so this is safe */
-	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf,

".", NULL);
+	exec_git_cmd("upload-pack", "--strict", timeout_buf, ".");
 	return -1;
 }
 

diff --git a/fetch-clone.c b/fetch-clone.c
index f46fe6e..5ae9bda 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c

@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "exec_cmd.h"
 #include <sys/wait.h>
 
 static int finish_pack(const char *pack_tmp_name, const char *me)

@@ -27,8 +28,7 @@ static int finish_pack(const char *pack_
 		dup2(pipe_fd[1], 1);
 		close(pipe_fd[0]);
 		close(pipe_fd[1]);
-		execlp("git-index-pack","git-index-pack",
-		       "-o", idx, pack_tmp_name, NULL);
+		exec_git_cmd("index-pack", "-o", idx, pack_tmp_name);
 		error("cannot exec git-index-pack <%s> <%s>",
 		      idx, pack_tmp_name);
 		exit(1);

@@ -105,8 +105,7 @@ int receive_unpack_pack(int fd[2], const
 		dup2(fd[0], 0);
 		close(fd[0]);
 		close(fd[1]);
-		execlp("git-unpack-objects", "git-unpack-objects",
-		       quiet ? "-q" : NULL, NULL);
+		exec_git_cmd("unpack-objects", quiet ? "-q" : NULL);
 		die("git-unpack-objects exec failed");
 	}
 	close(fd[0]);

diff --git a/git.c b/git.c
index 5e7da74..a45dc54 100644
--- a/git.c
+++ b/git.c

@@ -10,6 +10,7 @@
 #include <stdarg.h>
 #include <sys/ioctl.h>
 #include "git-compat-util.h"
+#include "exec_cmd.h"
 
 #ifndef PATH_MAX
 # define PATH_MAX 4096

@@ -192,25 +193,6 @@ static void cmd_usage(const char *exec_p
 	exit(1);
 }
 
-static void prepend_to_path(const char *dir, int len)
-{
-	char *path, *old_path = getenv("PATH");
-	int path_len = len;
-
-	if (!old_path)
-		old_path = "/usr/local/bin:/usr/bin:/bin";
-
-	path_len = len + strlen(old_path) + 1;
-
-	path = malloc(path_len + 1);
-
-	memcpy(path, dir, len);
-	path[len] = ':';
-	memcpy(path + len + 1, old_path, path_len - len);
-
-	setenv("PATH", path, 1);
-}
-
 static void show_man_page(char *git_cmd)
 {
 	char *page;

@@ -233,14 +215,10 @@ int main(int argc, char **argv, char **e
 {
 	char git_command[PATH_MAX + 1];
 	char wd[PATH_MAX + 1];
-	int i, len, show_help = 0;
-	char *exec_path = getenv("GIT_EXEC_PATH");
+	int i, show_help = 0;
 
 	getcwd(wd, PATH_MAX);
 
-	if (!exec_path)
-		exec_path = GIT_EXEC_PATH;
-
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
 

@@ -257,9 +235,9 @@ int main(int argc, char **argv, char **e
 		if (!strncmp(arg, "exec-path", 9)) {
 			arg += 9;
 			if (*arg == '=')
-				exec_path = arg + 1;
+				git_set_exec_path(arg + 1);
 			else {
-				puts(exec_path);
+				puts(git_exec_path());
 				exit(0);
 			}
 		}

@@ -275,48 +253,18 @@ int main(int argc, char **argv, char **e
 
 	if (i >= argc || show_help) {
 		if (i >= argc)
-			cmd_usage(exec_path, NULL);
+			cmd_usage(git_exec_path(), NULL);
 
 		show_man_page(argv[i]);
 	}
 
-	if (*exec_path != '/') {
-		if (!getcwd(git_command, sizeof(git_command))) {
-			fprintf(stderr,
-				"git: cannot determine current directory\n");
-			exit(1);
-		}
-		len = strlen(git_command);
-
-		/* Trivial cleanup */
-		while (!strncmp(exec_path, "./", 2)) {
-			exec_path += 2;
-			while (*exec_path == '/')
-				exec_path++;
-		}
-		snprintf(git_command + len, sizeof(git_command) - len,
-			 "/%s", exec_path);
-	}
-	else
-		strcpy(git_command, exec_path);
-	len = strlen(git_command);
-	prepend_to_path(git_command, len);
-
-	len += snprintf(git_command + len, sizeof(git_command) - len,
-			"/git-%s", argv[i]);
-	if (sizeof(git_command) <= len) {
-		fprintf(stderr, "git: command name given is too long.\n");
-		exit(1);
-	}
-
-	/* execve() can only ever return if it fails */
-	execve(git_command, &argv[i], envp);
+	execv_git_cmd(argv + i);
 
 	if (errno == ENOENT)
-		cmd_usage(exec_path, "'%s' is not a git-command", argv[i]);
+		cmd_usage(git_exec_path(), "'%s' is not a git-command",
+			  argv[i]);
 
 	fprintf(stderr, "Failed to run command '%s': %s\n",
 		git_command, strerror(errno));
-
 	return 1;
 }

diff --git a/receive-pack.c b/receive-pack.c
index f847ec2..8e78e32 100644
--- a/receive-pack.c
+++ b/receive-pack.c

@@ -257,7 +257,7 @@ static void read_head_info(void)
 
 static const char *unpack(int *error_code)
 {
-	int code = run_command(unpacker, NULL);
+	int code = run_command_v_opt(1, &unpacker, RUN_GIT_CMD);
 
 	*error_code = 0;
 	switch (code) {

diff --git a/run-command.c b/run-command.c
index 8bf5922..b3d287e 100644
--- a/run-command.c
+++ b/run-command.c

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include <sys/wait.h>
+#include "exec_cmd.h"
 
 int run_command_v_opt(int argc, char **argv, int flags)
 {

@@ -13,9 +14,13 @@ int run_command_v_opt(int argc, char **a
 			int fd = open("/dev/null", O_RDWR);
 			dup2(fd, 0);
 			dup2(fd, 1);
-			close(fd);			
+			close(fd);
+		}
+		if (flags & RUN_GIT_CMD) {
+			execv_git_cmd(argv);
+		} else {
+			execvp(argv[0], (char *const*) argv);
 		}
-		execvp(argv[0], (char *const*) argv);
 		die("exec %s failed.", argv[0]);
 	}
 	for (;;) {

diff --git a/run-command.h b/run-command.h
index 2469eea..ef3ee05 100644
--- a/run-command.h
+++ b/run-command.h

@@ -12,7 +12,7 @@ enum {
 };
 
 #define RUN_COMMAND_NO_STDIO 1
-
+#define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 int run_command_v_opt(int argc, char **argv, int opt);
 int run_command_v(int argc, char **argv);
 int run_command(const char *cmd, ...);

diff --git a/shell.c b/shell.c
index cd31618..0d4891f 100644
--- a/shell.c
+++ b/shell.c

@@ -12,7 +12,7 @@ static int do_generic_cmd(const char *me
 	my_argv[1] = arg;
 	my_argv[2] = NULL;
 
-	return execvp(me, (char**) my_argv);
+	return execv_git_cmd((char**) my_argv);
 }
 
 static struct commands {

diff --git a/upload-pack.c b/upload-pack.c
index 1834b6b..6602d68 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -4,6 +4,7 @@
 #include "tag.h"
 #include "object.h"
 #include "commit.h"
+#include "exec_cmd.h"
 
 static const char upload_pack_usage[] = "git-upload-pack [--strict]

[--timeout=nn] <dir>";
 

@@ -60,7 +61,7 @@ static void create_pack_file(void)
 		close(0);
 		close(fd[0]);
 		close(fd[1]);
-		*p++ = "git-rev-list";
+		*p++ = "rev-list";
 		*p++ = "--objects";
 		if (create_full_pack || MAX_NEEDS <= nr_needs)
 			*p++ = "--all";

@@ -79,13 +80,13 @@ static void create_pack_file(void)
 				buf += 41;
 			}
 		*p++ = NULL;
-		execvp("git-rev-list", argv);
+		execv_git_cmd(argv);
 		die("git-upload-pack: unable to exec git-rev-list");
 	}
 	dup2(fd[0], 0);
 	close(fd[0]);
 	close(fd[1]);
-	execlp("git-pack-objects", "git-pack-objects", "--stdout", NULL);
+	exec_git_cmd("pack-objects", "--stdout");
 	die("git-upload-pack: unable to exec git-pack-objects");
 }
 

Michal Ostrowski wrote:

On Mon, 2006-01-09 at 18:52 -0800, Junio C Hamano wrote:

quoted

About fetch-clone.c (which is shared by fetch-pack and
clone-pack), it runs "git-index-pack" from finish_pack and
"git-unpack-objects" from unpack_pack, so spelling these exec
with execlp("git", "git", "index-pack", ...) might be cleaner,
since "git" is required to be in users' PATH even though git-*
may be moved out of the PATH in later versions of git.  I
dunno...

In send-pack.c, I wonder why you didn't do a setup_exec_path()
at the beginning of main() instead of having two calls close to
exec*() call site.

The same comment applies for run-command.c; you do it once for
each child, but calling it once at the beginning of receive-pack
would be good enough.  The same thing for daemon.c.

I suspect you are trying to limit the extent of damage, but I do
not think of a downside if we just call setup_exec_path() once
at the beginning of main().  $GIT_EXEC_PATH _could_ have a
private copy of broken "diff" to confuse diff-* family, but you
cannot say "git diff" in such a setup anyway because "git" does
the PATH prefixing already, so it would be a moot point.

I'm not actually happy with the idea of mucking around with PATH, even
within git.c.  Hence I tried to only change PATH if the code had already
committed to an exec.

This is the case in the git potty already. git.c must prepend 
--exec-path to $PATH, or the whole idea of being able to move scripts 
out of the $PATH fails (at least it fails without changing quite a few 
of the scripts).

Since it's already in place in the potty and that's required to be in 
the $PATH, I think Junio's suggestion of running execlp("git", "git", 
...) is a good one. It will add one extra fork() and execve() for each 
clone/pull/push, but that isn't much of an issue, really.

An approach that I think is better is to require all exec's of git
programs from within git programs to use a specific git interface,
rather than letting each one set up it's own exec parameters.

A better idea would be to teach {send,upload}-pack about $GIT_EXEX_PATH 
and export it from your shells rc-file.

Once you have that implemented, we can have a separate discussion of how
the executable is to be found; 
 - should we use PATH?
 - should we change PATH?
 - should we always exec using an absolute file name? (my preference)
 
If a user invokes /home/user/bin/git-foo, and git-foo wants to call
git-bar, is it legitimate for git-foo to call /usr/local/bin/git-bar, or
should it require /home/user/bin/git-bar?

If a user invokes "/home/user/bin/git-foo" rather than 
"/home/user/bin/git foo" he/she will have to have the rest of the 
git-suite in the $PATH. Prepending whatever directory any git-* program 
happens to reside in to $PATH is not a good idea. Trying to execute 
programs residing in the same directory is an even worse.

Should the same rules be applied to the shell scripts? (In which case
we'd want to do something like s:git-:$(GIT_EXEC_PATH)/git-:g.)

All shell-scripts (that I'm aware of) are porcelainish. They should be 
run through the git potty and thus should always run the git-programs 
from the same release as they themselves were built from regardless of 
whether they call them through the potty or directly. This is both sane 
and simple. It was also one of the reasons that the 'git' program was 
implemented in C to begin with.

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

On Tue, 2006-01-10 at 16:01 +0100, Andreas Ericsson wrote:

This is the case in the git potty already. git.c must prepend 
--exec-path to $PATH, or the whole idea of being able to move scripts 
out of the $PATH fails (at least it fails without changing quite a few 
of the scripts).

One could make all the scripts depend on GIT_EXEC_PATH instead of PATH.
At build time one could generate wrapper functions in git-sh-setup:

function git-foo () {
	$(GIT_EXEC_PATH)/git-foo $*;
}

Presuming that all scripts include git-sh-setup, no other shell script
changes would be needed.

Since it's already in place in the potty and that's required to be in 
the $PATH, I think Junio's suggestion of running execlp("git", "git", 
...) is a good one. It will add one extra fork() and execve() for each 
clone/pull/push, but that isn't much of an issue, really.

The patch I posted most recently does something comparable; all exec's
by C git programs go through exec_git_cmd, which actually implements the
"git potty" logic (and git.c itself uses exec_git_cmd).  If there is to
be a consistent rule for how to exec a git program from a git C program,
I think that it's reasonable that there be an API to enforce it.  

Note that the creation and use of such a function simply means that we
hide the logic that handles PATH/GIT_EXEC_PATH; how git_exec_cmd()
actually calls execve() and how PATH and GIT_EXEC_PATH are used is a
separate issue.  When it comes to the former, I think it is best to have
all exec's of git programs go through an interface that imposes the same
PATH/GIT_EXEC_PATH logics.  As to the latter, my only concern is that we
should never do 'setenv("PATH",....)'.

quoted

An approach that I think is better is to require all exec's of git
programs from within git programs to use a specific git interface,
rather than letting each one set up it's own exec parameters.

A better idea would be to teach {send,upload}-pack about $GIT_EXEX_PATH 
and export it from your shells rc-file.

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

quoted

Once you have that implemented, we can have a separate discussion of how
the executable is to be found; 
 - should we use PATH?
 - should we change PATH?
 - should we always exec using an absolute file name? (my preference)
 
If a user invokes /home/user/bin/git-foo, and git-foo wants to call
git-bar, is it legitimate for git-foo to call /usr/local/bin/git-bar, or
should it require /home/user/bin/git-bar?

If a user invokes "/home/user/bin/git-foo" rather than 
"/home/user/bin/git foo" he/she will have to have the rest of the 
git-suite in the $PATH. Prepending whatever directory any git-* program 
happens to reside in to $PATH is not a good idea. 

Isn't this exactly what git.c is doing currently via prepend_to_path()?

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

Suppose git is installed in /usr/bin, where a "diff" resided.
I've got my own version of "diff" in /home/user/bin.
PATH=/home/user/bin:/usr/bin.

If git now tries to execute "diff", after having run
prepend_to_path(), /usr/bin/diff gets executed, not /home/user/bin/diff.
The user has set up PATH to ensure that /home/user/bin/diff is the diff,
but by mucking with PATH we subvert their intentions.

This is why in my original patch I tried to put the manipulations to
PATH only in points where I knew that it would only affect the exec'ing
of a git program.

quoted

Should the same rules be applied to the shell scripts? (In which case
we'd want to do something like s:git-:$(GIT_EXEC_PATH)/git-:g.)

All shell-scripts (that I'm aware of) are porcelainish. They should be 
run through the git potty and thus should always run the git-programs 
from the same release as they themselves were built from regardless of 
whether they call them through the potty or directly. This is both sane 
and simple. It was also one of the reasons that the 'git' program was 
implemented in C to begin with.

As described above, we can have shells scripts "always run the
git-programs from the same release as they themselves were built from"
without ever changing PATH.


-- 
Michal Ostrowski [off-list ref]

Michal Ostrowski wrote:

On Tue, 2006-01-10 at 16:01 +0100, Andreas Ericsson wrote:

quoted

This is the case in the git potty already. git.c must prepend 
--exec-path to $PATH, or the whole idea of being able to move scripts 
out of the $PATH fails (at least it fails without changing quite a few 
of the scripts).

One could make all the scripts depend on GIT_EXEC_PATH instead of PATH.
At build time one could generate wrapper functions in git-sh-setup:

function git-foo () {
	$(GIT_EXEC_PATH)/git-foo $*;
}

Presuming that all scripts include git-sh-setup, no other shell script
changes would be needed.

Yuck, for two reasons.

* Not all scripts include git-sh-setup, and for good reasons. If this is 
what you intend please make sure you don't break anything in the process.

* This will spawn a sub-shell for each git-foo process called. Shells 
are way more expensive than the git potty, so the performance hit in 
iterations might be considerable. Think StGit and Cogito as well.


On a side-note, $* will break quoting (you should use "$@" instead, with 
double-quotes attached), and

	$(GIT_EXEC_PATH)/git-foo $*

will try to execute GIT_EXEC_PATH and prepend its output to the rest of 
the command, which is quite obviously wrong.

quoted

Since it's already in place in the potty and that's required to be in 
the $PATH, I think Junio's suggestion of running execlp("git", "git", 
...) is a good one. It will add one extra fork() and execve() for each 
clone/pull/push, but that isn't much of an issue, really.

The patch I posted most recently does something comparable; all exec's
by C git programs go through exec_git_cmd, which actually implements the
"git potty" logic (and git.c itself uses exec_git_cmd).  If there is to
be a consistent rule for how to exec a git program from a git C program,
I think that it's reasonable that there be an API to enforce it.  

True. Perhaps I misread your patch or your reasoning.

Note that the creation and use of such a function simply means that we
hide the logic that handles PATH/GIT_EXEC_PATH; how git_exec_cmd()
actually calls execve() and how PATH and GIT_EXEC_PATH are used is a
separate issue.  When it comes to the former, I think it is best to have
all exec's of git programs go through an interface that imposes the same
PATH/GIT_EXEC_PATH logics.  As to the latter, my only concern is that we
should never do 'setenv("PATH",....)'.

setenv("PATH", ..) is way preferrable over the git-setup.sh hackery 
suggested above, so long as it's only ever done in the git potty. That's 
what the potty is there for, after all.

quoted

quoted

An approach that I think is better is to require all exec's of git
programs from within git programs to use a specific git interface,
rather than letting each one set up it's own exec parameters.

A better idea would be to teach {send,upload}-pack about $GIT_EXEX_PATH 
and export it from your shells rc-file.

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

It does for me and everybody else. $HOME/.bashrc is read even for 
non-interactive shells. $HOME/.bash_profile isn't. If you're using 
git-shell you're in to a whole different situation, but you didn't say 
so so I don't think you are.

quoted

If a user invokes "/home/user/bin/git-foo" rather than 
"/home/user/bin/git foo" he/she will have to have the rest of the 
git-suite in the $PATH. Prepending whatever directory any git-* program 
happens to reside in to $PATH is not a good idea. 

Isn't this exactly what git.c is doing currently via prepend_to_path()?

No. git prepends whatever was passed in --exec-path=/some/path, what's 
found in $GIT_EXEC_PATH or the GIT_EXEC_PATH pre-processor macro set at 
compile-time, in that order of preference. There's a very big difference.

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

Suppose git is installed in /usr/bin, where a "diff" resided.
I've got my own version of "diff" in /home/user/bin.
PATH=/home/user/bin:/usr/bin.

If git now tries to execute "diff", after having run
prepend_to_path(), /usr/bin/diff gets executed, not /home/user/bin/diff.
The user has set up PATH to ensure that /home/user/bin/diff is the diff,
but by mucking with PATH we subvert their intentions.

Good point. Perhaps we should only prepend to path when the directory 
isn't already in $PATH, or append rather than prepend.

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

Michal Ostrowski [off-list ref] writes:

One could make all the scripts depend on GIT_EXEC_PATH instead of PATH.
At build time one could generate wrapper functions in git-sh-setup:

function git-foo () {
	$(GIT_EXEC_PATH)/git-foo $*;
}

Presuming that all scripts include git-sh-setup, no other shell script
changes would be needed.

Is "git-foo" a valid name to define shell function as?

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

Not any rc, or are you bitten by bash/ssh misfeature that
noninteractive sessions do not start with .bash_profile?

quoted

quoted

Once you have that implemented, we can have a separate discussion of how
the executable is to be found; 
 - should we use PATH?
 - should we change PATH?
 - should we always exec using an absolute file name? (my preference)

The goal here is to make sure we exec the program from the same
release (unless user overrides it with GIT_EXEC_PATH to say "I
want to try 0.99.9k, not the latest one"), but how?  The last
one feels the most correct way if done right.

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

This is a valid concern.

Hi,

On Tue, 10 Jan 2006, Junio C Hamano wrote:

Michal Ostrowski [off-list ref] writes:

quoted

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

This is a valid concern.

Why? If what is prepended to PATH only contains git programs?

Ciao,
Dscho

Andreas Ericsson, Tue, Jan 10, 2006 20:13:34 +0100:

quoted

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

It does for me and everybody else. $HOME/.bashrc is read even for 
non-interactive shells. ...

Not really:

$ man bash

       When bash is started non-interactively, to  run  a  shell  script,  for
       example, it looks for the variable BASH_ENV in the environment, expands
       its value if it appears there, and uses the expanded value as the  name
       of  a  file to read and execute.  Bash behaves as if the following com-
       mand were executed:
              if [ -n "$BASH_ENV" ]; then . "$BASH_ENV"; fi
       but the value of the PATH variable is not used to search for  the  file
       name.

$ ssh host2 strace -e open bash -c :
open("/etc/ld.so.cache", O_RDONLY)      = 3
open("/lib/libncurses.so.5", O_RDONLY)  = 3
open("/lib/tls/libdl.so.2", O_RDONLY)   = 3
open("/lib/tls/libc.so.6", O_RDONLY)    = 3
open("/dev/tty", O_RDWR|O_NONBLOCK|O_LARGEFILE) = -1 ENXIO (No such device or address)
open("/etc/mtab", O_RDONLY)             = 3
open("/proc/meminfo", O_RDONLY)         = 3
open("/proc/sys/kernel/ngroups_max", O_RDONLY) = 3

On Tue, 2006-01-10 at 20:55 +0100, Johannes Schindelin wrote:

Hi,

On Tue, 10 Jan 2006, Junio C Hamano wrote:

quoted

Michal Ostrowski [off-list ref] writes:

quoted

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

This is a valid concern.

Why? If what is prepended to PATH only contains git programs?

If git is installed with prefix=/usr, then that won't be the case.


-- 
Michal Ostrowski [off-list ref]

On Tue, 2006-01-10 at 21:15 +0100, Alex Riesen wrote:

Andreas Ericsson, Tue, Jan 10, 2006 20:13:34 +0100:

quoted

quoted

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

It does for me and everybody else. $HOME/.bashrc is read even for 
non-interactive shells. ...

On the system I'm dealing with, ssh does not invoke bash if a command is
specified.

If I do ssh user@system /home/user/bin/foo,  ssh performs an exec
of /home/user/bin/foo; my shell is never invoked.  This isn't up to me;
I don't have root on this system.


-- 
Michal Ostrowski [off-list ref]

Hi,

On Tue, 10 Jan 2006, Michal Ostrowski wrote:

On Tue, 2006-01-10 at 20:55 +0100, Johannes Schindelin wrote:

quoted

Hi,

On Tue, 10 Jan 2006, Junio C Hamano wrote:

quoted

Michal Ostrowski [off-list ref] writes:

quoted

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

This is a valid concern.

Why? If what is prepended to PATH only contains git programs?

If git is installed with prefix=/usr, then that won't be the case.

Okay, so here we have the problem: Two completely different setups. One 
into a standard location on the PATH (which used to be the default), the 
other with a libexec/ directory (which some want in the future). And a git 
wrapper which makes no difference between both.

Wouldn't it make much more sense to have a switch in the Makefile, which 
says *if* we have a libexec/ directory? This switch would decide if we 
ever prepend the path with the libexec/ directory or not.

Ciao,
Dscho

Johannes Schindelin wrote:

quoted

quoted

quoted

quoted

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

This is a valid concern.

Why? If what is prepended to PATH only contains git programs?

If git is installed with prefix=/usr, then that won't be the case.

Okay, so here we have the problem: Two completely different setups. One 
into a standard location on the PATH (which used to be the default), the 
other with a libexec/ directory (which some want in the future). And a git 
wrapper which makes no difference between both.

Wouldn't it make much more sense to have a switch in the Makefile, which 
says *if* we have a libexec/ directory?

No, it wouldn't, because then we can't use a different release of the 
git-tools without re-compiling the potty.

void prepend_to_path(old_path, to_prep)
{
	if (strstr(old_path, to_prep))
		return;

	really_prepend_to_path(old_path, to_prep);
}

would work just fine though.

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