Re: [PATCH v4 30/39] builtin/verify-pack: implement an --object-format option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 26, 2020, 21:29 UTC
Message-ID: <CAPig+cRUj2RGz1=+3EDr5G_Mq2sXPnztCr-iPDGH+WycQ2iwjA@mail.gmail.com>
In-Reply-To: <20200726195424.626969-31-sandals@crustytoothpaste.net>

On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 12 quoted lines

> A recently added test in t5702 started using git verify-pack outside of
> a repository.  While this poses no problems with SHA-1, with SHA-256 we
> implicitly rely on the setup of the repository to initialize our hash
> algorithm settings.
>
> Since we're not in a repository here, we need to provide git verify-pack
> help to set things up properly.  git index-pack already knows an
> --object-format option, so let's accept one as well and pass it down to
> our git index-pack invocation.  Since we're now dynamically adjusting
> the elements in argv, let's switch to using struct argv_array to manage
> them.  Finally, let's make t5702 pass the proper argument on down to its
> git verify-pack caller.

A few comments below... use your own judgment as to whether they are worth a re-roll.

Show 13 quoted lines

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
> @@ -7,21 +7,28 @@
> +static int verify_one_pack(const char *path, unsigned int flags, const char *hash_algo)
>  {
> +       struct argv_array argv = ARGV_ARRAY_INIT;
> +       struct strbuf arg = STRBUF_INIT, hash_arg = STRBUF_INIT;
>
> +       if (hash_algo) {
> +               strbuf_addf(&hash_arg, "--object-format=%s", hash_algo);
> +               argv_array_push(&argv, hash_arg.buf);
> +       }

Rather than bothering with a separate strbuf, this would be easier:

    argv_array_pushf(&argv, "--object-format=%s", hash_algo);

Doing so would also fix a secondary problem that 'hash_arg' is being leaked.

> @@ -31,9 +38,9 @@ static int verify_one_pack(const char *path, unsigned int flags)
> -       index_pack.argv = argv;
> +       index_pack.argv = argv.argv;

'struct child_process' already has an 'args' member which is a 'struct argv_array' of which you can take advantage instead of creating your own local 'argv' in this function. run_command() automatically clears the built-in 'argv_array', which frees you the effort of having to do so manually.

>         err = run_command(&index_pack);
> @@ -47,6 +54,7 @@ static int verify_one_pack(const char *path, unsigned int flags)
>         strbuf_release(&arg);
> +       argv_array_clear(&argv);

This is where 'hash_arg' is being linked. This is also where you could eliminate the call to argv_array_clear() if you take advantage of the 'argv_array' already provided by 'child_process'.