Jeff King [off-list ref] writes:
On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:
quoted
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.
Yeah, I agree 100% with that reasoning. I seem to recall that was how this
commit was done in what I privately reviewed after Clemens announced his
github branch?
Several git commands are so-called dashed externals, that is commands
executed as a child process of the git wrapper command. If the git
wrapper is killed by a signal, the child process will continue to run.
This is different from internal commands, which always die with the git
wrapper command.
Enable the recently introduced cleanup mechanism for child processes in
order to make dashed externals act more in line with internal commands.
Signed-off-by: Clemens Buchacher <redacted>
---
On Sat, Jan 07, 2012 at 10:26:49PM -0800, Junio C Hamano wrote:
Yeah, I agree 100% with that reasoning. I seem to recall that was how this
commit was done in what I privately reviewed after Clemens announced his
github branch?
What I had previously enabled child cleanup for all callers of
run_command_v_opt. There is quite a few of those as well, most of them
not related to dashed externals at all.
So I reworked that patch a bit to enable cleanup only for dashed
externals. This is a replacement for "[PATCH 2/5] run-command: kill
children on exit by default".
I also re-added Jeff's "send-pack: kill pack-objects helper on signal or
exit" and I dropped the extraneous newline that Erik spotted in
"[PATCH 1/5] run-command: optionally kill children on exit".
I have pushed the reworked series to cb/git-daemon-tests on my github
repo.
git.c | 2 +-
run-command.c | 1 +
run-command.h | 1 +
3 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/git.c b/git.c
index fb9029c..3805616 100644
--- a/git.c
+++ b/git.c
@@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv)
* 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, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
if (status >= 0 || errno != ENOENT)
exit(status);
diff --git a/run-command.c b/run-command.c
index fff9073..90bfd8c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -496,6 +496,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+ cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
}
int run_command_v_opt(const char **argv, int opt)
diff --git a/run-command.h b/run-command.h
index 2a69466..44f7d2b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...);
#define RUN_COMMAND_STDOUT_TO_STDERR 4
#define RUN_SILENT_EXEC_FAILURE 8
#define RUN_USING_SHELL 16
+#define RUN_CLEAN_ON_EXIT 32
int run_command_v_opt(const char **argv, int opt);
/*
--
1.7.8
On Sun, Jan 08, 2012 at 09:41:09PM +0100, Clemens Buchacher wrote:
What I had previously enabled child cleanup for all callers of
run_command_v_opt. There is quite a few of those as well, most of them
not related to dashed externals at all.
So I reworked that patch a bit to enable cleanup only for dashed
externals. This is a replacement for "[PATCH 2/5] run-command: kill
children on exit by default".
Thanks, this looks much closer to what I was expecting.
-Peff