Re: [PATCHv19 00/11] Expose submodule parallelism to the user
On Mon, Feb 29, 2016 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> Maybe we want to remove the struct child_process from the
>> function signature of the callbacks and callbacks need to rely on
>> the data provided solely thru the pointer as passed around for
>> callback purposes, which the user is free to use for any kind
>> of data.
>
> I think that is the most sensible.
Ok I'll send the diff above as a patch.
Show 7 quoted lines
>
>> As a rather quickfix for 2.8 (and 2.7) we could however just
>> breakup the finish_command function and call child_process_clear
>> after the callbacks have run.
>
> That would not fix the case where run_command() fails, clears child
> and then start failure callback, no?
For that I was about to propose (broken whitespace below):
commit a0dcfbf86b6b720529958a4043d3679ed6f340d0
Author: Stefan Beller <sbeller@google.com>
Date: Mon Feb 29 13:20:38 2016 -0800
run-command: factor cleanup out of start_command
The default callback of the parallel processing infrastructure assumes
start_command will not clear the child process upon failing to start a
child process. However having access to the child's argument vector
enables easy error reporting, so move cleaning the child into a wrapper
for general use and the parallel processing infrastructure needs to take
care of cleaning up the child itself Signed-off-by: Stefan Beller <sbeller@google.com>
diff --git a/run-command.c b/run-command.c
index 863dad5..b1d97d0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -265,7 +265,7 @@ static int wait_or_whine(pid_t pid, const char
*argv0, int in_signal)
return code;
}
-int start_command(struct child_process *cmd)
+static int start_command_uncleaned(struct child_process *cmd)
{
int need_in, need_out, need_err;
int fdin[2], fdout[2], fderr[2];
@@ -326,7 +326,6 @@ int start_command(struct child_process *cmd)
fail_pipe:
error("cannot create %s pipe for %s: %s",
str, cmd->argv[0], strerror(failed_errno));
- child_process_clear(cmd);
errno = failed_errno;
return -1;
}
@@ -510,7 +509,7 @@ fail_pipe:
close_pair(fderr);
else if (cmd->err)
close(cmd->err);
- child_process_clear(cmd);
+
errno = failed_errno;
return -1;
}
@@ -533,6 +532,14 @@ fail_pipe:
return 0;
}
+int start_command(struct child_process *cmd)
+{
+ int code = start_command_uncleaned(cmd);
+ if (code == -1)
+ child_process_clear(cmd);
+ return code;
+}
+
int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
@@ -1047,11 +1054,12 @@ static int pp_start_one(struct parallel_processes *pp)
pp->children[i].process.stdout_to_stderr = 1;
pp->children[i].process.no_stdin = 1;
- if (start_command(&pp->children[i].process)) {
+ if (start_command_uncleaned(&pp->children[i].process)) {
code = pp->start_failure(&pp->children[i].process,
&pp->children[i].err,
pp->data,
&pp->children[i].data);
+ child_process_clear(&pp->children[i].process);
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
if (code)
Stefan Beller <sbeller@google.com>