{"thread":{"id":"17049","subject":"Funny: git -p submodule summary","startedAt":"2009-01-08T15:07:08Z","lastAt":"2009-01-28T07:54:40Z","messageCount":41,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"99704","messageId":"alpine.DEB.1.00.0901081601240.30769@pacific.mpi-cbg.de","threadId":"17049","inReplyTo":null,"subject":"Funny: git -p submodule summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-08T15:07:08Z","receivedAt":"2009-01-08T15:07:08Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi list,\n\nJust try this with a submodule that has more changes than fit on a screen:\n\n\t$ git -p submodule summary\n\nIn my tests, it consistently fscks up my console.  I wonder if this is \nrelated to ea27a18(spawn pager via run_command interface).\n\n*reverts that commit* Yep, that fixes it.\n\nCiao,\nDscho\n"},{"id":"99706","messageId":"alpine.DEB.1.00.0901081629280.30769@pacific.mpi-cbg.de","threadId":"17049","inReplyTo":"alpine.DEB.1.00.0901081601240.30769@pacific.mpi-cbg.de","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-08T15:30:43Z","receivedAt":"2009-01-08T15:30:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 8 Jan 2009, Johannes Schindelin wrote:\n\n> Just try this with a submodule that has more changes than fit on a \n> screen:\n> \n> \t$ git -p submodule summary\n> \n> In my tests, it consistently fscks up my console.\n\nUpdate: even if the changes do fit on a screen, the console is fscked up \n(I have to stty echo to get it back to normal).\n\nCiao,\nDscho\n"},{"id":"99768","messageId":"20090109083836.GB21389@coredump.intra.peff.net","threadId":"17049","inReplyTo":"alpine.DEB.1.00.0901081601240.30769@pacific.mpi-cbg.de","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T08:38:36Z","receivedAt":"2009-01-09T08:38:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:\n\n> Just try this with a submodule that has more changes than fit on a screen:\n> \n> \t$ git -p submodule summary\n> \n> In my tests, it consistently fscks up my console.  I wonder if this is \n> related to ea27a18(spawn pager via run_command interface).\n> \n> *reverts that commit* Yep, that fixes it.\n\nHmm. I can reproduce your problem here, like this:\n\n  $ git -p submodule summary\n\nbut when I tried to dig deeper using strace, the problem goes away:\n\n  $ strace -f -o foo.out git -p submodule summary\n\nHowever, I was able to get some data by stracing _just_ less:\n\n  $ GIT_PAGER='strace -f -o foo.out less' git -p submodule summary\n\nand that reproduces the problem. And here's the interesting bit:\n\n  open(\"/dev/tty\", O_RDONLY|O_LARGEFILE)  = 3\n  ...\n  read(0, $SOME_SUBMODULE_OUTPUT, ...)\n  write(1, $SOME_SUBMODULE_OUTPUT, ...)\n  read(3, 0xbfd3442f, 1)                  = -1 EIO (Input/output error)\n\nThat is, after displaying the actual output, the next thing less does is\ntry to get input from the user on /dev/tty. But it returns EIO. At which\npoint less just dies, leaving your terminal in a funny state.\n\nHrm. Here's a theory. The new pager behavior goes something like this\nfor a builtin:\n\n  1. fork, child becomes pager\n  2. register atexit handler to wait for pager to finish\n  3. run builtin\n  4. exit, triggering handler\n\nbut for an external command (like a shell script), we exec the command,\nand presumably forget about out atexit handler entirely. Which means\nthat our shell script writes all of its output and exits before less has\na chance to prompt and get a response from the tty.\n\nAnd I'll admit to being very hazy on the details of process groups and\ncontrolling terminals, but from what I recall, perhaps the EIO is\nrelated to the process group leader being dead. Which means we could\npaper over it by putting less in its own pgrp.\n\nSo here's a little test (in bash):\n\n  $ set +m ;# disable job control, leaving bash as the pgrp leader\n  $ git -p submodule summary\n\nA-ha. That \"works\" in the sense that less runs fine and show the output.\nSo it is a pgrp issue.  _But_ we still have a problem, which is that the\noriginal process has exited, and bash thinks the command is finished. So\nnow annoyingly we have both bash and less trying to read from the\nterminal.\n\nSo the _real_ problem is that we are not always triggering the \"wait for\npager to finish\" code because we exec and forget about it. Which means\nthis strategy of \"git runs child pager\" will never work properly.\nInstead, we have to use three processes: git and the pager become child\nprocesses, while the original process waits for both to exit and returns\nthe proper exit code from git.\n\nLet me try to work up a patch.\n\n-Peff\n\nPS I believe this is related to a similar bug which I have been hunting\n   fruitlessly for a few weeks: if you use ^C to kill git, the pager\n   will sometimes do funny things. I also traced that to an EIO on\n   reading from the terminal, which makes sense: we are killing git\n   before it gets a chance to do wait_for_pager.\n"},{"id":"99771","messageId":"20090109092250.GA1809@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090109083836.GB21389@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T09:22:50Z","receivedAt":"2009-01-09T09:22:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 03:38:36AM -0500, Jeff King wrote:\n\n> So the _real_ problem is that we are not always triggering the \"wait for\n> pager to finish\" code because we exec and forget about it. Which means\n> this strategy of \"git runs child pager\" will never work properly.\n> Instead, we have to use three processes: git and the pager become child\n> processes, while the original process waits for both to exit and returns\n> the proper exit code from git.\n> \n> Let me try to work up a patch.\n\nBelow is a patch that uses the three-process mechanism, and it fixes the\nproblem. _But_ it is not satisfactory for inclusion, because it won't\nwork on MINGW32. Since it is actually splitting git into two processes\n(one to monitor the pager and one to actually run git), it uses fork.\n\nSo I think to do things right, we have to be even more complicated. When\nwe spawn the pager, we keep git as a single process. We register the\natexit() handler to wait for the pager, and intercept any death signals\nto do the same. Then, if we are running a builtin, it is business as\nusual. But if we want to exec something, instead we have to actually\nspawn into the three-process form. Meaning we have to use run_command to\nstart it, and then wait for it and the pager to return.\n\nOf course, we don't know ahead of time whether exec'ing a command will\nwork: we find out by trying. So now we will end up creating a pipe and\nfork()ing every time we want to see whether we can exec a command. But I\nsuppose that only happens once or twice, so maybe the performance impact\nisn't relevant.\n\nThis is all getting complicated enough that I am tempted to just suggest\nreverting ea27a18c. But even that won't fix everything, though, since\nMINGW32 still needs to use run_command to spawn the pager. IOW, I think\nthe breakage you are seeing has always been broken on MINGW32.\n\nBlah. Anyway, here is the Unix-only patch.\n\n---\ndiff --git a/pager.c b/pager.c\nindex f19ddbc..68ae669 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -28,18 +28,10 @@ static void pager_preexec(void)\n static const char *pager_argv[] = { \"sh\", \"-c\", NULL, NULL };\n static struct child_process pager_process;\n \n-static void wait_for_pager(void)\n-{\n-\tfflush(stdout);\n-\tfflush(stderr);\n-\t/* signal EOF to pager */\n-\tclose(1);\n-\tclose(2);\n-\tfinish_command(&pager_process);\n-}\n-\n void setup_pager(void)\n {\n+\tpid_t git_child;\n+\tint status;\n \tconst char *pager = getenv(\"GIT_PAGER\");\n \n \tif (!isatty(1))\n@@ -68,14 +60,24 @@ void setup_pager(void)\n \tif (start_command(&pager_process))\n \t\treturn;\n \n-\t/* original process continues, but writes to the pipe */\n-\tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n-\t\tdup2(pager_process.in, 2);\n-\tclose(pager_process.in);\n+\t/* now spawn the actual git process */\n+\tgit_child = fork();\n+\tif (git_child == -1)\n+\t\tdie(\"unable to fork: %s\", strerror(errno));\n+\tif (git_child == 0) {\n+\t\tdup2(pager_process.in, 1);\n+\t\tif (isatty(2))\n+\t\t\tdup2(pager_process.in, 2);\n+\t\tclose(pager_process.in);\n+\t\treturn;\n+\t}\n \n-\t/* this makes sure that the parent terminates after the pager */\n-\tatexit(wait_for_pager);\n+\t/* and the original process just waits for both to finish */\n+\tclose(pager_process.in);\n+\tif (waitpid(git_child, &status, 0) < 0)\n+\t\tdie(\"wait failure: %s\", strerror(errno));\n+\tfinish_command(&pager_process);\n+\texit(status);\n }\n \n int pager_in_use(void)\n"},{"id":"99772","messageId":"7veizc25e8.fsf@gitster.siamese.dyndns.org","threadId":"17049","inReplyTo":"20090109083836.GB21389@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-09T09:30:23Z","receivedAt":"2009-01-09T09:30:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So the _real_ problem is that we are not always triggering the \"wait for\n> pager to finish\" code because we exec and forget about it. Which means\n> this strategy of \"git runs child pager\" will never work properly.\n> Instead, we have to use three processes: git and the pager become child\n> processes, while the original process waits for both to exit and returns\n> the proper exit code from git.\n>\n> Let me try to work up a patch.\n\nThis arrangement to have the third process could even open the possibility\nof having it read from git and write to pager, and not launching the pager\nif there is no interesting data from git to feed it with.\n\nI do not know if I like the performance implications associated with it,\nthough.\n"},{"id":"99773","messageId":"20090109093307.GA2039@coredump.intra.peff.net","threadId":"17049","inReplyTo":"7veizc25e8.fsf@gitster.siamese.dyndns.org","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T09:33:07Z","receivedAt":"2009-01-09T09:33:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 01:30:23AM -0800, Junio C Hamano wrote:\n\n> This arrangement to have the third process could even open the possibility\n> of having it read from git and write to pager, and not launching the pager\n> if there is no interesting data from git to feed it with.\n> \n> I do not know if I like the performance implications associated with it,\n> though.\n\nUgh. That has definitely been a requested feature, but the thought of\nessentially running \"cat\" in our pipeline strikes me as a bit kludgey.\n\nOn the other hand, we are by definition going to the pager in that case,\nso in theory performance is less of a consideration.\n\nBut see my other mail for why a third process is hard to always do on\nWindows.\n\n-Peff\n"},{"id":"99774","messageId":"7vaba024zw.fsf@gitster.siamese.dyndns.org","threadId":"17049","inReplyTo":"20090109093307.GA2039@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-09T09:38:59Z","receivedAt":"2009-01-09T09:38:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jan 09, 2009 at 01:30:23AM -0800, Junio C Hamano wrote:\n>\n>> This arrangement to have the third process could even open the possibility\n>> of having it read from git and write to pager, and not launching the pager\n>> if there is no interesting data from git to feed it with.\n>> \n>> I do not know if I like the performance implications associated with it,\n>> though.\n>\n> Ugh. That has definitely been a requested feature, but the thought of\n> essentially running \"cat\" in our pipeline strikes me as a bit kludgey.\n>\n> On the other hand, we are by definition going to the pager in that case,\n> so in theory performance is less of a consideration.\n>\n> But see my other mail for why a third process is hard to always do on\n> Windows.\n\nHeh, this late at night just before going to bed, I am allowed to say that\nI do not care about Windows at all ;-).  More dedicated and competent\npeople will solve it for us while I am sleeping.\n"},{"id":"99775","messageId":"20090109094843.GA4056@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090109092250.GA1809@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T09:48:43Z","receivedAt":"2009-01-09T09:48:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 04:22:50AM -0500, Jeff King wrote:\n\n> So I think to do things right, we have to be even more complicated. When\n> we spawn the pager, we keep git as a single process. We register the\n> atexit() handler to wait for the pager, and intercept any death signals\n> to do the same. Then, if we are running a builtin, it is business as\n> usual. But if we want to exec something, instead we have to actually\n> spawn into the three-process form. Meaning we have to use run_command to\n> start it, and then wait for it and the pager to return.\n\nActually, this turned out to be quite a small patch. It fixes the\nproblem you were seeing, and it should work on Windows. It incurs an\nextra fork() for every dashed-external that we try.\n\nThere is a little noise in the patch, so let me highlight the three\nthings that are happening (a \"real\" patch should break this into three\npatches in a series):\n\n  1. The noise is from renaming the static run_command, which conflicts\n     with the definition in run_command.h\n\n  2. Substituting run_command for execvp.\n\n  3. run_command needs to signal \"I couldn't exec this\" as opposed to\n     other errors, since we care about the difference here. I do this\n     here with exit(127), but probably we should recognize this in\n     wait_or_whine and hand out the same error code for posix and mingw\n     platforms.\n\nAs for the extra fork, we could do away with it if we scanned the path\nlooking for the external before just exec'ing it. I think this is better\nin the long run anyway, because then we can do other setup specific to\nrunning an external command (I don't remember the details, but I ran\nafoul of this when I was doing pager stuff a while ago).\n\nPatch is below.\n\n---\ndiff --git a/git.c b/git.c\nindex ecc8fad..fa946b9 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]\";\n@@ -219,7 +220,7 @@ struct cmd_struct {\n \tint option;\n };\n \n-static int run_command(struct cmd_struct *p, int argc, const char **argv)\n+static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n {\n \tint status;\n \tstruct stat st;\n@@ -384,7 +385,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n-\t\texit(run_command(p, argc, argv));\n+\t\texit(run_builtin(p, argc, argv));\n \t}\n }\n \n@@ -392,6 +393,7 @@ static void execv_dashed_external(const char **argv)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *tmp;\n+\tint status;\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\n \n@@ -406,8 +408,13 @@ static void execv_dashed_external(const char **argv)\n \n \ttrace_argv_printf(argv, \"trace: exec:\");\n \n-\t/* execvp() can only ever return if it fails */\n-\texecvp(cmd.buf, (char **)argv);\n+\t/*\n+\t * if we failed because the command was not found, it is\n+\t * OK to return. Otherwise, we just pass along the status code.\n+\t */\n+\tstatus = run_command_v_opt(argv, 0);\n+\tif (status != 127 && status != -ERR_RUN_COMMAND_FORK)\n+\t\texit(-status);\n \n \ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n \ndiff --git a/run-command.c b/run-command.c\nindex c90cdc5..539609e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -118,7 +118,7 @@ int start_command(struct child_process *cmd)\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\tdie(\"exec %s failed.\", cmd->argv[0]);\n+\t\texit(127);\n \t}\n #else\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n"},{"id":"99777","messageId":"49672244.80200@viscovery.net","threadId":"17049","inReplyTo":"20090109092250.GA1809@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-09T10:09:08Z","receivedAt":"2009-01-09T10:09:08Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> Below is a patch that uses the three-process mechanism, and it fixes the\n> problem. _But_ it is not satisfactory for inclusion, because it won't\n> work on MINGW32. Since it is actually splitting git into two processes\n> (one to monitor the pager and one to actually run git), it uses fork.\n\nWe have start_async()/finish_async() to replace a fork() of the sort that\nwe have here.\n\n> IOW, I think\n> the breakage you are seeing has always been broken on MINGW32.\n\nIndeed. Hitting Ctrl-C while the pager was open has messed up the console\nsince day 1.\n\n-- Hannes\n"},{"id":"99778","messageId":"20090109101335.GA4346@coredump.intra.peff.net","threadId":"17049","inReplyTo":"49672244.80200@viscovery.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T10:13:35Z","receivedAt":"2009-01-09T10:13:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 11:09:08AM +0100, Johannes Sixt wrote:\n\n> > Below is a patch that uses the three-process mechanism, and it fixes the\n> > problem. _But_ it is not satisfactory for inclusion, because it won't\n> > work on MINGW32. Since it is actually splitting git into two processes\n> > (one to monitor the pager and one to actually run git), it uses fork.\n> \n> We have start_async()/finish_async() to replace a fork() of the sort that\n> we have here.\n\nIt looks like start_async is implemented using threads on Windows. Will\nthat survive an execvp call? Because we don't know at this point whether\nwe are going to actually run builtin code, or if we will exec an\nexternal.\n\n-Peff\n"},{"id":"99781","messageId":"496728B9.7090200@viscovery.net","threadId":"17049","inReplyTo":"20090109101335.GA4346@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-09T10:36:41Z","receivedAt":"2009-01-09T10:36:41Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Fri, Jan 09, 2009 at 11:09:08AM +0100, Johannes Sixt wrote:\n> \n>>> Below is a patch that uses the three-process mechanism, and it fixes the\n>>> problem. _But_ it is not satisfactory for inclusion, because it won't\n>>> work on MINGW32. Since it is actually splitting git into two processes\n>>> (one to monitor the pager and one to actually run git), it uses fork.\n>> We have start_async()/finish_async() to replace a fork() of the sort that\n>> we have here.\n> \n> It looks like start_async is implemented using threads on Windows. Will\n> that survive an execvp call? Because we don't know at this point whether\n> we are going to actually run builtin code, or if we will exec an\n> external.\n\nAh, no, it would not survive.\n\nBut there's a more serious problem why we cannot use start_async() in its\ncurrent form: It expects that there is a *function* that produces the\noutput; but here we don't have a function - output is produced by\n*returning* (from setup_pager).\n\nI'll test your other patch (that replaces the execvp in git.c by run_command).\n\n-- Hannes\n"},{"id":"99782","messageId":"20090109104718.GA11033@coredump.intra.peff.net","threadId":"17049","inReplyTo":"496728B9.7090200@viscovery.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-09T10:47:18Z","receivedAt":"2009-01-09T10:47:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:\n\n> Ah, no, it would not survive.\n> \n> But there's a more serious problem why we cannot use start_async() in its\n> current form: It expects that there is a *function* that produces the\n> output; but here we don't have a function - output is produced by\n> *returning* (from setup_pager).\n\nGood point.\n\n> I'll test your other patch (that replaces the execvp in git.c by run_command).\n\nNote that it only covers the case of external commands. Hitting ctrl-c\nto interrupt git will still cause funniness. For that we need to\nintercept signals to call wait_for_pager(). But there is a slight snag:\nwe also intercept signals elsewhere (e.g., for tempfile cleanup). So we\nneed to start remembering the old signal handlers everywhere and\nchaining to them.\n\n-Peff\n"},{"id":"99945","messageId":"20090111112222.GA29656@coredump.intra.peff.net","threadId":"17049","inReplyTo":"496728B9.7090200@viscovery.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:22:22Z","receivedAt":"2009-01-11T11:22:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:\n\n> I'll test your other patch (that replaces the execvp in git.c by\n> run_command).\n\nThere is something funny with it that I have not diagnosed: aliases are\nbroken, and \"git foobar\" does not return an error. Presumably just\nchecking the \"we did not exec succesfully\" case is not triggering\nproperly.  However, I think the right solution is actually to refactor\ngit.c to figure out ahead of time whether we have a builtin, external,\nor alias. I can work on that, but not tonight, as my git-time is up for\nnow.\n\nBut other than that, did it work for you on Windows?\n\nHowever, here is a 4-patch series that handles the separate signal\ndelivery problem. It should fix the \"^C makes funny things happen\"\nproblems you were seeing. Please test and let me know how it works on\nWindows.\n\nThe patches are:\n  1/4: Makefile: clean up TEST_PROGRAMS definition\n  2/4: chain kill signals for cleanup functions\n  3/4: refactor signal handling for cleanup functions\n  4/4: pager: do wait_for_pager on signal death\n\n-Peff\n"},{"id":"99946","messageId":"20090111112506.GA29791@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090111112222.GA29656@coredump.intra.peff.net","subject":"[PATCH 1/4] Makefile: clean up TEST_PROGRAMS definition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:25:06Z","receivedAt":"2009-01-11T11:25:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We try to keep lines under 80 characters, not to mention\nthat sticking a bunch of stuff on one line makes diffs\nmessier.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust a cleanup I noticed while working on 2/4.\n\n Makefile |    9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex dee97c1..2b873fa 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1356,7 +1356,14 @@ endif\n \n ### Testing rules\n \n-TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-parse-options$X test-path-utils$X\n+TEST_PROGRAMS += test-chmtime$X\n+TEST_PROGRAMS += test-date$X\n+TEST_PROGRAMS += test-delta$X\n+TEST_PROGRAMS += test-genrandom$X\n+TEST_PROGRAMS += test-match-trees$X\n+TEST_PROGRAMS += test-parse-options$X\n+TEST_PROGRAMS += test-path-utils$X\n+TEST_PROGRAMS += test-sha1$X\n \n all:: $(TEST_PROGRAMS)\n \n-- \n1.6.1.84.g8150\n"},{"id":"99947","messageId":"20090111113211.GB29791@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090111112222.GA29656@coredump.intra.peff.net","subject":"[PATCH 2/4] chain kill signals for cleanup functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:32:12Z","receivedAt":"2009-01-11T11:32:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a piece of code wanted to do some cleanup before exiting\n(e.g., cleaing up a lockfile or a tempfile), our usual\nstrategy was to install a signal handler that did something\nlike this:\n\n  do_cleanup(); /* actual work */\n  signal(signo, SIG_DFL); /* restore previous behavior */\n  raise(signo); /* deliver signal, killing ourselves */\n\nFor a single handler, this works fine. However, if we want\nto clean up two _different_ things, we run into a problem.\nThe most recently installed handler will run, but when it\nremoves itself as a handler, it doesn't put back the first\nhandler.\n\nThis patch introduces sigchain, a tiny library for handling\na stack of signal handlers. You sigchain_push each handler,\nand use sigchain_pop to restore whoever was before you in\nthe stack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't know if it is possible to trigger this issue with current git or\nnot. There are several instances that catch signals in different ways,\nbut I don't know if there are code paths that actually combine them. So\nmaybe it is fixing a bug, but it is definitely needed for 4/4, which\nwill install the wait_for_pager handler in most git runs.\n\nI tried to neither over- nor under-engineer. On one hand, you could\nactually accomplish the same thing by always keeping a \"this was the\nlast handler\" pointer at each callsite when installing the new handler.\nBut it's easy to mess up, so I think a little library helps keep the\ncode clean.\n\nOn the other hand, callsites still have to have separate clean_foo() and\nclean_foo_on_signal() handlers, and manually pop() and raise() in the\nlatter. If the code just assumed you always wanted to chain to the next\nhandler immediately, then we could cut out even more client code.\n\nThe test script is probably overkill, but I wrote it to convince myself\nI hadn't screwed up too badly (and because we probably wouldn't even\nnotice, as signals aren't well tested in our scripts). So I figured I\nmight as well share it.\n\n .gitignore            |    1 +\n Makefile              |    3 +++\n builtin-clone.c       |    5 +++--\n builtin-fetch--tool.c |    5 +++--\n builtin-fetch.c       |    5 +++--\n diff.c                |    5 +++--\n http-push.c           |   11 ++++++-----\n lockfile.c            |   13 +++++++------\n sigchain.c            |   43 +++++++++++++++++++++++++++++++++++++++++++\n sigchain.h            |    9 +++++++++\n t/t0005-signals.sh    |   18 ++++++++++++++++++\n test-sigchain.c       |   22 ++++++++++++++++++++++\n 12 files changed, 121 insertions(+), 19 deletions(-)\n create mode 100644 sigchain.c\n create mode 100644 sigchain.h\n create mode 100755 t/t0005-signals.sh\n create mode 100644 test-sigchain.c\n\ndiff --git a/.gitignore b/.gitignore\nindex d9adce5..f28a54d 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -152,6 +152,7 @@ test-match-trees\n test-parse-options\n test-path-utils\n test-sha1\n+test-sigchain\n common-cmds.h\n *.tar.gz\n *.dsc\ndiff --git a/Makefile b/Makefile\nindex 2b873fa..fd02dec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -388,6 +388,7 @@ LIB_H += revision.h\n LIB_H += run-command.h\n LIB_H += sha1-lookup.h\n LIB_H += sideband.h\n+LIB_H += sigchain.h\n LIB_H += strbuf.h\n LIB_H += tag.h\n LIB_H += transport.h\n@@ -481,6 +482,7 @@ LIB_OBJS += sha1-lookup.o\n LIB_OBJS += sha1_name.o\n LIB_OBJS += shallow.o\n LIB_OBJS += sideband.o\n+LIB_OBJS += sigchain.o\n LIB_OBJS += strbuf.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n@@ -1364,6 +1366,7 @@ TEST_PROGRAMS += test-match-trees$X\n TEST_PROGRAMS += test-parse-options$X\n TEST_PROGRAMS += test-path-utils$X\n TEST_PROGRAMS += test-sha1$X\n+TEST_PROGRAMS += test-sigchain$X\n \n all:: $(TEST_PROGRAMS)\n \ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex f1a1a0c..18b9392 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -19,6 +19,7 @@\n #include \"strbuf.h\"\n #include \"dir.h\"\n #include \"pack-refs.h\"\n+#include \"sigchain.h\"\n \n /*\n  * Overall FIXMEs:\n@@ -288,7 +289,7 @@ static void remove_junk(void)\n static void remove_junk_on_signal(int signo)\n {\n \tremove_junk();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -438,7 +439,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \tjunk_git_dir = git_dir;\n \tatexit(remove_junk);\n-\tsignal(SIGINT, remove_junk_on_signal);\n+\tsigchain_push(SIGINT, remove_junk_on_signal);\n \n \tsetenv(CONFIG_ENVIRONMENT, xstrdup(mkpath(\"%s/config\", git_dir)), 1);\n \ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 469b07e..b1d7f8f 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -2,6 +2,7 @@\n #include \"cache.h\"\n #include \"refs.h\"\n #include \"commit.h\"\n+#include \"sigchain.h\"\n \n static char *get_stdin(void)\n {\n@@ -186,7 +187,7 @@ static void remove_keep(void)\n static void remove_keep_on_signal(int signo)\n {\n \tremove_keep();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -245,7 +246,7 @@ static int fetch_native_store(FILE *fp,\n \tchar buffer[1024];\n \tint err = 0;\n \n-\tsignal(SIGINT, remove_keep_on_signal);\n+\tsigchain_push(SIGINT, remove_keep_on_signal);\n \tatexit(remove_keep);\n \n \twhile (fgets(buffer, sizeof(buffer), stdin)) {\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex de6f307..8c86974 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -10,6 +10,7 @@\n #include \"transport.h\"\n #include \"run-command.h\"\n #include \"parse-options.h\"\n+#include \"sigchain.h\"\n \n static const char * const builtin_fetch_usage[] = {\n \t\"git fetch [options] [<repository> <refspec>...]\",\n@@ -58,7 +59,7 @@ static void unlock_pack(void)\n static void unlock_pack_on_signal(int signo)\n {\n \tunlock_pack();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -672,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tref_nr = j;\n \t}\n \n-\tsignal(SIGINT, unlock_pack_on_signal);\n+\tsigchain_push(SIGINT, unlock_pack_on_signal);\n \tatexit(unlock_pack);\n \texit_code = do_fetch(transport,\n \t\t\tparse_fetch_refspec(ref_nr, refs), ref_nr);\ndiff --git a/diff.c b/diff.c\nindex d235482..5a74012 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -12,6 +12,7 @@\n #include \"run-command.h\"\n #include \"utf8.h\"\n #include \"userdiff.h\"\n+#include \"sigchain.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -1933,7 +1934,7 @@ static void remove_tempfile(void)\n static void remove_tempfile_on_signal(int signo)\n {\n \tremove_tempfile();\n-\tsignal(SIGINT, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -1968,7 +1969,7 @@ static void run_external_diff(const char *pgm,\n \t\t\tatexit_asked = 1;\n \t\t\tatexit(remove_tempfile);\n \t\t}\n-\t\tsignal(SIGINT, remove_tempfile_on_signal);\n+\t\tsigchain_push(SIGINT, remove_tempfile_on_signal);\n \t}\n \n \tif (one && two) {\ndiff --git a/http-push.c b/http-push.c\nindex a4b7d08..dec395d 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -10,6 +10,7 @@\n #include \"exec_cmd.h\"\n #include \"remote.h\"\n #include \"list-objects.h\"\n+#include \"sigchain.h\"\n \n #include <expat.h>\n \n@@ -1363,7 +1364,7 @@ static void remove_locks(void)\n static void remove_locks_on_signal(int signo)\n {\n \tremove_locks();\n-\tsignal(signo, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -2261,10 +2262,10 @@ int main(int argc, char **argv)\n \t\tgoto cleanup;\n \t}\n \n-\tsignal(SIGINT, remove_locks_on_signal);\n-\tsignal(SIGHUP, remove_locks_on_signal);\n-\tsignal(SIGQUIT, remove_locks_on_signal);\n-\tsignal(SIGTERM, remove_locks_on_signal);\n+\tsigchain_push(SIGINT, remove_locks_on_signal);\n+\tsigchain_push(SIGHUP, remove_locks_on_signal);\n+\tsigchain_push(SIGQUIT, remove_locks_on_signal);\n+\tsigchain_push(SIGTERM, remove_locks_on_signal);\n \n \t/* Check whether the remote has server info files */\n \tremote->can_update_info_refs = 0;\ndiff --git a/lockfile.c b/lockfile.c\nindex 8589155..3cd57dc 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -2,6 +2,7 @@\n  * Copyright (c) 2005, Junio C Hamano\n  */\n #include \"cache.h\"\n+#include \"sigchain.h\"\n \n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n@@ -24,7 +25,7 @@ static void remove_lock_file(void)\n static void remove_lock_file_on_signal(int signo)\n {\n \tremove_lock_file();\n-\tsignal(signo, SIG_DFL);\n+\tsigchain_pop(signo);\n \traise(signo);\n }\n \n@@ -136,11 +137,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tif (!lock_file_list) {\n-\t\t\tsignal(SIGINT, remove_lock_file_on_signal);\n-\t\t\tsignal(SIGHUP, remove_lock_file_on_signal);\n-\t\t\tsignal(SIGTERM, remove_lock_file_on_signal);\n-\t\t\tsignal(SIGQUIT, remove_lock_file_on_signal);\n-\t\t\tsignal(SIGPIPE, remove_lock_file_on_signal);\n+\t\t\tsigchain_push(SIGINT, remove_lock_file_on_signal);\n+\t\t\tsigchain_push(SIGHUP, remove_lock_file_on_signal);\n+\t\t\tsigchain_push(SIGTERM, remove_lock_file_on_signal);\n+\t\t\tsigchain_push(SIGQUIT, remove_lock_file_on_signal);\n+\t\t\tsigchain_push(SIGPIPE, remove_lock_file_on_signal);\n \t\t\tatexit(remove_lock_file);\n \t\t}\n \t\tlk->owner = getpid();\ndiff --git a/sigchain.c b/sigchain.c\nnew file mode 100644\nindex 0000000..a18d505\n--- /dev/null\n+++ b/sigchain.c\n@@ -0,0 +1,43 @@\n+#include \"sigchain.h\"\n+#include \"cache.h\"\n+\n+#define SIGCHAIN_MAX_SIGNALS 32\n+\n+struct sigchain_signal {\n+\tsigchain_fun *old;\n+\tint n;\n+\tint alloc;\n+};\n+static struct sigchain_signal signals[SIGCHAIN_MAX_SIGNALS];\n+\n+static void check_signum(int sig)\n+{\n+\tif (sig < 1 || sig >= SIGCHAIN_MAX_SIGNALS)\n+\t\tdie(\"BUG: signal out of range: %d\", sig);\n+}\n+\n+int sigchain_push(int sig, sigchain_fun f)\n+{\n+\tstruct sigchain_signal *s = signals + sig;\n+\tcheck_signum(sig);\n+\n+\tALLOC_GROW(s->old, s->n + 1, s->alloc);\n+\ts->old[s->n] = signal(sig, f);\n+\tif (s->old[s->n] == SIG_ERR)\n+\t\treturn -1;\n+\ts->n++;\n+\treturn 0;\n+}\n+\n+int sigchain_pop(int sig)\n+{\n+\tstruct sigchain_signal *s = signals + sig;\n+\tcheck_signum(sig);\n+\tif (s->n < 1)\n+\t\treturn 0;\n+\n+\tif (signal(sig, s->old[s->n - 1]) == SIG_ERR)\n+\t\treturn -1;\n+\ts->n--;\n+\treturn 0;\n+}\ndiff --git a/sigchain.h b/sigchain.h\nnew file mode 100644\nindex 0000000..254ebb0\n--- /dev/null\n+++ b/sigchain.h\n@@ -0,0 +1,9 @@\n+#ifndef SIGCHAIN_H\n+#define SIGCHAIN_H\n+\n+typedef void (*sigchain_fun)(int);\n+\n+int sigchain_push(int sig, sigchain_fun f);\n+int sigchain_pop(int sig);\n+\n+#endif /* SIGCHAIN_H */\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nnew file mode 100755\nindex 0000000..d900df4\n--- /dev/null\n+++ b/t/t0005-signals.sh\n@@ -0,0 +1,18 @@\n+#!/bin/sh\n+\n+test_description='signals work as we expect'\n+. ./test-lib.sh\n+\n+cat >expect <<EOF\n+three\n+two\n+one\n+EOF\n+\n+test_expect_success 'sigchain works' '\n+  test-sigchain >actual\n+  test \"$?\" = 130 &&\n+  test_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/test-sigchain.c b/test-sigchain.c\nnew file mode 100644\nindex 0000000..8747dea\n--- /dev/null\n+++ b/test-sigchain.c\n@@ -0,0 +1,22 @@\n+#include \"sigchain.h\"\n+#include \"cache.h\"\n+\n+#define X(f) \\\n+static void f(int sig) { \\\n+\tputs(#f); \\\n+\tfflush(stdout); \\\n+\tsigchain_pop(sig); \\\n+\traise(sig); \\\n+}\n+X(one)\n+X(two)\n+X(three)\n+#undef X\n+\n+int main(int argc, char **argv) {\n+\tsigchain_push(SIGINT, one);\n+\tsigchain_push(SIGINT, two);\n+\tsigchain_push(SIGINT, three);\n+\traise(SIGINT);\n+\treturn 0;\n+}\n-- \n1.6.1.84.g8150\n"},{"id":"99948","messageId":"20090111113627.GC29791@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090111112222.GA29656@coredump.intra.peff.net","subject":"[PATCH 3/4] refactor signal handling for cleanup functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:36:27Z","receivedAt":"2009-01-11T11:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current code is very inconsistent about which signals\nare caught for doing cleanup of temporary files and lock\nfiles. Some callsites checked only SIGINT, while others\nchecked a variety of death-dealing signals.\n\nThis patch factors out those signals to a single function,\nand then calls it everywhere. For some sites, that means\nthis is a simple clean up. For others, it is an improvement\nin that they will now properly clean themselves up after a\nlarger variety of signals.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm assuming there was no good reason _not_ to be handling those other\nsignals at some of the \"just handle SIGINT\" sites. A sigchain\nimplementation which handled \"remove_lock_file\" without needing\n\"remove_lock_file_on_signal\" could also call atexit(), too. So you could\nhave a one liner\n\n  register_cleanup(remove_lock_file);\n\nHowever, there is one case that doesn't use an atexit() handler:\nhttp-push.c. I don't know if this is a bug or an intentional omission.\n\n builtin-clone.c       |    2 +-\n builtin-fetch--tool.c |    2 +-\n builtin-fetch.c       |    2 +-\n diff.c                |    2 +-\n http-push.c           |    5 +----\n lockfile.c            |    6 +-----\n sigchain.c            |    9 +++++++++\n sigchain.h            |    2 ++\n 8 files changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 18b9392..44c8073 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -439,7 +439,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \tjunk_git_dir = git_dir;\n \tatexit(remove_junk);\n-\tsigchain_push(SIGINT, remove_junk_on_signal);\n+\tsigchain_push_common(remove_junk_on_signal);\n \n \tsetenv(CONFIG_ENVIRONMENT, xstrdup(mkpath(\"%s/config\", git_dir)), 1);\n \ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex b1d7f8f..29356d2 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -246,7 +246,7 @@ static int fetch_native_store(FILE *fp,\n \tchar buffer[1024];\n \tint err = 0;\n \n-\tsigchain_push(SIGINT, remove_keep_on_signal);\n+\tsigchain_push_common(remove_keep_on_signal);\n \tatexit(remove_keep);\n \n \twhile (fgets(buffer, sizeof(buffer), stdin)) {\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 8c86974..1e4a3d9 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -673,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tref_nr = j;\n \t}\n \n-\tsigchain_push(SIGINT, unlock_pack_on_signal);\n+\tsigchain_push_common(unlock_pack_on_signal);\n \tatexit(unlock_pack);\n \texit_code = do_fetch(transport,\n \t\t\tparse_fetch_refspec(ref_nr, refs), ref_nr);\ndiff --git a/diff.c b/diff.c\nindex 5a74012..7fc8512 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1969,7 +1969,7 @@ static void run_external_diff(const char *pgm,\n \t\t\tatexit_asked = 1;\n \t\t\tatexit(remove_tempfile);\n \t\t}\n-\t\tsigchain_push(SIGINT, remove_tempfile_on_signal);\n+\t\tsigchain_push_common(remove_tempfile_on_signal);\n \t}\n \n \tif (one && two) {\ndiff --git a/http-push.c b/http-push.c\nindex dec395d..7d5c23e 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -2262,10 +2262,7 @@ int main(int argc, char **argv)\n \t\tgoto cleanup;\n \t}\n \n-\tsigchain_push(SIGINT, remove_locks_on_signal);\n-\tsigchain_push(SIGHUP, remove_locks_on_signal);\n-\tsigchain_push(SIGQUIT, remove_locks_on_signal);\n-\tsigchain_push(SIGTERM, remove_locks_on_signal);\n+\tsigchain_push_common(remove_locks_on_signal);\n \n \t/* Check whether the remote has server info files */\n \tremote->can_update_info_refs = 0;\ndiff --git a/lockfile.c b/lockfile.c\nindex 3cd57dc..021c337 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -137,11 +137,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tif (!lock_file_list) {\n-\t\t\tsigchain_push(SIGINT, remove_lock_file_on_signal);\n-\t\t\tsigchain_push(SIGHUP, remove_lock_file_on_signal);\n-\t\t\tsigchain_push(SIGTERM, remove_lock_file_on_signal);\n-\t\t\tsigchain_push(SIGQUIT, remove_lock_file_on_signal);\n-\t\t\tsigchain_push(SIGPIPE, remove_lock_file_on_signal);\n+\t\t\tsigchain_push_common(remove_lock_file_on_signal);\n \t\t\tatexit(remove_lock_file);\n \t\t}\n \t\tlk->owner = getpid();\ndiff --git a/sigchain.c b/sigchain.c\nindex a18d505..1118b99 100644\n--- a/sigchain.c\n+++ b/sigchain.c\n@@ -41,3 +41,12 @@ int sigchain_pop(int sig)\n \ts->n--;\n \treturn 0;\n }\n+\n+void sigchain_push_common(sigchain_fun f)\n+{\n+\tsigchain_push(SIGINT, f);\n+\tsigchain_push(SIGHUP, f);\n+\tsigchain_push(SIGTERM, f);\n+\tsigchain_push(SIGQUIT, f);\n+\tsigchain_push(SIGPIPE, f);\n+}\ndiff --git a/sigchain.h b/sigchain.h\nindex 254ebb0..618083b 100644\n--- a/sigchain.h\n+++ b/sigchain.h\n@@ -6,4 +6,6 @@ typedef void (*sigchain_fun)(int);\n int sigchain_push(int sig, sigchain_fun f);\n int sigchain_pop(int sig);\n \n+void sigchain_push_common(sigchain_fun f);\n+\n #endif /* SIGCHAIN_H */\n-- \n1.6.1.84.g8150\n"},{"id":"99949","messageId":"20090111113649.GD29791@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090111112222.GA29656@coredump.intra.peff.net","subject":"[PATCH 4/4] pager: do wait_for_pager on signal death","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:36:49Z","receivedAt":"2009-01-11T11:36:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since ea27a18 (spawn pager via run_command interface), the\noriginal git process actually does git work, and the pager\nis a child process (actually, on Windows it has always been\nthat way, since Windows lacks fork). After spawning the\npager, we register an atexit() handler that waits for the\npager to finish.\n\nUnfortunately, that handler does not always run. In\nparticular, if git is killed by a signal, then we exit\nimmediately. The calling shell then thinks that git is done;\nhowever, the pager is still trying to run and impact the\nterminal. The result can be seen by running a long git\nprocess with a pager (e.g., \"git log -p\") and hitting ^C.\nDepending on your config, you should see the shell prompt,\nbut pressing a key causes the pager to do any terminal\nde-initialization sequence.\n\nThis patch just intercepts any death-dealing signals and\nwaits for the pager before dying. Under typical less\nconfiguration, that means hitting ^C will cause git to stop\ngenerating output, but the pager will keep running.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pager.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex f19ddbc..4921843 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n+#include \"sigchain.h\"\n \n /*\n  * This is split up from the rest of git so that we can do\n@@ -38,6 +39,13 @@ static void wait_for_pager(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_signal(int signo)\n+{\n+\twait_for_pager();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n void setup_pager(void)\n {\n \tconst char *pager = getenv(\"GIT_PAGER\");\n@@ -75,6 +83,7 @@ void setup_pager(void)\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n+\tsigchain_push_common(wait_for_pager_signal);\n \tatexit(wait_for_pager);\n }\n \n-- \n1.6.1.84.g8150\n"},{"id":"99950","messageId":"20090111114032.GA30032@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090111113211.GB29791@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] chain kill signals for cleanup functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-11T11:40:32Z","receivedAt":"2009-01-11T11:40:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 11, 2009 at 06:32:12AM -0500, Jeff King wrote:\n\n> @@ -1968,7 +1969,7 @@ static void run_external_diff(const char *pgm,\n>  \t\t\tatexit_asked = 1;\n>  \t\t\tatexit(remove_tempfile);\n>  \t\t}\n> -\t\tsignal(SIGINT, remove_tempfile_on_signal);\n> +\t\tsigchain_push(SIGINT, remove_tempfile_on_signal);\n\nHmm.\n\nNote that because we are now pushing instead of just replacing the\nsignal handler, it might matter if it gets called multiple times (though\nI think most of the cleanup functions are relatively harmless if run\nmultiple times). Most of the callsites protect against installing the\nsignal handler twice, but I think this one should probably be moved up\ninside the atexit_asked condition:\n\n  if (! atexit_asked &&\n      (temp[0].name == temp[0].tmp_path ||\n       temp[1].name == temp[1].tmp_path)) {\n          atexit_asked = 1;\n          atexit(remove_tempfile);\n  }\n  sigchain_push_common(remove_tempfile_on_signal);\n\n-Peff\n"},{"id":"100008","messageId":"7vzlhxr1g8.fsf@gitster.siamese.dyndns.org","threadId":"17049","inReplyTo":"20090111113649.GD29791@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] pager: do wait_for_pager on signal death","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-11T21:13:11Z","receivedAt":"2009-01-11T21:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; I agree with you that I do not see a good reason _not_ to be\nhandling those other signals at some of the \"just handle SIGINT\" sites,\nand I like the direction in whcih this series is taking us very much.\n"},{"id":"100102","messageId":"496B2278.9050905@viscovery.net","threadId":"17049","inReplyTo":"20090111112222.GA29656@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-12T10:59:04Z","receivedAt":"2009-01-12T10:59:04Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt <j6t@kdbg.org>\n\nJeff King schrieb:\n> On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:\n> \n>> I'll test your other patch (that replaces the execvp in git.c by\n>> run_command).\n> \n> There is something funny with it that I have not diagnosed: aliases are\n> broken, and \"git foobar\" does not return an error. Presumably just\n> checking the \"we did not exec succesfully\" case is not triggering\n> properly.  However, I think the right solution is actually to refactor\n> git.c to figure out ahead of time whether we have a builtin, external,\n> or alias. I can work on that, but not tonight, as my git-time is up for\n> now.\n> \n> But other than that, did it work for you on Windows?\n\nIt passed the test suite. This should already work better on Windows,\nbecause we already *do* look-up the program, and exit from\nmingw_spawnvpe() before the equivalent of fork+exec happens.\n\n> However, here is a 4-patch series that handles the separate signal\n> delivery problem. It should fix the \"^C makes funny things happen\"\n> problems you were seeing. Please test and let me know how it works on\n> Windows.\n\nIt does help a bit. The interesting thing is that the only case where I\ncan now reproduces the unwanted behavior with the unpatched version is\nwhen all output was completely read by 'less' and git already waits in\nwait_for_pager(), such as in 'git show'. But Ctrl-C'ing a 'git log -p'\nworks as expected even without these patches. With the patches, the 'git\nshow' case now works as well.\n\n> The patches are:\n>   1/4: Makefile: clean up TEST_PROGRAMS definition\n>   2/4: chain kill signals for cleanup functions\n>   3/4: refactor signal handling for cleanup functions\n>   4/4: pager: do wait_for_pager on signal death\n\nBut we need to insert the patch below *before* 2/4. The test case needs a\nchange, too,(exit code on Windows is 3, not 130) but I'll keep that in my\nrepository, like with all other Windows related test suite changes.\n\n-- Hannes\n\n-- 8< --\nFrom: Johannes Sixt <j6t@kdbg.org>\nSubject: Windows: Fix signal numbers\n\nWe had defined some SIG_FOO macros that appear in the code, but that are\nnot supported on Windows, in order to make the code compile.  But a\nsubsequent change will assert that a signal number is non-zero.  We now\nuse the signal numbers that are commonly used on POSIX systems.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.h |   12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 4f275cb..a255898 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -21,12 +21,12 @@ typedef int pid_t;\n #define WEXITSTATUS(x) ((x) & 0xff)\n #define WIFSIGNALED(x) ((unsigned)(x) > 259)\n\n-#define SIGKILL 0\n-#define SIGCHLD 0\n-#define SIGPIPE 0\n-#define SIGHUP 0\n-#define SIGQUIT 0\n-#define SIGALRM 100\n+#define SIGHUP 1\n+#define SIGQUIT 3\n+#define SIGKILL 9\n+#define SIGPIPE 13\n+#define SIGALRM 14\n+#define SIGCHLD 17\n\n #define F_GETFD 1\n #define F_SETFD 2\n-- \n1.6.1.rc4.959.gcece.dirty\n"},{"id":"100105","messageId":"20090112112109.GA3825@coredump.intra.peff.net","threadId":"17049","inReplyTo":"496B2278.9050905@viscovery.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-12T11:21:09Z","receivedAt":"2009-01-12T11:21:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2009 at 11:59:04AM +0100, Johannes Sixt wrote:\n\n> But we need to insert the patch below *before* 2/4. The test case needs a\n> change, too,(exit code on Windows is 3, not 130) but I'll keep that in my\n> repository, like with all other Windows related test suite changes.\n\nHrm. How do you properly detect \"killed by SIGINT\" on Windows? That is\nthe intent of that test.\n\n> -#define SIGKILL 0\n> -#define SIGCHLD 0\n> -#define SIGPIPE 0\n> -#define SIGHUP 0\n> -#define SIGQUIT 0\n> -#define SIGALRM 100\n> +#define SIGHUP 1\n> +#define SIGQUIT 3\n> +#define SIGKILL 9\n> +#define SIGPIPE 13\n> +#define SIGALRM 14\n> +#define SIGCHLD 17\n\nDon't these get fed to signal()? Does Windows really not care about\ngetting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or\nare we just ignoring the return code everywhere?\n\n-Peff\n"},{"id":"100110","messageId":"496B30C5.2060207@viscovery.net","threadId":"17049","inReplyTo":"20090112112109.GA3825@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-12T12:00:05Z","receivedAt":"2009-01-12T12:00:05Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> Hrm. How do you properly detect \"killed by SIGINT\" on Windows?\n\nGood question. The exit code is 0xc000013a if the signal was generated by\nhitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the\nsignal was \"generated\" by 'raise(SIGINT)'. The implementation of the\nlatter just calls _exit(3) (same for other deadly signals). Wow!\n\n>> -#define SIGKILL 0\n>> -#define SIGCHLD 0\n>> -#define SIGPIPE 0\n>> -#define SIGHUP 0\n>> -#define SIGQUIT 0\n>> -#define SIGALRM 100\n>> +#define SIGHUP 1\n>> +#define SIGQUIT 3\n>> +#define SIGKILL 9\n>> +#define SIGPIPE 13\n>> +#define SIGALRM 14\n>> +#define SIGCHLD 17\n> \n> Don't these get fed to signal()? Does Windows really not care about\n> getting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or\n> are we just ignoring the return code everywhere?\n\nWindows's signal() does return EINVAL; appearently, we never check the\nreturn code. :-P\n\n-- Hannes\n"},{"id":"100111","messageId":"20090112120345.GA12087@coredump.intra.peff.net","threadId":"17049","inReplyTo":"496B30C5.2060207@viscovery.net","subject":"Re: Funny: git -p submodule summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-12T12:03:45Z","receivedAt":"2009-01-12T12:03:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2009 at 01:00:05PM +0100, Johannes Sixt wrote:\n\n> Good question. The exit code is 0xc000013a if the signal was generated by\n> hitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the\n> signal was \"generated\" by 'raise(SIGINT)'. The implementation of the\n> latter just calls _exit(3) (same for other deadly signals). Wow!\n\nEh? So it doesn't even run the signal handlers? Or if it is SIG_DFL\n(i.e., we have already run all the handlers), then it just calls _exit?\n\n> > Don't these get fed to signal()? Does Windows really not care about\n> > getting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or\n> > are we just ignoring the return code everywhere?\n> \n> Windows's signal() does return EINVAL; appearently, we never check the\n> return code. :-P\n\nOK. That makes sense; we don't bother checking the return code because\nit's mostly just registering a cleanup function. The only sane thing to\ndo on an error (besides ignore it) is to abort whatever we're doing, and\nI'm not sure it's worth it.\n\nI do feel a little like this is a trap waiting to spring for future\ncoders to break Windows, but I think your change doesn't make anything\n_worse_ in that regard.\n\n-Peff\n"},{"id":"100113","messageId":"496B353E.5070701@viscovery.net","threadId":"17049","inReplyTo":"20090112120345.GA12087@coredump.intra.peff.net","subject":"Re: Funny: git -p submodule summary","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-12T12:19:10Z","receivedAt":"2009-01-12T12:19:10Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Mon, Jan 12, 2009 at 01:00:05PM +0100, Johannes Sixt wrote:\n> \n>> Good question. The exit code is 0xc000013a if the signal was generated by\n>> hitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the\n>> signal was \"generated\" by 'raise(SIGINT)'. The implementation of the\n>> latter just calls _exit(3) (same for other deadly signals). Wow!\n> \n> Eh? So it doesn't even run the signal handlers? Or if it is SIG_DFL\n> (i.e., we have already run all the handlers), then it just calls _exit?\n\nIt's silly, but not *that* silly: It calls _exit(3) only if the handler is\nSIG_DFL.\n\n-- Hannes\n"},{"id":"102088","messageId":"20090127062512.GA10487@coredump.intra.peff.net","threadId":"17049","inReplyTo":"alpine.DEB.1.00.0901081601240.30769@pacific.mpi-cbg.de","subject":"[RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T06:25:12Z","receivedAt":"2009-01-27T06:25:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:\n\n> Just try this with a submodule that has more changes than fit on a screen:\n> \n> \t$ git -p submodule summary\n> \n> In my tests, it consistently fscks up my console.  I wonder if this is \n> related to ea27a18(spawn pager via run_command interface).\n\nOK, here is a patch series that fixes the problem:\n\n  1/3: git: s/run_command/run_builtin/\n  2/3: run_command: handle missing command errors more gracefully\n  3/3: git: use run_command to execute dashed externals\n\n1 is a cleanup, 2 is infrastructure support, and 3 is the actual fix.\n\nThere are two potential downsides to the fix:\n\n 1. There is an extra fork and a parent process sitting in memory for\n    dashed externals. This is pretty necessary to any fix, since\n    something has to wait to do pager cleanup, and we can't rely on the\n    child to do so.\n\n 2. A failed attempt to execute a dashed external results in an extra\n    fork. For builtins, this has no impact, since they take precedence.\n    For aliases, though, it means we will do an extra fork before\n    realizing that there is no dashed external and trying the alias.\n\nWe can fix '2' by actually doing the PATH lookup ourselves, and only\ncalling run_command if we know we have a match.  We can also reduce the\nimpact of both by only doing this multi-process magic if we have spawned\na pager; then only a small subset of invocations needs to pay for it.\n\nI chose not to do the second optimization because it makes the code more\ncomplex and inconsistent (we now have two different ways of doing the\nsame thing, depending on a seemingly unrelated setting) and fragile\n(the pager might not be the only atexit handler installed).\n\nThe first (doing PATH lookup ourselves) might make sense, though.\n\nJSixt, there are some tweaks to the Windows code to report back the exec\nerror. They look obviously correct to me, but I have no box to test\n(even compile test) them on.\n\n-Peff\n"},{"id":"102089","messageId":"20090127062632.GA13161@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090127062512.GA10487@coredump.intra.peff.net","subject":"[RFC/PATCH 1/3] git: s/run_command/run_builtin/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T06:26:32Z","receivedAt":"2009-01-27T06:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is a static function called run_command which\nconflicts with the library function in run-command.c; this\nisn't a problem currently, but prevents including\nrun-command.h in git.c.\n\nThis patch just renames the static function to something\nmore specific and non-conflicting.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex ecc8fad..45e493d 100644\n--- a/git.c\n+++ b/git.c\n@@ -219,7 +219,7 @@ struct cmd_struct {\n \tint option;\n };\n \n-static int run_command(struct cmd_struct *p, int argc, const char **argv)\n+static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n {\n \tint status;\n \tstruct stat st;\n@@ -384,7 +384,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n-\t\texit(run_command(p, argc, argv));\n+\t\texit(run_builtin(p, argc, argv));\n \t}\n }\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102090","messageId":"20090127062705.GB13161@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090127062512.GA10487@coredump.intra.peff.net","subject":"[RFC/PATCH 2/3] run_command: handle missing command errors more gracefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T06:27:05Z","receivedAt":"2009-01-27T06:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When run_command was asked to run a non-existant command,\nits behavior varied depending on the platform:\n\n  - on POSIX systems, we would fork, and then after the\n    execvp call failed, we could call die(), which prints a\n    message to stderr and exits with code 128.\n\n  - on Windows, we do a PATH lookup, realize the program\n    isn't there, and then return ERR_RUN_COMMAND_FORK\n\nThe goal of this patch is to make it clear to callers that\nthe specific error was a missing command. To do this, we\nwill return the error code ERR_RUN_COMMAND_EXEC, which is\nalready defined in run-command.h, checked for in several\nplaces, but never actually gets set.\n\nThe new behavior is:\n\n  - on POSIX systems, we exit the forked process with code\n    127 (the same as the shell uses to report missing\n    commands). The parent process recognizes this code and\n    returns an EXEC error. The stderr message is silenced,\n    since the caller may be speculatively trying to run a\n    command. Instead, we use trace_printf so that somebody\n    interested in debugging can see the error that occured.\n\n  - on Windows, we check errno, which is already set\n    correctly by mingw_spawnvpe, and report an EXEC error\n    instead of a FORK error\n\nThus it is safe to speculatively run a command:\n\n  int r = run_command_v_opt(argv, 0);\n  if (r == -ERR_RUN_COMMAND_EXEC)\n\t  /* oops, it wasn't found; try something else */\n  else\n\t  /* we failed for some other reason, error is in r */\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c |   19 ++++++++++++++++---\n 1 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex db9ce59..2437798 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -118,7 +118,9 @@ int start_command(struct child_process *cmd)\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\tdie(\"exec %s failed.\", cmd->argv[0]);\n+\t\ttrace_printf(\"trace: exec '%s' failed: %s\\n\", cmd->argv[0],\n+\t\t\t\tstrerror(errno));\n+\t\texit(127);\n \t}\n #else\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n@@ -197,7 +199,13 @@ int start_command(struct child_process *cmd)\n \t\t\tclose(cmd->out);\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n+#ifndef __MINGW32__\n \t\treturn -ERR_RUN_COMMAND_FORK;\n+#else\n+\t\treturn errno == ENOENT ?\n+\t\t\t-ERR_RUN_COMMAND_EXEC :\n+\t\t\t-ERR_RUN_COMMAND_FORK;\n+#endif\n \t}\n \n \tif (need_in)\n@@ -236,9 +244,14 @@ static int wait_or_whine(pid_t pid)\n \t\tif (!WIFEXITED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_NOEXIT;\n \t\tcode = WEXITSTATUS(status);\n-\t\tif (code)\n+\t\tswitch (code) {\n+\t\tcase 127:\n+\t\t\treturn -ERR_RUN_COMMAND_EXEC;\n+\t\tcase 0:\n+\t\t\treturn 0;\n+\t\tdefault:\n \t\t\treturn -code;\n-\t\treturn 0;\n+\t\t}\n \t}\n }\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102091","messageId":"20090127062740.GC13161@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090127062512.GA10487@coredump.intra.peff.net","subject":"[RFC/PATCH 3/3] git: use run_command to execute dashed externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T06:27:40Z","receivedAt":"2009-01-27T06:27:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We used to simply try calling execvp(); if it succeeded,\nthen we were done and the new program was running. If it\ndidn't, then we knew that it wasn't a valid command.\n\nUnfortunately, this interacted badly with the new pager\nhandling. Now that git remains the parent process and the\npager is spawned, git has to hang around until the pager is\nfinished. We install an atexit handler to do this, but that\nhandler never gets called if we successfully run execvp.\n\nYou could see this behavior by running any dashed external\nusing a pager (e.g., \"git -p stash list\"). The command\nfinishes running, but the pager is still going. In the case\nof less, it then gets an error reading from the terminal and\nexits, potentially leaving the terminal in a broken state\n(and not showing the output).\n\nThis patch just uses run_command to try running the\ndashed external. The parent git process then waits for the\nexternal process to complete and then handles the pager\ncleanup as it would for an internal command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git.c |   14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 45e493d..79a836c 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]\";\n@@ -392,6 +393,7 @@ static void execv_dashed_external(const char **argv)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *tmp;\n+\tint status;\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\n \n@@ -406,10 +408,14 @@ static void execv_dashed_external(const char **argv)\n \n \ttrace_argv_printf(argv, \"trace: exec:\");\n \n-\t/* execvp() can only ever return if it fails */\n-\texecvp(cmd.buf, (char **)argv);\n-\n-\ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n+\t/*\n+\t * if we fail because the command is not found, it is\n+\t * OK to return. Otherwise, we just pass along the status code.\n+\t */\n+\tstatus = run_command_v_opt(argv, 0);\n+\tif (status != -ERR_RUN_COMMAND_EXEC)\n+\t\texit(status);\n+\terrno = ENOENT; /* as if we called execvp */\n \n \targv[0] = tmp;\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102101","messageId":"497EDCB0.8080806@kdbg.org","threadId":"17049","inReplyTo":"20090127062512.GA10487@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-01-27T10:06:40Z","receivedAt":"2009-01-27T10:06:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> JSixt, there are some tweaks to the Windows code to report back the exec\n> error. They look obviously correct to me, but I have no box to test\n> (even compile test) them on.\n\nGenerally, I like this series, in particular since it does not degrade \nperformance on Windows ;)\n\nBut the following is needed in addition.\n\n- Note that run_command returns the negated exit code, therefore, we \nmust negate it again in the call to exit(). Without this t6030 failed. \n(And negative exit codes causes grief on Windows because bash for some \nreason does not recognize that as failure.)\n\n- The close() calls can overwrite errno. And since fork() should not \n(cannot?) fail with ENOENT, it's safe to remove the #ifdef __MINGW32__.\n\n-- Hannes\n\ndiff --git a/git.c b/git.c\nindex 79a836c..35635d1 100644\n--- a/git.c\n+++ b/git.c\n@@ -414,7 +414,7 @@ static void execv_dashed_external\n  \t */\n  \tstatus = run_command_v_opt(argv, 0);\n  \tif (status != -ERR_RUN_COMMAND_EXEC)\n-\t\texit(status);\n+\t\texit(-status);\n  \terrno = ENOENT; /* as if we called execvp */\n\n  \targv[0] = tmp;\ndiff --git a/run-command.c b/run-command.c\nindex 2437798..b05c734 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -187,27 +187,24 @@ int start_command\n  \tif (s2 >= 0)\n  \t\tdup2(s2, 2), close(s2);\n  #endif\n\n  \tif (cmd->pid < 0) {\n+\t\tint err = errno;\n  \t\tif (need_in)\n  \t\t\tclose_pair(fdin);\n  \t\telse if (cmd->in)\n  \t\t\tclose(cmd->in);\n  \t\tif (need_out)\n  \t\t\tclose_pair(fdout);\n  \t\telse if (cmd->out)\n  \t\t\tclose(cmd->out);\n  \t\tif (need_err)\n  \t\t\tclose_pair(fderr);\n-#ifndef __MINGW32__\n-\t\treturn -ERR_RUN_COMMAND_FORK;\n-#else\n-\t\treturn errno == ENOENT ?\n+\t\treturn err == ENOENT ?\n  \t\t\t-ERR_RUN_COMMAND_EXEC :\n  \t\t\t-ERR_RUN_COMMAND_FORK;\n-#endif\n  \t}\n\n  \tif (need_in)\n  \t\tclose(fdin[0]);\n  \telse if (cmd->in)\n"},{"id":"102111","messageId":"20090127122315.GA22628@sigill.intra.peff.net","threadId":"17049","inReplyTo":"497EDCB0.8080806@kdbg.org","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T12:23:15Z","receivedAt":"2009-01-27T12:23:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2009 at 11:06:40AM +0100, Johannes Sixt wrote:\n\n> - Note that run_command returns the negated exit code, therefore, we must \n> negate it again in the call to exit(). Without this t6030 failed. (And \n> negative exit codes causes grief on Windows because bash for some reason \n> does not recognize that as failure.)\n\nOops, indeed. And you made me realize that I forgot to run the test\nscript against this patchset.\n\nHowever, I'm not sure just negating the exit code is sufficient.\nrun_command can return codes in the 10000 range for its own internal\nerrors. We don't want to pass those out through exit, which will\ntruncate them to 8 bits.\n\n> - The close() calls can overwrite errno.\n\nGood point.\n\n> And since fork() should not  (cannot?) fail with ENOENT, it's safe to\n> remove the #ifdef __MINGW32__.\n\nYeah, I thought of that, but I was worried it might make the code a\nlittle bit non-obvious (but it does clean up an ifdef, which is ugly,\ntoo).\n\nThanks for your feedback. I'll squash in your fixes and repost 2/3 later\ntoday.\n\n-Peff\n"},{"id":"102112","messageId":"497F021B.2050306@viscovery.net","threadId":"17049","inReplyTo":"20090127122315.GA22628@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-27T12:46:19Z","receivedAt":"2009-01-27T12:46:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> However, I'm not sure just negating the exit code is sufficient.\n> run_command can return codes in the 10000 range for its own internal\n> errors. We don't want to pass those out through exit, which will\n> truncate them to 8 bits.\n\nExit code and start_command/finish_command's return code handling is a\ncomplete mess IMHO and deserves a clean-up series of its own. If the few\ncodes at 10000 and above are truncated to 8 bits, then we get exit codes\n16 and higher; I think that's good enough for this series.\n\n-- Hannes\n"},{"id":"102131","messageId":"alpine.DEB.1.00.0901271728130.3586@pacific.mpi-cbg.de","threadId":"17049","inReplyTo":"20090127062512.GA10487@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-27T16:31:02Z","receivedAt":"2009-01-27T16:31:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 27 Jan 2009, Jeff King wrote:\n\n> On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:\n> \n> > Just try this with a submodule that has more changes than fit on a \n> > screen:\n> > \n> > \t$ git -p submodule summary\n> > \n> > In my tests, it consistently fscks up my console.  I wonder if this is \n> > related to ea27a18(spawn pager via run_command interface).\n> \n> OK, here is a patch series that fixes the problem:\n> \n>   1/3: git: s/run_command/run_builtin/\n>   2/3: run_command: handle missing command errors more gracefully\n>   3/3: git: use run_command to execute dashed externals\n> \n> 1 is a cleanup, 2 is infrastructure support, and 3 is the actual fix.\n\nI like the patch series, well designed and concise (especially with the \nfixes Hannes proposed).\n\n> There are two potential downsides to the fix:\n> \n>  1. There is an extra fork and a parent process sitting in memory for\n>     dashed externals. This is pretty necessary to any fix, since\n>     something has to wait to do pager cleanup, and we can't rely on the\n>     child to do so.\n\nActually, I think this is a good thing; that way, we can catch \nsegmentation fault properly and display an error message in the pager.  \nThat was not possible previously.\n\n>  2. A failed attempt to execute a dashed external results in an extra\n>     fork. For builtins, this has no impact, since they take precedence.\n>     For aliases, though, it means we will do an extra fork before\n>     realizing that there is no dashed external and trying the alias.\n\nAll the more reason to build more programs in :-)\n\nCiao,\nDscho\n"},{"id":"102260","messageId":"20090128071721.GC19165@coredump.intra.peff.net","threadId":"17049","inReplyTo":"497F021B.2050306@viscovery.net","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:17:22Z","receivedAt":"2009-01-28T07:17:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2009 at 01:46:19PM +0100, Johannes Sixt wrote:\n\n> Exit code and start_command/finish_command's return code handling is a\n> complete mess IMHO and deserves a clean-up series of its own. If the few\n\nYes, the negation is a bit confusing, just for being allowed to say\n\n  if (run_command(foo) < 0)\n\nsince you end up having to store and re-negate anyway to get the actual\ncode. Plus the value of errno is untrustworthy, since we may have been\ndoing cleanup calls.\n\n> codes at 10000 and above are truncated to 8 bits, then we get exit codes\n> 16 and higher; I think that's good enough for this series.\n\nI think it is nice to differentiate between an exit code from the\nsub-program and our own error, though. See my updated series for what I\nthink is a reasonable one-liner fix.\n\n-Peff\n"},{"id":"102263","messageId":"20090128073059.GD19165@coredump.intra.peff.net","threadId":"17049","inReplyTo":"alpine.DEB.1.00.0901271728130.3586@pacific.mpi-cbg.de","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:30:59Z","receivedAt":"2009-01-28T07:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2009 at 05:31:02PM +0100, Johannes Schindelin wrote:\n\n> I like the patch series, well designed and concise (especially with the \n> fixes Hannes proposed).\n\nGood. Response seems positive, so I will drop the RFC, then, and post a\nfixed-up series meant for inclusion.\n\n> > There are two potential downsides to the fix:\n> > \n> >  1. There is an extra fork and a parent process sitting in memory for\n> >     dashed externals. This is pretty necessary to any fix, since\n> >     something has to wait to do pager cleanup, and we can't rely on the\n> >     child to do so.\n> \n> Actually, I think this is a good thing; that way, we can catch \n> segmentation fault properly and display an error message in the pager.  \n> That was not possible previously.\n\nTrue. On the other hand, most of our externals are shell scripts. It's\nthe builtins that segfault, and we don't have a watcher process for\nthat. :)\n\n-Peff\n"},{"id":"102264","messageId":"20090128073353.GA31884@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128073059.GD19165@coredump.intra.peff.net","subject":"[PATCHv2 1/4] git: s/run_command/run_builtin/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:33:53Z","receivedAt":"2009-01-28T07:33:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is a static function called run_command which\nconflicts with the library function in run-command.c; this\nisn't a problem currently, but prevents including\nrun-command.h in git.c.\n\nThis patch just renames the static function to something\nmore specific and non-conflicting.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as before.\n\n git.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex ecc8fad..45e493d 100644\n--- a/git.c\n+++ b/git.c\n@@ -219,7 +219,7 @@ struct cmd_struct {\n \tint option;\n };\n \n-static int run_command(struct cmd_struct *p, int argc, const char **argv)\n+static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n {\n \tint status;\n \tstruct stat st;\n@@ -384,7 +384,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n-\t\texit(run_command(p, argc, argv));\n+\t\texit(run_builtin(p, argc, argv));\n \t}\n }\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102265","messageId":"20090128073533.GB31884@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128073059.GD19165@coredump.intra.peff.net","subject":"[PATCHv2 2/4] run_command: handle missing command errors more gracefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:35:33Z","receivedAt":"2009-01-28T07:35:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When run_command was asked to run a non-existant command,\nits behavior varied depending on the platform:\n\n  - on POSIX systems, we would fork, and then after the\n    execvp call failed, we could call die(), which prints a\n    message to stderr and exits with code 128.\n\n  - on Windows, we do a PATH lookup, realize the program\n    isn't there, and then return ERR_RUN_COMMAND_FORK\n\nThe goal of this patch is to make it clear to callers that\nthe specific error was a missing command. To do this, we\nwill return the error code ERR_RUN_COMMAND_EXEC, which is\nalready defined in run-command.h, checked for in several\nplaces, but never actually gets set.\n\nThe new behavior is:\n\n  - on POSIX systems, we exit the forked process with code\n    127 (the same as the shell uses to report missing\n    commands). The parent process recognizes this code and\n    returns an EXEC error. The stderr message is silenced,\n    since the caller may be speculatively trying to run a\n    command. Instead, we use trace_printf so that somebody\n    interested in debugging can see the error that occured.\n\n  - on Windows, we check errno, which is already set\n    correctly by mingw_spawnvpe, and report an EXEC error\n    instead of a FORK error\n\nThus it is safe to speculatively run a command:\n\n  int r = run_command_v_opt(argv, 0);\n  if (r == -ERR_RUN_COMMAND_EXEC)\n\t  /* oops, it wasn't found; try something else */\n  else\n\t  /* we failed for some other reason, error is in r */\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIncorporates JSixt's fix to retain errno across system calls.\n\n run-command.c |   18 ++++++++++++++----\n 1 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex db9ce59..b05c734 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -118,7 +118,9 @@ int start_command(struct child_process *cmd)\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\tdie(\"exec %s failed.\", cmd->argv[0]);\n+\t\ttrace_printf(\"trace: exec '%s' failed: %s\\n\", cmd->argv[0],\n+\t\t\t\tstrerror(errno));\n+\t\texit(127);\n \t}\n #else\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n@@ -187,6 +189,7 @@ int start_command(struct child_process *cmd)\n #endif\n \n \tif (cmd->pid < 0) {\n+\t\tint err = errno;\n \t\tif (need_in)\n \t\t\tclose_pair(fdin);\n \t\telse if (cmd->in)\n@@ -197,7 +200,9 @@ int start_command(struct child_process *cmd)\n \t\t\tclose(cmd->out);\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n-\t\treturn -ERR_RUN_COMMAND_FORK;\n+\t\treturn err == ENOENT ?\n+\t\t\t-ERR_RUN_COMMAND_EXEC :\n+\t\t\t-ERR_RUN_COMMAND_FORK;\n \t}\n \n \tif (need_in)\n@@ -236,9 +241,14 @@ static int wait_or_whine(pid_t pid)\n \t\tif (!WIFEXITED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_NOEXIT;\n \t\tcode = WEXITSTATUS(status);\n-\t\tif (code)\n+\t\tswitch (code) {\n+\t\tcase 127:\n+\t\t\treturn -ERR_RUN_COMMAND_EXEC;\n+\t\tcase 0:\n+\t\t\treturn 0;\n+\t\tdefault:\n \t\t\treturn -code;\n-\t\treturn 0;\n+\t\t}\n \t}\n }\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102266","messageId":"20090128073639.GC31884@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128073059.GD19165@coredump.intra.peff.net","subject":"[PATCHv2 3/4] run-command: help callers distinguish errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:36:39Z","receivedAt":"2009-01-28T07:36:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"run_command returns a single integer specifying either an\nerror code or the exit status of the spawned program. The\nonly way to tell the difference is that the error codes are\noutside of the allowed range of exit status values.\n\nRather than make each caller implement the test against a\nmagic limit, let's provide a macro.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNew since v1 of the series.\n\n run-command.h |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.h b/run-command.h\nindex 0211e1d..a88b3cd 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -10,6 +10,7 @@ enum {\n \tERR_RUN_COMMAND_WAITPID_SIGNAL,\n \tERR_RUN_COMMAND_WAITPID_NOEXIT,\n };\n+#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)\n \n struct child_process {\n \tconst char **argv;\n-- \n1.6.1.1.367.g30b36\n"},{"id":"102268","messageId":"20090128073814.GD31884@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128073059.GD19165@coredump.intra.peff.net","subject":"[PATCHv2 4/4] git: use run_command to execute dashed externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:38:14Z","receivedAt":"2009-01-28T07:38:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We used to simply try calling execvp(); if it succeeded,\nthen we were done and the new program was running. If it\ndidn't, then we knew that it wasn't a valid command.\n\nUnfortunately, this interacted badly with the new pager\nhandling. Now that git remains the parent process and the\npager is spawned, git has to hang around until the pager is\nfinished. We install an atexit handler to do this, but that\nhandler never gets called if we successfully run execvp.\n\nYou could see this behavior by running any dashed external\nusing a pager (e.g., \"git -p stash list\"). The command\nfinishes running, but the pager is still going. In the case\nof less, it then gets an error reading from the terminal and\nexits, potentially leaving the terminal in a broken state\n(and not showing the output).\n\nThis patch just uses run_command to try running the\ndashed external. The parent git process then waits for the\nexternal process to complete and then handles the pager\ncleanup as it would for an internal command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIncorporates negated status fix from JSixt.\n\nThis version also differentiates in the exit code and stderr output\nwhether we simply failed to exec the command versus passing along its\nstatus code.\n\n git.c |   17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 45e493d..b02b05b 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]\";\n@@ -392,6 +393,7 @@ static void execv_dashed_external(const char **argv)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *tmp;\n+\tint status;\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\n \n@@ -406,10 +408,17 @@ static void execv_dashed_external(const char **argv)\n \n \ttrace_argv_printf(argv, \"trace: exec:\");\n \n-\t/* execvp() can only ever return if it fails */\n-\texecvp(cmd.buf, (char **)argv);\n-\n-\ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n+\t/*\n+\t * if we fail because the command is not found, it is\n+\t * OK to return. Otherwise, we just pass along the status code.\n+\t */\n+\tstatus = run_command_v_opt(argv, 0);\n+\tif (status != -ERR_RUN_COMMAND_EXEC) {\n+\t\tif (IS_RUN_COMMAND_ERR(status))\n+\t\t\tdie(\"unable to run '%s'\", argv[0]);\n+\t\texit(-status);\n+\t}\n+\terrno = ENOENT; /* as if we called execvp */\n \n \targv[0] = tmp;\n \n-- \n1.6.1.1.367.g30b36\n"},{"id":"102271","messageId":"20090128074334.GB31951@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128073639.GC31884@coredump.intra.peff.net","subject":"Re: [PATCHv2 3/4] run-command: help callers distinguish errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:43:34Z","receivedAt":"2009-01-28T07:43:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 02:36:39AM -0500, Jeff King wrote:\n\n> +#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)\n\n<sigh> This should be \"<= -ERR_RUN_COMMAND_FORK\", since we expect the\nnegated status.\n\nMaybe it is time I wrote a test for a failed external.\n\n-Peff\n"},{"id":"102272","messageId":"20090128074732.GC31951@coredump.intra.peff.net","threadId":"17049","inReplyTo":"20090128074334.GB31951@coredump.intra.peff.net","subject":"Re: [PATCHv2 3/4] run-command: help callers distinguish errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:47:33Z","receivedAt":"2009-01-28T07:47:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 02:43:34AM -0500, Jeff King wrote:\n\n> On Wed, Jan 28, 2009 at 02:36:39AM -0500, Jeff King wrote:\n> \n> > +#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)\n> \n> <sigh> This should be \"<= -ERR_RUN_COMMAND_FORK\", since we expect the\n> negated status.\n> \n> Maybe it is time I wrote a test for a failed external.\n\nHmm. Actually, it is hard to write a test case for this; it only\ntriggers if we actually have an error running a program, like fork()\nfailing.\n\n-Peff\n"},{"id":"102274","messageId":"7vk58fvpa7.fsf@gitster.siamese.dyndns.org","threadId":"17049","inReplyTo":"20090128073059.GD19165@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] fix \"Funny: git -p submodule summary\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T07:54:40Z","receivedAt":"2009-01-28T07:54:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jan 27, 2009 at 05:31:02PM +0100, Johannes Schindelin wrote:\n>\n>> I like the patch series, well designed and concise (especially with the \n>> fixes Hannes proposed).\n\nThe series looks quite sane and clean.  Will queue once I finish wrapping\nup the push with \".have\" thing.\n"}]}