Re: [PATCH 08/10] reftable/dump: drop unused printing functionality

[Patrick Steinhardt <ps@pks.im>]

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