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

Michal Ostrowski [off-list ref] 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

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

+extern const char* git_exec_path();

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

 	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.

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?

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;