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

[karthik nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

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.

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

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]