Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 22, 2021, 21:36 UTC
- Message-ID
- <211122.867dczyirm.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <YZwHNkf2l9uPrIJv@coredump.intra.peff.net>
On Mon, Nov 22 2021, Jeff King wrote:
Show 24 quoted lines
> 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.