On 2020-07-26 at 21:29:46, Eric Sunshine wrote:
> On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson
> A few comments below... use your own judgment as to whether they are
> worth a re-roll.

I'll do a re-roll, since I think there's enough issues that one would be
worthwhile.

> > 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.

This is a great idea.

> > @@ -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.

Great, I'll switch to that.
-- 
brian m. carlson: Houston, Texas, US