On Fri, Jan 09, 2009 at 04:22:50AM -0500, Jeff King wrote:
> 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 */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
