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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2021, 20:53 UTC
Message-ID: <211122.86fsrnyko4.gmgdl@evledraar.gmail.com>
In-Reply-To: <patch-2.5-a411098699d-20211122T153605Z-avarab@gmail.com>

On Mon, Nov 22 2021, Ævar Arnfjörð Bjarmason wrote:

Show 39 quoted lines

> This pattern added [1] in seems to have been intentional, but since
> [2] and [3] we've wanted do initialization of what's now the "struct
> strvec" "args" and "env_array" members. Let's not trample on that
> initialization here.
>
> 1. 1bc01efed17 (upload-archive: use start_command instead of fork,
>    2011-11-19)
> 2. c460c0ecdca (run-command: store an optional argv_array, 2014-05-15)
> 3. 9a583dc39e (run-command: add env_array, an optional argv_array for
>    env, 2014-10-19)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/upload-archive.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 24654b4c9bf..b4b9b3a6262 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -77,7 +77,7 @@ static ssize_t process_input(int child_fd, int band)
>  
>  int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>  {
> -	struct child_process writer = { argv };
> +	struct child_process writer = CHILD_PROCESS_INIT;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage(upload_archive_usage);
> @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>  	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");

Changing it to argv + 1 makes sense, I don't know why I did it like that. It works, but is unusual.

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?

(Even with strvec_pushl() we can't interpolate it as-is into the list, since it'll point to uninitialized memory in this case).