Funny: git -p submodule summary

Funny: git -p submodule summary

[Johannes Schindelin]

Hi list,

Just try this with a submodule that has more changes than fit on a screen:

	$ git -p submodule summary

In my tests, it consistently fscks up my console.  I wonder if this is 
related to ea27a18(spawn pager via run_command interface).

*reverts that commit* Yep, that fixes it.

Ciao,
Dscho

Re: Funny: git -p submodule summary

[Johannes Schindelin]

Hi,

On Thu, 8 Jan 2009, Johannes Schindelin wrote:

> Just try this with a submodule that has more changes than fit on a 
> screen:
> 
> 	$ git -p submodule summary
> 
> In my tests, it consistently fscks up my console.

Update: even if the changes do fit on a screen, the console is fscked up 
(I have to stty echo to get it back to normal).

Ciao,
Dscho

Re: Funny: git -p submodule summary

[Jeff King]

On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:

> Just try this with a submodule that has more changes than fit on a screen:
> 
> 	$ git -p submodule summary
> 
> In my tests, it consistently fscks up my console.  I wonder if this is 
> related to ea27a18(spawn pager via run_command interface).
> 
> *reverts that commit* Yep, that fixes it.

Hmm. I can reproduce your problem here, like this:

  $ git -p submodule summary

but when I tried to dig deeper using strace, the problem goes away:

  $ strace -f -o foo.out git -p submodule summary

However, I was able to get some data by stracing _just_ less:

  $ GIT_PAGER='strace -f -o foo.out less' git -p submodule summary

and that reproduces the problem. And here's the interesting bit:

  open("/dev/tty", O_RDONLY|O_LARGEFILE)  = 3
  ...
  read(0, $SOME_SUBMODULE_OUTPUT, ...)
  write(1, $SOME_SUBMODULE_OUTPUT, ...)
  read(3, 0xbfd3442f, 1)                  = -1 EIO (Input/output error)

That is, after displaying the actual output, the next thing less does is
try to get input from the user on /dev/tty. But it returns EIO. At which
point less just dies, leaving your terminal in a funny state.

Hrm. Here's a theory. The new pager behavior goes something like this
for a builtin:

  1. fork, child becomes pager
  2. register atexit handler to wait for pager to finish
  3. run builtin
  4. exit, triggering handler

but for an external command (like a shell script), we exec the command,
and presumably forget about out atexit handler entirely. Which means
that our shell script writes all of its output and exits before less has
a chance to prompt and get a response from the tty.

And I'll admit to being very hazy on the details of process groups and
controlling terminals, but from what I recall, perhaps the EIO is
related to the process group leader being dead. Which means we could
paper over it by putting less in its own pgrp.

So here's a little test (in bash):

  $ set +m ;# disable job control, leaving bash as the pgrp leader
  $ git -p submodule summary

A-ha. That "works" in the sense that less runs fine and show the output.
So it is a pgrp issue.  _But_ we still have a problem, which is that the
original process has exited, and bash thinks the command is finished. So
now annoyingly we have both bash and less trying to read from the
terminal.

So the _real_ problem is that we are not always triggering the "wait for
pager to finish" code because we exec and forget about it. Which means
this strategy of "git runs child pager" will never work properly.
Instead, we have to use three processes: git and the pager become child
processes, while the original process waits for both to exit and returns
the proper exit code from git.

Let me try to work up a patch.

-Peff

PS I believe this is related to a similar bug which I have been hunting
   fruitlessly for a few weeks: if you use ^C to kill git, the pager
   will sometimes do funny things. I also traced that to an EIO on
   reading from the terminal, which makes sense: we are killing git
   before it gets a chance to do wait_for_pager.

Re: Funny: git -p submodule summary

[Jeff King]

On Fri, Jan 09, 2009 at 03:38:36AM -0500, Jeff King wrote:

> So the _real_ problem is that we are not always triggering the "wait for
> pager to finish" code because we exec and forget about it. Which means
> this strategy of "git runs child pager" will never work properly.
> Instead, we have to use three processes: git and the pager become child
> processes, while the original process waits for both to exit and returns
> the proper exit code from git.
> 
> Let me try to work up a patch.

Below is a patch that uses the three-process mechanism, and it fixes the
problem. _But_ it is not satisfactory for inclusion, because it won't
work on MINGW32. Since it is actually splitting git into two processes
(one to monitor the pager and one to actually run git), it uses fork.

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.

Of course, we don't know ahead of time whether exec'ing a command will
work: we find out by trying. So now we will end up creating a pipe and
fork()ing every time we want to see whether we can exec a command. But I
suppose that only happens once or twice, so maybe the performance impact
isn't relevant.

This is all getting complicated enough that I am tempted to just suggest
reverting ea27a18c. But even that won't fix everything, though, since
MINGW32 still needs to use run_command to spawn the pager. IOW, I think
the breakage you are seeing has always been broken on MINGW32.

Blah. Anyway, here is the Unix-only patch.

---
diff --git a/pager.c b/pager.c
index f19ddbc..68ae669 100644
--- a/pager.c
+++ b/pager.c
@@ -28,18 +28,10 @@ static void pager_preexec(void)
 static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
 static struct child_process pager_process;
 
-static void wait_for_pager(void)
-{
-	fflush(stdout);
-	fflush(stderr);
-	/* signal EOF to pager */
-	close(1);
-	close(2);
-	finish_command(&pager_process);
-}
-
 void setup_pager(void)
 {
+	pid_t git_child;
+	int status;
 	const char *pager = getenv("GIT_PAGER");
 
 	if (!isatty(1))
@@ -68,14 +60,24 @@ void setup_pager(void)
 	if (start_command(&pager_process))
 		return;
 
-	/* original process continues, but writes to the pipe */
-	dup2(pager_process.in, 1);
-	if (isatty(2))
-		dup2(pager_process.in, 2);
-	close(pager_process.in);
+	/* now spawn the actual git process */
+	git_child = fork();
+	if (git_child == -1)
+		die("unable to fork: %s", strerror(errno));
+	if (git_child == 0) {
+		dup2(pager_process.in, 1);
+		if (isatty(2))
+			dup2(pager_process.in, 2);
+		close(pager_process.in);
+		return;
+	}
 
-	/* this makes sure that the parent terminates after the pager */
-	atexit(wait_for_pager);
+	/* and the original process just waits for both to finish */
+	close(pager_process.in);
+	if (waitpid(git_child, &status, 0) < 0)
+		die("wait failure: %s", strerror(errno));
+	finish_command(&pager_process);
+	exit(status);
 }
 
 int pager_in_use(void)

Re: Funny: git -p submodule summary

[Jeff King]

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 */

Re: Funny: git -p submodule summary

[Johannes Sixt]

Jeff King schrieb:
> Below is a patch that uses the three-process mechanism, and it fixes the
> problem. _But_ it is not satisfactory for inclusion, because it won't
> work on MINGW32. Since it is actually splitting git into two processes
> (one to monitor the pager and one to actually run git), it uses fork.

We have start_async()/finish_async() to replace a fork() of the sort that
we have here.

> IOW, I think
> the breakage you are seeing has always been broken on MINGW32.

Indeed. Hitting Ctrl-C while the pager was open has messed up the console
since day 1.

-- Hannes

Re: Funny: git -p submodule summary

[Jeff King]

On Fri, Jan 09, 2009 at 11:09:08AM +0100, Johannes Sixt wrote:

> > Below is a patch that uses the three-process mechanism, and it fixes the
> > problem. _But_ it is not satisfactory for inclusion, because it won't
> > work on MINGW32. Since it is actually splitting git into two processes
> > (one to monitor the pager and one to actually run git), it uses fork.
> 
> We have start_async()/finish_async() to replace a fork() of the sort that
> we have here.

It looks like start_async is implemented using threads on Windows. Will
that survive an execvp call? Because we don't know at this point whether
we are going to actually run builtin code, or if we will exec an
external.

-Peff

Re: Funny: git -p submodule summary

[Johannes Sixt]

Jeff King schrieb:
> On Fri, Jan 09, 2009 at 11:09:08AM +0100, Johannes Sixt wrote:
> 
>>> Below is a patch that uses the three-process mechanism, and it fixes the
>>> problem. _But_ it is not satisfactory for inclusion, because it won't
>>> work on MINGW32. Since it is actually splitting git into two processes
>>> (one to monitor the pager and one to actually run git), it uses fork.
>> We have start_async()/finish_async() to replace a fork() of the sort that
>> we have here.
> 
> It looks like start_async is implemented using threads on Windows. Will
> that survive an execvp call? Because we don't know at this point whether
> we are going to actually run builtin code, or if we will exec an
> external.

Ah, no, it would not survive.

But there's a more serious problem why we cannot use start_async() in its
current form: It expects that there is a *function* that produces the
output; but here we don't have a function - output is produced by
*returning* (from setup_pager).

I'll test your other patch (that replaces the execvp in git.c by run_command).

-- Hannes

Re: Funny: git -p submodule summary

[Jeff King]

On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:

> Ah, no, it would not survive.
> 
> But there's a more serious problem why we cannot use start_async() in its
> current form: It expects that there is a *function* that produces the
> output; but here we don't have a function - output is produced by
> *returning* (from setup_pager).

Good point.

> I'll test your other patch (that replaces the execvp in git.c by run_command).

Note that it only covers the case of external commands. Hitting ctrl-c
to interrupt git will still cause funniness. For that we need to
intercept signals to call wait_for_pager(). But there is a slight snag:
we also intercept signals elsewhere (e.g., for tempfile cleanup). So we
need to start remembering the old signal handlers everywhere and
chaining to them.

-Peff

Re: Funny: git -p submodule summary

[Jeff King]

On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:

> I'll test your other patch (that replaces the execvp in git.c by
> run_command).

There is something funny with it that I have not diagnosed: aliases are
broken, and "git foobar" does not return an error. Presumably just
checking the "we did not exec succesfully" case is not triggering
properly.  However, I think the right solution is actually to refactor
git.c to figure out ahead of time whether we have a builtin, external,
or alias. I can work on that, but not tonight, as my git-time is up for
now.

But other than that, did it work for you on Windows?

However, here is a 4-patch series that handles the separate signal
delivery problem. It should fix the "^C makes funny things happen"
problems you were seeing. Please test and let me know how it works on
Windows.

The patches are:
  1/4: Makefile: clean up TEST_PROGRAMS definition
  2/4: chain kill signals for cleanup functions
  3/4: refactor signal handling for cleanup functions
  4/4: pager: do wait_for_pager on signal death

-Peff

[PATCH 1/4] Makefile: clean up TEST_PROGRAMS definition

[Jeff King]

We try to keep lines under 80 characters, not to mention
that sticking a bunch of stuff on one line makes diffs
messier.

Signed-off-by: Jeff King <peff@peff.net>
---
Just a cleanup I noticed while working on 2/4.

 Makefile |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index dee97c1..2b873fa 100644
--- a/Makefile
+++ b/Makefile
@@ -1356,7 +1356,14 @@ endif
 
 ### Testing rules
 
-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
+TEST_PROGRAMS += test-chmtime$X
+TEST_PROGRAMS += test-date$X
+TEST_PROGRAMS += test-delta$X
+TEST_PROGRAMS += test-genrandom$X
+TEST_PROGRAMS += test-match-trees$X
+TEST_PROGRAMS += test-parse-options$X
+TEST_PROGRAMS += test-path-utils$X
+TEST_PROGRAMS += test-sha1$X
 
 all:: $(TEST_PROGRAMS)
 
-- 
1.6.1.84.g8150

[PATCH 2/4] chain kill signals for cleanup functions

[Jeff King]

If a piece of code wanted to do some cleanup before exiting
(e.g., cleaing up a lockfile or a tempfile), our usual
strategy was to install a signal handler that did something
like this:

  do_cleanup(); /* actual work */
  signal(signo, SIG_DFL); /* restore previous behavior */
  raise(signo); /* deliver signal, killing ourselves */

For a single handler, this works fine. However, if we want
to clean up two _different_ things, we run into a problem.
The most recently installed handler will run, but when it
removes itself as a handler, it doesn't put back the first
handler.

This patch introduces sigchain, a tiny library for handling
a stack of signal handlers. You sigchain_push each handler,
and use sigchain_pop to restore whoever was before you in
the stack.

Signed-off-by: Jeff King <peff@peff.net>
---
I don't know if it is possible to trigger this issue with current git or
not. There are several instances that catch signals in different ways,
but I don't know if there are code paths that actually combine them. So
maybe it is fixing a bug, but it is definitely needed for 4/4, which
will install the wait_for_pager handler in most git runs.

I tried to neither over- nor under-engineer. On one hand, you could
actually accomplish the same thing by always keeping a "this was the
last handler" pointer at each callsite when installing the new handler.
But it's easy to mess up, so I think a little library helps keep the
code clean.

On the other hand, callsites still have to have separate clean_foo() and
clean_foo_on_signal() handlers, and manually pop() and raise() in the
latter. If the code just assumed you always wanted to chain to the next
handler immediately, then we could cut out even more client code.

The test script is probably overkill, but I wrote it to convince myself
I hadn't screwed up too badly (and because we probably wouldn't even
notice, as signals aren't well tested in our scripts). So I figured I
might as well share it.

 .gitignore            |    1 +
 Makefile              |    3 +++
 builtin-clone.c       |    5 +++--
 builtin-fetch--tool.c |    5 +++--
 builtin-fetch.c       |    5 +++--
 diff.c                |    5 +++--
 http-push.c           |   11 ++++++-----
 lockfile.c            |   13 +++++++------
 sigchain.c            |   43 +++++++++++++++++++++++++++++++++++++++++++
 sigchain.h            |    9 +++++++++
 t/t0005-signals.sh    |   18 ++++++++++++++++++
 test-sigchain.c       |   22 ++++++++++++++++++++++
 12 files changed, 121 insertions(+), 19 deletions(-)
 create mode 100644 sigchain.c
 create mode 100644 sigchain.h
 create mode 100755 t/t0005-signals.sh
 create mode 100644 test-sigchain.c

diff --git a/.gitignore b/.gitignore
index d9adce5..f28a54d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@ test-match-trees
 test-parse-options
 test-path-utils
 test-sha1
+test-sigchain
 common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 2b873fa..fd02dec 100644
--- a/Makefile
+++ b/Makefile
@@ -388,6 +388,7 @@ LIB_H += revision.h
 LIB_H += run-command.h
 LIB_H += sha1-lookup.h
 LIB_H += sideband.h
+LIB_H += sigchain.h
 LIB_H += strbuf.h
 LIB_H += tag.h
 LIB_H += transport.h
@@ -481,6 +482,7 @@ LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1_name.o
 LIB_OBJS += shallow.o
 LIB_OBJS += sideband.o
+LIB_OBJS += sigchain.o
 LIB_OBJS += strbuf.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
@@ -1364,6 +1366,7 @@ TEST_PROGRAMS += test-match-trees$X
 TEST_PROGRAMS += test-parse-options$X
 TEST_PROGRAMS += test-path-utils$X
 TEST_PROGRAMS += test-sha1$X
+TEST_PROGRAMS += test-sigchain$X
 
 all:: $(TEST_PROGRAMS)
 
diff --git a/builtin-clone.c b/builtin-clone.c
index f1a1a0c..18b9392 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -19,6 +19,7 @@
 #include "strbuf.h"
 #include "dir.h"
 #include "pack-refs.h"
+#include "sigchain.h"
 
 /*
  * Overall FIXMEs:
@@ -288,7 +289,7 @@ static void remove_junk(void)
 static void remove_junk_on_signal(int signo)
 {
 	remove_junk();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -438,7 +439,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	junk_git_dir = git_dir;
 	atexit(remove_junk);
-	signal(SIGINT, remove_junk_on_signal);
+	sigchain_push(SIGINT, remove_junk_on_signal);
 
 	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
 
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 469b07e..b1d7f8f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "refs.h"
 #include "commit.h"
+#include "sigchain.h"
 
 static char *get_stdin(void)
 {
@@ -186,7 +187,7 @@ static void remove_keep(void)
 static void remove_keep_on_signal(int signo)
 {
 	remove_keep();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -245,7 +246,7 @@ static int fetch_native_store(FILE *fp,
 	char buffer[1024];
 	int err = 0;
 
-	signal(SIGINT, remove_keep_on_signal);
+	sigchain_push(SIGINT, remove_keep_on_signal);
 	atexit(remove_keep);
 
 	while (fgets(buffer, sizeof(buffer), stdin)) {
diff --git a/builtin-fetch.c b/builtin-fetch.c
index de6f307..8c86974 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -10,6 +10,7 @@
 #include "transport.h"
 #include "run-command.h"
 #include "parse-options.h"
+#include "sigchain.h"
 
 static const char * const builtin_fetch_usage[] = {
 	"git fetch [options] [<repository> <refspec>...]",
@@ -58,7 +59,7 @@ static void unlock_pack(void)
 static void unlock_pack_on_signal(int signo)
 {
 	unlock_pack();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -672,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		ref_nr = j;
 	}
 
-	signal(SIGINT, unlock_pack_on_signal);
+	sigchain_push(SIGINT, unlock_pack_on_signal);
 	atexit(unlock_pack);
 	exit_code = do_fetch(transport,
 			parse_fetch_refspec(ref_nr, refs), ref_nr);
diff --git a/diff.c b/diff.c
index d235482..5a74012 100644
--- a/diff.c
+++ b/diff.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "utf8.h"
 #include "userdiff.h"
+#include "sigchain.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1933,7 +1934,7 @@ static void remove_tempfile(void)
 static void remove_tempfile_on_signal(int signo)
 {
 	remove_tempfile();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -1968,7 +1969,7 @@ static void run_external_diff(const char *pgm,
 			atexit_asked = 1;
 			atexit(remove_tempfile);
 		}
-		signal(SIGINT, remove_tempfile_on_signal);
+		sigchain_push(SIGINT, remove_tempfile_on_signal);
 	}
 
 	if (one && two) {
diff --git a/http-push.c b/http-push.c
index a4b7d08..dec395d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -10,6 +10,7 @@
 #include "exec_cmd.h"
 #include "remote.h"
 #include "list-objects.h"
+#include "sigchain.h"
 
 #include <expat.h>
 
@@ -1363,7 +1364,7 @@ static void remove_locks(void)
 static void remove_locks_on_signal(int signo)
 {
 	remove_locks();
-	signal(signo, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -2261,10 +2262,10 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
-	signal(SIGINT, remove_locks_on_signal);
-	signal(SIGHUP, remove_locks_on_signal);
-	signal(SIGQUIT, remove_locks_on_signal);
-	signal(SIGTERM, remove_locks_on_signal);
+	sigchain_push(SIGINT, remove_locks_on_signal);
+	sigchain_push(SIGHUP, remove_locks_on_signal);
+	sigchain_push(SIGQUIT, remove_locks_on_signal);
+	sigchain_push(SIGTERM, remove_locks_on_signal);
 
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
diff --git a/lockfile.c b/lockfile.c
index 8589155..3cd57dc 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -2,6 +2,7 @@
  * Copyright (c) 2005, Junio C Hamano
  */
 #include "cache.h"
+#include "sigchain.h"
 
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
@@ -24,7 +25,7 @@ static void remove_lock_file(void)
 static void remove_lock_file_on_signal(int signo)
 {
 	remove_lock_file();
-	signal(signo, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -136,11 +137,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
-			signal(SIGINT, remove_lock_file_on_signal);
-			signal(SIGHUP, remove_lock_file_on_signal);
-			signal(SIGTERM, remove_lock_file_on_signal);
-			signal(SIGQUIT, remove_lock_file_on_signal);
-			signal(SIGPIPE, remove_lock_file_on_signal);
+			sigchain_push(SIGINT, remove_lock_file_on_signal);
+			sigchain_push(SIGHUP, remove_lock_file_on_signal);
+			sigchain_push(SIGTERM, remove_lock_file_on_signal);
+			sigchain_push(SIGQUIT, remove_lock_file_on_signal);
+			sigchain_push(SIGPIPE, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();
diff --git a/sigchain.c b/sigchain.c
new file mode 100644
index 0000000..a18d505
--- /dev/null
+++ b/sigchain.c
@@ -0,0 +1,43 @@
+#include "sigchain.h"
+#include "cache.h"
+
+#define SIGCHAIN_MAX_SIGNALS 32
+
+struct sigchain_signal {
+	sigchain_fun *old;
+	int n;
+	int alloc;
+};
+static struct sigchain_signal signals[SIGCHAIN_MAX_SIGNALS];
+
+static void check_signum(int sig)
+{
+	if (sig < 1 || sig >= SIGCHAIN_MAX_SIGNALS)
+		die("BUG: signal out of range: %d", sig);
+}
+
+int sigchain_push(int sig, sigchain_fun f)
+{
+	struct sigchain_signal *s = signals + sig;
+	check_signum(sig);
+
+	ALLOC_GROW(s->old, s->n + 1, s->alloc);
+	s->old[s->n] = signal(sig, f);
+	if (s->old[s->n] == SIG_ERR)
+		return -1;
+	s->n++;
+	return 0;
+}
+
+int sigchain_pop(int sig)
+{
+	struct sigchain_signal *s = signals + sig;
+	check_signum(sig);
+	if (s->n < 1)
+		return 0;
+
+	if (signal(sig, s->old[s->n - 1]) == SIG_ERR)
+		return -1;
+	s->n--;
+	return 0;
+}
diff --git a/sigchain.h b/sigchain.h
new file mode 100644
index 0000000..254ebb0
--- /dev/null
+++ b/sigchain.h
@@ -0,0 +1,9 @@
+#ifndef SIGCHAIN_H
+#define SIGCHAIN_H
+
+typedef void (*sigchain_fun)(int);
+
+int sigchain_push(int sig, sigchain_fun f);
+int sigchain_pop(int sig);
+
+#endif /* SIGCHAIN_H */
diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
new file mode 100755
index 0000000..d900df4
--- /dev/null
+++ b/t/t0005-signals.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='signals work as we expect'
+. ./test-lib.sh
+
+cat >expect <<EOF
+three
+two
+one
+EOF
+
+test_expect_success 'sigchain works' '
+  test-sigchain >actual
+  test "$?" = 130 &&
+  test_cmp expect actual
+'
+
+test_done
diff --git a/test-sigchain.c b/test-sigchain.c
new file mode 100644
index 0000000..8747dea
--- /dev/null
+++ b/test-sigchain.c
@@ -0,0 +1,22 @@
+#include "sigchain.h"
+#include "cache.h"
+
+#define X(f) \
+static void f(int sig) { \
+	puts(#f); \
+	fflush(stdout); \
+	sigchain_pop(sig); \
+	raise(sig); \
+}
+X(one)
+X(two)
+X(three)
+#undef X
+
+int main(int argc, char **argv) {
+	sigchain_push(SIGINT, one);
+	sigchain_push(SIGINT, two);
+	sigchain_push(SIGINT, three);
+	raise(SIGINT);
+	return 0;
+}
-- 
1.6.1.84.g8150

Re: [PATCH 2/4] chain kill signals for cleanup functions

[Jeff King]

On Sun, Jan 11, 2009 at 06:32:12AM -0500, Jeff King wrote:

> @@ -1968,7 +1969,7 @@ static void run_external_diff(const char *pgm,
>  			atexit_asked = 1;
>  			atexit(remove_tempfile);
>  		}
> -		signal(SIGINT, remove_tempfile_on_signal);
> +		sigchain_push(SIGINT, remove_tempfile_on_signal);

Hmm.

Note that because we are now pushing instead of just replacing the
signal handler, it might matter if it gets called multiple times (though
I think most of the cleanup functions are relatively harmless if run
multiple times). Most of the callsites protect against installing the
signal handler twice, but I think this one should probably be moved up
inside the atexit_asked condition:

  if (! atexit_asked &&
      (temp[0].name == temp[0].tmp_path ||
       temp[1].name == temp[1].tmp_path)) {
          atexit_asked = 1;
          atexit(remove_tempfile);
  }
  sigchain_push_common(remove_tempfile_on_signal);

-Peff

[PATCH 3/4] refactor signal handling for cleanup functions

[Jeff King]

The current code is very inconsistent about which signals
are caught for doing cleanup of temporary files and lock
files. Some callsites checked only SIGINT, while others
checked a variety of death-dealing signals.

This patch factors out those signals to a single function,
and then calls it everywhere. For some sites, that means
this is a simple clean up. For others, it is an improvement
in that they will now properly clean themselves up after a
larger variety of signals.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm assuming there was no good reason _not_ to be handling those other
signals at some of the "just handle SIGINT" sites. A sigchain
implementation which handled "remove_lock_file" without needing
"remove_lock_file_on_signal" could also call atexit(), too. So you could
have a one liner

  register_cleanup(remove_lock_file);

However, there is one case that doesn't use an atexit() handler:
http-push.c. I don't know if this is a bug or an intentional omission.

 builtin-clone.c       |    2 +-
 builtin-fetch--tool.c |    2 +-
 builtin-fetch.c       |    2 +-
 diff.c                |    2 +-
 http-push.c           |    5 +----
 lockfile.c            |    6 +-----
 sigchain.c            |    9 +++++++++
 sigchain.h            |    2 ++
 8 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 18b9392..44c8073 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -439,7 +439,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	junk_git_dir = git_dir;
 	atexit(remove_junk);
-	sigchain_push(SIGINT, remove_junk_on_signal);
+	sigchain_push_common(remove_junk_on_signal);
 
 	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
 
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index b1d7f8f..29356d2 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -246,7 +246,7 @@ static int fetch_native_store(FILE *fp,
 	char buffer[1024];
 	int err = 0;
 
-	sigchain_push(SIGINT, remove_keep_on_signal);
+	sigchain_push_common(remove_keep_on_signal);
 	atexit(remove_keep);
 
 	while (fgets(buffer, sizeof(buffer), stdin)) {
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8c86974..1e4a3d9 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -673,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		ref_nr = j;
 	}
 
-	sigchain_push(SIGINT, unlock_pack_on_signal);
+	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack);
 	exit_code = do_fetch(transport,
 			parse_fetch_refspec(ref_nr, refs), ref_nr);
diff --git a/diff.c b/diff.c
index 5a74012..7fc8512 100644
--- a/diff.c
+++ b/diff.c
@@ -1969,7 +1969,7 @@ static void run_external_diff(const char *pgm,
 			atexit_asked = 1;
 			atexit(remove_tempfile);
 		}
-		sigchain_push(SIGINT, remove_tempfile_on_signal);
+		sigchain_push_common(remove_tempfile_on_signal);
 	}
 
 	if (one && two) {
diff --git a/http-push.c b/http-push.c
index dec395d..7d5c23e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2262,10 +2262,7 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
-	sigchain_push(SIGINT, remove_locks_on_signal);
-	sigchain_push(SIGHUP, remove_locks_on_signal);
-	sigchain_push(SIGQUIT, remove_locks_on_signal);
-	sigchain_push(SIGTERM, remove_locks_on_signal);
+	sigchain_push_common(remove_locks_on_signal);
 
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
diff --git a/lockfile.c b/lockfile.c
index 3cd57dc..021c337 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -137,11 +137,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
-			sigchain_push(SIGINT, remove_lock_file_on_signal);
-			sigchain_push(SIGHUP, remove_lock_file_on_signal);
-			sigchain_push(SIGTERM, remove_lock_file_on_signal);
-			sigchain_push(SIGQUIT, remove_lock_file_on_signal);
-			sigchain_push(SIGPIPE, remove_lock_file_on_signal);
+			sigchain_push_common(remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();
diff --git a/sigchain.c b/sigchain.c
index a18d505..1118b99 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -41,3 +41,12 @@ int sigchain_pop(int sig)
 	s->n--;
 	return 0;
 }
+
+void sigchain_push_common(sigchain_fun f)
+{
+	sigchain_push(SIGINT, f);
+	sigchain_push(SIGHUP, f);
+	sigchain_push(SIGTERM, f);
+	sigchain_push(SIGQUIT, f);
+	sigchain_push(SIGPIPE, f);
+}
diff --git a/sigchain.h b/sigchain.h
index 254ebb0..618083b 100644
--- a/sigchain.h
+++ b/sigchain.h
@@ -6,4 +6,6 @@ typedef void (*sigchain_fun)(int);
 int sigchain_push(int sig, sigchain_fun f);
 int sigchain_pop(int sig);
 
+void sigchain_push_common(sigchain_fun f);
+
 #endif /* SIGCHAIN_H */
-- 
1.6.1.84.g8150

[PATCH 4/4] pager: do wait_for_pager on signal death

[Jeff King]

Since ea27a18 (spawn pager via run_command interface), the
original git process actually does git work, and the pager
is a child process (actually, on Windows it has always been
that way, since Windows lacks fork). After spawning the
pager, we register an atexit() handler that waits for the
pager to finish.

Unfortunately, that handler does not always run. In
particular, if git is killed by a signal, then we exit
immediately. The calling shell then thinks that git is done;
however, the pager is still trying to run and impact the
terminal. The result can be seen by running a long git
process with a pager (e.g., "git log -p") and hitting ^C.
Depending on your config, you should see the shell prompt,
but pressing a key causes the pager to do any terminal
de-initialization sequence.

This patch just intercepts any death-dealing signals and
waits for the pager before dying. Under typical less
configuration, that means hitting ^C will cause git to stop
generating output, but the pager will keep running.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/pager.c b/pager.c
index f19ddbc..4921843 100644
--- a/pager.c
+++ b/pager.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "run-command.h"
+#include "sigchain.h"
 
 /*
  * This is split up from the rest of git so that we can do
@@ -38,6 +39,13 @@ static void wait_for_pager(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_signal(int signo)
+{
+	wait_for_pager();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
 void setup_pager(void)
 {
 	const char *pager = getenv("GIT_PAGER");
@@ -75,6 +83,7 @@ void setup_pager(void)
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
+	sigchain_push_common(wait_for_pager_signal);
 	atexit(wait_for_pager);
 }
 
-- 
1.6.1.84.g8150

Re: [PATCH 4/4] pager: do wait_for_pager on signal death

[Junio C Hamano]

Thanks; I agree with you that I do not see a good reason _not_ to be
handling those other signals at some of the "just handle SIGINT" sites,
and I like the direction in whcih this series is taking us very much.

Re: Funny: git -p submodule summary

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

Jeff King schrieb:
> On Fri, Jan 09, 2009 at 11:36:41AM +0100, Johannes Sixt wrote:
> 
>> I'll test your other patch (that replaces the execvp in git.c by
>> run_command).
> 
> There is something funny with it that I have not diagnosed: aliases are
> broken, and "git foobar" does not return an error. Presumably just
> checking the "we did not exec succesfully" case is not triggering
> properly.  However, I think the right solution is actually to refactor
> git.c to figure out ahead of time whether we have a builtin, external,
> or alias. I can work on that, but not tonight, as my git-time is up for
> now.
> 
> But other than that, did it work for you on Windows?

It passed the test suite. This should already work better on Windows,
because we already *do* look-up the program, and exit from
mingw_spawnvpe() before the equivalent of fork+exec happens.

> However, here is a 4-patch series that handles the separate signal
> delivery problem. It should fix the "^C makes funny things happen"
> problems you were seeing. Please test and let me know how it works on
> Windows.

It does help a bit. The interesting thing is that the only case where I
can now reproduces the unwanted behavior with the unpatched version is
when all output was completely read by 'less' and git already waits in
wait_for_pager(), such as in 'git show'. But Ctrl-C'ing a 'git log -p'
works as expected even without these patches. With the patches, the 'git
show' case now works as well.

> The patches are:
>   1/4: Makefile: clean up TEST_PROGRAMS definition
>   2/4: chain kill signals for cleanup functions
>   3/4: refactor signal handling for cleanup functions
>   4/4: pager: do wait_for_pager on signal death

But we need to insert the patch below *before* 2/4. The test case needs a
change, too,(exit code on Windows is 3, not 130) but I'll keep that in my
repository, like with all other Windows related test suite changes.

-- Hannes

-- 8< --
From: Johannes Sixt <j6t@kdbg.org>
Subject: Windows: Fix signal numbers

We had defined some SIG_FOO macros that appear in the code, but that are
not supported on Windows, in order to make the code compile.  But a
subsequent change will assert that a signal number is non-zero.  We now
use the signal numbers that are commonly used on POSIX systems.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/mingw.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 4f275cb..a255898 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -21,12 +21,12 @@ typedef int pid_t;
 #define WEXITSTATUS(x) ((x) & 0xff)
 #define WIFSIGNALED(x) ((unsigned)(x) > 259)

-#define SIGKILL 0
-#define SIGCHLD 0
-#define SIGPIPE 0
-#define SIGHUP 0
-#define SIGQUIT 0
-#define SIGALRM 100
+#define SIGHUP 1
+#define SIGQUIT 3
+#define SIGKILL 9
+#define SIGPIPE 13
+#define SIGALRM 14
+#define SIGCHLD 17

 #define F_GETFD 1
 #define F_SETFD 2
-- 
1.6.1.rc4.959.gcece.dirty

Re: Funny: git -p submodule summary

[Jeff King]

On Mon, Jan 12, 2009 at 11:59:04AM +0100, Johannes Sixt wrote:

> But we need to insert the patch below *before* 2/4. The test case needs a
> change, too,(exit code on Windows is 3, not 130) but I'll keep that in my
> repository, like with all other Windows related test suite changes.

Hrm. How do you properly detect "killed by SIGINT" on Windows? That is
the intent of that test.

> -#define SIGKILL 0
> -#define SIGCHLD 0
> -#define SIGPIPE 0
> -#define SIGHUP 0
> -#define SIGQUIT 0
> -#define SIGALRM 100
> +#define SIGHUP 1
> +#define SIGQUIT 3
> +#define SIGKILL 9
> +#define SIGPIPE 13
> +#define SIGALRM 14
> +#define SIGCHLD 17

Don't these get fed to signal()? Does Windows really not care about
getting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or
are we just ignoring the return code everywhere?

-Peff

Re: Funny: git -p submodule summary

[Johannes Sixt]

Jeff King schrieb:
> Hrm. How do you properly detect "killed by SIGINT" on Windows?

Good question. The exit code is 0xc000013a if the signal was generated by
hitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the
signal was "generated" by 'raise(SIGINT)'. The implementation of the
latter just calls _exit(3) (same for other deadly signals). Wow!

>> -#define SIGKILL 0
>> -#define SIGCHLD 0
>> -#define SIGPIPE 0
>> -#define SIGHUP 0
>> -#define SIGQUIT 0
>> -#define SIGALRM 100
>> +#define SIGHUP 1
>> +#define SIGQUIT 3
>> +#define SIGKILL 9
>> +#define SIGPIPE 13
>> +#define SIGALRM 14
>> +#define SIGCHLD 17
> 
> Don't these get fed to signal()? Does Windows really not care about
> getting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or
> are we just ignoring the return code everywhere?

Windows's signal() does return EINVAL; appearently, we never check the
return code. :-P

-- Hannes

Re: Funny: git -p submodule summary

[Jeff King]

On Mon, Jan 12, 2009 at 01:00:05PM +0100, Johannes Sixt wrote:

> Good question. The exit code is 0xc000013a if the signal was generated by
> hitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the
> signal was "generated" by 'raise(SIGINT)'. The implementation of the
> latter just calls _exit(3) (same for other deadly signals). Wow!

Eh? So it doesn't even run the signal handlers? Or if it is SIG_DFL
(i.e., we have already run all the handlers), then it just calls _exit?

> > Don't these get fed to signal()? Does Windows really not care about
> > getting bogus numbers versus 0 (which is, admittedly, bogus itself)? Or
> > are we just ignoring the return code everywhere?
> 
> Windows's signal() does return EINVAL; appearently, we never check the
> return code. :-P

OK. That makes sense; we don't bother checking the return code because
it's mostly just registering a cleanup function. The only sane thing to
do on an error (besides ignore it) is to abort whatever we're doing, and
I'm not sure it's worth it.

I do feel a little like this is a trap waiting to spring for future
coders to break Windows, but I think your change doesn't make anything
_worse_ in that regard.

-Peff

Re: Funny: git -p submodule summary

[Johannes Sixt]

Jeff King schrieb:
> On Mon, Jan 12, 2009 at 01:00:05PM +0100, Johannes Sixt wrote:
> 
>> Good question. The exit code is 0xc000013a if the signal was generated by
>> hitting Ctrl-C (tested with 'git --no-pager log'), but it is 3 if the
>> signal was "generated" by 'raise(SIGINT)'. The implementation of the
>> latter just calls _exit(3) (same for other deadly signals). Wow!
> 
> Eh? So it doesn't even run the signal handlers? Or if it is SIG_DFL
> (i.e., we have already run all the handlers), then it just calls _exit?

It's silly, but not *that* silly: It calls _exit(3) only if the handler is
SIG_DFL.

-- Hannes

Re: Funny: git -p submodule summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So the _real_ problem is that we are not always triggering the "wait for
> pager to finish" code because we exec and forget about it. Which means
> this strategy of "git runs child pager" will never work properly.
> Instead, we have to use three processes: git and the pager become child
> processes, while the original process waits for both to exit and returns
> the proper exit code from git.
>
> Let me try to work up a patch.

This arrangement to have the third process could even open the possibility
of having it read from git and write to pager, and not launching the pager
if there is no interesting data from git to feed it with.

I do not know if I like the performance implications associated with it,
though.

Re: Funny: git -p submodule summary

[Jeff King]

On Fri, Jan 09, 2009 at 01:30:23AM -0800, Junio C Hamano wrote:

> This arrangement to have the third process could even open the possibility
> of having it read from git and write to pager, and not launching the pager
> if there is no interesting data from git to feed it with.
> 
> I do not know if I like the performance implications associated with it,
> though.

Ugh. That has definitely been a requested feature, but the thought of
essentially running "cat" in our pipeline strikes me as a bit kludgey.

On the other hand, we are by definition going to the pager in that case,
so in theory performance is less of a consideration.

But see my other mail for why a third process is hard to always do on
Windows.

-Peff

Re: Funny: git -p submodule summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 09, 2009 at 01:30:23AM -0800, Junio C Hamano wrote:
>
>> This arrangement to have the third process could even open the possibility
>> of having it read from git and write to pager, and not launching the pager
>> if there is no interesting data from git to feed it with.
>> 
>> I do not know if I like the performance implications associated with it,
>> though.
>
> Ugh. That has definitely been a requested feature, but the thought of
> essentially running "cat" in our pipeline strikes me as a bit kludgey.
>
> On the other hand, we are by definition going to the pager in that case,
> so in theory performance is less of a consideration.
>
> But see my other mail for why a third process is hard to always do on
> Windows.

Heh, this late at night just before going to bed, I am allowed to say that
I do not care about Windows at all ;-).  More dedicated and competent
people will solve it for us while I am sleeping.

[RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Jeff King]

On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:

> Just try this with a submodule that has more changes than fit on a screen:
> 
> 	$ git -p submodule summary
> 
> In my tests, it consistently fscks up my console.  I wonder if this is 
> related to ea27a18(spawn pager via run_command interface).

OK, here is a patch series that fixes the problem:

  1/3: git: s/run_command/run_builtin/
  2/3: run_command: handle missing command errors more gracefully
  3/3: git: use run_command to execute dashed externals

1 is a cleanup, 2 is infrastructure support, and 3 is the actual fix.

There are two potential downsides to the fix:

 1. There is an extra fork and a parent process sitting in memory for
    dashed externals. This is pretty necessary to any fix, since
    something has to wait to do pager cleanup, and we can't rely on the
    child to do so.

 2. A failed attempt to execute a dashed external results in an extra
    fork. For builtins, this has no impact, since they take precedence.
    For aliases, though, it means we will do an extra fork before
    realizing that there is no dashed external and trying the alias.

We can fix '2' by actually doing the PATH lookup ourselves, and only
calling run_command if we know we have a match.  We can also reduce the
impact of both by only doing this multi-process magic if we have spawned
a pager; then only a small subset of invocations needs to pay for it.

I chose not to do the second optimization because it makes the code more
complex and inconsistent (we now have two different ways of doing the
same thing, depending on a seemingly unrelated setting) and fragile
(the pager might not be the only atexit handler installed).

The first (doing PATH lookup ourselves) might make sense, though.

JSixt, there are some tweaks to the Windows code to report back the exec
error. They look obviously correct to me, but I have no box to test
(even compile test) them on.

-Peff

[RFC/PATCH 1/3] git: s/run_command/run_builtin/

[Jeff King]

There is a static function called run_command which
conflicts with the library function in run-command.c; this
isn't a problem currently, but prevents including
run-command.h in git.c.

This patch just renames the static function to something
more specific and non-conflicting.

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index ecc8fad..45e493d 100644
--- a/git.c
+++ b/git.c
@@ -219,7 +219,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 +384,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));
 	}
 }
 
-- 
1.6.1.1.367.g30b36

[RFC/PATCH 2/3] run_command: handle missing command errors more gracefully

[Jeff King]

When run_command was asked to run a non-existant command,
its behavior varied depending on the platform:

  - on POSIX systems, we would fork, and then after the
    execvp call failed, we could call die(), which prints a
    message to stderr and exits with code 128.

  - on Windows, we do a PATH lookup, realize the program
    isn't there, and then return ERR_RUN_COMMAND_FORK

The goal of this patch is to make it clear to callers that
the specific error was a missing command. To do this, we
will return the error code ERR_RUN_COMMAND_EXEC, which is
already defined in run-command.h, checked for in several
places, but never actually gets set.

The new behavior is:

  - on POSIX systems, we exit the forked process with code
    127 (the same as the shell uses to report missing
    commands). The parent process recognizes this code and
    returns an EXEC error. The stderr message is silenced,
    since the caller may be speculatively trying to run a
    command. Instead, we use trace_printf so that somebody
    interested in debugging can see the error that occured.

  - on Windows, we check errno, which is already set
    correctly by mingw_spawnvpe, and report an EXEC error
    instead of a FORK error

Thus it is safe to speculatively run a command:

  int r = run_command_v_opt(argv, 0);
  if (r == -ERR_RUN_COMMAND_EXEC)
	  /* oops, it wasn't found; try something else */
  else
	  /* we failed for some other reason, error is in r */

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index db9ce59..2437798 100644
--- a/run-command.c
+++ b/run-command.c
@@ -118,7 +118,9 @@ int start_command(struct child_process *cmd)
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		die("exec %s failed.", cmd->argv[0]);
+		trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0],
+				strerror(errno));
+		exit(127);
 	}
 #else
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
@@ -197,7 +199,13 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		if (need_err)
 			close_pair(fderr);
+#ifndef __MINGW32__
 		return -ERR_RUN_COMMAND_FORK;
+#else
+		return errno == ENOENT ?
+			-ERR_RUN_COMMAND_EXEC :
+			-ERR_RUN_COMMAND_FORK;
+#endif
 	}
 
 	if (need_in)
@@ -236,9 +244,14 @@ static int wait_or_whine(pid_t pid)
 		if (!WIFEXITED(status))
 			return -ERR_RUN_COMMAND_WAITPID_NOEXIT;
 		code = WEXITSTATUS(status);
-		if (code)
+		switch (code) {
+		case 127:
+			return -ERR_RUN_COMMAND_EXEC;
+		case 0:
+			return 0;
+		default:
 			return -code;
-		return 0;
+		}
 	}
 }
 
-- 
1.6.1.1.367.g30b36

[RFC/PATCH 3/3] git: use run_command to execute dashed externals

[Jeff King]

We used to simply try calling execvp(); if it succeeded,
then we were done and the new program was running. If it
didn't, then we knew that it wasn't a valid command.

Unfortunately, this interacted badly with the new pager
handling. Now that git remains the parent process and the
pager is spawned, git has to hang around until the pager is
finished. We install an atexit handler to do this, but that
handler never gets called if we successfully run execvp.

You could see this behavior by running any dashed external
using a pager (e.g., "git -p stash list"). The command
finishes running, but the pager is still going. In the case
of less, it then gets an error reading from the terminal and
exits, potentially leaving the terminal in a broken state
(and not showing the output).

This patch just uses run_command to try running the
dashed external. The parent git process then waits for the
external process to complete and then handles the pager
cleanup as it would for an internal command.

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 45e493d..79a836c 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]";
@@ -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,10 +408,14 @@ 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);
-
-	trace_printf("trace: exec failed: %s\n", strerror(errno));
+	/*
+	 * if we fail because the command is not found, it is
+	 * OK to return. Otherwise, we just pass along the status code.
+	 */
+	status = run_command_v_opt(argv, 0);
+	if (status != -ERR_RUN_COMMAND_EXEC)
+		exit(status);
+	errno = ENOENT; /* as if we called execvp */
 
 	argv[0] = tmp;
 
-- 
1.6.1.1.367.g30b36

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Johannes Sixt]

Jeff King schrieb:
> JSixt, there are some tweaks to the Windows code to report back the exec
> error. They look obviously correct to me, but I have no box to test
> (even compile test) them on.

Generally, I like this series, in particular since it does not degrade 
performance on Windows ;)

But the following is needed in addition.

- Note that run_command returns the negated exit code, therefore, we 
must negate it again in the call to exit(). Without this t6030 failed. 
(And negative exit codes causes grief on Windows because bash for some 
reason does not recognize that as failure.)

- The close() calls can overwrite errno. And since fork() should not 
(cannot?) fail with ENOENT, it's safe to remove the #ifdef __MINGW32__.

-- Hannes

diff --git a/git.c b/git.c
index 79a836c..35635d1 100644
--- a/git.c
+++ b/git.c
@@ -414,7 +414,7 @@ static void execv_dashed_external
  	 */
  	status = run_command_v_opt(argv, 0);
  	if (status != -ERR_RUN_COMMAND_EXEC)
-		exit(status);
+		exit(-status);
  	errno = ENOENT; /* as if we called execvp */

  	argv[0] = tmp;
diff --git a/run-command.c b/run-command.c
index 2437798..b05c734 100644
--- a/run-command.c
+++ b/run-command.c
@@ -187,27 +187,24 @@ int start_command
  	if (s2 >= 0)
  		dup2(s2, 2), close(s2);
  #endif

  	if (cmd->pid < 0) {
+		int err = errno;
  		if (need_in)
  			close_pair(fdin);
  		else if (cmd->in)
  			close(cmd->in);
  		if (need_out)
  			close_pair(fdout);
  		else if (cmd->out)
  			close(cmd->out);
  		if (need_err)
  			close_pair(fderr);
-#ifndef __MINGW32__
-		return -ERR_RUN_COMMAND_FORK;
-#else
-		return errno == ENOENT ?
+		return err == ENOENT ?
  			-ERR_RUN_COMMAND_EXEC :
  			-ERR_RUN_COMMAND_FORK;
-#endif
  	}

  	if (need_in)
  		close(fdin[0]);
  	else if (cmd->in)

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Jeff King]

On Tue, Jan 27, 2009 at 11:06:40AM +0100, Johannes Sixt wrote:

> - Note that run_command returns the negated exit code, therefore, we must 
> negate it again in the call to exit(). Without this t6030 failed. (And 
> negative exit codes causes grief on Windows because bash for some reason 
> does not recognize that as failure.)

Oops, indeed. And you made me realize that I forgot to run the test
script against this patchset.

However, I'm not sure just negating the exit code is sufficient.
run_command can return codes in the 10000 range for its own internal
errors. We don't want to pass those out through exit, which will
truncate them to 8 bits.

> - The close() calls can overwrite errno.

Good point.

> And since fork() should not  (cannot?) fail with ENOENT, it's safe to
> remove the #ifdef __MINGW32__.

Yeah, I thought of that, but I was worried it might make the code a
little bit non-obvious (but it does clean up an ifdef, which is ugly,
too).

Thanks for your feedback. I'll squash in your fixes and repost 2/3 later
today.

-Peff

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Johannes Sixt]

Jeff King schrieb:
> However, I'm not sure just negating the exit code is sufficient.
> run_command can return codes in the 10000 range for its own internal
> errors. We don't want to pass those out through exit, which will
> truncate them to 8 bits.

Exit code and start_command/finish_command's return code handling is a
complete mess IMHO and deserves a clean-up series of its own. If the few
codes at 10000 and above are truncated to 8 bits, then we get exit codes
16 and higher; I think that's good enough for this series.

-- Hannes

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Jeff King]

On Tue, Jan 27, 2009 at 01:46:19PM +0100, Johannes Sixt wrote:

> Exit code and start_command/finish_command's return code handling is a
> complete mess IMHO and deserves a clean-up series of its own. If the few

Yes, the negation is a bit confusing, just for being allowed to say

  if (run_command(foo) < 0)

since you end up having to store and re-negate anyway to get the actual
code. Plus the value of errno is untrustworthy, since we may have been
doing cleanup calls.

> codes at 10000 and above are truncated to 8 bits, then we get exit codes
> 16 and higher; I think that's good enough for this series.

I think it is nice to differentiate between an exit code from the
sub-program and our own error, though. See my updated series for what I
think is a reasonable one-liner fix.

-Peff

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Jeff King wrote:

> On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:
> 
> > Just try this with a submodule that has more changes than fit on a 
> > screen:
> > 
> > 	$ git -p submodule summary
> > 
> > In my tests, it consistently fscks up my console.  I wonder if this is 
> > related to ea27a18(spawn pager via run_command interface).
> 
> OK, here is a patch series that fixes the problem:
> 
>   1/3: git: s/run_command/run_builtin/
>   2/3: run_command: handle missing command errors more gracefully
>   3/3: git: use run_command to execute dashed externals
> 
> 1 is a cleanup, 2 is infrastructure support, and 3 is the actual fix.

I like the patch series, well designed and concise (especially with the 
fixes Hannes proposed).

> There are two potential downsides to the fix:
> 
>  1. There is an extra fork and a parent process sitting in memory for
>     dashed externals. This is pretty necessary to any fix, since
>     something has to wait to do pager cleanup, and we can't rely on the
>     child to do so.

Actually, I think this is a good thing; that way, we can catch 
segmentation fault properly and display an error message in the pager.  
That was not possible previously.

>  2. A failed attempt to execute a dashed external results in an extra
>     fork. For builtins, this has no impact, since they take precedence.
>     For aliases, though, it means we will do an extra fork before
>     realizing that there is no dashed external and trying the alias.

All the more reason to build more programs in :-)

Ciao,
Dscho

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Jeff King]

On Tue, Jan 27, 2009 at 05:31:02PM +0100, Johannes Schindelin wrote:

> I like the patch series, well designed and concise (especially with the 
> fixes Hannes proposed).

Good. Response seems positive, so I will drop the RFC, then, and post a
fixed-up series meant for inclusion.

> > There are two potential downsides to the fix:
> > 
> >  1. There is an extra fork and a parent process sitting in memory for
> >     dashed externals. This is pretty necessary to any fix, since
> >     something has to wait to do pager cleanup, and we can't rely on the
> >     child to do so.
> 
> Actually, I think this is a good thing; that way, we can catch 
> segmentation fault properly and display an error message in the pager.  
> That was not possible previously.

True. On the other hand, most of our externals are shell scripts. It's
the builtins that segfault, and we don't have a watcher process for
that. :)

-Peff

[PATCHv2 1/4] git: s/run_command/run_builtin/

[Jeff King]

There is a static function called run_command which
conflicts with the library function in run-command.c; this
isn't a problem currently, but prevents including
run-command.h in git.c.

This patch just renames the static function to something
more specific and non-conflicting.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before.

 git.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index ecc8fad..45e493d 100644
--- a/git.c
+++ b/git.c
@@ -219,7 +219,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 +384,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));
 	}
 }
 
-- 
1.6.1.1.367.g30b36

[PATCHv2 2/4] run_command: handle missing command errors more gracefully

[Jeff King]

When run_command was asked to run a non-existant command,
its behavior varied depending on the platform:

  - on POSIX systems, we would fork, and then after the
    execvp call failed, we could call die(), which prints a
    message to stderr and exits with code 128.

  - on Windows, we do a PATH lookup, realize the program
    isn't there, and then return ERR_RUN_COMMAND_FORK

The goal of this patch is to make it clear to callers that
the specific error was a missing command. To do this, we
will return the error code ERR_RUN_COMMAND_EXEC, which is
already defined in run-command.h, checked for in several
places, but never actually gets set.

The new behavior is:

  - on POSIX systems, we exit the forked process with code
    127 (the same as the shell uses to report missing
    commands). The parent process recognizes this code and
    returns an EXEC error. The stderr message is silenced,
    since the caller may be speculatively trying to run a
    command. Instead, we use trace_printf so that somebody
    interested in debugging can see the error that occured.

  - on Windows, we check errno, which is already set
    correctly by mingw_spawnvpe, and report an EXEC error
    instead of a FORK error

Thus it is safe to speculatively run a command:

  int r = run_command_v_opt(argv, 0);
  if (r == -ERR_RUN_COMMAND_EXEC)
	  /* oops, it wasn't found; try something else */
  else
	  /* we failed for some other reason, error is in r */

Signed-off-by: Jeff King <peff@peff.net>
---
Incorporates JSixt's fix to retain errno across system calls.

 run-command.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index db9ce59..b05c734 100644
--- a/run-command.c
+++ b/run-command.c
@@ -118,7 +118,9 @@ int start_command(struct child_process *cmd)
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		die("exec %s failed.", cmd->argv[0]);
+		trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0],
+				strerror(errno));
+		exit(127);
 	}
 #else
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
@@ -187,6 +189,7 @@ int start_command(struct child_process *cmd)
 #endif
 
 	if (cmd->pid < 0) {
+		int err = errno;
 		if (need_in)
 			close_pair(fdin);
 		else if (cmd->in)
@@ -197,7 +200,9 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		if (need_err)
 			close_pair(fderr);
-		return -ERR_RUN_COMMAND_FORK;
+		return err == ENOENT ?
+			-ERR_RUN_COMMAND_EXEC :
+			-ERR_RUN_COMMAND_FORK;
 	}
 
 	if (need_in)
@@ -236,9 +241,14 @@ static int wait_or_whine(pid_t pid)
 		if (!WIFEXITED(status))
 			return -ERR_RUN_COMMAND_WAITPID_NOEXIT;
 		code = WEXITSTATUS(status);
-		if (code)
+		switch (code) {
+		case 127:
+			return -ERR_RUN_COMMAND_EXEC;
+		case 0:
+			return 0;
+		default:
 			return -code;
-		return 0;
+		}
 	}
 }
 
-- 
1.6.1.1.367.g30b36

[PATCHv2 3/4] run-command: help callers distinguish errors

[Jeff King]

run_command returns a single integer specifying either an
error code or the exit status of the spawned program. The
only way to tell the difference is that the error codes are
outside of the allowed range of exit status values.

Rather than make each caller implement the test against a
magic limit, let's provide a macro.

Signed-off-by: Jeff King <peff@peff.net>
---
New since v1 of the series.

 run-command.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/run-command.h b/run-command.h
index 0211e1d..a88b3cd 100644
--- a/run-command.h
+++ b/run-command.h
@@ -10,6 +10,7 @@ enum {
 	ERR_RUN_COMMAND_WAITPID_SIGNAL,
 	ERR_RUN_COMMAND_WAITPID_NOEXIT,
 };
+#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)
 
 struct child_process {
 	const char **argv;
-- 
1.6.1.1.367.g30b36

Re: [PATCHv2 3/4] run-command: help callers distinguish errors

[Jeff King]

On Wed, Jan 28, 2009 at 02:36:39AM -0500, Jeff King wrote:

> +#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)

<sigh> This should be "<= -ERR_RUN_COMMAND_FORK", since we expect the
negated status.

Maybe it is time I wrote a test for a failed external.

-Peff

Re: [PATCHv2 3/4] run-command: help callers distinguish errors

[Jeff King]

On Wed, Jan 28, 2009 at 02:43:34AM -0500, Jeff King wrote:

> On Wed, Jan 28, 2009 at 02:36:39AM -0500, Jeff King wrote:
> 
> > +#define IS_RUN_COMMAND_ERR(x) (x >= ERR_RUN_COMMAND_FORK)
> 
> <sigh> This should be "<= -ERR_RUN_COMMAND_FORK", since we expect the
> negated status.
> 
> Maybe it is time I wrote a test for a failed external.

Hmm. Actually, it is hard to write a test case for this; it only
triggers if we actually have an error running a program, like fork()
failing.

-Peff

[PATCHv2 4/4] git: use run_command to execute dashed externals

[Jeff King]

We used to simply try calling execvp(); if it succeeded,
then we were done and the new program was running. If it
didn't, then we knew that it wasn't a valid command.

Unfortunately, this interacted badly with the new pager
handling. Now that git remains the parent process and the
pager is spawned, git has to hang around until the pager is
finished. We install an atexit handler to do this, but that
handler never gets called if we successfully run execvp.

You could see this behavior by running any dashed external
using a pager (e.g., "git -p stash list"). The command
finishes running, but the pager is still going. In the case
of less, it then gets an error reading from the terminal and
exits, potentially leaving the terminal in a broken state
(and not showing the output).

This patch just uses run_command to try running the
dashed external. The parent git process then waits for the
external process to complete and then handles the pager
cleanup as it would for an internal command.

Signed-off-by: Jeff King <peff@peff.net>
---
Incorporates negated status fix from JSixt.

This version also differentiates in the exit code and stderr output
whether we simply failed to exec the command versus passing along its
status code.

 git.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 45e493d..b02b05b 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]";
@@ -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,10 +408,17 @@ 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);
-
-	trace_printf("trace: exec failed: %s\n", strerror(errno));
+	/*
+	 * if we fail because the command is not found, it is
+	 * OK to return. Otherwise, we just pass along the status code.
+	 */
+	status = run_command_v_opt(argv, 0);
+	if (status != -ERR_RUN_COMMAND_EXEC) {
+		if (IS_RUN_COMMAND_ERR(status))
+			die("unable to run '%s'", argv[0]);
+		exit(-status);
+	}
+	errno = ENOENT; /* as if we called execvp */
 
 	argv[0] = tmp;
 
-- 
1.6.1.1.367.g30b36

Re: [RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 27, 2009 at 05:31:02PM +0100, Johannes Schindelin wrote:
>
>> I like the patch series, well designed and concise (especially with the 
>> fixes Hannes proposed).

The series looks quite sane and clean.  Will queue once I finish wrapping
up the push with ".have" thing.