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@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.
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
Attachment:
signature.asc
Description: PGP signature
