Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT

[Johannes Sixt <j6t@kdbg.org>]

Am 17.08.2014 00:55, schrieb René Scharfe:
> Most struct child_process variables are cleared using memset right after
> declaration.  Provide a macro, CHILD_PROCESS_INIT, that can be used to
> initialize them statically instead.  That's shorter, doesn't require a
> function call and is slightly more readable (especially given that we
> already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.).

This is a step in the right direction, IMO. This way to initialize the
struct feels mucth better because it does not depend on that the bit
pattern of the NULL pointer is all zeros.

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  Documentation/technical/api-run-command.txt |  4 ++--
>  archive-tar.c                               |  3 +--
>  builtin/add.c                               |  3 +--
>  builtin/commit.c                            |  3 +--
>  builtin/help.c                              |  3 +--
>  builtin/merge.c                             |  3 +--
>  builtin/notes.c                             |  3 +--
>  builtin/remote-ext.c                        |  3 +--
>  builtin/repack.c                            |  3 +--
>  builtin/replace.c                           |  4 ++--
>  builtin/verify-pack.c                       |  3 +--
>  bundle.c                                    |  6 ++----
>  connected.c                                 |  3 +--
>  convert.c                                   |  3 +--
>  credential-cache.c                          |  3 +--
>  credential.c                                |  3 +--
>  daemon.c                                    |  8 +++-----
>  diff.c                                      |  3 +--
>  editor.c                                    |  3 +--
>  fetch-pack.c                                |  3 +--
>  gpg-interface.c                             |  6 ++----
>  http-backend.c                              |  3 +--
>  http.c                                      |  3 +--
>  imap-send.c                                 |  2 +-
>  prompt.c                                    |  3 +--
>  remote-curl.c                               |  3 +--
>  remote-testsvn.c                            |  3 +--
>  run-command.h                               |  2 ++
>  send-pack.c                                 |  3 +--
>  submodule.c                                 | 21 +++++++--------------
>  test-run-command.c                          |  4 +---
>  test-subprocess.c                           |  3 +--
>  transport.c                                 | 12 ++++--------
>  upload-pack.c                               |  3 +--
>  wt-status.c                                 |  3 +--
>  35 files changed, 51 insertions(+), 93 deletions(-)

You missed a few instances in builtin/receive-pack.c. Is this deliberate?

Another one is in transport-helper.c::fetch_with_import() that is
currently initialized in get_importer(), and another one in
push_refs_with_export() that is initialized in get_exporter().

Should the static struct child_process variables in pager.c and
connect.c use the macro? It would just be for completeness, but does not
change the correctness. I dunno.

> diff --git a/run-command.h b/run-command.h
> index ea73de3..95a8fb8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -44,6 +44,8 @@ struct child_process {
>  	unsigned clean_on_exit:1;
>  };
>  
> +#define CHILD_PROCESS_INIT { NULL }

I would have expected this to read

#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }

It does change the bit pattern of the initialized struct child_process
because ARGV_ARRAY_INIT uses a non-NULL address. But IMHO
ARGV_ARRAY_INIT should be used here as a defensive measure.

-- Hannes