git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] SQUASH???

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 25, 2015, 00:49 UTC
Message-ID
<xmqqa8sbuxu0.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1443129187-18572-3-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
>  * If you do not die() in start_failure_fn or return_value_fn, you
>    don't want to write to stderr directly as you would destroy the fine
>    ordering of the processes output. So make the err strbuf available in
>    both these functions, and make sure the strbuf is appended to the
>    buffered output in both cases

I think that is a sensible change. Don't we want the same for the other failure handler, though. Capture any message from it and append it to the output of the process that just finished, or something?

By the way, I understand that these two are solely for early review and I'll be getting them as either new patches or part of updated patches in the next reroll (i.e. you are not expecting me to split these apart and do "rebase -i" for you to the last-posted version). Asking only to make sure we are on the same wavelength.

Thanks.
Show 120 quoted lines
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 43 ++++++++++++++++++++++++++++++-------------
>  run-command.h |  1 +
>  2 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 494e1f8..0d22291 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -907,6 +907,7 @@ void default_start_failure(void *data,
>  
>  void default_return_value(void *data,
>  			  struct child_process *cp,
> +			  struct strbuf *err,
>  			  int result)
>  {
>  	int i;
> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
>  			"output will be degraded");
>  }
>  
> -/* returns 1 if a process was started, 0 otherwise */
> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
>  static int pp_start_one(struct parallel_processes *pp)
>  {
>  	int i;
> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
>  	if (!pp->get_next_task(pp->data,
>  			       &pp->children[i].process,
>  			       &pp->children[i].err))
> -		return 1;
> +		return 0;
>  
> -	if (start_command(&pp->children[i].process))
> +	if (start_command(&pp->children[i].process)) {
>  		pp->start_failure(pp->data,
>  				  &pp->children[i].process,
>  				  &pp->children[i].err);
> +		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
> +		strbuf_reset(&pp->children[i].err);
> +		return -1;
> +	}
>  
>  	set_nonblocking(pp->children[i].process.err);
>  
>  	pp->nr_processes++;
>  	pp->children[i].in_use = 1;
>  	pp->pfd[i].fd = pp->children[i].process.err;
> -	return 0;
> +	return 1;
>  }
>  
> -static void pp_buffer_stderr(struct parallel_processes *pp)
> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>  {
>  	int i;
>  
> -	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
> +	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
>  		if (errno == EINTR)
>  			continue;
>  		pp_cleanup(pp);
> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)
>  			error("waitpid is confused (%s)",
>  			      pp->children[i].process.argv[0]);
>  
> -		pp->return_value(pp->data, &pp->children[i].process, code);
> +		pp->return_value(pp->data, &pp->children[i].process,
> +				 &pp->children[i].err, code);
>  
>  		argv_array_clear(&pp->children[i].process.args);
>  		argv_array_clear(&pp->children[i].process.env_array);
> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
>  			   return_value_fn return_value)
>  {
>  	struct parallel_processes pp;
> -	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>  
> +	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>  	while (1) {
> -		while (pp.nr_processes < pp.max_processes &&
> -		       !pp_start_one(&pp))
> -			; /* nothing */
> -		if (!pp.nr_processes)
> +		int no_more_task, cnt;
> +		int output_timeout = 100;
> +		int spawn_cap = 4;
> +
> +		for (cnt = spawn_cap, no_more_task = 0;
> +		     cnt && pp.nr_processes < pp.max_processes;
> +		     cnt--) {
> +			if (!pp_start_one(&pp)) {
> +				no_more_task = 1;
> +				break;
> +			}
> +		}
> +
> +		if (no_more_task && !pp.nr_processes)
>  			break;
> -		pp_buffer_stderr(&pp);
> +		pp_buffer_stderr(&pp, output_timeout);
> +
>  		pp_output(&pp);
>  		pp_collect_finished(&pp);
>  	}
> diff --git a/run-command.h b/run-command.h
> index 3807fd1..f7035cb 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,
>  
>  typedef void (*return_value_fn)(void *data,
>  				struct child_process *cp,
> +				struct strbuf *err,
>  				int result);
>  
>  /**
Previous: Stefan BellerNext: Junio C Hamano
Message 22 of 39 in “[PATCHv4 00/14] fetch submodules in parallel and a preview on parallel "submodule update"”
  1. Stefan BellerSep 23, 2015
  2. 01/14 submodule: Send "Fetching submodule <foo>" to standard errorStefan Beller, Sep 23, 2015
  3. 02/14 xread: poll on non blocking fdsStefan Beller, Sep 23, 2015
  4. 03/14 xread_nonblock: add functionality to read from fds without blockingStefan Beller, Sep 23, 2015
  5. 04/14 strbuf: add strbuf_read_once to read without blockingStefan Beller, Sep 23, 2015
  6. 05/14 run-command: factor out return value computationStefan Beller, Sep 23, 2015
  7. 06/14 run-command: add an asynchronous parallel child processorStefan Beller, Sep 23, 2015
  8. Junio C HamanoSep 23, 2015
  9. Stefan BellerSep 23, 2015
  10. Junio C HamanoSep 23, 2015
  11. Junio C HamanoSep 23, 2015
  12. Stefan BellerSep 23, 2015
  13. Junio C HamanoSep 23, 2015
  14. Junio C HamanoSep 23, 2015
  15. Junio C HamanoSep 23, 2015
  16. Junio C HamanoSep 23, 2015
  17. [PATCHv5] Another squash on run-command: add an asynchronous parallel child processorStefan Beller, Sep 23, 2015
  18. Junio C HamanoSep 24, 2015
  19. 0/2 Another squash on run-command: add an asynchronous parallel child processorStefan Beller, Sep 24, 2015
  20. 2/2 SQUASH for "fetch_populated_submodules: use new parallel job processing"Stefan Beller, Sep 24, 2015
  21. 1/2 SQUASH???Stefan Beller, Sep 24, 2015
  22. Junio C HamanoSep 25, 2015
  23. Junio C HamanoSep 25, 2015
  24. Stefan BellerSep 25, 2015
  25. Junio C HamanoSep 25, 2015
  26. Junio C HamanoSep 25, 2015
  27. Stefan BellerSep 25, 2015
  28. Junio C HamanoSep 25, 2015
  29. Stefan BellerSep 25, 2015
  30. Junio C HamanoSep 25, 2015
  31. 07/14 fetch_populated_submodules: use new parallel job processingStefan Beller, Sep 23, 2015
  32. 08/14 submodules: allow parallel fetching, add tests and documentationStefan Beller, Sep 23, 2015
  33. 09/14 submodule-config: Untangle logic in parse_configStefan Beller, Sep 23, 2015
  34. 10/14 submodule config: keep update strategy aroundStefan Beller, Sep 23, 2015
  35. 11/14 git submodule update: cmd_update_recursiveStefan Beller, Sep 23, 2015
  36. 12/14 git submodule update: cmd_update_cloneStefan Beller, Sep 23, 2015
  37. Junio C HamanoSep 23, 2015
  38. 13/14 git submodule update: cmd_update_fetchStefan Beller, Sep 23, 2015
  39. 14/14 Rewrite submodule update in CStefan Beller, Sep 23, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.