From: Giuseppe Bilotta Date: Wed, 24 Feb 2010 08:55:14 GMT Subject: Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly Message-ID: In-Reply-To: <4B84DDE9.8000301@viscovery.net> On Wed, Feb 24, 2010 at 9:06 AM, Johannes Sixt wrote: > Giuseppe Bilotta schrieb: >> +     const char *env[local_repo_env_size+2]; > > Variable sized arrays are prohibited. Ah, sorry. Is alloca() allowed? I don't see it being used anywhere else in the code, and malloc would be a little too much for this case. >>       struct strbuf buf = STRBUF_INIT; >> >> +     /* Copy local_repo_env to env, letting i >> +        rest at the last NULL */ >> +     while (env[i] = local_repo_env[i]) >> +             ++i; /* do nothing */ >> + > > This looks very inconsistent: At the one hand, you use l_r_e_size to > allocate the space, but then you iterate over it assuming that the list is > (also) NULL-terminated. But this is only a minor nit. Well, if you consider that using l-r-e-size is just a way to spare a double-walk, the inconsistency is tolerable. OTOH, in this particular case NULL-walking the list doesn't give the usual benefit of sparing a counter, so I could rework the patch to use a standard for loop. (I also notice that I forgot to remove the /* do nothing */ comment from the while(env[i] = local_repo_env[i++]) ; --i approach I was going with at first) -- Giuseppe "Oblomov" Bilotta