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

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

[Michal Ostrowski]

I've been trying to setup a git repository for access via ssh, on a
system where git is to be installed under ~user/bin, and ~user/bin is
not included in the PATH that sshd provides.

Consequently I need to execute something like:

git-clone -u /home/user/bin/git-upload-pack \
ssh://user@system/home/user/repo.git repo

When git-upload-pack executes on the remote system, it tries to execute
git-rev-list and fails, since /home/user/bin is not in the path.

The following patches handle this issue by appending GIT_EXEC_PATH to
PATH prior to exec calls (via git_setup_exec_path()).  Also, the value
of ${bindir} at build time is encoded and used as a default value for
"GIT_EXEC_PATH", if the latter is not present. 


-- 
Michal Ostrowski <mostrows@watson.ibm.com>

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

[Michal Ostrowski]

If git-upload-pack is invoked by ssh, it may have been invoked because
ssh was explicitly told which program to execute on the remote end
(i.e. --exec had been used with git-clone-pack).  In this case, the
git suite may not be in the PATH, and so subsequent exec's by
git-upload-pack (i.e. git-rev-list, git-pack-objects) will fail.

These changes provide for ${bindir} to be stored at compile time in
environment.c. git_setup_exec_path() is implemented; this function
will append GIT_EXEC_PATH or the saved ${bindir} to PATH.

Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 Makefile      |    7 +++++++
 cache.h       |    1 +
 environment.c |   29 +++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

b24fde016c2d7382016b80bc0b9a011db3413bb3
diff --git a/Makefile b/Makefile
index c9c15b5..ffd2a68 100644
--- a/Makefile
+++ b/Makefile
@@ -437,6 +437,13 @@ init-db.o: init-db.c
 	$(CC) -c $(ALL_CFLAGS) \
 		-DDEFAULT_GIT_TEMPLATE_DIR=$(call shellquote,"$(template_dir)") $*.c
 
+# Recompile environment.o if GIT_EXEC_PATH changes
+.environment.GIT_EXEC_PATH:
+	@(test -e $@ && grep -h -e '^$(bindir)$$' $@) || echo $(bindir) > $@
+environment.o: .environment.GIT_EXEC_PATH
+environment.o: CFLAGS+= -DGIT_EXEC_PATH=\"$(bindir)\"
+
+
 $(LIB_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H)
 $(DIFF_OBJS): diffcore.h
diff --git a/cache.h b/cache.h
index 29c9e81..d73071e 100644
--- a/cache.h
+++ b/cache.h
@@ -244,6 +244,7 @@ unsigned long approxidate(const char *);
 extern int setup_ident(void);
 extern const char *git_author_info(void);
 extern const char *git_committer_info(void);
+extern void git_setup_exec_path(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/environment.c b/environment.c
index 0596fc6..4dc0249 100644
--- a/environment.c
+++ b/environment.c
@@ -9,6 +9,10 @@
  */
 #include "cache.h"
 
+#ifndef GIT_EXEC_PATH
+#define GIT_EXEC_PATH NULL
+#endif
+
 char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int trust_executable_bit = 1;
@@ -16,6 +20,7 @@ int only_use_symrefs = 0;
 int repository_format_version = 0;
 char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = 0;
+char *git_exec_path = GIT_EXEC_PATH;
 
 static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
 	*git_graft_file;
@@ -76,4 +81,28 @@ char *get_graft_file(void)
 	return git_graft_file;
 }
 
+void git_setup_exec_path(void)
+{
+	char *path, *old_path = getenv("PATH");
+	int path_len, len, old_len;
+	char *exec_path = getenv("GIT_EXEC_PATH");
+
+	if (!exec_path)
+		exec_path = git_exec_path;
+
+	len = strlen(exec_path);
+
+	if (!old_path)
+		old_path = "/usr/local/bin:/usr/bin:/bin";
 
+	old_len = strlen(old_path);
+	path_len = len + old_len + 1;
+
+	path = malloc(path_len + 1);
+
+	memcpy(path, old_path, old_len);
+	path[old_len] = ':';
+	memcpy(path + old_len + 1, exec_path, len);
+
+	setenv("PATH", path, 1);
+}
-- 
0.99.9m-g02ad

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

[Michal Ostrowski]

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.

Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 daemon.c      |    2 ++
 fetch-clone.c |    2 ++
 run-command.c |    1 +
 send-pack.c   |    2 ++
 upload-pack.c |    4 +++-
 5 files changed, 10 insertions(+), 1 deletions(-)

bb14b4b61f53f755486695e5bdc45b5623a6f8c5
diff --git a/daemon.c b/daemon.c
index 3bd1426..d653f33 100644
--- a/daemon.c
+++ b/daemon.c
@@ -226,6 +226,8 @@ static int upload(char *dir)
 
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
 
+	git_setup_exec_path();
+
 	/* git-upload-pack only ever reads stuff, so this is safe */
 	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf,
".", NULL);
 	return -1;
diff --git a/fetch-clone.c b/fetch-clone.c
index f46fe6e..afbbb79 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c
@@ -98,6 +98,8 @@ int receive_unpack_pack(int fd[2], const
 	int status;
 	pid_t pid;
 
+	git_setup_exec_path();
+
 	pid = fork();
 	if (pid < 0)
 		die("%s: unable to fork off git-unpack-objects", me);
diff --git a/run-command.c b/run-command.c
index 8bf5922..993a3f9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -9,6 +9,7 @@ int run_command_v_opt(int argc, char **a
 	if (pid < 0)
 		return -ERR_RUN_COMMAND_FORK;
 	if (!pid) {
+		git_setup_exec_path();
 		if (flags & RUN_COMMAND_NO_STDIO) {
 			int fd = open("/dev/null", O_RDWR);
 			dup2(fd, 0);
diff --git a/send-pack.c b/send-pack.c
index cd36193..a241f00 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -30,6 +30,7 @@ static void exec_pack_objects(void)
 		"--stdout",
 		NULL
 	};
+	git_setup_exec_path();
 	execvp("git-pack-objects", args);
 	die("git-pack-objects exec failed (%s)", strerror(errno));
 }
@@ -58,6 +59,7 @@ static void exec_rev_list(struct ref *re
 		refs = refs->next;
 	}
 	args[i] = NULL;
+	git_setup_exec_path();
 	execvp("git-rev-list", args);
 	die("git-rev-list exec failed (%s)", strerror(errno));
 }
diff --git a/upload-pack.c b/upload-pack.c
index 1834b6b..f8d4fbe 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -270,7 +270,9 @@ int main(int argc, char **argv)
 			break;
 		}
 	}
-	
+
+	git_setup_exec_path();
+
 	if (i != argc-1)
 		usage(upload_pack_usage);
 	dir = argv[i];
-- 
0.99.9m-g02ad

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

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> 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.

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

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> writes:

> If git-upload-pack is invoked by ssh, it may have been invoked because
> ssh was explicitly told which program to execute on the remote end
> (i.e. --exec had been used with git-clone-pack).  In this case, the
> git suite may not be in the PATH, and so subsequent exec's by
> git-upload-pack (i.e. git-rev-list, git-pack-objects) will fail.

True.

> +.environment.GIT_EXEC_PATH:
> +	@(test -e $@ && grep -h -e '^$(bindir)$$' $@) || echo $(bindir) > $@

Hmph.

  * I did not know "test -e" was portable (it is in POSIX.1),
    but since you are creating the file yourself anyway,
    wouldn't "test -f" look more familiar?

  * Perhaps grep -F (--fixed-strings), not as regexp?

  * I do not get the point of using "grep -h" here (it's not in
    POSIX.1).  Perhaps just >/dev/null?

But I like the timestamp trick here that uses ||.  Maybe I
should borrow it for GIT-VERSION-GEN.  Maybe not.

> --- a/environment.c
> +++ b/environment.c
> @@ -9,6 +9,10 @@
>   */
>  #include "cache.h"
>  
> +#ifndef GIT_EXEC_PATH
> +#define GIT_EXEC_PATH NULL
> +#endif

I wonder if not having GIT_EXEC_PATH defined should be an error here.

> +void git_setup_exec_path(void)
> +{
>...
> +}

Maybe move git.c::prepend_to_path() to a single library file and
use it here?

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

[Michal Ostrowski]

On Mon, 2006-01-09 at 18:52 -0800, Junio C Hamano wrote:
> Michal Ostrowski <mostrows@watson.ibm.com> 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.
> 

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 <mostrows@watson.ibm.com>

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");
 }
 

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

[Andreas Ericsson]

Michal Ostrowski wrote:
> On Mon, 2006-01-09 at 18:52 -0800, Junio C Hamano wrote:
> 
>>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

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

[Michal Ostrowski]

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",....)'.

> 
> > 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.

> 
> > 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.




> 
> 
> > 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 <mostrows@watson.ibm.com>

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

[Andreas Ericsson]

Michal Ostrowski wrote:
> 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.
> 

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.


> 
>>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.

> 
>>>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.

>>
>>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

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

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> 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?

>> > 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.

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

[Johannes Schindelin]

Hi,

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

> Michal Ostrowski <mostrows@watson.ibm.com> writes:
> 
> > 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

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

[Alex Riesen]

Andreas Ericsson, Tue, Jan 10, 2006 20:13:34 +0100:
> >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

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

[Michal Ostrowski]

On Tue, 2006-01-10 at 11:47 -0800, Junio C Hamano wrote:


> > 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.
> 
> True.
> 
> > Good point. Perhaps we should only prepend to path when the directory
> > isn't already in $PATH, or append rather than prepend.
> 
> I think appending not prepending would stop letting me say
> 
>  $ GIT_EXEC_PATH=/usr/libexec/git-core/0.99.9k git foo
> 
> to try out older version, if I have more recent git in my PATH.
> But I agree with Michal it is not nice to affect invocations of
> "diff" (and things spawned from hooks, which would inherit PATH
> from receive-pack).
> 
> 

How about searching for executables in the following places, and in this
order:

1. --exec-path setting, if any
2. GIT_EXEC_PATH env var, if set
3. PATH (never modified)
4. Value of ${bindir} at build time


To accomplish this I'd suggest reworking the logic the git c programs
use to exec another git program.  The patch I sent out this morning
attempts to do this.  (I'll append again to avoid confusion with
previous patches.)  But note that the patch below doesn't implement this
policy above; the point is that it provides for a single location where
it has to be implemented (in exec_cmd.c).  I'd be happy to clean it up
and submit it formally as a clean-up patch (one that doesn't change the
PATH/GIT_EXEC_PATH search behavior currently implemented).

Secondly, the shell scripts as is cannot utilize this search order as
long as they don't religiously use the git potty internally.  If we were
to "sed -e 's/git-/git /g' -i git*.sh" (grotesquely simplified of
course) then they would. 



-- 
Michal Ostrowski <mostrows@watson.ibm.com>

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");
 }
 

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

[Michal Ostrowski]

On Tue, 2006-01-10 at 20:55 +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 10 Jan 2006, Junio C Hamano wrote:
> 
> > Michal Ostrowski <mostrows@watson.ibm.com> writes:
> > 
> > > 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 <mostrows@watson.ibm.com>

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

[Michal Ostrowski]

On Tue, 2006-01-10 at 21:15 +0100, Alex Riesen wrote:
> Andreas Ericsson, Tue, Jan 10, 2006 20:13:34 +0100:
> > >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 <mostrows@watson.ibm.com>

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

[Johannes Schindelin]

Hi,

On Tue, 10 Jan 2006, Michal Ostrowski wrote:

> On Tue, 2006-01-10 at 20:55 +0100, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Tue, 10 Jan 2006, Junio C Hamano wrote:
> > 
> > > Michal Ostrowski <mostrows@watson.ibm.com> writes:
> > > 
> > > > 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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Recall the "diff" example Michal gave us.


 

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

[Andreas Ericsson]

Michal Ostrowski wrote:
> On Tue, 2006-01-10 at 11:47 -0800, Junio C Hamano wrote:
> 
>>
>>>Good point. Perhaps we should only prepend to path when the directory
>>>isn't already in $PATH, or append rather than prepend.
>>
>>I think appending not prepending would stop letting me say
>>
>> $ GIT_EXEC_PATH=/usr/libexec/git-core/0.99.9k git foo
>>
>>to try out older version, if I have more recent git in my PATH.
>>But I agree with Michal it is not nice to affect invocations of
>>"diff" (and things spawned from hooks, which would inherit PATH
>>from receive-pack).
>>

So how about prepending only when the directory isn't already in the 
PATH? That can be done with a two-line patch to git.c only.

It will break the "diff in another dir" scenario in the highly unlikely 
event that the other diff is located in the same dir as the git suite, 
isn't supposed to be used, and the directory in question isn't in $PATH 
already. People who have such a setup will be too ashamed to admit it, 
so we're not likely to be blamed for it either. ;)

>>
> 
> 
> How about searching for executables in the following places, and in this
> order:
> 
> 1. --exec-path setting, if any
> 2. GIT_EXEC_PATH env var, if set
> 3. PATH (never modified)
> 4. Value of ${bindir} at build time
> 

This is more or less what's done today, with the exception that $PATH 
isn't searched and it throws an error immediately no matter where 
exec_path (the git.c variable) came from.

Adding $PATH to the search-pattern would be a simple matter of falling 
back to execvp() if execve(), but then we could end up with running 
programs from a different release while the user thinks he/she's 
specifically running 1.0.3... Tricky problem, really.

> 
> Secondly, the shell scripts as is cannot utilize this search order as
> long as they don't religiously use the git potty internally.  If we were
> to "sed -e 's/git-/git /g' -i git*.sh" (grotesquely simplified of
> course) then they would. 
> 

I think this has been done, but as it happened to be convenient. I'd 
prefer if the git potty could keep prepending the GIT_EXEC_PATH to the 
path, really. We're bound to run into setup-related problems otherwise, 
such as;

Alice writes a script that works fine for her and her friends, so she 
shares it freely with Bob and whoever else might be listening. Bob's 
git-tools aren't in the $PATH but he keeps the potty handy at all times. 
He can't always run it through the potty because in some code-paths 
Alice's script uses git-tools without going through the potty. Bob 
thinks Alice puts entropy in her programs on purpose, so Alice flees, 
sobbing and shaking in anger and betrayed trust. The two never speak again.

Luckily, Bruce Schneier shows up and saves the day.

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

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

[Andreas Ericsson]

Johannes Schindelin wrote:
>>>>>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

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

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> writes:

> How about searching for executables in the following places, and in this
> order:
>
> 1. --exec-path setting, if any
> 2. GIT_EXEC_PATH env var, if set
> 3. PATH (never modified)
> 4. Value of ${bindir} at build time

My preference is to first do this to the Makefile:

-- >8 --
diff --git a/Makefile b/Makefile
index 5817e86..b1e3055 100644
--- a/Makefile
+++ b/Makefile
@@ -71,6 +71,7 @@ ALL_LDFLAGS = $(LDFLAGS)
 
 prefix = $(HOME)
 bindir = $(prefix)/bin
+gitexecdir = $(prefix)/bin
 template_dir = $(prefix)/share/git-core/templates/
 GIT_PYTHON_DIR = $(prefix)/share/git-core/python
 # DESTDIR=
@@ -144,7 +145,7 @@ PROGRAMS = \
 	git-describe$X
 
 # what 'all' will build and 'install' will install.
-ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS) git$X
+ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS)
 
 # Backward compatibility -- to be removed after 1.0
 PROGRAMS += git-ssh-pull$X git-ssh-push$X
@@ -368,13 +369,13 @@ LIB_OBJS += $(COMPAT_OBJS)
 export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 ### Build rules
 
-all: $(ALL_PROGRAMS)
+all: $(ALL_PROGRAMS) git$X
 
 all:
 	$(MAKE) -C templates
 
 git$X: git.c $(LIB_FILE)
-	$(CC) -DGIT_EXEC_PATH='"$(bindir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
+	$(CC) -DGIT_EXEC_PATH='"$(gitexecdir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
 		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(LIB_FILE)
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
@@ -470,7 +471,9 @@ check:
 
 install: all
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(bindir))
-	$(INSTALL) $(ALL_PROGRAMS) $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(gitexecdir))
+	$(INSTALL) $(ALL_PROGRAMS) $(call shellquote,$(DESTDIR)$(gitexecdir))
+	$(INSTALL) git$X $(call shellquote,$(DESTDIR)$(bindir))
 	$(MAKE) -C templates install
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
 	$(INSTALL) $(PYMODULES) $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))

-- 8< --

and then make the rule for git things:

 1. --exec-path
 2. GIT_EXEC_PATH environment
 3. $(gitexecdir)

in this order.  Non git things should just use $PATH without
looking at anything else --- as long as a hook script calls a git
wrapper (i.e. "git foo" not "git-foo") I think things should
work fine.

> ...  The patch I sent out this morning
> attempts to do this.  (I'll append again to avoid confusion with
> previous patches)...

> +		if (flags & RUN_GIT_CMD) {
> +			execv_git_cmd(argv);
> +		} else {
> +			execvp(argv[0], (char *const*) argv);
>  		}

This bit sounds good, but if you were to go this route I'd
suggest to rename your exec_git_cmd() to execl_git_cmd() (and
terminate the vararg list with NULL) for naming consistency.
execv_git_cmd() is good---we do not *want* execvp_git_cmd(),
because we do not run git subcommand using PATH.

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

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Johannes Schindelin wrote:

>> 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.

True, but *please* stop calling "git wrapper" a potty.  It gives
me an impression that it is not connected to the plumbing.

I do not do Porcelain, but I do not do plastics nor glass either
;-).

Thanks.

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

[Michal Ostrowski]

On Tue, 2006-01-10 at 16:42 -0800, Junio C Hamano wrote:
> Michal Ostrowski <mostrows@watson.ibm.com> writes:
> 
> > How about searching for executables in the following places, and in this
> > order:
> >
> > 1. --exec-path setting, if any
> > 2. GIT_EXEC_PATH env var, if set
> > 3. PATH (never modified)
> > 4. Value of ${bindir} at build time
> 


> and then make the rule for git things:
> 
>  1. --exec-path
>  2. GIT_EXEC_PATH environment
>  3. $(gitexecdir)
> 
> in this order.  Non git things should just use $PATH without
> looking at anything else --- as long as a hook script calls a git
> wrapper (i.e. "git foo" not "git-foo") I think things should
> work fine.
> 

The patch that follows implements your suggested search order and
includes suggested Makefile changes.

-- 
Michal Ostrowski <mostrows@watson.ibm.com>

[PATCH] Exec git programs without using PATH.

[Michal Ostrowski]

The git suite may not be in PATH (and thus programs such as
git-send-pack could not exec git-rev-list).  Thus there is a need for
logic that will locate these programs.  Modifying PATH is not
desirable as it result in behavior differing from the user's
intentions, as we may end up prepending "/usr/bin" to PATH.

- git C programs will use exec*_git_cmd() APIs to exec sub-commands.
- exec*_git_cmd() will execute a git program by searching for it in
  the following directories:
	1. --exec-path (as used by "git")
	2. The GIT_EXEC_PATH environment variable.
	3. $(gitexecdir) as set in Makefile (default value $(bindir)).
- git potty will modify PATH as before to enable shell scripts to  
  invoke "git-foo" commands.

Ideally, shell scripts should use the git potty to become independent of
PATH, and then modifying PATH will not be necessary.

Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 Makefile       |   21 ++++++++--
 daemon.c       |    3 +
 exec_cmd.c     |  118
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 exec_cmd.h     |   10 +++++
 fetch-clone.c  |    7 +--
 git.c          |   54 ++++++--------------------
 receive-pack.c |    2 -
 run-command.c  |    9 +++-
 run-command.h  |    2 -
 send-pack.c    |    8 ++--
 shell.c        |    2 -
 upload-pack.c  |    7 ++-
 12 files changed, 181 insertions(+), 62 deletions(-)
 create mode 100644 exec_cmd.c
 create mode 100644 exec_cmd.h

d6dbbb5b9b47c37f44c3494962b9fa534677729f
diff --git a/Makefile b/Makefile
index c9c15b5..912c223 100644
--- a/Makefile
+++ b/Makefile
@@ -68,6 +68,7 @@ ALL_LDFLAGS = $(LDFLAGS)
 
 prefix = $(HOME)
 bindir = $(prefix)/bin
+gitexecdir = $(prefix)/bin
 template_dir = $(prefix)/share/git-core/templates/
 GIT_PYTHON_DIR = $(prefix)/share/git-core/python
 # DESTDIR=
@@ -141,7 +142,7 @@ PROGRAMS = \
 	git-describe$X
 
 # what 'all' will build and 'install' will install.
-ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS) git$X
+ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS)
 
 # Backward compatibility -- to be removed after 1.0
 PROGRAMS += git-ssh-pull$X git-ssh-push$X
@@ -173,7 +174,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 +185,16 @@ LIB_OBJS = \
 LIBS = $(LIB_FILE)
 LIBS += -lz
 
+
+# .exec_cmd.gitexecdir stores $(gitexecir) 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.gitexecdir 2>/dev/null),$(gitexecdir))
+.PHONY: exec_cmd.c
+$(shell echo $(gitexecdir) > .exec_cmd.gitexecdir)
+endif
+
+exec_cmd.o: CFLAGS+=-DGIT_EXEC_PATH=\"$(gitexecdir)\"
+
 # Shell quote;
 # Result of this needs to be placed inside ''
 shq = $(subst ','\'',$(1))
@@ -366,13 +377,13 @@ LIB_OBJS += $(COMPAT_OBJS)
 export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 ### Build rules
 
-all: $(ALL_PROGRAMS)
+all: $(ALL_PROGRAMS) git$X
 
 all:
 	$(MAKE) -C templates
 
 git$X: git.c $(LIB_FILE)
-	$(CC) -DGIT_EXEC_PATH='"$(bindir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
+	$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
 		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(LIB_FILE)
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
@@ -468,7 +479,9 @@ check:
 
 install: all
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(gitexecdir))
 	$(INSTALL) $(ALL_PROGRAMS) $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) git$X $(call shellquote,$(DESTDIR)$(gitexecdir))
 	$(MAKE) -C templates install
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
 	$(INSTALL) $(PYMODULES) $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
diff --git a/daemon.c b/daemon.c
index 3bd1426..bb014fa 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);
+	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL);
 	return -1;
 }
 
diff --git a/exec_cmd.c b/exec_cmd.c
new file mode 100644
index 0000000..a3bd40a
--- /dev/null
+++ b/exec_cmd.c
@@ -0,0 +1,118 @@
+#include "cache.h"
+#include "exec_cmd.h"
+#define MAX_ARGS	32
+
+extern char **environ;
+static const char *builtin_exec_path = GIT_EXEC_PATH;
+static const char *current_exec_path = NULL;
+
+void git_set_exec_path(const char *exec_path)
+{
+	current_exec_path = exec_path;
+}
+
+
+/* Returns the highest-priority, location to look for git programs. */
+const char *git_exec_path()
+{
+	const char *env;
+
+	if (current_exec_path)
+		return current_exec_path;
+
+	env = getenv("GIT_EXEC_PATH");
+	if (env) {
+		return env;
+	}
+
+	return builtin_exec_path;
+}
+
+
+int execv_git_cmd(char **argv)
+{
+	char git_command[PATH_MAX + 1];
+	char *tmp;
+	int len, err, i;
+	const char *paths[] = { current_exec_path,
+				getenv("GIT_EXEC_PATH"),
+				builtin_exec_path,
+				NULL };
+
+	for (i = 0; i < 4; ++i) {
+		const char *exec_dir = paths[i];
+		if (!exec_dir) continue;
+
+		if (*exec_dir != '/') {
+			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_dir, "./", 2)) {
+				exec_dir += 2;
+				while (*exec_dir == '/')
+					exec_dir++;
+			}
+			snprintf(git_command + len, sizeof(git_command) - len,
+				 "/%s", exec_dir);
+		} else {
+			strcpy(git_command, exec_dir);
+		}
+
+		len = strlen(git_command);
+		len += snprintf(git_command + len, sizeof(git_command) - len,
+				"/git-%s", argv[0]);
+
+		if (sizeof(git_command) <= len) {
+			fprintf(stderr,
+				"git: command name given is too long.\n");
+			break;
+		}
+
+		/* 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] = git_command;
+
+		/* execve() can only ever return if it fails */
+		execve(git_command, argv, environ);
+
+		err = errno;
+
+		argv[0] = tmp;
+	}
+	return -1;
+
+}
+
+
+int execl_git_cmd(char *cmd,...)
+{
+	int argc;
+	char *argv[MAX_ARGS + 1];
+	char *arg;
+	va_list param;
+
+	va_start(param, cmd);
+	argv[0] = cmd;
+	argc = 1;
+	while (argc < MAX_ARGS) {
+		arg = argv[argc++] = va_arg(param, char *);
+		if (!arg)
+			break;
+	}
+	va_end(param);
+	if (MAX_ARGS <= argc)
+		return error("too many args to run %s", cmd);
+
+	argv[argc] = NULL;
+	return execv_git_cmd(argv);
+}
diff --git a/exec_cmd.h b/exec_cmd.h
new file mode 100644
index 0000000..06d5ec3
--- /dev/null
+++ b/exec_cmd.h
@@ -0,0 +1,10 @@
+#ifndef __GIT_EXEC_CMD_H_
+#define __GIT_EXEC_CMD_H_
+
+extern void git_set_exec_path(const char *exec_path);
+extern const char* git_exec_path();
+extern int execv_git_cmd(char **argv); /* NULL terminated */
+extern int execl_git_cmd(char *cmd, ...);
+
+
+#endif /* __GIT_EXEC_CMD_H_ */
diff --git a/fetch-clone.c b/fetch-clone.c
index f46fe6e..859f400 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);
+		execl_git_cmd("index-pack", "-o", idx, pack_tmp_name, NULL);
 		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);
+		execl_git_cmd("unpack-objects", quiet ? "-q" : NULL, NULL);
 		die("git-unpack-objects exec failed");
 	}
 	close(fd[0]);
diff --git a/git.c b/git.c
index 5e7da74..fdd02ed 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
@@ -233,14 +234,11 @@ 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;
+	char *exec_path;
 
 	getcwd(wd, PATH_MAX);
 
-	if (!exec_path)
-		exec_path = GIT_EXEC_PATH;
-
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
 
@@ -256,10 +254,11 @@ int main(int argc, char **argv, char **e
 
 		if (!strncmp(arg, "exec-path", 9)) {
 			arg += 9;
-			if (*arg == '=')
+			if (*arg == '=') {
 				exec_path = arg + 1;
-			else {
-				puts(exec_path);
+				git_set_exec_path(exec_path);
+			} else {
+				puts(git_exec_path());
 				exit(0);
 			}
 		}
@@ -275,48 +274,21 @@ 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);
+	exec_path = git_exec_path();
+	prepend_to_path(exec_path, strlen(exec_path));
 
-		/* 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/send-pack.c b/send-pack.c
index cd36193..4a420a6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -26,11 +26,11 @@ static int is_zero_sha1(const unsigned c
 static void exec_pack_objects(void)
 {
 	static char *args[] = {
-		"git-pack-objects",
+		"pack-objects",
 		"--stdout",
 		NULL
 	};
-	execvp("git-pack-objects", args);
+	execv_git_cmd(args);
 	die("git-pack-objects exec failed (%s)", strerror(errno));
 }
 
@@ -39,7 +39,7 @@ static void exec_rev_list(struct ref *re
 	static char *args[1000];
 	int i = 0;
 
-	args[i++] = "git-rev-list";	/* 0 */
+	args[i++] = "rev-list";	/* 0 */
 	args[i++] = "--objects";	/* 1 */
 	while (refs) {
 		char *buf = malloc(100);
@@ -58,7 +58,7 @@ static void exec_rev_list(struct ref *re
 		refs = refs->next;
 	}
 	args[i] = NULL;
-	execvp("git-rev-list", args);
+	execv_git_cmd(args);
 	die("git-rev-list exec failed (%s)", strerror(errno));
 }
 
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..d198055 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);
+	execl_git_cmd("pack-objects", "--stdout", NULL);
 	die("git-upload-pack: unable to exec git-pack-objects");
 }
 
-- 
0.99.9m-g5a22

Re: [PATCH] Exec git programs without using PATH.

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> writes:

>  exec_cmd.c     |  118
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

This is a giveaway sign that your MUA is wrapping lines, so
please double-check before sending things out next time around.

It seems yours is not as bad as others I've seen; I could manage
to trivially hand-edit to make it appliable (although I am
inclined to ask you to polish it a bit further).

> +ifneq ($(shell cat .exec_cmd.gitexecdir 2>/dev/null),$(gitexecdir))
> +.PHONY: exec_cmd.c
> +$(shell echo $(gitexecdir) > .exec_cmd.gitexecdir)
> +endif

Although this check is much better than the last round, I changed
my mind about this part.  We do not do the equivalent checks and
rebuild when somebody once builds with NO_OPENSSL and then tries
to rebuild without, or for any other compile time configuration
(template_dir, for example).  Ideally we _could_ keep track of
all of them but I suspect that would be an overkill, so I am
inclined to drop this part.

We need to make sure that we clean .exec_cmd.gitexecdir on the
clean: target in the Makefile, if we _were_ to keep this, OTOH.

> +exec_cmd.o: CFLAGS+=-DGIT_EXEC_PATH=\"$(gitexecdir)\"

Please do not override CFLAGS like this; it breaks this common
usage:

	$ make CFLAGS='-g -O1' exec_cmd.o

> diff --git a/exec_cmd.c b/exec_cmd.c
> +void git_set_exec_path(const char *exec_path)
> +{
> +	current_exec_path = exec_path;
> +}

I've always wondered what we should do when --exec-path is given
more than once, but you decided it for me.  I agree that
silently overwriting is just fine.

> +	const char *paths[] = { current_exec_path,
> +				getenv("GIT_EXEC_PATH"),
> +				builtin_exec_path,
> +				NULL };

I do not think you need the last NULL element in paths[] here,
given what the loop that uses this array does.

> +	for (i = 0; i < 4; ++i) {

If your paths[] is fixed size, please do this instead:

	for (i = 0; sizeof(paths)/sizeof(paths[0]); i++)

Then I do not have to remember that I need to update "4" when I
update paths[].

> diff --git a/exec_cmd.h b/exec_cmd.h

> +extern const char* git_exec_path();

ANSI C please: extern const char* git_exec_path(void);

> diff --git a/git.c b/git.c

>  	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]);

You have assigned to exec_path before the failed execv_git_cmd()
already; I think change in this line is unneeded.

> 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);

This one is questionable; run_command_v_opt() calls
execv_git_cmd() which expects the second argument of
run_command_v_opt() to be NULL terminated, doesn't it?

> 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);
>  }

Here my_argv comes from cmd_list[] which has "git-" prefix to
their command name, but I thought your exec[vl]_git_cmd() took
command names without.

Here is my suggestion on top of your patch, summarizing all of
the above comments.

-- >8 --

diff --git a/Makefile b/Makefile
index e7c7763..6694627 100644
--- a/Makefile
+++ b/Makefile
@@ -185,16 +185,6 @@ LIB_OBJS = \
 LIBS = $(LIB_FILE)
 LIBS += -lz
 
-
-# .exec_cmd.gitexecdir stores $(gitexecir) 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.gitexecdir 2>/dev/null),$(gitexecdir))
-.PHONY: exec_cmd.c
-$(shell echo $(gitexecdir) > .exec_cmd.gitexecdir)
-endif
-
-exec_cmd.o: CFLAGS+=-DGIT_EXEC_PATH=\"$(gitexecdir)\"
-
 # Shell quote;
 # Result of this needs to be placed inside ''
 shq = $(subst ','\'',$(1))
@@ -423,6 +413,8 @@ git$X git.spec \
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
+exec_cmd.o: ALL_CFLAGS += -DGIT_EXEC_PATH=\"$(gitexecdir)\"
+
 git-%$X: %.o $(LIB_FILE)
 	$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
diff --git a/exec_cmd.c b/exec_cmd.c
index a3bd40a..55af33b 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -36,10 +36,9 @@ int execv_git_cmd(char **argv)
 	int len, err, i;
 	const char *paths[] = { current_exec_path,
 				getenv("GIT_EXEC_PATH"),
-				builtin_exec_path,
-				NULL };
+				builtin_exec_path };
 
-	for (i = 0; i < 4; ++i) {
+	for (i = 0; i < sizeof(paths)/sizeof(paths[0]); ++i) {
 		const char *exec_dir = paths[i];
 		if (!exec_dir) continue;
 
diff --git a/exec_cmd.h b/exec_cmd.h
index 06d5ec3..5150ee2 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -2,7 +2,7 @@
 #define __GIT_EXEC_CMD_H_
 
 extern void git_set_exec_path(const char *exec_path);
-extern const char* git_exec_path();
+extern const char* git_exec_path(void);
 extern int execv_git_cmd(char **argv); /* NULL terminated */
 extern int execl_git_cmd(char *cmd, ...);
 
diff --git a/git.c b/git.c
index fdd02ed..e3a0b59 100644
--- a/git.c
+++ b/git.c
@@ -285,10 +285,10 @@ int main(int argc, char **argv, char **e
 	execv_git_cmd(argv + i);
 
 	if (errno == ENOENT)
-		cmd_usage(git_exec_path(), "'%s' is not a git-command",
-			  argv[i]);
+		cmd_usage(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 8e78e32..6120dbe 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -6,7 +6,7 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
-static const char unpacker[] = "git-unpack-objects";
+static char *unpacker[] = { "git-unpack-objects", NULL };
 
 static int report_status = 0;
 
@@ -257,7 +257,7 @@ static void read_head_info(void)
 
 static const char *unpack(int *error_code)
 {
-	int code = run_command_v_opt(1, &unpacker, RUN_GIT_CMD);
+	int code = run_command_v_opt(1, unpacker, RUN_GIT_CMD);
 
 	*error_code = 0;
 	switch (code) {
diff --git a/shell.c b/shell.c
index 0d4891f..d40dfe4 100644
--- a/shell.c
+++ b/shell.c
@@ -7,8 +7,10 @@ static int do_generic_cmd(const char *me
 
 	if (!arg || !(arg = sq_dequote(arg)))
 		die("bad argument");
+	if (strncmp(me, "git-", 4))
+		die("bad command");
 
-	my_argv[0] = me;
+	my_argv[0] = me + 4;
 	my_argv[1] = arg;
 	my_argv[2] = NULL;
 

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

[Andreas Ericsson]

Junio C Hamano wrote:
> 
> True, but *please* stop calling "git wrapper" a potty.  It gives
> me an impression that it is not connected to the plumbing.
> 

Aww... And here I was being quite pleased with this wording in the man-page:

"The program git is just a wrapper to reach the core git programs (or  a 
potty if  you like, as it's not exactly porcelain but still brings your 
stuff to the plumbing)."

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

[PATCH] (Updated) Exec git programs without using PATH.

[Michal Ostrowski]

The git suite may not be in PATH (and thus programs such as
git-send-pack could not exec git-rev-list).  Thus there is a need for
logic that will locate these programs.  Modifying PATH is not
desirable as it result in behavior differing from the user's
intentions, as we may end up prepending "/usr/bin" to PATH.

- git C programs will use exec*_git_cmd() APIs to exec sub-commands.
- exec*_git_cmd() will execute a git program by searching for it in
  the following directories:
	1. --exec-path (as used by "git")
	2. The GIT_EXEC_PATH environment variable.
	3. $(gitexecdir) as set in Makefile (default value $(bindir)).
- all exec's of git programs will use exec*_git_cmd() interfaces,
- C programs will not modify PATH

If the git suite is not in PATH, C programs will function, but there
is no guarantee that shell scripts will, as shell scripts may invoke
"git-foo" rather than "git foo".  Shell scripts that are modified to
use the git potty should become independent of PATH.

Includes modifications by Junio C Hamano <junkio@cox.net>.

Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 Makefile       |   13 ++++--
 daemon.c       |    3 +
 exec_cmd.c     |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 exec_cmd.h     |   10 +++++
 fetch-clone.c  |    7 +--
 git.c          |   50 +++++-------------------
 receive-pack.c |    4 +-
 run-command.c  |    9 +++-
 run-command.h  |    2 -
 send-pack.c    |    8 ++--
 shell.c        |    6 ++-
 upload-pack.c  |    7 ++-
 12 files changed, 174 insertions(+), 62 deletions(-)
 create mode 100644 exec_cmd.c
 create mode 100644 exec_cmd.h

565e290d6e7f24bd4e72ca7494297230799d118a
diff --git a/Makefile b/Makefile
index c9c15b5..abcb771 100644
--- a/Makefile
+++ b/Makefile
@@ -68,6 +68,7 @@ ALL_LDFLAGS = $(LDFLAGS)
 
 prefix = $(HOME)
 bindir = $(prefix)/bin
+gitexecdir = $(prefix)/bin
 template_dir = $(prefix)/share/git-core/templates/
 GIT_PYTHON_DIR = $(prefix)/share/git-core/python
 # DESTDIR=
@@ -141,7 +142,7 @@ PROGRAMS = \
 	git-describe$X
 
 # what 'all' will build and 'install' will install.
-ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS) git$X
+ALL_PROGRAMS = $(PROGRAMS) $(SIMPLE_PROGRAMS) $(SCRIPTS)
 
 # Backward compatibility -- to be removed after 1.0
 PROGRAMS += git-ssh-pull$X git-ssh-push$X
@@ -173,7 +174,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 \
@@ -366,13 +367,13 @@ LIB_OBJS += $(COMPAT_OBJS)
 export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 ### Build rules
 
-all: $(ALL_PROGRAMS)
+all: $(ALL_PROGRAMS) git$X
 
 all:
 	$(MAKE) -C templates
 
 git$X: git.c $(LIB_FILE)
-	$(CC) -DGIT_EXEC_PATH='"$(bindir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
+	$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
 		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(LIB_FILE)
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
@@ -412,6 +413,8 @@ git$X git.spec \
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
+exec_cmd.o: ALL_CFLAGS += -DGIT_EXEC_PATH=\"$(gitexecdir)\"
+
 git-%$X: %.o $(LIB_FILE)
 	$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
@@ -468,7 +471,9 @@ check:
 
 install: all
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(gitexecdir))
 	$(INSTALL) $(ALL_PROGRAMS) $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) git$X $(call shellquote,$(DESTDIR)$(gitexecdir))
 	$(MAKE) -C templates install
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
 	$(INSTALL) $(PYMODULES) $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
diff --git a/daemon.c b/daemon.c
index 3bd1426..bb014fa 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);
+	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL);
 	return -1;
 }
 
diff --git a/exec_cmd.c b/exec_cmd.c
new file mode 100644
index 0000000..55af33b
--- /dev/null
+++ b/exec_cmd.c
@@ -0,0 +1,117 @@
+#include "cache.h"
+#include "exec_cmd.h"
+#define MAX_ARGS	32
+
+extern char **environ;
+static const char *builtin_exec_path = GIT_EXEC_PATH;
+static const char *current_exec_path = NULL;
+
+void git_set_exec_path(const char *exec_path)
+{
+	current_exec_path = exec_path;
+}
+
+
+/* Returns the highest-priority, location to look for git programs. */
+const char *git_exec_path()
+{
+	const char *env;
+
+	if (current_exec_path)
+		return current_exec_path;
+
+	env = getenv("GIT_EXEC_PATH");
+	if (env) {
+		return env;
+	}
+
+	return builtin_exec_path;
+}
+
+
+int execv_git_cmd(char **argv)
+{
+	char git_command[PATH_MAX + 1];
+	char *tmp;
+	int len, err, i;
+	const char *paths[] = { current_exec_path,
+				getenv("GIT_EXEC_PATH"),
+				builtin_exec_path };
+
+	for (i = 0; i < sizeof(paths)/sizeof(paths[0]); ++i) {
+		const char *exec_dir = paths[i];
+		if (!exec_dir) continue;
+
+		if (*exec_dir != '/') {
+			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_dir, "./", 2)) {
+				exec_dir += 2;
+				while (*exec_dir == '/')
+					exec_dir++;
+			}
+			snprintf(git_command + len, sizeof(git_command) - len,
+				 "/%s", exec_dir);
+		} else {
+			strcpy(git_command, exec_dir);
+		}
+
+		len = strlen(git_command);
+		len += snprintf(git_command + len, sizeof(git_command) - len,
+				"/git-%s", argv[0]);
+
+		if (sizeof(git_command) <= len) {
+			fprintf(stderr,
+				"git: command name given is too long.\n");
+			break;
+		}
+
+		/* 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] = git_command;
+
+		/* execve() can only ever return if it fails */
+		execve(git_command, argv, environ);
+
+		err = errno;
+
+		argv[0] = tmp;
+	}
+	return -1;
+
+}
+
+
+int execl_git_cmd(char *cmd,...)
+{
+	int argc;
+	char *argv[MAX_ARGS + 1];
+	char *arg;
+	va_list param;
+
+	va_start(param, cmd);
+	argv[0] = cmd;
+	argc = 1;
+	while (argc < MAX_ARGS) {
+		arg = argv[argc++] = va_arg(param, char *);
+		if (!arg)
+			break;
+	}
+	va_end(param);
+	if (MAX_ARGS <= argc)
+		return error("too many args to run %s", cmd);
+
+	argv[argc] = NULL;
+	return execv_git_cmd(argv);
+}
diff --git a/exec_cmd.h b/exec_cmd.h
new file mode 100644
index 0000000..5150ee2
--- /dev/null
+++ b/exec_cmd.h
@@ -0,0 +1,10 @@
+#ifndef __GIT_EXEC_CMD_H_
+#define __GIT_EXEC_CMD_H_
+
+extern void git_set_exec_path(const char *exec_path);
+extern const char* git_exec_path(void);
+extern int execv_git_cmd(char **argv); /* NULL terminated */
+extern int execl_git_cmd(char *cmd, ...);
+
+
+#endif /* __GIT_EXEC_CMD_H_ */
diff --git a/fetch-clone.c b/fetch-clone.c
index f46fe6e..859f400 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);
+		execl_git_cmd("index-pack", "-o", idx, pack_tmp_name, NULL);
 		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);
+		execl_git_cmd("unpack-objects", quiet ? "-q" : NULL, NULL);
 		die("git-unpack-objects exec failed");
 	}
 	close(fd[0]);
diff --git a/git.c b/git.c
index 5e7da74..4616df6 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
@@ -233,14 +234,11 @@ 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;
+	const char *exec_path;
 
 	getcwd(wd, PATH_MAX);
 
-	if (!exec_path)
-		exec_path = GIT_EXEC_PATH;
-
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
 
@@ -256,10 +254,11 @@ int main(int argc, char **argv, char **e
 
 		if (!strncmp(arg, "exec-path", 9)) {
 			arg += 9;
-			if (*arg == '=')
+			if (*arg == '=') {
 				exec_path = arg + 1;
-			else {
-				puts(exec_path);
+				git_set_exec_path(exec_path);
+			} else {
+				puts(git_exec_path());
 				exit(0);
 			}
 		}
@@ -275,42 +274,15 @@ 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);
-	}
+	exec_path = git_exec_path();
+	prepend_to_path(exec_path, strlen(exec_path));
 
-	/* 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]);
diff --git a/receive-pack.c b/receive-pack.c
index f847ec2..6120dbe 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -6,7 +6,7 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
-static const char unpacker[] = "git-unpack-objects";
+static char *unpacker[] = { "git-unpack-objects", NULL };
 
 static int report_status = 0;
 
@@ -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/send-pack.c b/send-pack.c
index cd36193..4a420a6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -26,11 +26,11 @@ static int is_zero_sha1(const unsigned c
 static void exec_pack_objects(void)
 {
 	static char *args[] = {
-		"git-pack-objects",
+		"pack-objects",
 		"--stdout",
 		NULL
 	};
-	execvp("git-pack-objects", args);
+	execv_git_cmd(args);
 	die("git-pack-objects exec failed (%s)", strerror(errno));
 }
 
@@ -39,7 +39,7 @@ static void exec_rev_list(struct ref *re
 	static char *args[1000];
 	int i = 0;
 
-	args[i++] = "git-rev-list";	/* 0 */
+	args[i++] = "rev-list";	/* 0 */
 	args[i++] = "--objects";	/* 1 */
 	while (refs) {
 		char *buf = malloc(100);
@@ -58,7 +58,7 @@ static void exec_rev_list(struct ref *re
 		refs = refs->next;
 	}
 	args[i] = NULL;
-	execvp("git-rev-list", args);
+	execv_git_cmd(args);
 	die("git-rev-list exec failed (%s)", strerror(errno));
 }
 
diff --git a/shell.c b/shell.c
index cd31618..d40dfe4 100644
--- a/shell.c
+++ b/shell.c
@@ -7,12 +7,14 @@ static int do_generic_cmd(const char *me
 
 	if (!arg || !(arg = sq_dequote(arg)))
 		die("bad argument");
+	if (strncmp(me, "git-", 4))
+		die("bad command");
 
-	my_argv[0] = me;
+	my_argv[0] = me + 4;
 	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..d198055 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);
+	execl_git_cmd("pack-objects", "--stdout", NULL);
 	die("git-upload-pack: unable to exec git-pack-objects");
 }
 
-- 
0.99.9m-g5a22

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

[Jon Loeliger]

On Wed, 2006-01-11 at 05:57, Andreas Ericsson wrote:
> Junio C Hamano wrote:
> > 
> > True, but *please* stop calling "git wrapper" a potty.  It gives
> > me an impression that it is not connected to the plumbing.
> > 
> 
> Aww... And here I was being quite pleased with this wording in the man-page:
> 
> "The program git is just a wrapper to reach the core git programs (or  a 
> potty if  you like, as it's not exactly porcelain but still brings your 
> stuff to the plumbing)."

Oh man.  So, do we have to go with "kitchen sink" instead? ;-)

jdl

Re: [PATCH] (Updated) Exec git programs without using PATH.

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> writes:

> The git suite may not be in PATH (and thus programs such as
>...
> Includes modifications by Junio C Hamano <junkio@cox.net>.

Thanks for the update (you did it even without line wrapping
this time).

I made further fixes last night and it is in the proposed
updates ("pu") branch.  This branch is used to hold and showcase
what I plan to eventually merge into the master branch, and
consists of the topic branches merged on top of what is in the
master branch.  To disect to see what shape I munged your patch
into, please look at it with gitk or gitweb and find "Merge
branch mo/exec" commit (my topic branches are named after patch
author and topic keyword).  The second parent of that commit is
the tip of the topic branch mo/exec when the merge was made.

A word of caution to people on the list (not limited to Michal)
is needed again, because it's been a while since I talked about
the proposed updates branch last time (which was pre 1.0.0
release IIRC).

The "pu" branch is regularly rebuilt by merging private topic
branches on top of the then-current master branch, by doing
this:

	$ git checkout pu
        $ git reset --hard master ;# build from scratch!
        $ git pull . topic1
        $ git pull . topic2
        $ ...
        $ git pull . topicN

What this means is that you (not just Michal---"people") should
not base your development on top of "pu" branch proper, since
you will have a hard time updating and merging.  Disecting my
"pu" branch to find the topic branch that corresponds to your
topic, and doing your development and patch preparation based on
that branch tip (and tell me that the patch is against your
topic branch, or "pu") is possible, but even that is not always
an easy option, since I may occasionally have to rebase my topic
branches as well.  You can always send in an updated patch based
on my master, and I'll manage with three-way merge to pick out
the real changes since your last iteration.

For your (primarily Michal, but other interested parties as
well) reference, here is the diff between your patch in the
message I am replying to and what I placed in the "pu" branch
last night.

-- >8 --
Updates on top of comments last night.

 - git-unpack-objects are run via execv_git_cmd, so no "git-"
   prefix should be given.

 - two more files need #include "exec_cmd.h".

---
diff --git a/receive-pack.c b/receive-pack.c
index 6120dbe..eae31e3 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -6,7 +6,7 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
-static char *unpacker[] = { "git-unpack-objects", NULL };
+static char *unpacker[] = { "unpack-objects", NULL };
 
 static int report_status = 0;
 
diff --git a/send-pack.c b/send-pack.c
index 4a420a6..990be3f 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -3,6 +3,7 @@
 #include "tag.h"
 #include "refs.h"
 #include "pkt-line.h"
+#include "exec_cmd.h"
 
 static const char send_pack_usage[] =
 "git-send-pack [--all] [--exec=git-receive-pack] <remote> [<head>...]\n"
diff --git a/shell.c b/shell.c
index d40dfe4..fc0c73c 100644
--- a/shell.c
+++ b/shell.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "quote.h"
+#include "exec_cmd.h"
 
 static int do_generic_cmd(const char *me, char *arg)
 {

Re: [PATCH] (Updated) Exec git programs without using PATH.

[Linus Torvalds]

On Wed, 11 Jan 2006, Junio C Hamano wrote:
> 
> For your (primarily Michal, but other interested parties as
> well) reference, here is the diff between your patch in the
> message I am replying to and what I placed in the "pu" branch
> last night.

Tangentially related note: maybe we should try to move to a "spawn()" like 
interface so that it could port better to native Windows interfaces?

Even under Linux, using vfork()+exec() is actually faster than a regular 
fork/exec, so there are potential advantages.

The real advantage of fork+exec is how you can do arbitrary setup between 
the two, without needing insanely complex rules for file descriptors etc. 
But maybe we don't have tons of those issues?

		Linus

Re: [PATCH] (Updated) Exec git programs without using PATH.

[Michal Ostrowski]

On Wed, 2006-01-11 at 12:42 -0800, Linus Torvalds wrote:
> 
> On Wed, 11 Jan 2006, Junio C Hamano wrote:
> > 
> > For your (primarily Michal, but other interested parties as
> > well) reference, here is the diff between your patch in the
> > message I am replying to and what I placed in the "pu" branch
> > last night.
> 
> Tangentially related note: maybe we should try to move to a "spawn()" like 
> interface so that it could port better to native Windows interfaces?
> 
> Even under Linux, using vfork()+exec() is actually faster than a regular 
> fork/exec, so there are potential advantages.
> 
> The real advantage of fork+exec is how you can do arbitrary setup between 
> the two, without needing insanely complex rules for file descriptors etc. 
> But maybe we don't have tons of those issues?
> 

I briefly tried to consider if I could hide the various fork()+exec()
sequences behind something like the run_command*() interfaces (which
would move us down the direction of something "spawn()"-like).  I found
that there's a lot of variation between the various paths in terms of
what happens between fork() and exec() on the various paths that does
not lend itself to such consolidation.

I'd love to be convinced otherwise.

-- 
Michal Ostrowski <mostrows@watson.ibm.com>

Re: [PATCH] (Updated) Exec git programs without using PATH.

[Junio C Hamano]

Michal Ostrowski <mostrows@watson.ibm.com> writes:

> I briefly tried to consider if I could hide the various fork()+exec()
> sequences behind something like the run_command*() interfaces (which
> would move us down the direction of something "spawn()"-like).  I found
> that there's a lot of variation between the various paths in terms of
> what happens between fork() and exec() on the various paths that does
> not lend itself to such consolidation.
>
> I'd love to be convinced otherwise.

Unfortunately I am with Michal on this one (both "eh, I do not
think that is feasible" and "I'd love to be...").

The run_command*() interfaces and its users were the best I
could come up with as far as such consolidations could go when I
did it.

Re: [PATCH] (Updated) Exec git programs without using PATH.

[Andreas Ericsson]

Junio C Hamano wrote:
> Michal Ostrowski <mostrows@watson.ibm.com> writes:
> 
> 
>>I briefly tried to consider if I could hide the various fork()+exec()
>>sequences behind something like the run_command*() interfaces (which
>>would move us down the direction of something "spawn()"-like).  I found
>>that there's a lot of variation between the various paths in terms of
>>what happens between fork() and exec() on the various paths that does
>>not lend itself to such consolidation.
>>
>>I'd love to be convinced otherwise.
> 
> 
> Unfortunately I am with Michal on this one (both "eh, I do not
> think that is feasible" and "I'd love to be...").
> 
> The run_command*() interfaces and its users were the best I
> could come up with as far as such consolidations could go when I
> did it.
> 

Not being entirely knowledgeable on what spawn() actually does and how 
its semantics differ from fork() and exec*() style API's (Google was 
depressingly unhelpful and wikipedia dredged up froglings...), I've got 
a decent "clone-lots-of-processes-and-multiplex-between-them" kind of 
library lying around. Would it be of any use?

 From the prototypes I've seen on spawn it doesn't seem to be much more 
than a fork() + execve(), either closing or dup2'ing all the 
file-descriptors, so I don't understand why that couldn't be implemented 
for git. Some pointers, anyone?

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

Re: [PATCH] (Updated) Exec git programs without using PATH.

[H. Peter Anvin]

Andreas Ericsson wrote:
> 
> Not being entirely knowledgeable on what spawn() actually does and how 
> its semantics differ from fork() and exec*() style API's (Google was 
> depressingly unhelpful and wikipedia dredged up froglings...), I've got 
> a decent "clone-lots-of-processes-and-multiplex-between-them" kind of 
> library lying around. Would it be of any use?
> 
>  From the prototypes I've seen on spawn it doesn't seem to be much more 
> than a fork() + execve(), either closing or dup2'ing all the 
> file-descriptors, so I don't understand why that couldn't be implemented 
> for git. Some pointers, anyone?
> 

RTFM(posix_spawn)...

	-hpa