Re: [PATCH] use child_process_init() to initialize struct child_process variables

[Jeff King <peff@peff.net>]

On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:

> --- a/bundle.c
> +++ b/bundle.c
> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	write_or_die(bundle_fd, "\n", 1);
>  
>  	/* write pack */
> -	memset(&rls, 0, sizeof(rls));
> +	child_process_init(&rls);
>  	argv_array_pushl(&rls.args,
>  			 "pack-objects", "--all-progress-implied",
>  			 "--stdout", "--thin", "--delta-base-offset",

I wondered if this one could use CHILD_PROCESS_INIT in the declaration
instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use
the same variable twice for two different child processes in the same
function. Besides variable reuse being slightly confusing, the name
"rls" (which presumably stands for "rev-list" for the first child) means
nothing here, where we are calling "pack-objects". Maybe it would be
cleaner to introduce a second variable?

I also suspect the function would be a lot more readable broken into two
sub-functions (reading from rev-list and writing to pack-objects), but I
did not look closely enough to see whether there were any complicating
factors.

> diff --git a/column.c b/column.c
> index 8082a94..786abe6 100644
> --- a/column.c
> +++ b/column.c
> @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
>  	if (fd_out != -1)
>  		return -1;
>  
> -	memset(&column_process, 0, sizeof(column_process));
> +	child_process_init(&column_process);
>  	argv = &column_process.args;
>  
>  	argv_array_push(argv, "column");

This one uses a static child_process which needs to be reinitialized on
each run of the function. So it definitely needs child_process_init.

> diff --git a/trailer.c b/trailer.c
> index 8514566..7ff036c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)
>  		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
>  
>  	argv[0] = cmd.buf;
> -	memset(&cp, 0, sizeof(cp));
> +	child_process_init(&cp);
>  	cp.argv = argv;
>  	cp.env = local_repo_env;
>  	cp.no_stdin = 1;

I think this one can use CHILD_PROCESS_INIT in the declaration. I guess
it is debatable whether that is actually preferable, but I tend to think
it is cleaner and less error-prone.

-Peff