Re: Funny: git -p submodule summary

From: Jeff King <peff@peff.net>
Date: Jan 9, 2009, 09:48 UTC
Message-ID: <20090109094843.GA4056@coredump.intra.peff.net>
In-Reply-To: <20090109092250.GA1809@coredump.intra.peff.net>

On Fri, Jan 09, 2009 at 04:22:50AM -0500, Jeff King wrote:

Show 7 quoted lines

> So I think to do things right, we have to be even more complicated. When
> we spawn the pager, we keep git as a single process. We register the
> atexit() handler to wait for the pager, and intercept any death signals
> to do the same. Then, if we are running a builtin, it is business as
> usual. But if we want to exec something, instead we have to actually
> spawn into the three-process form. Meaning we have to use run_command to
> start it, and then wait for it and the pager to return.

Actually, this turned out to be quite a small patch. It fixes the problem you were seeing, and it should work on Windows. It incurs an extra fork() for every dashed-external that we try.

There is a little noise in the patch, so let me highlight the three things that are happening (a "real" patch should break this into three patches in a series):

  1. The noise is from renaming the static run_command, which conflicts
     with the definition in run_command.h

  2. Substituting run_command for execvp.

  3. run_command needs to signal "I couldn't exec this" as opposed to
     other errors, since we care about the difference here. I do this
     here with exit(127), but probably we should recognize this in
     wait_or_whine and hand out the same error code for posix and mingw
     platforms.

As for the extra fork, we could do away with it if we scanned the path looking for the external before just exec'ing it. I think this is better in the long run anyway, because then we can do other setup specific to running an external command (I don't remember the details, but I ran afoul of this when I was doing pager stuff a while ago).

Patch is below.

---

diff --git a/git.c b/git.c
index ecc8fad..fa946b9 100644
--- a/git.c
+++ b/git.c
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "cache.h"
 #include "quote.h"
+#include "run-command.h"
 
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
@@ -219,7 +220,7 @@ struct cmd_struct {
 	int option;
 };
 
-static int run_command(struct cmd_struct *p, int argc, const char **argv)
+static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status;
 	struct stat st;
@@ -384,7 +385,7 @@ static void handle_internal_command(int argc, const char **argv)
 		struct cmd_struct *p = commands+i;
 		if (strcmp(p->cmd, cmd))
 			continue;
-		exit(run_command(p, argc, argv));
+		exit(run_builtin(p, argc, argv));
 	}
 }
 
@@ -392,6 +393,7 @@ static void execv_dashed_external(const char **argv)
 {
 	struct strbuf cmd = STRBUF_INIT;
 	const char *tmp;
+	int status;
 
 	strbuf_addf(&cmd, "git-%s", argv[0]);
 
@@ -406,8 +408,13 @@ static void execv_dashed_external(const char **argv)
 
 	trace_argv_printf(argv, "trace: exec:");
 
-	/* execvp() can only ever return if it fails */
-	execvp(cmd.buf, (char **)argv);
+	/*
+	 * if we failed because the command was not found, it is
+	 * OK to return. Otherwise, we just pass along the status code.
+	 */
+	status = run_command_v_opt(argv, 0);
+	if (status != 127 && status != -ERR_RUN_COMMAND_FORK)
+		exit(-status);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
diff --git a/run-command.c b/run-command.c
index c90cdc5..539609e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -118,7 +118,7 @@ int start_command(struct child_process *cmd)
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		die("exec %s failed.", cmd->argv[0]);
+		exit(127);
 	}
 #else
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */