git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <junkio@cox.net>
Date
Jan 11, 2006, 06:13 UTC
Message-ID
<7v4q4bwavi.fsf@assigned-by-dhcp.cox.net>
In-Reply-To
<1136945538.11717.643.camel@brick.watson.ibm.com>
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
Show 5 quoted lines
> 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.

Show 10 quoted lines
> 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?

Show 11 quoted lines
> 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;
Previous: Michal OstrowskiNext: Michal Ostrowski
Message 26 of 33 in “Remember and use GIT_EXEC_PATH on exec()'s”
  1. 0/2 Remember and use GIT_EXEC_PATH on exec()'sMichal Ostrowski, Jan 9, 2006
  2. 1/2 Remember and use GIT_EXEC_PATH on exec()'sMichal Ostrowski, Jan 9, 2006
  3. Junio C HamanoJan 10, 2006
  4. 2/2 Remember and use GIT_EXEC_PATH on exec()'sMichal Ostrowski, Jan 9, 2006
  5. Junio C HamanoJan 10, 2006
  6. Michal OstrowskiJan 10, 2006
  7. Andreas EricssonJan 10, 2006
  8. Michal OstrowskiJan 10, 2006
  9. Andreas EricssonJan 10, 2006
  10. Alex RiesenJan 10, 2006
  11. Michal OstrowskiJan 10, 2006
  12. Junio C HamanoJan 10, 2006
  13. Johannes SchindelinJan 10, 2006
  14. Michal OstrowskiJan 10, 2006
  15. Johannes SchindelinJan 10, 2006
  16. Andreas EricssonJan 11, 2006
  17. Junio C HamanoJan 11, 2006
  18. Andreas EricssonJan 11, 2006
  19. Jon LoeligerJan 11, 2006
  20. Junio C HamanoJan 10, 2006
  21. Michal OstrowskiJan 10, 2006
  22. Andreas EricssonJan 11, 2006
  23. Junio C HamanoJan 11, 2006
  24. Michal OstrowskiJan 11, 2006
  25. Exec git programs without using PATH.Michal Ostrowski, Jan 11, 2006
  26. Junio C HamanoJan 11, 2006
  27. (Updated) Exec git programs without using PATH.Michal Ostrowski, Jan 11, 2006
  28. Junio C HamanoJan 11, 2006
  29. Linus TorvaldsJan 11, 2006
  30. Michal OstrowskiJan 11, 2006
  31. Junio C HamanoJan 11, 2006
  32. Andreas EricssonJan 12, 2006
  33. H. Peter AnvinJan 12, 2006

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.