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