git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH v4 30/39] builtin/verify-pack: implement an --object-format option
Date: Sun, 26 Jul 2020 17:29:46 -0400	[thread overview]
Message-ID: <CAPig+cRUj2RGz1=+3EDr5G_Mq2sXPnztCr-iPDGH+WycQ2iwjA@mail.gmail.com> (raw)
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:
> 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@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'.

  reply	other threads:[~2020-07-26 21:30 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 19:53 [PATCH v4 00/39] SHA-256, part 3/3 brian m. carlson
2020-07-26 19:53 ` [PATCH v4 01/39] t: make test-bloom initialize repository brian m. carlson
2020-07-26 19:53 ` [PATCH v4 02/39] t1001: use $ZERO_OID brian m. carlson
2020-07-26 19:53 ` [PATCH v4 03/39] t3305: make hash agnostic brian m. carlson
2020-07-26 19:53 ` [PATCH v4 04/39] t3404: prepare 'short SHA-1 collision' tests for SHA-256 brian m. carlson
2020-07-26 22:31   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 05/39] t6100: make hash size independent brian m. carlson
2020-07-26 19:53 ` [PATCH v4 06/39] t6101: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 07/39] t6301: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 08/39] t6500: specify test values for SHA-256 brian m. carlson
2020-07-26 19:53 ` [PATCH v4 09/39] t6501: avoid hard-coded objects brian m. carlson
2020-07-26 19:53 ` [PATCH v4 10/39] t7003: compute appropriate length constant brian m. carlson
2020-07-26 19:53 ` [PATCH v4 11/39] t7063: make hash size independent brian m. carlson
2020-07-26 22:40   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 12/39] t7201: abstract away SHA-1-specific constants brian m. carlson
2020-07-26 22:54   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 13/39] t7102: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 14/39] t7400: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 15/39] t7405: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 16/39] t7506: avoid checking for SHA-1-specific constants brian m. carlson
2020-07-26 19:54 ` [PATCH v4 17/39] t7508: use $ZERO_OID instead of hard-coded constant brian m. carlson
2020-07-26 19:54 ` [PATCH v4 18/39] t8002: make hash size independent brian m. carlson
2020-07-26 22:49   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 19/39] t8003: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 20/39] t8011: " brian m. carlson
2020-07-26 23:00   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 21/39] t9300: abstract away SHA-1-specific constants brian m. carlson
2020-07-26 23:14   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 22/39] t9300: use $ZERO_OID instead of hard-coded object ID brian m. carlson
2020-07-26 19:54 ` [PATCH v4 23/39] t9301: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 24/39] t9350: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 25/39] t9500: ensure that algorithm info is preserved in config brian m. carlson
2020-07-26 19:54 ` [PATCH v4 26/39] t9700: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 27/39] t5308: make test work with SHA-256 brian m. carlson
2020-07-26 19:54 ` [PATCH v4 28/39] t0410: mark test with SHA1 prerequisite brian m. carlson
2020-07-26 19:54 ` [PATCH v4 29/39] http-fetch: set up git directory before parsing pack hashes brian m. carlson
2020-07-26 23:28   ` Eric Sunshine
2020-07-26 23:30     ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 30/39] builtin/verify-pack: implement an --object-format option brian m. carlson
2020-07-26 21:29   ` Eric Sunshine [this message]
2020-07-26 22:56     ` brian m. carlson
2020-07-26 19:54 ` [PATCH v4 31/39] bundle: add new version for use with SHA-256 brian m. carlson
2020-07-26 22:18   ` Eric Sunshine
2020-07-26 22:59     ` brian m. carlson
2020-07-26 19:54 ` [PATCH v4 32/39] setup: add support for reading extensions.objectformat brian m. carlson
2020-07-26 23:34   ` Eric Sunshine
2020-07-26 23:41     ` brian m. carlson
2020-07-26 19:54 ` [PATCH v4 33/39] Enable SHA-256 support by default brian m. carlson
2020-07-26 23:41   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 34/39] t: add test_oid option to select hash algorithm brian m. carlson
2020-07-26 19:54 ` [PATCH v4 35/39] t: allow testing different hash algorithms via environment brian m. carlson
2020-07-26 19:54 ` [PATCH v4 36/39] t: make SHA1 prerequisite depend on default hash brian m. carlson
2020-07-26 23:52   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 37/39] ci: run tests with SHA-256 brian m. carlson
2020-07-26 23:54   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 38/39] docs: add documentation for extensions.objectFormat brian m. carlson
2020-07-26 23:56   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 39/39] t: remove test_oid_init in tests brian m. carlson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cRUj2RGz1=+3EDr5G_Mq2sXPnztCr-iPDGH+WycQ2iwjA@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH v4 30/39] builtin/verify-pack: implement an --object-format option' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).