Re: [PATCHv19 00/11] Expose submodule parallelism to the user

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 29, 2016 at 1:19 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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.
>
>> 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@xxxxxxxxxx>
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@xxxxxxxxxx>

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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]