Re: [PATCH 1/2] daemon: add tests

11 messages, 4 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 1/2] daemon: add tests

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:44

Clemens Buchacher [off-list ref] writes:

I have rebased Junio's cb/git-daemon-tests onto your
jk/child-cleanup and replaced the call to pkill with a regular kill
command.

On top of that, I have added two commits to fix the discussed race
condition. I also verified that the race condition actually happens
by adding an artificial delay in the daemon (this change is
obviously not included).

I pushed the new cb/git-daemon-tests to
https://github.com/drizzd/git . If you have no objections I will
post the entire series including your run-command and send-pack
patches to the list.

Looked fine except that some patches seem to lack enough justification
(justification in Peff's reply was good enough).

I actually was thinking that the previous round was good enough (perhaps
dropping the "pkill" bit altogether and replacing it with "kill" on the
daemon process itself, if OSX folks complain loudly), so it is in "next"
already, but it seems that the best course of action would be to drop it
and queue your re-roll afresh, aiming for the next cycle.

Thanks.

[PATCH 2/5] run-command: kill children on exit by default

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

It feels natural for a user to view git commands as monolithic
commands with a single thread of execution. If the parent git
command dies, it should therefore clean up its child processes as
well. So enable the cleanup mechanism by default.

For dashed externals, this means that killing the git wrapper will
kill the command itself, just like what would happen in case of an
internal command. A notable exception is the credentials cache
daemon, which must stay alive after the store command has
completed.

Signed-off-by: Clemens Buchacher <redacted>
---

I considered squashing this into the previous commit. But it's a fairly
small change and may help with bisecting in case of problems.

 credential-cache.c |    1 +
 run-command.c      |    4 ++--
 run-command.h      |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/credential-cache.c b/credential-cache.c
index dc98372..15e7236 100644
--- a/credential-cache.c
+++ b/credential-cache.c

@@ -48,6 +48,7 @@ static void spawn_daemon(const char *socket)
 	daemon.argv = argv;
 	daemon.no_stdin = 1;
 	daemon.out = -1;
+	daemon.stay_alive_on_exit = 1;
 
 	if (start_command(&daemon))
 		die_errno("unable to start cache daemon");

diff --git a/run-command.c b/run-command.c
index 0204aaf..fe07b20 100644
--- a/run-command.c
+++ b/run-command.c

@@ -353,7 +353,7 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
-	else if (cmd->clean_on_exit)
+	else if (!cmd->stay_alive_on_exit)
 		mark_child_for_cleanup(cmd->pid);
 
 	/*

@@ -420,7 +420,7 @@ fail_pipe:
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
-	if (cmd->clean_on_exit && cmd->pid >= 0)
+	if (!cmd->stay_alive_on_exit && cmd->pid >= 0)
 		mark_child_for_cleanup(cmd->pid);
 
 	if (cmd->env)

diff --git a/run-command.h b/run-command.h
index 2a69466..69dbea1 100644
--- a/run-command.h
+++ b/run-command.h

@@ -38,7 +38,7 @@ struct child_process {
 	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
-	unsigned clean_on_exit:1;
+	unsigned stay_alive_on_exit:1;
 	void (*preexec_cb)(void);
 };

-- 
1.7.8

[PATCH 4/5] git-daemon: produce output when ready

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

If a client tries to connect after git-daemon starts, but before it
opens a listening socket, the connection will fail. Output "[PID]
Ready to rumble]" after opening the socket successfully in order to
inform the user that the daemon is now ready to receive
connections.

Signed-off-by: Clemens Buchacher <redacted>
---
 daemon.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemon.c b/daemon.c
index 15ce918..ab21e66 100644
--- a/daemon.c
+++ b/daemon.c

@@ -1086,6 +1086,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
 
 	drop_privileges(cred);
 
+	loginfo("Ready to rumble");
+
 	return service_loop(&socklist);
 }

@@ -1270,10 +1272,8 @@ int main(int argc, char **argv)
 	if (inetd_mode || serve_mode)
 		return execute();
 
-	if (detach) {
+	if (detach)
 		daemonize();
-		loginfo("Ready to rumble");
-	}
 	else
 		sanitize_stdfds();

-- 
1.7.8

Re: [PATCH 1/2] daemon: add tests

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

On Fri, Jan 06, 2012 at 02:49:05PM -0800, Junio C Hamano wrote:

but it seems that the best course of action would be to drop it
and queue your re-roll afresh, aiming for the next cycle.

Here's the re-rolled series, also available as cb/git-daemon-tests based
on current master at https://github.com/drizzd/git .

[PATCH 1/5] run-command: optionally kill children on exit
[PATCH 2/5] run-command: kill children on exit by default
[PATCH 3/5] git-daemon: add tests
[PATCH 4/5] git-daemon: produce output when ready
[PATCH 5/5] git-daemon tests: wait until daemon is ready

On Fri, Jan 06, 2012 at 05:32:15PM -0500, Jeff King wrote:

On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:

quoted

I have rebased Junio's cb/git-daemon-tests onto your
jk/child-cleanup and replaced the call to pkill with a regular kill
command.

Looks pretty good from my cursory examination. I think you should fill
out the rationale for "kill dashed externals on exit" a bit. My
reasoning is that whether a git command is an internal or external
process is purely an implementation detail, and killing the git wrapper
should behave identically in both cases.

The previous version of this patch only changed the behavior for users
of run_command_v_opt, but not for those who filled out the child_process
structure by themselves. I could have manually enabled all of those, but
that felt unnatural. Instead, I have now reversed the meaning of
clean_on_exit to stay_alive_on_exit in [PATCH 2/5] run-command: kill
children on exit by default.  Cleanup is on by default and callers of
run_command must disable it if children should stay alive.

[PATCH 5/5] git-daemon tests: wait until daemon is ready

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

In start_daemon, git-daemon is started as a background process.  In
theory, the tests may try to connect before the daemon had a chance
to open a listening socket. Avoid this race condition by waiting
for it to output "Ready to rumble". Any other output is considered
an error and the test is aborted.

Should git-daemon produce no output at all, lib-git-daemon would
block forever. This could be fixed by introducing a timeout.  On
the other hand, we have no timeout for other git commands which
could suffer from the same problem. Since such a mechanism adds
some complexity, I have decided against it.

Signed-off-by: Clemens Buchacher <redacted>
---
 t/lib-git-daemon.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 5e81a25..ef2d01f 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh

@@ -23,12 +23,27 @@ start_git_daemon() {
 	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
 
 	say >&3 "Starting git daemon ..."
+	mkfifo git_daemon_output
 	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>&4 &
+		>&3 2>git_daemon_output &
 	GIT_DAEMON_PID=$!
+	{
+		read line
+		echo >&4 "$line"
+		cat >&4 &
+
+		# Check expected output
+		if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
+		then
+			kill "$GIT_DAEMON_PID"
+			wait "$GIT_DAEMON_PID"
+			trap 'die' EXIT
+			error "git daemon failed to start"
+		fi
+	} <git_daemon_output
 }
 
 stop_git_daemon() {

@@ -50,4 +65,5 @@ stop_git_daemon() {
 		error "git daemon exited with status: $ret"
 	fi
 	GIT_DAEMON_PID=
+	rm -f git_daemon_output
 }

-- 
1.7.8

[PATCH 1/5] run-command: optionally kill children on exit

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

From: Jeff King <redacted>

When we spawn a helper process, it should generally be done
and finish_command called before we exit. However, if we
exit abnormally due to an early return or a signal, the
helper may continue to run in our absence.

In the best case, this may simply be wasted CPU cycles or a
few stray messages on a terminal. But it could also mean a
process that the user thought was aborted continues to run
to completion (e.g., a push's pack-objects helper will
complete the push, even though you killed the push process).

This patch provides infrastructure for run-command to keep
track of PIDs to be killed, and clean them on signal
reception or input, just as we do with tempfiles. PIDs can
be added in two ways:

  1. If NO_PTHREADS is defined, async helper processes are
     automatically marked. By definition this code must be
     ready to die when the parent dies, since it may be
     implemented as a thread of the parent process.

  2. If the run-command caller specifies the "clean_on_exit"
     option. This is not the default, as there are cases
     where it is OK for the child to outlive us (e.g., when
     spawning a pager).

PIDs are cleared from the kill-list automatically during
wait_or_whine, which is called from finish_command and
finish_async.

Signed-off-by: Clemens Buchacher <redacted>
---

Not sure if I can sign off without your sign-off. Should I have
replaced this with Acked-by?

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

diff --git a/run-command.c b/run-command.c
index 1c51043..0204aaf 100644
--- a/run-command.c
+++ b/run-command.c

@@ -1,8 +1,66 @@
 #include "cache.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "sigchain.h"
 #include "argv-array.h"
 
+struct child_to_clean {
+	pid_t pid;
+	struct child_to_clean *next;
+};
+static struct child_to_clean *children_to_clean;
+static int installed_child_cleanup_handler;
+
+static void cleanup_children(int sig)
+{
+	while (children_to_clean) {
+		struct child_to_clean *p = children_to_clean;
+		children_to_clean = p->next;
+		kill(p->pid, sig);
+		free(p);
+	}
+}
+
+static void cleanup_children_on_signal(int sig)
+{
+	cleanup_children(sig);
+	sigchain_pop(sig);
+	raise(sig);
+}
+
+static void cleanup_children_on_exit(void)
+{
+	cleanup_children(SIGTERM);
+}
+
+static void mark_child_for_cleanup(pid_t pid)
+{
+	struct child_to_clean *p = xmalloc(sizeof(*p));
+	p->pid = pid;
+	p->next = children_to_clean;
+	children_to_clean = p;
+
+	if (!installed_child_cleanup_handler) {
+		atexit(cleanup_children_on_exit);
+		sigchain_push_common(cleanup_children_on_signal);
+		installed_child_cleanup_handler = 1;
+	}
+}
+
+static void clear_child_for_cleanup(pid_t pid)
+{
+	struct child_to_clean **last, *p;
+
+	last = &children_to_clean;
+	for (p = children_to_clean; p; p = p->next) {
+		if (p->pid == pid) {
+			*last = p->next;
+			free(p);
+			return;
+		}
+	}
+}
+
 static inline void close_pair(int fd[2])
 {
 	close(fd[0]);

@@ -130,6 +188,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	} else {
 		error("waitpid is confused (%s)", argv0);
 	}
+
+	clear_child_for_cleanup(pid);
+
 	errno = failed_errno;
 	return code;
 }

@@ -292,6 +353,8 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+	else if (cmd->clean_on_exit)
+		mark_child_for_cleanup(cmd->pid);
 
 	/*
 	 * Wait for child's execvp. If the execvp succeeds (or if fork()

@@ -312,6 +375,7 @@ fail_pipe:
 		cmd->pid = -1;
 	}
 	close(notify_pipe[0]);
+
 }
 #else
 {

@@ -356,6 +420,8 @@ fail_pipe:
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
+	if (cmd->clean_on_exit && cmd->pid >= 0)
+		mark_child_for_cleanup(cmd->pid);
 
 	if (cmd->env)
 		free_environ(env);

@@ -540,6 +606,8 @@ int start_async(struct async *async)
 		exit(!!async->proc(proc_in, proc_out, async->data));
 	}
 
+	mark_child_for_cleanup(async->pid);
+
 	if (need_in)
 		close(fdin[0]);
 	else if (async->in)

diff --git a/run-command.h b/run-command.h
index 56491b9..2a69466 100644
--- a/run-command.h
+++ b/run-command.h

@@ -38,6 +38,7 @@ struct child_process {
 	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
+	unsigned clean_on_exit:1;
 	void (*preexec_cb)(void);
 };

-- 
1.7.8

[PATCH 3/5] git-daemon: add tests

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

The semantics of the git daemon tests are similar to the http transport
tests.  In fact, they are only a slightly modified copy of t5550, plus the
newly added remote error tests.

All git-daemon tests will be skipped unless the environment variable
GIT_TEST_GIT_DAEMON is set.

Signed-off-by: Clemens Buchacher <redacted>
Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 t/lib-git-daemon.sh   |   53 +++++++++++++++++
 t/t5570-git-daemon.sh |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-git-daemon.sh
 create mode 100755 t/t5570-git-daemon.sh

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
new file mode 100644
index 0000000..5e81a25
--- /dev/null
+++ b/t/lib-git-daemon.sh

@@ -0,0 +1,53 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_GIT_DAEMON"
+then
+	skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)"
+	test_done
+fi
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}
+
+GIT_DAEMON_PID=
+GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT
+
+start_git_daemon() {
+	if test -n "$GIT_DAEMON_PID"
+	then
+		error "start_git_daemon already called"
+	fi
+
+	mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
+
+	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
+
+	say >&3 "Starting git daemon ..."
+	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+		--reuseaddr --verbose \
+		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		>&3 2>&4 &
+	GIT_DAEMON_PID=$!
+}
+
+stop_git_daemon() {
+	if test -z "$GIT_DAEMON_PID"
+	then
+		return
+	fi
+
+	trap 'die' EXIT
+
+	# kill git-daemon child of git
+	say >&3 "Stopping git daemon ..."
+	kill "$GIT_DAEMON_PID"
+	wait "$GIT_DAEMON_PID" >&3 2>&4
+	ret=$?
+	# expect exit with status 143 = 128+15 for signal TERM=15
+	if test $ret -ne 143
+	then
+		error "git daemon exited with status: $ret"
+	fi
+	GIT_DAEMON_PID=
+}

diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..7cbc999
--- /dev/null
+++ b/t/t5570-git-daemon.sh

@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+start_git_daemon
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+	mkdir "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 : >git-daemon-export-ok
+	) &&
+	git remote add public "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+	git clone "$GIT_DAEMON_URL/repo.git" clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public &&
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head -d origin &&
+	 git remote set-head -a origin &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
+test_expect_success 'prepare pack objects' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack -a -d
+	)
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad1.git" &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad2.git" &&
+	 test 0 = `ls objects/pack | wc -l`
+	)
+'
+
+test_remote_error()
+{
+	do_export=YesPlease
+	while test $# -gt 0
+	do
+		case $1 in
+		-x)
+			shift
+			chmod -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			;;
+		-n)
+			shift
+			do_export=
+			;;
+		*)
+			break
+		esac
+	done
+
+	if test $# -ne 3
+	then
+		error "invalid number of arguments"
+	fi
+
+	cmd=$1
+	repo=$2
+	msg=$3
+
+	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
+	then
+		if test -n "$do_export"
+		then
+			: >"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		else
+			rm -f "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		fi
+	fi
+
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	echo "fatal: remote error: $msg: /$repo" >expect &&
+	test_cmp expect output
+	ret=$?
+	chmod +x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	(exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+
+stop_git_daemon
+start_git_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+
+stop_git_daemon
+test_done

-- 
1.7.8

Re: [PATCH 1/5] run-command: optionally kill children on exit

From: Erik Faye-Lund <hidden>
Date: 2016-06-15 22:52:44

On Sat, Jan 7, 2012 at 12:42 PM, Clemens Buchacher [off-list ref] wrote:

+static void cleanup_children(int sig)
+{
+       while (children_to_clean) {
+               struct child_to_clean *p = children_to_clean;
+               children_to_clean = p->next;
+               kill(p->pid, sig);
+               free(p);
+       }
+}
+
+static void cleanup_children_on_signal(int sig)
+{
+       cleanup_children(sig);
+       sigchain_pop(sig);
+       raise(sig);
+}
+

Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
supports SIGKILL, so propagating other signals to child-processes will
fail with EINVAL. That being said, Windows' support for signals is
severely limited, but I'm not entirely sure which ones can be
generated in this case.

quoted hunk

@@ -312,6 +375,7 @@ fail_pipe:

               cmd->pid = -1;
       }
       close(notify_pipe[0]);
+
 }
 #else
 {

This hunk is probably unintentional...

Re: [PATCH 1/5] run-command: optionally kill children on exit

From: Jeff King <hidden>
Date: 2016-06-15 22:52:44

On Sat, Jan 07, 2012 at 12:42:43PM +0100, Clemens Buchacher wrote:

Signed-off-by: Clemens Buchacher <redacted>
---

Not sure if I can sign off without your sign-off. Should I have
replaced this with Acked-by?

Sorry, I usually sign-off when I sent to the list. But:

Signed-off-by: Jeff King <redacted>

for this and the other patch in this series.

As for whether you can sign-off, I think it is OK in this case. You are
basically signing off on the "Certificate of Origin" found in
SubmittingPatches. I think you are covered under (b), which is that to
the best of your knowledge it is based on open source work (i.e., even
though I didn't sign off explicitly, it is pretty obvious that this is
meant to be open source). But it's nicer to be explicit.

-Peff

Re: [PATCH 2/5] run-command: kill children on exit by default

From: Jeff King <hidden>
Date: 2016-06-15 22:52:44

On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:

It feels natural for a user to view git commands as monolithic
commands with a single thread of execution. If the parent git
command dies, it should therefore clean up its child processes as
well. So enable the cleanup mechanism by default.

I'm not sure this is a good idea. run_command is used in ~70 places in
git, and I'm sure at least one of them is going to be unhappy (I see you
found one in credential-cache, but how many others are there). I'd
rather be conservative and leave the default the same, and then switch
over callsites that make sense.

-Peff

PS I thought this would certainly break the pager, since it should
   outlast us after we finish producing output. But I think at one point
   I switched the pager invocation so that the git wrapper lives and
   waits until the pager dies.

Re: [PATCH 1/5] run-command: optionally kill children on exit

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:52:44

On Sat, Jan 07, 2012 at 01:45:03PM +0100, Erik Faye-Lund wrote:

Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
supports SIGKILL, so propagating other signals to child-processes will
fail with EINVAL. That being said, Windows' support for signals is
severely limited, but I'm not entirely sure which ones can be
generated in this case.

On Linux at least, SIGKILL is not a viable alternative for SIGTERM,
since it does not give the child process to do any cleanup of its own
(such as signaling its own children, for example).

In any case, due this whole experience, and recently another one with
overzealous virus scanners, I have added a "get rid of dashed externals"
work item to my TODO list.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help