[v3,2/8] t/helper: add 'find-pack' test-tool

Commit Message

Christian Couder July 24, 2023, 8:59 a.m. UTC

In a following commit, we will make it possible to separate objects in
different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a
new test-tool that can display which packfile(s) a given object is in.

Signed-off-by: Christian Couder <[e-mail]>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 4 files changed, 38 insertions(+)
 create mode 100644 t/helper/test-find-pack.c

Comments

Taylor Blau July 25, 2023, 10:44 p.m. UTC | #1

On Mon, Jul 24, 2023 at 10:59:03AM +0200, Christian Couder wrote:
> ---
>  Makefile                  |  1 +
>  t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
>  t/helper/test-tool.c      |  1 +
>  t/helper/test-tool.h      |  1 +
>  4 files changed, 38 insertions(+)
>  create mode 100645 t/helper/test-find-pack.c

Everything that you wrote here seems reasonable to me, and the
implementation of the new test tool is very straightforward.

I'm pretty sure that everything here is correct, and we'll implicitly
test the behavior of the new helper in following patches.

That said, I think that it might be prudent here to "test the tests" and
write a simple test script that exercises this test helper over a more
trivial case. There is definitely prior art for testing our helpers
directly in the t00?? tests.

Among the test helpers that I can think of off the top of my head, I
think a good handful of them have tests:

  - t0011-hashmap.sh
  - t0015-hash.sh
  - t0016-oidmap.sh
  - t0019-json-writer.sh
  - t0052-simple-ipc.sh
  - t0060-path-utils.sh
  - t0061-run-command.sh
  - t0063-string-list.sh
  - t0064-oid-array.sh
  - t0066-dir-iterator.sh
  - t0095-bloom.sh

I would definitely recommend adding a test here, too. Like I said
earlier, I think that you are implicitly testing the new behavior here,
but it's going to happen in much more complicated environments than
something you could construct synthetically here.

Thanks,
Taylor

Christian Couder Aug. 8, 2023, 8:28 a.m. UTC | #2

On Wed, Jul 26, 2023 at 12:44 AM Taylor Blau <[e-mail]> wrote:
>
> On Mon, Jul 24, 2023 at 10:59:03AM +0200, Christian Couder wrote:
> > ---
> >  Makefile                  |  1 +
> >  t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
> >  t/helper/test-tool.c      |  1 +
> >  t/helper/test-tool.h      |  1 +
> >  4 files changed, 38 insertions(+)
> >  create mode 100645 t/helper/test-find-pack.c
>
> Everything that you wrote here seems reasonable to me, and the
> implementation of the new test tool is very straightforward.
>
> I'm pretty sure that everything here is correct, and we'll implicitly
> test the behavior of the new helper in following patches.
>
> That said, I think that it might be prudent here to "test the tests" and
> write a simple test script that exercises this test helper over a more
> trivial case. There is definitely prior art for testing our helpers
> directly in the t00?? tests.

Ok, I have written a new t0080-find-pack.sh test script for this in
the version 4 I just sent.

I have also changed `test-tool find-pack` so that it now accepts a
`--check-count <n>` option. This addresses some of your comments on
another patch in the previous version of this series. As the code is
now a bit more complex, there is more justification for a test script.

Thanks.

Patch

diff --git a/Makefile b/Makefile
index fb541dedc9..14ee0c45d4 100644
--- a/Makefile
+++ b/Makefile
@@ -800,6 +800,7 @@  TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-env-helper.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-fast-rebase.o
+TEST_BUILTINS_OBJS += test-find-pack.o
 TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-genzeros.o
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
new file mode 100644
index 0000000000..1928fe7329
--- /dev/null
+++ b/t/helper/test-find-pack.c
@@ -0,0 +1,35 @@ 
+#include "test-tool.h"
+#include "object-name.h"
+#include "object-store.h"
+#include "packfile.h"
+#include "setup.h"
+
+/*
+ * Display the path(s), one per line, of the packfile(s) containing
+ * the given object.
+ */
+
+static const char *find_pack_usage = "\n"
+"  test-tool find-pack <object>";
+
+
+int cmd__find_pack(int argc, const char **argv)
+{
+	struct object_id oid;
+	struct packed_git *p;
+
+	setup_git_directory();
+
+	if (argc != 2)
+		usage(find_pack_usage);
+
+	if (repo_get_oid(the_repository, argv[1], &oid))
+		die("cannot parse %s as an object name", argv[1]);
+
+	for (p = get_all_packs(the_repository); p; p = p->next) {
+		if (find_pack_entry_one(oid.hash, p))
+			printf("%s\n", p->pack_name);
+	}
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index abe8a785eb..41da40c296 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -31,6 +31,7 @@  static struct test_cmd cmds[] = {
 	{ "env-helper", cmd__env_helper },
 	{ "example-decorate", cmd__example_decorate },
 	{ "fast-rebase", cmd__fast_rebase },
+	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
 	{ "genrandom", cmd__genrandom },
 	{ "genzeros", cmd__genzeros },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index ea2672436c..411dbf2db4 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -25,6 +25,7 @@  int cmd__dump_reftable(int argc, const char **argv);
 int cmd__env_helper(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
 int cmd__fast_rebase(int argc, const char **argv);
+int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__genzeros(int argc, const char **argv);