Subject: Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 22 Nov 2021 22:36:31 +0100

On Mon, Nov 22 2021, Jeff King wrote:

> On Mon, Nov 22, 2021 at 09:53:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> But as to skipping the "argc > 1" test I've got this still:
>>     
>>     @@ -89,9 +89,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>>              * multiplexed out to our fd#1.  If the child dies, we tell the other
>>              * end over channel #3.
>>              */
>>     -       argv[0] = "upload-archive--writer";
>>             writer.out = writer.err = -1;
>>             writer.git_cmd = 1;
>>     +       strvec_push(&writer.args, "upload-archive--writer");
>>     +       if (argc > 1)
>>     +               strvec_pushv(&writer.args, argv + 1);
>>             if (start_command(&writer)) {
>>                     int err = errno;
>>                     packet_write_fmt(1, "NACK unable to spawn subprocess\n");
>> 
>> We'll segfault if we give NULL to strvec_pushv() so we still need that
>> check. Were you thinking of strvec_pushl(), or am I missing something?
>
> We wouldn't be giving NULL to strvec_pushv(). We'd be giving argv+1,
> which is guaranteed non-NULL, but may point to NULL. In that case the
> loop condition in strvec_pushv() would turn it into a noop.

D'oh. Thanks. Yes I was stupidly conflating a case where I'd tested it
with strvec_pushl() and moved that dereferenced version to
strvec_pushv(), sorry.