Re: [PATCH 16/16] reftable/table: move printing logic into test helper

> The logic to print individual blocks in a table is hosted in the
> reftable library. This is only the case due to historical reasons though
> because users of the library had no interfaces to read blocks one by
> one. Otherwise, printing individual blocks has no place in the reftable
> library given that the format will not be generic in the first place.
> 
> We have now grown a public interface to iterate through blocks contained
> in a table, and thus we can finally move the logic to print them into
> the test helper.
> 
> Move over the logic and refactor it accordingly. Note that the iterator
> also trivially allows us to access index sections, which we previously
> didn't print at all. This omission wasn't intentional though, so start
> dumping those sections as well so that we can assert that indices are
> written as expected.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/reftable-table.h         |  3 --
>  reftable/table.c                  | 65 ------------------------------------
>  t/helper/test-reftable.c          | 69 ++++++++++++++++++++++++++++++++++++++-
>  t/t0613-reftable-write-options.sh |  9 +++++
>  4 files changed, 77 insertions(+), 69 deletions(-)
> 
> diff --git a/reftable/reftable-table.h b/reftable/reftable-table.h
> index f0f1784c664..293fffbddc6 100644
> --- a/reftable/reftable-table.h
> +++ b/reftable/reftable-table.h
> @@ -97,9 +97,6 @@ uint64_t reftable_table_max_update_index(struct reftable_table *t);
>  /* return the min_update_index for a table */
>  uint64_t reftable_table_min_update_index(struct reftable_table *t);
>  
> -/* print blocks onto stdout for debugging. */
> -int reftable_table_print_blocks(const char *tablename);
> -
>  /*
>   * An iterator that iterates through the blocks contained in a given table.
>   */
> diff --git a/reftable/table.c b/reftable/table.c
> index 48f0cdfd42b..8a7581b9800 100644
> --- a/reftable/table.c
> +++ b/reftable/table.c
> @@ -740,71 +740,6 @@ uint64_t reftable_table_min_update_index(struct reftable_table *t)
>  	return t->min_update_index;
>  }
>  
> -int reftable_table_print_blocks(const char *tablename)
> -{
> -	struct {
> -		const char *name;
> -		int type;
> -	} sections[] = {
> -		{
> -			.name = "ref",
> -			.type = REFTABLE_BLOCK_TYPE_REF,
> -		},
> -		{
> -			.name = "obj",
> -			.type = REFTABLE_BLOCK_TYPE_OBJ,
> -		},
> -		{
> -			.name = "log",
> -			.type = REFTABLE_BLOCK_TYPE_LOG,
> -		},
> -	};
> -	struct reftable_block_source src = { 0 };
> -	struct reftable_table *table = NULL;
> -	struct table_iter ti = { 0 };
> -	size_t i;
> -	int err;
> -
> -	err = reftable_block_source_from_file(&src, tablename);
> -	if (err < 0)
> -		goto done;
> -
> -	err = reftable_table_new(&table, &src, tablename);
> -	if (err < 0)
> -		goto done;
> -
> -	table_iter_init(&ti, table);
> -
> -	printf("header:\n");
> -	printf("  block_size: %d\n", table->block_size);
> -
> -	for (i = 0; i < sizeof(sections) / sizeof(*sections); i++) {
> -		err = table_iter_seek_start(&ti, sections[i].type, 0);
> -		if (err < 0)
> -			goto done;
> -		if (err > 0)
> -			continue;
> -
> -		printf("%s:\n", sections[i].name);
> -
> -		while (1) {
> -			printf("  - length: %u\n", ti.block.restart_off);
> -			printf("    restarts: %u\n", ti.block.restart_count);
> -
> -			err = table_iter_next_block(&ti);
> -			if (err < 0)
> -				goto done;
> -			if (err > 0)
> -				break;
> -		}
> -	}
> -
> -done:
> -	reftable_table_decref(table);
> -	table_iter_close(&ti);
> -	return err;
> -}

>  int reftable_table_init_table_iterator(struct reftable_table *t,
>  				       struct reftable_table_iterator *it)
>  {
> diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> index f8f1956f4f3..c465137826c 100644
> --- a/t/helper/test-reftable.c
> +++ b/t/helper/test-reftable.c
> @@ -2,6 +2,7 @@
>  #include "hash.h"
>  #include "hex.h"
>  #include "reftable/system.h"
> +#include "reftable/reftable-constants.h"
>  #include "reftable/reftable-error.h"
>  #include "reftable/reftable-merged.h"
>  #include "reftable/reftable-stack.h"
> @@ -20,6 +21,72 @@ static void print_help(void)
>  	       "\n");
>  }
>  
> +static int dump_blocks(const char *tablename)
> +{
> +	struct reftable_table_iterator ti = { 0 };
> +	struct reftable_block_source src = { 0 };
> +	struct reftable_table *table = NULL;
> +	uint8_t section_type = 0;
> +	int err;
> +
> +	err = reftable_block_source_from_file(&src, tablename);
> +	if (err < 0)
> +		goto done;
> +
> +	err = reftable_table_new(&table, &src, tablename);
> +	if (err < 0)
> +		goto done;
> +
> +	err = reftable_table_init_table_iterator(table, &ti);
> +	if (err < 0)
> +		goto done;
> +
> +	printf("header:\n");
> +	printf("  block_size: %d\n", table->block_size);
> +
> +	while (1) {
> +		const struct reftable_block *block;
> +
> +		err = reftable_table_iterator_next(&ti, &block);
> +		if (err < 0)
> +			goto done;
> +		if (err > 0)
> +			break;
> +
> +		if (block->block_type != section_type) {
> +			const char *section;
> +			switch (block->block_type) {
> +			case REFTABLE_BLOCK_TYPE_LOG:
> +				section = "log";
> +				break;
> +			case REFTABLE_BLOCK_TYPE_REF:
> +				section = "ref";
> +				break;
> +			case REFTABLE_BLOCK_TYPE_OBJ:
> +				section = "obj";
> +				break;
> +			case REFTABLE_BLOCK_TYPE_INDEX:
> +				section = "idx";
> +				break;
> +			default:
> +				err = -1;
> +				goto done;
> +			}
> +
> +			section_type = block->block_type;
> +			printf("%s:\n", section);
> +		}
> +
> +		printf("  - length: %u\n", block->restart_off);
> +		printf("    restarts: %u\n", block->restart_count);
> +	}
> +
> +done:
> +	reftable_table_iterator_release(&ti);
> +	reftable_table_decref(table);
> +	return err;
> +}
> +
>  static int dump_table(struct reftable_merged_table *mt)
>  {
>  	struct reftable_iterator it = { NULL };
> @@ -184,7 +251,7 @@ int cmd__dump_reftable(int argc, const char **argv)
>  	arg = argv[1];
>  
>  	if (opt_dump_blocks) {
> -		err = reftable_table_print_blocks(arg);
> +		err = dump_blocks(arg);

>  	} else if (opt_dump_table) {
>  		err = dump_reftable(arg);
>  	} else if (opt_dump_stack) {
> diff --git a/t/t0613-reftable-write-options.sh b/t/t0613-reftable-write-options.sh
> index e2708e11d5b..e4c7461ce9e 100755
> --- a/t/t0613-reftable-write-options.sh
> +++ b/t/t0613-reftable-write-options.sh
> @@ -93,6 +93,9 @@ test_expect_success 'many refs results in multiple blocks' '
>  		    restarts: 3
>  		  - length: 3289
>  		    restarts: 3
> +		idx:
> +		  - length: 103
> +		    restarts: 1
>  		EOF
>  		test-tool dump-reftable -b .git/reftable/*.ref >actual &&
>  		test_cmp expect actual
> @@ -241,6 +244,9 @@ test_expect_success 'object index gets written by default with ref index' '
>  		    restarts: 1
>  		  - length: 80
>  		    restarts: 1
> +		idx:
> +		  - length: 55
> +		    restarts: 2
>  		obj:
>  		  - length: 11
>  		    restarts: 1
> @@ -277,6 +283,9 @@ test_expect_success 'object index can be disabled' '
>  		    restarts: 1
>  		  - length: 80
>  		    restarts: 1
> +		idx:
> +		  - length: 55
> +		    restarts: 2
>  		EOF
>  		test-tool dump-reftable -b .git/reftable/*.ref >actual &&
>  		test_cmp expect actual
> 
> -- 
> 2.49.0.604.gff1f9ca942.dirty
> 
>