Re: [PATCH] gc: fix off-by-one in append_option

From: Jeff King <peff@peff.net>
Date: Apr 18, 2012, 20:21 UTC
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:

Show 11 quoted lines

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