| Message ID | 20200726195424.626969-31-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/39] t: make test-bloom initialize repository | expand |
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'.
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.
diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c index c2a1a5c504..033e05d1a7 100644 --- a/builtin/verify-pack.c +++ b/builtin/verify-pack.c @@ -7,21 +7,28 @@ #define VERIFY_PACK_VERBOSE 01 #define VERIFY_PACK_STAT_ONLY 02 -static int verify_one_pack(const char *path, unsigned int flags) +static int verify_one_pack(const char *path, unsigned int flags, const char *hash_algo) { struct child_process index_pack = CHILD_PROCESS_INIT; - const char *argv[] = {"index-pack", NULL, NULL, NULL }; - struct strbuf arg = STRBUF_INIT; + struct argv_array argv = ARGV_ARRAY_INIT; + struct strbuf arg = STRBUF_INIT, hash_arg = STRBUF_INIT; int verbose = flags & VERIFY_PACK_VERBOSE; int stat_only = flags & VERIFY_PACK_STAT_ONLY; int err; + argv_array_push(&argv, "index-pack"); + if (stat_only) - argv[1] = "--verify-stat-only"; + argv_array_push(&argv, "--verify-stat-only"); else if (verbose) - argv[1] = "--verify-stat"; + argv_array_push(&argv, "--verify-stat"); else - argv[1] = "--verify"; + argv_array_push(&argv, "--verify"); + + if (hash_algo) { + strbuf_addf(&hash_arg, "--object-format=%s", hash_algo); + argv_array_push(&argv, hash_arg.buf); + } /* * In addition to "foo.pack" we accept "foo.idx" and "foo"; @@ -31,9 +38,9 @@ static int verify_one_pack(const char *path, unsigned int flags) if (strbuf_strip_suffix(&arg, ".idx") || !ends_with(arg.buf, ".pack")) strbuf_addstr(&arg, ".pack"); - argv[2] = arg.buf; + argv_array_push(&argv, arg.buf); - index_pack.argv = argv; + index_pack.argv = argv.argv; index_pack.git_cmd = 1; 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); return err; } @@ -60,12 +68,15 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix) { int err = 0; unsigned int flags = 0; + const char *object_format = NULL; int i; const struct option verify_pack_options[] = { OPT_BIT('v', "verbose", &flags, N_("verbose"), VERIFY_PACK_VERBOSE), OPT_BIT('s', "stat-only", &flags, N_("show statistics only"), VERIFY_PACK_STAT_ONLY), + OPT_STRING(0, "object-format", &object_format, N_("hash"), + N_("specify the hash algorithm to use")), OPT_END() }; @@ -75,7 +86,7 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix) if (argc < 1) usage_with_options(verify_pack_usage, verify_pack_options); for (i = 0; i < argc; i++) { - if (verify_one_pack(argv[i], flags)) + if (verify_one_pack(argv[i], flags, object_format)) err = 1; } diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 1b54c35b01..7fc22171e7 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -829,7 +829,7 @@ test_expect_success 'part of packfile response provided as URI' ' # Ensure that my-blob and other-blob are in separate packfiles. for idx in http_child/.git/objects/pack/*.idx do - git verify-pack --verbose $idx >out && + git verify-pack --object-format=$(test_oid algo) --verbose $idx >out && { grep "^[0-9a-f]\{16,\} " out || : } >out.objectlist &&
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. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/verify-pack.c | 29 ++++++++++++++++++++--------- t/t5702-protocol-v2.sh | 2 +- 2 files changed, 21 insertions(+), 10 deletions(-)