On Tue, Aug 13, 2024 at 06:14:33AM -0400, karthik nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > 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 <ps@xxxxxx>
> > ---
> > 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
