Re: Scripted clone generating an incomplete, unusable .git/config

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> I don't think it makes sense for git-clone to do this itself. If we are
> going to say "SIGPIPE should default to SIG_DFL on startup" then we
> should do it as the very first thing in the git wrapper, not just for
> git-clone. That gives each git program a known starting point of
> behavior.

Here's what that might look like.

-- 8< --
Subject: SIGPIPE and other fatal signals should default to SIG_DFL

The intuitive behavior when a git command receives a fatal
signal is for it to die.

Indeed, that is the default handling.  However, POSIX semantics
allow the parent of a process to override that default by
ignoring a signal, since ignored signals are preserved by fork() and
exec().  For example, Python 2.6 and 2.7's os.popen() runs a shell
with SIGPIPE ignored (Python issue 1736483).

Worst of all, in such a situation, many git commands use
sigchain_push_common() to run cleanup actions (removing temporary
files, discarding locks, that kind of thing) before passing control to
the original handler; if that handler ignores the signal rather than
exiting, then execution will continue in an unplanned-for and unusual
state.

So give the signals in question default handling at the start of
main(), before sigchain_push_common can be called.

NEEDSWORK:

 - imposes an obnoxious maintenance burden.  New non-builtins
   that might call sigchain_push_common() would need to have
   default handling restored at the start of main().

 - needs tests, especially for interaction with "git daemon"
   client shutdown

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Currently git daemon uses SIG_IGN state on SIGTERM to protect
children with active connections.  Why isn't that causing the same
sort of problems as os.popen() causes?

 check-racy.c   |    1 +
 daemon.c       |    1 +
 fast-import.c  |    1 +
 git.c          |    2 ++
 http-backend.c |    1 +
 http-fetch.c   |    1 +
 http-push.c    |    1 +
 imap-send.c    |    1 +
 remote-curl.c  |    1 +
 shell.c        |    2 ++
 show-index.c   |    2 ++
 upload-pack.c  |    1 +
 12 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/check-racy.c b/check-racy.c
index 00d92a1..f1ad9d5 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -6,6 +6,7 @@ int main(int ac, char **av)
 	int dirty, clean, racy;
 
 	dirty = clean = racy = 0;
+	sigchain_push_common(SIG_DFL);
 	read_cache();
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
diff --git a/daemon.c b/daemon.c
index 7ccd097..7719f33 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1001,6 +1001,7 @@ int main(int argc, char **argv)
 	gid_t gid = 0;
 	int i;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 
 	for (i = 1; i < argc; i++) {
diff --git a/fast-import.c b/fast-import.c
index dc48245..ce28759 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3027,6 +3027,7 @@ int main(int argc, const char **argv)
 {
 	unsigned int i;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
diff --git a/git.c b/git.c
index e08d0f1..a8677fb 100644
--- a/git.c
+++ b/git.c
@@ -502,6 +502,8 @@ int main(int argc, const char **argv)
 	if (!cmd)
 		cmd = "git-help";
 
+	sigchain_push_common(SIG_DFL);
+
 	/*
 	 * "git-xxxx" is the same as "git xxxx", but we obviously:
 	 *
diff --git a/http-backend.c b/http-backend.c
index 14c90c2..b03455a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -550,6 +550,7 @@ int main(int argc, char **argv)
 	char *cmd_arg = NULL;
 	int i;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 	set_die_routine(die_webcgi);
 
diff --git a/http-fetch.c b/http-fetch.c
index 762c750..aca4e8f 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -24,6 +24,7 @@ int main(int argc, const char **argv)
 	int get_verbosely = 0;
 	int get_recover = 0;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 
 	while (arg < argc && argv[arg][0] == '-') {
diff --git a/http-push.c b/http-push.c
index c9bcd11..a9d0894 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1791,6 +1791,7 @@ int main(int argc, char **argv)
 	struct remote *remote;
 	char *rewritten_url = NULL;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 
 	repo = xcalloc(sizeof(*repo), 1);
diff --git a/imap-send.c b/imap-send.c
index 71506a8..eb13adc 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1538,6 +1538,7 @@ int main(int argc, char **argv)
 	int nongit_ok;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 
 	if (argc != 1)
 		usage(imap_send_usage);
diff --git a/remote-curl.c b/remote-curl.c
index 04d4813..804492d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -787,6 +787,7 @@ int main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
diff --git a/shell.c b/shell.c
index dea4cfd..9798b99 100644
--- a/shell.c
+++ b/shell.c
@@ -137,6 +137,8 @@ int main(int argc, char **argv)
 	int devnull_fd;
 	int count;
 
+	sigchain_push_common(SIG_DFL);
+
 	/*
 	 * Always open file descriptors 0/1/2 to avoid clobbering files
 	 * in die().  It also avoids not messing up when the pipes are
diff --git a/show-index.c b/show-index.c
index 4c0ac13..7d48c88 100644
--- a/show-index.c
+++ b/show-index.c
@@ -11,6 +11,8 @@ int main(int argc, char **argv)
 	unsigned int version;
 	static unsigned int top_index[256];
 
+	sigchain_push_common(SIG_DFL);
+
 	if (argc != 1)
 		usage(show_index_usage);
 	if (fread(top_index, 2 * 4, 1, stdin) != 1)
diff --git a/upload-pack.c b/upload-pack.c
index f05e422..4c764f7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -682,6 +682,7 @@ int main(int argc, char **argv)
 	int i;
 	int strict = 0;
 
+	sigchain_push_common(SIG_DFL);
 	git_extract_argv0_path(argv[0]);
 	read_replace_refs = 0;