Re: [PATCH v7 02/12] run-command: add first helper for pp child states

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> There is a recurring pattern of testing parallel process child states
> and file descriptors to determine if a child is running, receiving any
> input or if it's ready for cleanup.
>
> Name the pp_child structure and introduce a first helper to make these
> checks more readable. Next commits will add more helpers and checks.

I needed to read this three times to understand what you meant by "a
first helper".  In the bigger picture, the helper being the first
one you invented does not really matter.  Perhaps drop "first"?

> +struct parallel_child {
> +	enum child_state state;
> +	struct child_process process;
> +	struct strbuf err;
> +	void *data;
> +};
> +
> +static int child_is_working(const struct parallel_child *pp_child)
> +{
> +	return pp_child->state == GIT_CP_WORKING;
> +}
> +
>  struct parallel_processes {
>  	size_t nr_processes;
>  
> -	struct {
> -		enum child_state state;
> -		struct child_process process;
> -		struct strbuf err;
> -		void *data;
> -	} *children;
> +	struct parallel_child *children;
>  	/*
>  	 * The struct pollfd is logically part of *children,
>  	 * but the system call expects it as its own array.
> @@ -1509,7 +1516,7 @@ static void kill_children(const struct parallel_processes *pp,
>  			  int signo)
>  {
>  	for (size_t i = 0; i < opts->processes; i++)
> -		if (pp->children[i].state == GIT_CP_WORKING)
> +		if (child_is_working(&pp->children[i]))
>  			kill(pp->children[i].process.pid, signo);
>  }
>  
> @@ -1665,7 +1672,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
>  
>  	/* Buffer output from all pipes. */
>  	for (size_t i = 0; i < opts->processes; i++) {
> -		if (pp->children[i].state == GIT_CP_WORKING &&
> +		if (child_is_working(&pp->children[i]) &&
>  		    pp->pfd[i].revents & (POLLIN | POLLHUP)) {
>  			int n = strbuf_read_once(&pp->children[i].err,
>  						 pp->children[i].process.err, 0);
> @@ -1683,7 +1690,7 @@ static void pp_output(const struct parallel_processes *pp)
>  {
>  	size_t i = pp->output_owner;
>  
> -	if (pp->children[i].state == GIT_CP_WORKING &&
> +	if (child_is_working(&pp->children[i]) &&
>  	    pp->children[i].err.len) {
>  		strbuf_write(&pp->children[i].err, stderr);
>  		strbuf_reset(&pp->children[i].err);
> @@ -1748,7 +1755,7 @@ static int pp_collect_finished(struct parallel_processes *pp,
>  			 * running process time.
>  			 */
>  			for (i = 0; i < n; i++)
> -				if (pp->children[(pp->output_owner + i) % n].state == GIT_CP_WORKING)
> +				if (child_is_working(&pp->children[(pp->output_owner + i) % n]))
>  					break;
>  			pp->output_owner = (pp->output_owner + i) % n;
>  		}