On Tue, Aug 13, 2024 at 06:14:33AM -0400, karthik nayak wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

We have a bunch of infrastructure wired up that allows us to print
reftable records, tables and stacks. While this functionality is wired
up via various "test-tool reftable" options, it is never used. It also
feels kind of dubious whether any other eventual user of the reftable
library should use it as it very much feels like a debugging aid rather
than something sensible. The format itself is somewhat inscrutable and
the infrastructure is non-extensible.

Drop this code. The only remaining function in this context is
`reftable_reader_print_blocks()`, which we do use in our tests.

Signed-off-by: Patrick Steinhardt <redacted>
---
 reftable/dump.c             |  16 +----
 reftable/generic.c          |  47 -------------
 reftable/reader.c           |  21 ------
 reftable/record.c           | 127 ------------------------------------
 reftable/record.h           |   4 --
 reftable/reftable-generic.h |   3 -
 reftable/reftable-reader.h  |   2 -
 reftable/reftable-record.h  |   8 ---
 reftable/reftable-stack.h   |   3 -
 reftable/stack.c            |  20 ------
 reftable/stack_test.c       |   7 --
 11 files changed, 1 insertion(+), 257 deletions(-)

diff --git a/reftable/dump.c b/reftable/dump.c
index 2953e0a83a..35a1731da9 100644
--- a/reftable/dump.c
+++ b/reftable/dump.c

@@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv)
 {
 	int err = 0;
 	int opt_dump_blocks = 0;
-	int opt_dump_table = 0;
-	int opt_dump_stack = 0;
-	uint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;
 	const char *arg = NULL, *argv0 = argv[0];

 	for (; argc > 1; argv++, argc--)

@@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)
 			break;
 		else if (!strcmp("-b", argv[1]))
 			opt_dump_blocks = 1;
-		else if (!strcmp("-t", argv[1]))
-			opt_dump_table = 1;
-		else if (!strcmp("-6", argv[1]))
-			opt_hash_id = GIT_SHA256_FORMAT_ID;
-		else if (!strcmp("-s", argv[1]))
-			opt_dump_stack = 1;
 		else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {
 			print_help();
 			return 2;

@@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)

I'm a bit skeptical about this change because I definitely have used the
`-t` and `-s` options a bunch of times to understand what a table holds.
Since the reftable format is binary, this is the only tooling we have
which allows us to read this format from a plumbing point of view. I'd
keep them. I guess the stack printing just iterates over the tables and
prints them and could be removed, but I'd keep the option to dump a
table.

Well, I have to say that I find the output to be rather useless. But
you're making a valid point: we don't have anything else to peek at the
table's contents.

I'll keep this in v2, but make it an internal implementation detail of
the test-tool. It certainly has no place in the reftable library in my
opinion.

Also this patch misses cleaning up `print_help` if we go down this
route.

Indeed, will fix.

Patrick