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

From: Jeff King <peff@peff.net>
Date: Oct 29, 2014, 17:21 UTC
Message-ID: <20141029172109.GA32234@peff.net>
In-Reply-To: <54500212.7040603@web.de>

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

Show 11 quoted lines

> --- 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.

Show 13 quoted lines

> 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.

Show 13 quoted lines

> 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