On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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'.
