From: Jeff King <peff@peff.net>
Date: Wed, 18 Apr 2012 20:21:16 GMT
Subject: Re: [PATCH] gc: fix off-by-one in append_option
Message-ID: <20120418202116.GA12964@sigill.intra.peff.net>
In-Reply-To: <7vd374ltqh.fsf@alter.siamese.dyndns.org>

On Wed, Apr 18, 2012 at 12:34:14PM -0700, Junio C Hamano wrote:

> > I've included a patch below that makes this look like:
> >
> >   static const char repack_cmd[] = {"repack", "-d", "-l", NULL };
> >   static struct argv_array repack = ARGV_ARRAY_INIT_DEFAULT(repack_cmd);
>
> I do not know it is worth it to try to be too fancy.
> 
> I was about to suggest, immediately after seeing the first one I quoted
> above, to omit NULL and instead use ARRAY_SIZE(), but I do not think that
> is even worth it, as some (possibly future) caller may have only "char **"
> as a usual NULL terminated array at hand.

Actually, that is broken already, because the initializer uses
ARRAY_SIZE to set argc properly. Omitting NULL wouldn't work anyway,
though, because then the state before any push violates the invariant
(that the value is NUL-terminated).

I think it really is impossible to make it nice, because we can't count
on running _any_ code before somebody peeks at array.argv (we don't even
have an accessor, but just let people look at that directly).

> I am perfectly OK with even without initializers, like this:
> 
> 	struct argv_array repack = ARGV_ARRAY_INIT;
> 	argv_array_push_strings(&repack, "repack", "-d", "-l", NULL);

I think that is sane, and certainly the simplest. I'll send a patch in a
moment.

-Peff