Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT

[Jeff King <peff@peff.net>]

On Sun, Aug 17, 2014 at 09:46:42AM +0200, Johannes Sixt wrote:

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

I think platforms with NULL as something besides all-bits-zero are a
lost cause with git. There are so many struct memsets that depend on
this (and it's probably not actually worth caring about).

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

I'd be OK with that.  The argv_array code is specifically OK with an
all-bits-zero initialization. The only thing you don't get is that an
empty array is non-NULL, but that should never matter here (true, we'd
segfault if you didn't add anything to the array, but that is clearly a
bug that needs to be fixed either way).

I'm a little worried, though, that use sites without initializers would
be left behind. For example, git_proxy_connect uses xcalloc to allocate
the child_process, which results in all-bits-zero. If we want to start
caring about the initialization, we probably need to provide a
child_process_init() function and use it consistently.

-Peff