Skip to contentSkip to conversation

reftable: drop generic `reftable_table` interface

PATCH Superseded Patrick Steinhardt 3 participants 40 messages Aug 14, 2024 – Aug 22, 2024 Kbuild

lore
Share & tools
lore
#0 lore raw

v2 cover reftable: drop generic `reftable_table` interface

Hi,

this is the second version of my patch series that gets rid of the generic reftable_table interface. It made it way harder to understand the reftable code base and is not really required nowadays anymore where we have generic re-seekable reftable iterators.

Changes compared to v1:

- Fix some commit message typos.

- Remove some braces while at it.

- Drop the "-c" switch from the test-tool's help output.

- Restore printing-related functionality, but move all of it into the test-helper. It has no reason to exist in the reftable library.

Thanks!

Patrick

Patrick Steinhardt (15):
reftable/merged: expose functions to initialize iterators reftable/merged: rename reftable_new_merged_table() reftable/merged: stop using generic tables in the merged table reftable/stack: open-code reading refs
reftable/iter: drop double-checking logic reftable/generic: move generic iterator code into iterator interface reftable/dump: drop unused compact_stack()
t/helper: inline reftable_dump_main()
t/helper: inline reftable_reader_print_file()
t/helper: inline reftable_stack_print_directory() t/helper: inline reftable_table_print()
t/helper: inline printing of reftable records
t/helper: use hash_to_hex_algop() to print hashes t/helper: refactor to not use struct reftable_table reftable/generic: drop interface

 Makefile                         |   2 -
 reftable/dump.c                  | 111 ---------------
 reftable/generic.c               | 229 -------------------------------
 reftable/generic.h               |  37 -----
 reftable/iter.c                  | 126 ++++++++++++++---
 reftable/iter.h                  |  30 +++-
 reftable/merged.c                |  72 ++++------
 reftable/merged.h                |   4 +-
 reftable/reader.c                |  70 +---------
 reftable/reader.h                |   4 +
 reftable/record.c                | 127 -----------------
 reftable/record.h                |   1 -
 reftable/reftable-generic.h      |  47 -------
 reftable/reftable-merged.h       |  26 ++--
 reftable/reftable-reader.h       |   9 --
 reftable/reftable-record.h       |   8 --
 reftable/reftable-stack.h        |   3 -
 reftable/reftable-tests.h        |   1 -
 reftable/stack.c                 |  94 ++++++-------
 reftable/stack_test.c            |  29 ++--
 t/helper/test-reftable.c         | 189 ++++++++++++++++++++++++-
 t/unit-tests/t-reftable-merged.c |  17 +--
 22 files changed, 422 insertions(+), 814 deletions(-)
 delete mode 100644 reftable/dump.c
 delete mode 100644 reftable/generic.c
 delete mode 100644 reftable/generic.h
 delete mode 100644 reftable/reftable-generic.h

Range-diff against v1:

 1:  404d64effd =  1:  472c169b50 reftable/merged: expose functions to initialize iterators
 2:  511416fb73 =  2:  bc6f1cd8c1 reftable/merged: rename `reftable_new_merged_table()`
 3:  86e2f9f5dc =  3:  58e91ab4b3 reftable/merged: stop using generic tables in the merged table
 4:  d5d24e03bd =  4:  6ba3fcee41 reftable/stack: open-code reading refs
 5:  ab7538d0bb !  5:  cac08a934c reftable/iter: drop double-checking logic

@@ Commit message

The filtering ref iterator can be used to only yield refs which are not in a specific skip list. This iterator has an option to double-check the
- results it returns, which causes us to seek tho reference we are about
+ results it returns, which causes us to seek the reference we are about
to yield via a separate table such that we detect whether the reference that the first iterator has yielded actually exists.

6: 14924604ce ! 6: 103262dc79 reftable/generic: move generic iterator code into iterator interface

    @@ Commit message
         Move functions relating to the reftable iterator from "generic.c" into
         "iter.c". This prepares for the removal of the former subsystem.
     
    +    While at it, remove some unneeded braces to conform to our coding style.
    +
         Signed-off-by: Patrick Steinhardt <[email protected]>
     
      ## reftable/generic.c ##
    @@ reftable/iter.c: void iterator_from_indexed_table_ref_iter(struct reftable_itera
     +
     +void reftable_iterator_destroy(struct reftable_iterator *it)
     +{
    -+	if (!it->ops) {
    ++	if (!it->ops)
     +		return;
    -+	}
     +	it->ops->close(it->iter_arg);
     +	it->ops = NULL;
     +	FREE_AND_NULL(it->iter_arg);
 -:  ---------- >  7:  4011fa65d8 reftable/dump: drop unused `compact_stack()`
 9:  4184f46f92 !  8:  ceaa296bfd reftable/dump: move code into "t/helper/test-reftable.c"
    @@ Metadata
     Author: Patrick Steinhardt <[email protected]>
     
      ## Commit message ##
    -    reftable/dump: move code into "t/helper/test-reftable.c"
    +    t/helper: inline `reftable_dump_main()`
     
    -    The code in "reftable/dump.c" is only ever used by our test-tool to
    -    implement the "reftable" subcommand. It also feels very unlikely that it
    -    will ever be useful to any other potential user of the reftable library,
    -    which makes it a weird candidate to have in the library interface.
    +    The printing functionality part of `reftable/dump.c` is really only used
    +    by our "dump-reftable" test helper. It is certainly not generic logic
    +    that is useful to anybody outside of Git, and the format it generates is
    +    quite specific. Still, parts of it are used in our test suite and the
    +    output may be useful to take a peek into reftable stacks, tables and
    +    blocks. So while it does not make sense to expose this as part of the
    +    reftable library, it does make sense to keep it around.
     
    -    Inline the code into "t/helper/test-reftable.c".
    +    Inline the `reftable_dump_main()` function into the "dump-reftable" test
    +    helper. This clarifies that its format is subject to change and not part
    +    of our public interface. Furthermore, this allows us to iterate on the
    +    implementation in subsequent patches.
     
         Signed-off-by: Patrick Steinhardt <[email protected]>
     
    @@ reftable/dump.c (deleted)
     -
     -static void print_help(void)
     -{
    --	printf("usage: dump [-cst] arg\n\n"
    +-	printf("usage: dump [-st] arg\n\n"
     -	       "options: \n"
    --	       "  -c compact\n"
     -	       "  -b dump blocks\n"
     -	       "  -t dump table\n"
     -	       "  -s dump stack\n"
    @@ reftable/dump.c (deleted)
     -{
     -	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--)
    @@ reftable/dump.c (deleted)
     -			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;
    @@ reftable/dump.c (deleted)
     -
     -	arg = argv[1];
     -
    --	if (opt_dump_blocks)
    +-	if (opt_dump_blocks) {
     -		err = reftable_reader_print_blocks(arg);
    +-	} else if (opt_dump_table) {
    +-		err = reftable_reader_print_file(arg);
    +-	} else if (opt_dump_stack) {
    +-		err = reftable_stack_print_directory(arg, opt_hash_id);
    +-	}
     -
     -	if (err < 0) {
     -		fprintf(stderr, "%s: %s: %s\n", argv0, arg,
    @@ reftable/dump.c (deleted)
     -	return 0;
     -}
     
    + ## reftable/reftable-tests.h ##
    +@@ reftable/reftable-tests.h: int record_test_main(int argc, const char **argv);
    + int readwrite_test_main(int argc, const char **argv);
    + int stack_test_main(int argc, const char **argv);
    + int tree_test_main(int argc, const char **argv);
    +-int reftable_dump_main(int argc, char *const *argv);
    + 
    + #endif
    +
      ## t/helper/test-reftable.c ##
     @@
      #include "reftable/system.h"
     +#include "reftable/reftable-error.h"
     +#include "reftable/reftable-reader.h"
    ++#include "reftable/reftable-stack.h"
      #include "reftable/reftable-tests.h"
      #include "test-tool.h"
      
    @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)
      
     +static void print_help(void)
     +{
    -+	printf("usage: dump [-cst] arg\n\n"
    ++	printf("usage: dump [-st] arg\n\n"
     +	       "options: \n"
    -+	       "  -c compact\n"
     +	       "  -b dump blocks\n"
     +	       "  -t dump table\n"
     +	       "  -s dump stack\n"
    @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)
     -	return reftable_dump_main(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--)
    @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **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;
    @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)
     +
     +	arg = argv[1];
     +
    -+	if (opt_dump_blocks)
    ++	if (opt_dump_blocks) {
     +		err = reftable_reader_print_blocks(arg);
    ++	} else if (opt_dump_table) {
    ++		err = reftable_reader_print_file(arg);
    ++	} else if (opt_dump_stack) {
    ++		err = reftable_stack_print_directory(arg, opt_hash_id);
    ++	}
     +
     +	if (err < 0) {
     +		fprintf(stderr, "%s: %s: %s\n", argv0, arg,
 -:  ---------- >  9:  a62e4612e9 t/helper: inline `reftable_reader_print_file()`
 -:  ---------- > 10:  7acfe4fecc t/helper: inline `reftable_stack_print_directory()`
 7:  0aa7186067 ! 11:  8bd53a1a65 reftable/dump: drop unused `compact_stack()`
    @@ Metadata
     Author: Patrick Steinhardt <[email protected]>
     
      ## Commit message ##
    -    reftable/dump: drop unused `compact_stack()`
    +    t/helper: inline `reftable_table_print()`
     
    -    The `compact_stack()` function is exposed via `reftable_dump_main()`,
    -    which ultimately ends up being wired into "test-tool reftable". It is
    -    never used by our tests though, and nowadays we have wired up support
    -    for stack compaction into git-pack-refs(1).
    -
    -    Remove the code.
    +    Move `reftable_table_print()` into the "dump-reftable" helper. This
    +    follows the same reasoning as the preceding commit.
     
         Signed-off-by: Patrick Steinhardt <[email protected]>
     
    - ## reftable/dump.c ##
    -@@ reftable/dump.c: license that can be found in the LICENSE file or at
    - #include <unistd.h>
    - #include <string.h>
    + ## reftable/generic.c ##
    +@@ reftable/generic.c: int reftable_table_read_ref(struct reftable_table *tab, const char *name,
    + 	return err;
    + }
      
    --static int compact_stack(const char *stackdir)
    --{
    --	struct reftable_stack *stack = NULL;
    --	struct reftable_write_options opts = { 0 };
    +-int reftable_table_print(struct reftable_table *tab) {
    +-	struct reftable_iterator it = { NULL };
    +-	struct reftable_ref_record ref = { NULL };
    +-	struct reftable_log_record log = { NULL };
    +-	uint32_t hash_id = reftable_table_hash_id(tab);
    +-	int err;
    +-
    +-	reftable_table_init_ref_iter(tab, &it);
     -
    --	int err = reftable_new_stack(&stack, stackdir, &opts);
    +-	err = reftable_iterator_seek_ref(&it, "");
     -	if (err < 0)
    --		goto done;
    +-		return err;
    +-
    +-	while (1) {
    +-		err = reftable_iterator_next_ref(&it, &ref);
    +-		if (err > 0) {
    +-			break;
    +-		}
    +-		if (err < 0) {
    +-			return err;
    +-		}
    +-		reftable_ref_record_print(&ref, hash_id);
    +-	}
    +-	reftable_iterator_destroy(&it);
    +-	reftable_ref_record_release(&ref);
    +-
    +-	reftable_table_init_log_iter(tab, &it);
     -
    --	err = reftable_stack_compact_all(stack, NULL);
    +-	err = reftable_iterator_seek_log(&it, "");
     -	if (err < 0)
    --		goto done;
    --done:
    --	if (stack) {
    --		reftable_stack_destroy(stack);
    +-		return err;
    +-
    +-	while (1) {
    +-		err = reftable_iterator_next_log(&it, &log);
    +-		if (err > 0) {
    +-			break;
    +-		}
    +-		if (err < 0) {
    +-			return err;
    +-		}
    +-		reftable_log_record_print(&log, hash_id);
     -	}
    --	return err;
    +-	reftable_iterator_destroy(&it);
    +-	reftable_log_record_release(&log);
    +-	return 0;
     -}
     -
    - static void print_help(void)
    + uint64_t reftable_table_max_update_index(struct reftable_table *tab)
    + {
    + 	return tab->ops->max_update_index(tab->table_arg);
    +
    + ## reftable/reftable-generic.h ##
    +@@ reftable/reftable-generic.h: uint64_t reftable_table_min_update_index(struct reftable_table *tab);
    + int reftable_table_read_ref(struct reftable_table *tab, const char *name,
    + 			    struct reftable_ref_record *ref);
    + 
    +-/* dump table contents onto stdout for debugging */
    +-int reftable_table_print(struct reftable_table *tab);
    +-
    + #endif
    +
    + ## t/helper/test-reftable.c ##
    +@@ t/helper/test-reftable.c: static void print_help(void)
    + 	       "\n");
    + }
    + 
    ++static int dump_table(struct reftable_table *tab)
    ++{
    ++	struct reftable_iterator it = { NULL };
    ++	struct reftable_ref_record ref = { NULL };
    ++	struct reftable_log_record log = { NULL };
    ++	uint32_t hash_id = reftable_table_hash_id(tab);
    ++	int err;
    ++
    ++	reftable_table_init_ref_iter(tab, &it);
    ++
    ++	err = reftable_iterator_seek_ref(&it, "");
    ++	if (err < 0)
    ++		return err;
    ++
    ++	while (1) {
    ++		err = reftable_iterator_next_ref(&it, &ref);
    ++		if (err > 0) {
    ++			break;
    ++		}
    ++		if (err < 0) {
    ++			return err;
    ++		}
    ++		reftable_ref_record_print(&ref, hash_id);
    ++	}
    ++	reftable_iterator_destroy(&it);
    ++	reftable_ref_record_release(&ref);
    ++
    ++	reftable_table_init_log_iter(tab, &it);
    ++
    ++	err = reftable_iterator_seek_log(&it, "");
    ++	if (err < 0)
    ++		return err;
    ++
    ++	while (1) {
    ++		err = reftable_iterator_next_log(&it, &log);
    ++		if (err > 0) {
    ++			break;
    ++		}
    ++		if (err < 0) {
    ++			return err;
    ++		}
    ++		reftable_log_record_print(&log, hash_id);
    ++	}
    ++	reftable_iterator_destroy(&it);
    ++	reftable_log_record_release(&log);
    ++	return 0;
    ++}
    ++
    + static int dump_stack(const char *stackdir, uint32_t hash_id)
      {
    - 	printf("usage: dump [-cst] arg\n\n"
    -@@ reftable/dump.c: int reftable_dump_main(int argc, char *const *argv)
    - 	int opt_dump_blocks = 0;
    - 	int opt_dump_table = 0;
    - 	int opt_dump_stack = 0;
    --	int opt_compact = 0;
    - 	uint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;
    - 	const char *arg = NULL, *argv0 = argv[0];
    + 	struct reftable_stack *stack = NULL;
    +@@ t/helper/test-reftable.c: static int dump_stack(const char *stackdir, uint32_t hash_id)
      
    -@@ reftable/dump.c: int reftable_dump_main(int argc, char *const *argv)
    - 			opt_hash_id = GIT_SHA256_FORMAT_ID;
    - 		else if (!strcmp("-s", argv[1]))
    - 			opt_dump_stack = 1;
    --		else if (!strcmp("-c", argv[1]))
    --			opt_compact = 1;
    - 		else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {
    - 			print_help();
    - 			return 2;
    -@@ reftable/dump.c: int reftable_dump_main(int argc, char *const *argv)
    - 		err = reftable_reader_print_file(arg);
    - 	} else if (opt_dump_stack) {
    - 		err = reftable_stack_print_directory(arg, opt_hash_id);
    --	} else if (opt_compact) {
    --		err = compact_stack(arg);
    - 	}
    + 	merged = reftable_stack_merged_table(stack);
    + 	reftable_table_from_merged_table(&table, merged);
    +-	err = reftable_table_print(&table);
    ++	err = dump_table(&table);
    + done:
    + 	if (stack)
    + 		reftable_stack_destroy(stack);
    +@@ t/helper/test-reftable.c: static int dump_reftable(const char *tablename)
    + 		goto done;
      
    - 	if (err < 0) {
    + 	reftable_table_from_reader(&tab, r);
    +-	err = reftable_table_print(&tab);
    ++	err = dump_table(&tab);
    + done:
    + 	reftable_reader_free(r);
    + 	return err;
 8:  1f211e514d ! 12:  c50aabbb80 reftable/dump: drop unused printing functionality
    @@ Metadata
     Author: Patrick Steinhardt <[email protected]>
     
      ## Commit message ##
    -    reftable/dump: drop unused printing functionality
    +    t/helper: inline printing of reftable records
     
    -    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.
    +    Move printing of reftable records into the "dump-reftable" helper. This
    +    follows the same reasoning as the preceding commit.
     
         Signed-off-by: Patrick Steinhardt <[email protected]>
     
    - ## reftable/dump.c ##
    -@@ reftable/dump.c: 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--)
    -@@ reftable/dump.c: 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;
    -@@ reftable/dump.c: int reftable_dump_main(int argc, char *const *argv)
    - 
    - 	arg = argv[1];
    - 
    --	if (opt_dump_blocks) {
    -+	if (opt_dump_blocks)
    - 		err = reftable_reader_print_blocks(arg);
    --	} else if (opt_dump_table) {
    --		err = reftable_reader_print_file(arg);
    --	} else if (opt_dump_stack) {
    --		err = reftable_stack_print_directory(arg, opt_hash_id);
    --	}
    - 
    - 	if (err < 0) {
    - 		fprintf(stderr, "%s: %s: %s\n", argv0, arg,
    -
    - ## reftable/generic.c ##
    -@@ reftable/generic.c: int reftable_table_read_ref(struct reftable_table *tab, const char *name,
    - 	return err;
    - }
    - 
    --int reftable_table_print(struct reftable_table *tab) {
    --	struct reftable_iterator it = { NULL };
    --	struct reftable_ref_record ref = { NULL };
    --	struct reftable_log_record log = { NULL };
    --	uint32_t hash_id = reftable_table_hash_id(tab);
    --	int err;
    --
    --	reftable_table_init_ref_iter(tab, &it);
    --
    --	err = reftable_iterator_seek_ref(&it, "");
    --	if (err < 0)
    --		return err;
    --
    --	while (1) {
    --		err = reftable_iterator_next_ref(&it, &ref);
    --		if (err > 0) {
    --			break;
    --		}
    --		if (err < 0) {
    --			return err;
    --		}
    --		reftable_ref_record_print(&ref, hash_id);
    --	}
    --	reftable_iterator_destroy(&it);
    --	reftable_ref_record_release(&ref);
    --
    --	reftable_table_init_log_iter(tab, &it);
    --
    --	err = reftable_iterator_seek_log(&it, "");
    --	if (err < 0)
    --		return err;
    --
    --	while (1) {
    --		err = reftable_iterator_next_log(&it, &log);
    --		if (err > 0) {
    --			break;
    --		}
    --		if (err < 0) {
    --			return err;
    --		}
    --		reftable_log_record_print(&log, hash_id);
    --	}
    --	reftable_iterator_destroy(&it);
    --	reftable_log_record_release(&log);
    --	return 0;
    --}
    --
    - uint64_t reftable_table_max_update_index(struct reftable_table *tab)
    - {
    - 	return tab->ops->max_update_index(tab->table_arg);
    -
    - ## reftable/reader.c ##
    -@@ reftable/reader.c: void reftable_table_from_reader(struct reftable_table *tab,
    - 	tab->table_arg = reader;
    - }
    - 
    --
    --int reftable_reader_print_file(const char *tablename)
    --{
    --	struct reftable_block_source src = { NULL };
    --	int err = reftable_block_source_from_file(&src, tablename);
    --	struct reftable_reader *r = NULL;
    --	struct reftable_table tab = { NULL };
    --	if (err < 0)
    --		goto done;
    --
    --	err = reftable_new_reader(&r, &src, tablename);
    --	if (err < 0)
    --		goto done;
    --
    --	reftable_table_from_reader(&tab, r);
    --	err = reftable_table_print(&tab);
    --done:
    --	reftable_reader_free(r);
    --	return err;
    --}
    --
    - int reftable_reader_print_blocks(const char *tablename)
    - {
    - 	struct {
    -
      ## reftable/record.c ##
     @@ reftable/record.c: static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
      	}
    @@ reftable/record.c: void reftable_record_init(struct reftable_record *rec, uint8_
     -}
     
      ## reftable/record.h ##
    -@@ reftable/record.h: struct reftable_record_vtable {
    - 	 * the same type.
    - 	 */
    - 	int (*cmp)(const void *a, const void *b);
    --
    --	/* Print on stdout, for debugging. */
    --	void (*print)(const void *rec, int hash_size);
    - };
    - 
    - /* returns true for recognized block types. Block start with the block type. */
     @@ reftable/record.h: void reftable_record_init(struct reftable_record *rec, uint8_t typ);
      /* see struct record_vtable */
      int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);
    @@ reftable/record.h: void reftable_record_init(struct reftable_record *rec, uint8_
      void reftable_record_copy_from(struct reftable_record *rec,
      			       struct reftable_record *src, int hash_size);
     
    - ## reftable/reftable-generic.h ##
    -@@ reftable/reftable-generic.h: uint64_t reftable_table_min_update_index(struct reftable_table *tab);
    - int reftable_table_read_ref(struct reftable_table *tab, const char *name,
    - 			    struct reftable_ref_record *ref);
    - 
    --/* dump table contents onto stdout for debugging */
    --int reftable_table_print(struct reftable_table *tab);
    --
    - #endif
    -
    - ## reftable/reftable-reader.h ##
    -@@ reftable/reftable-reader.h: uint64_t reftable_reader_min_update_index(struct reftable_reader *r);
    - void reftable_table_from_reader(struct reftable_table *tab,
    - 				struct reftable_reader *reader);
    - 
    --/* print table onto stdout for debugging. */
    --int reftable_reader_print_file(const char *tablename);
    - /* print blocks onto stdout for debugging. */
    - int reftable_reader_print_blocks(const char *tablename);
    - 
    -
      ## reftable/reftable-record.h ##
     @@ reftable/reftable-record.h: const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *
      /* returns whether 'ref' represents a deletion */
    @@ reftable/reftable-record.h: void reftable_log_record_release(struct reftable_log
     -
      #endif
     
    - ## reftable/reftable-stack.h ##
    -@@ reftable/reftable-stack.h: struct reftable_compaction_stats {
    - struct reftable_compaction_stats *
    - reftable_stack_compaction_stats(struct reftable_stack *st);
    - 
    --/* print the entire stack represented by the directory */
    --int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);
    --
    - #endif
    -
    - ## reftable/stack.c ##
    -@@ reftable/stack.c: int reftable_stack_clean(struct reftable_stack *st)
    - 	reftable_addition_destroy(add);
    - 	return err;
    + ## t/helper/test-reftable.c ##
    +@@ t/helper/test-reftable.c: static void print_help(void)
    + 	       "\n");
      }
    --
    --int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
    --{
    --	struct reftable_stack *stack = NULL;
    --	struct reftable_write_options opts = { .hash_id = hash_id };
    --	struct reftable_merged_table *merged = NULL;
    --	struct reftable_table table = { NULL };
    --
    --	int err = reftable_new_stack(&stack, stackdir, &opts);
    --	if (err < 0)
    --		goto done;
    --
    --	merged = reftable_stack_merged_table(stack);
    --	reftable_table_from_merged_table(&table, merged);
    --	err = reftable_table_print(&table);
    --done:
    --	if (stack)
    --		reftable_stack_destroy(stack);
    --	return err;
    --}
    -
    - ## reftable/stack_test.c ##
    -@@ reftable/stack_test.c: static void test_reftable_stack_add_one(void)
    - 	EXPECT(0 == strcmp("master", dest.value.symref));
    - 	EXPECT(st->readers_len > 0);
      
    --	printf("testing print functionality:\n");
    --	err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);
    --	EXPECT_ERR(err);
    --
    --	err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);
    --	EXPECT(err == REFTABLE_FORMAT_ERROR);
    --
    - #ifndef GIT_WINDOWS_NATIVE
    - 	strbuf_addstr(&scratch, dir);
    - 	strbuf_addstr(&scratch, "/tables.list");
    ++static char hexdigit(int c)
    ++{
    ++	if (c <= 9)
    ++		return '0' + c;
    ++	return 'a' + (c - 10);
    ++}
    ++
    ++static void hex_format(char *dest, const unsigned char *src, int hash_size)
    ++{
    ++	assert(hash_size > 0);
    ++	if (src) {
    ++		int i = 0;
    ++		for (i = 0; i < hash_size; i++) {
    ++			dest[2 * i] = hexdigit(src[i] >> 4);
    ++			dest[2 * i + 1] = hexdigit(src[i] & 0xf);
    ++		}
    ++		dest[2 * hash_size] = 0;
    ++	}
    ++}
    ++
    + static int dump_table(struct reftable_table *tab)
    + {
    + 	struct reftable_iterator it = { NULL };
    + 	struct reftable_ref_record ref = { NULL };
    + 	struct reftable_log_record log = { NULL };
    + 	uint32_t hash_id = reftable_table_hash_id(tab);
    ++	int hash_len = hash_size(hash_id);
    + 	int err;
    + 
    + 	reftable_table_init_ref_iter(tab, &it);
    +@@ t/helper/test-reftable.c: static int dump_table(struct reftable_table *tab)
    + 		return err;
    + 
    + 	while (1) {
    ++		char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
    ++
    + 		err = reftable_iterator_next_ref(&it, &ref);
    +-		if (err > 0) {
    ++		if (err > 0)
    + 			break;
    +-		}
    +-		if (err < 0) {
    ++		if (err < 0)
    + 			return err;
    ++
    ++		printf("ref{%s(%" PRIu64 ") ", ref.refname, ref.update_index);
    ++		switch (ref.value_type) {
    ++		case REFTABLE_REF_SYMREF:
    ++			printf("=> %s", ref.value.symref);
    ++			break;
    ++		case REFTABLE_REF_VAL2:
    ++			hex_format(hex, ref.value.val2.value, hash_len);
    ++			printf("val 2 %s", hex);
    ++			hex_format(hex, ref.value.val2.target_value,
    ++				   hash_len);
    ++			printf("(T %s)", hex);
    ++			break;
    ++		case REFTABLE_REF_VAL1:
    ++			hex_format(hex, ref.value.val1, hash_len);
    ++			printf("val 1 %s", hex);
    ++			break;
    ++		case REFTABLE_REF_DELETION:
    ++			printf("delete");
    ++			break;
    + 		}
    +-		reftable_ref_record_print(&ref, hash_id);
    ++		printf("}\n");
    + 	}
    + 	reftable_iterator_destroy(&it);
    + 	reftable_ref_record_release(&ref);
    +@@ t/helper/test-reftable.c: static int dump_table(struct reftable_table *tab)
    + 		return err;
    + 
    + 	while (1) {
    ++		char hex[GIT_MAX_HEXSZ + 1] = { 0 };
    ++
    + 		err = reftable_iterator_next_log(&it, &log);
    +-		if (err > 0) {
    ++		if (err > 0)
    + 			break;
    +-		}
    +-		if (err < 0) {
    ++		if (err < 0)
    + 			return err;
    ++
    ++		switch (log.value_type) {
    ++		case REFTABLE_LOG_DELETION:
    ++			printf("log{%s(%" PRIu64 ") delete\n", log.refname,
    ++			       log.update_index);
    ++			break;
    ++		case REFTABLE_LOG_UPDATE:
    ++			printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
    ++			       log.refname, log.update_index,
    ++			       log.value.update.name ? log.value.update.name : "",
    ++			       log.value.update.email ? log.value.update.email : "",
    ++			       log.value.update.time,
    ++			       log.value.update.tz_offset);
    ++			hex_format(hex, log.value.update.old_hash, hash_len);
    ++			printf("%s => ", hex);
    ++			hex_format(hex, log.value.update.new_hash, hash_len);
    ++			printf("%s\n\n%s\n}\n", hex,
    ++			       log.value.update.message ? log.value.update.message : "");
    ++			break;
    + 		}
    +-		reftable_log_record_print(&log, hash_id);
    + 	}
    + 	reftable_iterator_destroy(&it);
    + 	reftable_log_record_release(&log);
 -:  ---------- > 13:  5498395872 t/helper: use `hash_to_hex_algop()` to print hashes
 -:  ---------- > 14:  5390be75c3 t/helper: refactor to not use `struct reftable_table`
10:  723495adf6 = 15:  5aeab8ee07 reftable/generic: drop interface
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#1 lore raw

PATCH v2 1/15 reftable/merged: expose functions to initialize iterators

We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic reftable_table interface, which is somewhat roundabout.

Implement two new functions to initialize iterators for ref and log records to plug this gap.

---
 reftable/merged.c          | 12 ++++++++++++
 reftable/reftable-merged.h |  8 ++++++++
 2 files changed, 20 insertions(+)
reftable/merged.c +12 -0
index 6adce44f4b..8d78b3da71 100644
@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,
 	iterator_from_merged_iter(it, mi);
 }
 
+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it)
+{
+	merged_table_init_iter(mt, it, BLOCK_TYPE_REF);
+}
+
+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it)
+{
+	merged_table_init_iter(mt, it, BLOCK_TYPE_LOG);
+}
+
 uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)
 {
 	return mt->hash_id;
reftable/reftable-merged.h +8 -0
index 14d5fc9f05..4deb0ad22e 100644
@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id);
 
+/* Initialize a merged table iterator for reading refs. */
+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it);
+
+/* Initialize a merged table iterator for reading logs. */
+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it);
+
 /* returns the max update_index covered by this merged table. */
 uint64_t
 reftable_merged_table_max_update_index(struct reftable_merged_table *mt);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#2 lore raw

PATCH v2 2/15 reftable/merged: rename `reftable_new_merged_table()`

In reply to Patrick Steinhardt

Rename reftable_new_merged_table() to reftable_merged_table_new() such that the name matches our coding style.

---
 reftable/merged.c                | 2 +-
 reftable/reftable-merged.h       | 9 +++++----
 reftable/stack.c                 | 4 ++--
 t/unit-tests/t-reftable-merged.c | 8 ++++----
 4 files changed, 12 insertions(+), 11 deletions(-)
reftable/merged.c +1 -1
index 8d78b3da71..25d414ec41 100644
@@ -192,7 +192,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
 	it->ops = &merged_iter_vtable;
 }
 
-int reftable_new_merged_table(struct reftable_merged_table **dest,
+int reftable_merged_table_new(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id)
 {
reftable/reftable-merged.h +5 -4
index 4deb0ad22e..72762483b9 100644
@@ -29,10 +29,11 @@ struct reftable_merged_table;
 /* A generic reftable; see below. */
 struct reftable_table;
 
-/* reftable_new_merged_table creates a new merged table. It takes ownership of
-   the stack array.
-*/
-int reftable_new_merged_table(struct reftable_merged_table **dest,
+/*
+ * reftable_merged_table_new creates a new merged table. It takes ownership of
+ * the stack array.
+ */
+int reftable_merged_table_new(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id);
 
reftable/stack.c +2 -2
index 2071e428a8..64c7fdf8c4 100644
@@ -272,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 	}
 
 	/* success! */
-	err = reftable_new_merged_table(&new_merged, new_tables,
+	err = reftable_merged_table_new(&new_merged, new_tables,
 					new_readers_len, st->opts.hash_id);
 	if (err < 0)
 		goto done;
@@ -924,7 +924,7 @@ static int stack_write_compact(struct reftable_stack *st,
 	reftable_writer_set_limits(wr, st->readers[first]->min_update_index,
 				   st->readers[last]->max_update_index);
 
-	err = reftable_new_merged_table(&mt, subtabs, subtabs_len,
+	err = reftable_merged_table_new(&mt, subtabs, subtabs_len,
 					st->opts.hash_id);
 	if (err < 0) {
 		reftable_free(subtabs);
t/unit-tests/t-reftable-merged.c +4 -4
index b6263ee8b5..210603e8c7 100644
@@ -111,7 +111,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -289,7 +289,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -441,9 +441,9 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
-	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
 	check_int(err, ==, REFTABLE_FORMAT_ERROR);
-	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
 	reftable_reader_free(rd);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#3 lore raw

PATCH v2 3/15 reftable/merged: stop using generic tables in the merged table

In reply to Patrick Steinhardt

The merged table provides access to a reftable stack by merging the contents of those tables into a virtual table. These subtables are being tracked via struct reftable_table, which is a generic interface for accessing either a single reftable or a merged reftable. So in theory, it would be possible for the merged table to merge together other merged tables.

This is somewhat nonsensical though: we only ever set up a merged table over normal reftables, and there is no reason to do otherwise. This generic interface thus makes the code way harder to follow and reason about than really necessary. The abstraction layer may also have an impact on performance, even though the extra set of vtable function calls probably doesn't really matter.

Refactor the merged tables to use a struct reftable_reader for each of the subtables instead, which gives us direct access to the underlying tables. Adjust names accordingly.

---
 reftable/merged.c                | 28 +++++++++----------
 reftable/merged.h                |  4 +--
 reftable/reader.c                |  6 ++--
 reftable/reader.h                |  4 +++
 reftable/reftable-merged.h       |  7 +++--
 reftable/stack.c                 | 48 ++++++++++++--------------------
 reftable/stack_test.c            | 22 +++++++--------
 t/unit-tests/t-reftable-merged.c | 16 +++--------
 8 files changed, 60 insertions(+), 75 deletions(-)
reftable/merged.c +14 -14
index 25d414ec41..2e72eab306 100644
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "constants.h"
 #include "iter.h"
 #include "pq.h"
+#include "reader.h"
 #include "record.h"
 #include "generic.h"
 #include "reftable-merged.h"
@@ -25,7 +26,7 @@ struct merged_subiter {
 struct merged_iter {
 	struct merged_subiter *subiters;
 	struct merged_iter_pqueue pq;
-	size_t stack_len;
+	size_t subiters_len;
 	int suppress_deletions;
 	ssize_t advance_index;
 };
@@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi,
 	mi->advance_index = -1;
 	mi->suppress_deletions = mt->suppress_deletions;
 
-	REFTABLE_CALLOC_ARRAY(mi->subiters, mt->stack_len);
-	for (size_t i = 0; i < mt->stack_len; i++) {
+	REFTABLE_CALLOC_ARRAY(mi->subiters, mt->readers_len);
+	for (size_t i = 0; i < mt->readers_len; i++) {
 		reftable_record_init(&mi->subiters[i].rec, typ);
-		table_init_iter(&mt->stack[i], &mi->subiters[i].iter, typ);
+		reader_init_iter(mt->readers[i], &mi->subiters[i].iter, typ);
 	}
-	mi->stack_len = mt->stack_len;
+	mi->subiters_len = mt->readers_len;
 }
 
 static void merged_iter_close(void *p)
@@ -51,7 +52,7 @@ static void merged_iter_close(void *p)
 	struct merged_iter *mi = p;
 
 	merged_iter_pqueue_release(&mi->pq);
-	for (size_t i = 0; i < mi->stack_len; i++) {
+	for (size_t i = 0; i < mi->subiters_len; i++) {
 		reftable_iterator_destroy(&mi->subiters[i].iter);
 		reftable_record_release(&mi->subiters[i].rec);
 	}
@@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want
 
 	mi->advance_index = -1;
 
-	for (size_t i = 0; i < mi->stack_len; i++) {
+	for (size_t i = 0; i < mi->subiters_len; i++) {
 		err = iterator_seek(&mi->subiters[i].iter, want);
 		if (err < 0)
 			return err;
@@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
 }
 
 int reftable_merged_table_new(struct reftable_merged_table **dest,
-			      struct reftable_table *stack, size_t n,
+			      struct reftable_reader **readers, size_t n,
 			      uint32_t hash_id)
 {
 	struct reftable_merged_table *m = NULL;
@@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
 	uint64_t first_min = 0;
 
 	for (size_t i = 0; i < n; i++) {
-		uint64_t min = reftable_table_min_update_index(&stack[i]);
-		uint64_t max = reftable_table_max_update_index(&stack[i]);
+		uint64_t min = reftable_reader_min_update_index(readers[i]);
+		uint64_t max = reftable_reader_max_update_index(readers[i]);
 
-		if (reftable_table_hash_id(&stack[i]) != hash_id) {
+		if (reftable_reader_hash_id(readers[i]) != hash_id) {
 			return REFTABLE_FORMAT_ERROR;
 		}
 		if (i == 0 || min < first_min) {
@@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
 	}
 
 	REFTABLE_CALLOC_ARRAY(m, 1);
-	m->stack = stack;
-	m->stack_len = n;
+	m->readers = readers;
+	m->readers_len = n;
 	m->min = first_min;
 	m->max = last_max;
 	m->hash_id = hash_id;
@@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt)
 {
 	if (!mt)
 		return;
-	FREE_AND_NULL(mt->stack);
 	reftable_free(mt);
 }
 
reftable/merged.h +2 -2
index 2efe571da6..de5fd33f01 100644
@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at
 #include "system.h"
 
 struct reftable_merged_table {
-	struct reftable_table *stack;
-	size_t stack_len;
+	struct reftable_reader **readers;
+	size_t readers_len;
 	uint32_t hash_id;
 
 	/* If unset, produce deletions. This is useful for compaction. For the
reftable/reader.c +3 -3
index 29c99e2269..f7ae35da72 100644
@@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it,
 	it->ops = &table_iter_vtable;
 }
 
-static void reader_init_iter(struct reftable_reader *r,
-			     struct reftable_iterator *it,
-			     uint8_t typ)
+void reader_init_iter(struct reftable_reader *r,
+		      struct reftable_iterator *it,
+		      uint8_t typ)
 {
 	struct reftable_reader_offsets *offs = reader_offsets_for(r, typ);
 
reftable/reader.h +4 -0
index e869165f23..a2c204d523 100644
@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,
 void reader_close(struct reftable_reader *r);
 const char *reader_name(struct reftable_reader *r);
 
+void reader_init_iter(struct reftable_reader *r,
+		      struct reftable_iterator *it,
+		      uint8_t typ);
+
 /* initialize a block reader to read from `r` */
 int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
 			     uint64_t next_off, uint8_t want_typ);
reftable/reftable-merged.h +4 -3
index 72762483b9..03c2619c0f 100644
@@ -28,13 +28,14 @@ struct reftable_merged_table;
 
 /* A generic reftable; see below. */
 struct reftable_table;
+struct reftable_reader;
 
 /*
- * reftable_merged_table_new creates a new merged table. It takes ownership of
- * the stack array.
+ * reftable_merged_table_new creates a new merged table. The readers must be
+ * kept alive as long as the merged table is still in use.
  */
 int reftable_merged_table_new(struct reftable_merged_table **dest,
-			      struct reftable_table *stack, size_t n,
+			      struct reftable_reader **readers, size_t n,
 			      uint32_t hash_id);
 
 /* Initialize a merged table iterator for reading refs. */
reftable/stack.c +18 -30

146 lines not on this page · raw message

reftable/stack_test.c +11 -11
index 8c36590ff0..dbca9eaf4a 100644
@@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
 		 * all tables in the stack.
 		 */
 		if (i != n)
-			EXPECT(st->merged->stack_len == i + 1);
+			EXPECT(st->merged->readers_len == i + 1);
 		else
-			EXPECT(st->merged->stack_len == 1);
+			EXPECT(st->merged->readers_len == 1);
 	}
 
 	reftable_stack_destroy(st);
@@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
 
 	err = reftable_stack_add(st, write_test_ref, &ref);
 	EXPECT_ERR(err);
-	EXPECT(st->merged->stack_len == 1);
+	EXPECT(st->merged->readers_len == 1);
 	EXPECT(st->stats.attempts == 0);
 	EXPECT(st->stats.failures == 0);
 
@@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
 	ref.update_index = 2;
 	err = reftable_stack_add(st, write_test_ref, &ref);
 	EXPECT_ERR(err);
-	EXPECT(st->merged->stack_len == 2);
+	EXPECT(st->merged->readers_len == 2);
 	EXPECT(st->stats.attempts == 1);
 	EXPECT(st->stats.failures == 1);
 
@@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void)
 
 		err = reftable_stack_auto_compact(st);
 		EXPECT_ERR(err);
-		EXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));
+		EXPECT(i < 3 || st->merged->readers_len < 2 * fastlog2(i));
 	}
 
 	EXPECT(reftable_stack_compaction_stats(st)->entries_written <
@@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
 	EXPECT_ERR(err);
 
 	write_n_ref_tables(st, 5);
-	EXPECT(st->merged->stack_len == 5);
+	EXPECT(st->merged->readers_len == 5);
 
 	/*
 	 * Given that all tables we have written should be roughly the same
@@ -925,7 +925,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
 	err = reftable_stack_auto_compact(st);
 	EXPECT_ERR(err);
 	EXPECT(st->stats.failures == 0);
-	EXPECT(st->merged->stack_len == 4);
+	EXPECT(st->merged->readers_len == 4);
 
 	reftable_stack_destroy(st);
 	strbuf_release(&buf);
@@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void)
 		 * all tables in the stack.
 		 */
 		if (i != n)
-			EXPECT(st->merged->stack_len == i + 1);
+			EXPECT(st->merged->readers_len == i + 1);
 		else
-			EXPECT(st->merged->stack_len == 1);
+			EXPECT(st->merged->readers_len == 1);
 	}
 
 	reftable_stack_destroy(st);
@@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
 	EXPECT_ERR(err);
 
 	write_n_ref_tables(st, 3);
-	EXPECT(st->merged->stack_len == 3);
+	EXPECT(st->merged->readers_len == 3);
 
 	/* Lock one of the tables that we're about to compact. */
 	strbuf_reset(&buf);
@@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
 	err = reftable_stack_compact_all(st, NULL);
 	EXPECT(err == REFTABLE_LOCK_ERROR);
 	EXPECT(st->stats.failures == 1);
-	EXPECT(st->merged->stack_len == 3);
+	EXPECT(st->merged->readers_len == 3);
 
 	reftable_stack_destroy(st);
 	strbuf_release(&buf);
t/unit-tests/t-reftable-merged.c +4 -12
index 210603e8c7..577b1a5be8 100644
@@ -94,10 +94,8 @@ merged_table_from_records(struct reftable_ref_record **refs,
 			  struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
-	struct reftable_table *tabs;
 	int err;
 
-	REFTABLE_CALLOC_ARRAY(tabs, n);
 	REFTABLE_CALLOC_ARRAY(*readers, n);
 	REFTABLE_CALLOC_ARRAY(*source, n);
 
@@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs,
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
 		check(!err);
-		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
-	struct reftable_table *tabs;
 	int err;
 
-	REFTABLE_CALLOC_ARRAY(tabs, n);
 	REFTABLE_CALLOC_ARRAY(*readers, n);
 	REFTABLE_CALLOC_ARRAY(*source, n);
 
@@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
 		check(!err);
-		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -418,7 +412,6 @@ static void t_default_write_opts(void)
 	};
 	int err;
 	struct reftable_block_source source = { 0 };
-	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
 	struct reftable_merged_table *merged = NULL;
@@ -440,10 +433,9 @@ static void t_default_write_opts(void)
 	hash_id = reftable_reader_hash_id(rd);
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
-	reftable_table_from_reader(&tab[0], rd);
-	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA256_FORMAT_ID);
 	check_int(err, ==, REFTABLE_FORMAT_ERROR);
-	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
 	reftable_reader_free(rd);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#4 lore raw

PATCH v2 4/15 reftable/stack: open-code reading refs

In reply to Patrick Steinhardt

To read a reference for the reftable stack, we first create a generic reftable_table from the merged table and then read the reference via a convenience function. We are about to remove these generic interfaces, so let's instead open-code the logic to prepare for this removal.

---
 reftable/stack.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
reftable/stack.c +22 -3
index 7f4e267ea9..d08ec00959 100644
@@ -1468,9 +1468,28 @@ reftable_stack_compaction_stats(struct reftable_stack *st)
 int reftable_stack_read_ref(struct reftable_stack *st, const char *refname,
 			    struct reftable_ref_record *ref)
 {
-	struct reftable_table tab = { NULL };
-	reftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));
-	return reftable_table_read_ref(&tab, refname, ref);
+	struct reftable_iterator it = { 0 };
+	int ret;
+
+	reftable_merged_table_init_ref_iterator(st->merged, &it);
+	ret = reftable_iterator_seek_ref(&it, refname);
+	if (ret)
+		goto out;
+
+	ret = reftable_iterator_next_ref(&it, ref);
+	if (ret)
+		goto out;
+
+	if (strcmp(ref->refname, refname) ||
+	    reftable_ref_record_is_deletion(ref)) {
+		reftable_ref_record_release(ref);
+		ret = 1;
+		goto out;
+	}
+
+out:
+	reftable_iterator_destroy(&it);
+	return ret;
 }
 
 int reftable_stack_read_log(struct reftable_stack *st, const char *refname,
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#5 lore raw

PATCH v2 5/15 reftable/iter: drop double-checking logic

In reply to Patrick Steinhardt

The filtering ref iterator can be used to only yield refs which are not in a specific skip list. This iterator has an option to double-check the results it returns, which causes us to seek the reference we are about to yield via a separate table such that we detect whether the reference that the first iterator has yielded actually exists.

The value of this is somewhat dubious, and I cannot think of any usecase where this functionality should be required. Furthermore, this option is never set in our codebase, which means that it is essentially untested. And last but not least, the struct reftable_table that is used to implement it is about to go away.

So while we could refactor the code to not use a reftable_table, it very much feels like a wasted effort. Let's just drop this code.

---
 reftable/iter.c   | 20 --------------------
 reftable/iter.h   |  2 --
 reftable/reader.c |  2 --
 3 files changed, 24 deletions(-)
reftable/iter.c +0 -20
index fddea31e51..a7484aba60 100644
@@ -42,26 +42,6 @@ static int filtering_ref_iterator_next(void *iter_arg,
 			break;
 		}
 
-		if (fri->double_check) {
-			struct reftable_iterator it = { NULL };
-
-			reftable_table_init_ref_iter(&fri->tab, &it);
-
-			err = reftable_iterator_seek_ref(&it, ref->refname);
-			if (err == 0)
-				err = reftable_iterator_next_ref(&it, ref);
-
-			reftable_iterator_destroy(&it);
-
-			if (err < 0) {
-				break;
-			}
-
-			if (err > 0) {
-				continue;
-			}
-		}
-
 		if (ref->value_type == REFTABLE_REF_VAL2 &&
 		    (!memcmp(fri->oid.buf, ref->value.val2.target_value,
 			     fri->oid.len) ||
reftable/iter.h +0 -2
index 537431baba..b75d7ac2ac 100644
@@ -18,8 +18,6 @@ license that can be found in the LICENSE file or at
 
 /* iterator that produces only ref records that point to `oid` */
 struct filtering_ref_iterator {
-	int double_check;
-	struct reftable_table tab;
 	struct strbuf oid;
 	struct reftable_iterator it;
 };
reftable/reader.c +0 -2
index f7ae35da72..e3f5854229 100644
@@ -735,8 +735,6 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,
 	*filter = empty;
 
 	strbuf_add(&filter->oid, oid, oid_len);
-	reftable_table_from_reader(&filter->tab, r);
-	filter->double_check = 0;
 	iterator_from_table_iter(&filter->it, ti);
 
 	iterator_from_filtering_ref_iterator(it, filter);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#6 lore raw

PATCH v2 6/15 reftable/generic: move generic iterator code into iterator interface

In reply to Patrick Steinhardt

Move functions relating to the reftable iterator from "generic.c" into "iter.c". This prepares for the removal of the former subsystem.

While at it, remove some unneeded braces to conform to our coding style.

---
 reftable/generic.c | 107 +--------------------------------------------
 reftable/generic.h |  10 -----
 reftable/iter.c    | 105 ++++++++++++++++++++++++++++++++++++++++++++
 reftable/iter.h    |  27 ++++++++++++
 4 files changed, 133 insertions(+), 116 deletions(-)
reftable/generic.c +1 -106
index 28ae26145e..6ecf9b880f 100644
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
 #include "constants.h"
 #include "record.h"
 #include "generic.h"
+#include "iter.h"
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
@@ -32,37 +33,6 @@ void reftable_table_init_log_iter(struct reftable_table *tab,
 	table_init_iter(tab, it, BLOCK_TYPE_LOG);
 }
 
-int reftable_iterator_seek_ref(struct reftable_iterator *it,
-			       const char *name)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_REF,
-		.u.ref = {
-			.refname = (char *)name,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log_at(struct reftable_iterator *it,
-				  const char *name, uint64_t update_index)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_LOG,
-		.u.log = {
-			.refname = (char *)name,
-			.update_index = update_index,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log(struct reftable_iterator *it,
-			       const char *name)
-{
-	return reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));
-}
-
 int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 			    struct reftable_ref_record *ref)
 {
@@ -152,78 +122,3 @@ uint32_t reftable_table_hash_id(struct reftable_table *tab)
 {
 	return tab->ops->hash_id(tab->table_arg);
 }
-
-void reftable_iterator_destroy(struct reftable_iterator *it)
-{
-	if (!it->ops) {
-		return;
-	}
-	it->ops->close(it->iter_arg);
-	it->ops = NULL;
-	FREE_AND_NULL(it->iter_arg);
-}
-
-int reftable_iterator_next_ref(struct reftable_iterator *it,
-			       struct reftable_ref_record *ref)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_REF,
-		.u = {
-			.ref = *ref
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*ref = rec.u.ref;
-	return err;
-}
-
-int reftable_iterator_next_log(struct reftable_iterator *it,
-			       struct reftable_log_record *log)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_LOG,
-		.u = {
-			.log = *log,
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*log = rec.u.log;
-	return err;
-}
-
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)
-{
-	return it->ops->seek(it->iter_arg, want);
-}
-
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
-{
-	return it->ops->next(it->iter_arg, rec);
-}
-
-static int empty_iterator_seek(void *arg, struct reftable_record *want)
-{
-	return 0;
-}
-
-static int empty_iterator_next(void *arg, struct reftable_record *rec)
-{
-	return 1;
-}
-
-static void empty_iterator_close(void *arg)
-{
-}
-
-static struct reftable_iterator_vtable empty_vtable = {
-	.seek = &empty_iterator_seek,
-	.next = &empty_iterator_next,
-	.close = &empty_iterator_close,
-};
-
-void iterator_set_empty(struct reftable_iterator *it)
-{
-	assert(!it->ops);
-	it->iter_arg = NULL;
-	it->ops = &empty_vtable;
-}
reftable/generic.h +0 -10
index 8341fa570e..837fbb8df2 100644
@@ -24,14 +24,4 @@ void table_init_iter(struct reftable_table *tab,
 		     struct reftable_iterator *it,
 		     uint8_t typ);
 
-struct reftable_iterator_vtable {
-	int (*seek)(void *iter_arg, struct reftable_record *want);
-	int (*next)(void *iter_arg, struct reftable_record *rec);
-	void (*close)(void *iter_arg);
-};
-
-void iterator_set_empty(struct reftable_iterator *it);
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
-
 #endif
reftable/iter.c +105 -0

114 lines not on this page · raw message

reftable/iter.h +27 -0
index b75d7ac2ac..3b401f1259 100644
@@ -16,6 +16,33 @@ license that can be found in the LICENSE file or at
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
+/*
+ * The virtual function table for implementing generic reftable iterators.
+ */
+struct reftable_iterator_vtable {
+	int (*seek)(void *iter_arg, struct reftable_record *want);
+	int (*next)(void *iter_arg, struct reftable_record *rec);
+	void (*close)(void *iter_arg);
+};
+
+/*
+ * Position the iterator at the wanted record such that a call to
+ * `iterator_next()` would return that record, if it exists.
+ */
+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
+
+/*
+ * Yield the next record and advance the iterator. Returns <0 on error, 0 when
+ * a record was yielded, and >0 when the iterator hit an error.
+ */
+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
+
+/*
+ * Set up the iterator such that it behaves the same as an iterator with no
+ * entries.
+ */
+void iterator_set_empty(struct reftable_iterator *it);
+
 /* iterator that produces only ref records that point to `oid` */
 struct filtering_ref_iterator {
 	struct strbuf oid;
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#7 lore raw

PATCH v2 7/15 reftable/dump: drop unused `compact_stack()`

In reply to Patrick Steinhardt

The compact_stack() function is exposed via reftable_dump_main(), which ultimately ends up being wired into "test-tool reftable". It is never used by our tests though, and nowadays we have wired up support for stack compaction into git-pack-refs(1).

Remove the code.

---
 reftable/dump.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)
reftable/dump.c +1 -26
index dd65d9e8bb..391d93de6a 100644
@@ -24,30 +24,10 @@ license that can be found in the LICENSE file or at
 #include <unistd.h>
 #include <string.h>
 
-static int compact_stack(const char *stackdir)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { 0 };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	err = reftable_stack_compact_all(stack, NULL);
-	if (err < 0)
-		goto done;
-done:
-	if (stack) {
-		reftable_stack_destroy(stack);
-	}
-	return err;
-}
-
 static void print_help(void)
 {
-	printf("usage: dump [-cst] arg\n\n"
+	printf("usage: dump [-st] arg\n\n"
 	       "options: \n"
-	       "  -c compact\n"
 	       "  -b dump blocks\n"
 	       "  -t dump table\n"
 	       "  -s dump stack\n"
@@ -62,7 +42,6 @@ int reftable_dump_main(int argc, char *const *argv)
 	int opt_dump_blocks = 0;
 	int opt_dump_table = 0;
 	int opt_dump_stack = 0;
-	int opt_compact = 0;
 	uint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;
 	const char *arg = NULL, *argv0 = argv[0];
 
@@ -77,8 +56,6 @@ int reftable_dump_main(int argc, char *const *argv)
 			opt_hash_id = GIT_SHA256_FORMAT_ID;
 		else if (!strcmp("-s", argv[1]))
 			opt_dump_stack = 1;
-		else if (!strcmp("-c", argv[1]))
-			opt_compact = 1;
 		else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {
 			print_help();
 			return 2;
@@ -98,8 +75,6 @@ int reftable_dump_main(int argc, char *const *argv)
 		err = reftable_reader_print_file(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);
-	} else if (opt_compact) {
-		err = compact_stack(arg);
 	}
 
 	if (err < 0) {
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#8 lore raw

PATCH v2 8/15 t/helper: inline `reftable_dump_main()`

In reply to Patrick Steinhardt

The printing functionality part of reftable/dump.c is really only used by our "dump-reftable" test helper. It is certainly not generic logic that is useful to anybody outside of Git, and the format it generates is quite specific. Still, parts of it are used in our test suite and the output may be useful to take a peek into reftable stacks, tables and blocks. So while it does not make sense to expose this as part of the reftable library, it does make sense to keep it around.

Inline the reftable_dump_main() function into the "dump-reftable" test helper. This clarifies that its format is subject to change and not part of our public interface. Furthermore, this allows us to iterate on the implementation in subsequent patches.

---
 Makefile                  |  1 -
 reftable/dump.c           | 86 ---------------------------------------
 reftable/reftable-tests.h |  1 -
 t/helper/test-reftable.c  | 61 ++++++++++++++++++++++++++-
 4 files changed, 60 insertions(+), 89 deletions(-)
 delete mode 100644 reftable/dump.c
Makefile +0 -1
index 3863e60b66..343f19a488 100644
@@ -2680,7 +2680,6 @@ REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
-REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
reftable/dump.c deleted +0 -86

86 lines not on this page · raw message

reftable/reftable-tests.h +0 -1
index d5e03dcc1b..d005a8bb9e 100644
@@ -16,6 +16,5 @@ int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
-int reftable_dump_main(int argc, char *const *argv);
 
 #endif
t/helper/test-reftable.c +60 -1

71 lines not on this page · raw message

Signature · 1 line
2.46.0.46.g406f326d27.dirty
#9 lore raw

PATCH v2 9/15 t/helper: inline `reftable_reader_print_file()`

In reply to Patrick Steinhardt

Move reftable_reader_print_file() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/reader.c          | 21 ---------------------
 reftable/reftable-reader.h |  2 --
 t/helper/test-reftable.c   | 23 ++++++++++++++++++++++-
 3 files changed, 22 insertions(+), 24 deletions(-)
reftable/reader.c +0 -21
index e3f5854229..fbd93b88df 100644
@@ -798,27 +798,6 @@ void reftable_table_from_reader(struct reftable_table *tab,
 	tab->table_arg = reader;
 }
 
-
-int reftable_reader_print_file(const char *tablename)
-{
-	struct reftable_block_source src = { NULL };
-	int err = reftable_block_source_from_file(&src, tablename);
-	struct reftable_reader *r = NULL;
-	struct reftable_table tab = { NULL };
-	if (err < 0)
-		goto done;
-
-	err = reftable_new_reader(&r, &src, tablename);
-	if (err < 0)
-		goto done;
-
-	reftable_table_from_reader(&tab, r);
-	err = reftable_table_print(&tab);
-done:
-	reftable_reader_free(r);
-	return err;
-}
-
 int reftable_reader_print_blocks(const char *tablename)
 {
 	struct {
reftable/reftable-reader.h +0 -2
index a32f31d648..7c7d171651 100644
@@ -64,8 +64,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r);
 void reftable_table_from_reader(struct reftable_table *tab,
 				struct reftable_reader *reader);
 
-/* print table onto stdout for debugging. */
-int reftable_reader_print_file(const char *tablename);
 /* print blocks onto stdout for debugging. */
 int reftable_reader_print_blocks(const char *tablename);
 
t/helper/test-reftable.c +22 -1
index 7f37d0cd34..19367c25f9 100644
@@ -1,5 +1,6 @@
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
+#include "reftable/reftable-generic.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
 #include "reftable/reftable-tests.h"
@@ -28,6 +29,26 @@ static void print_help(void)
 	       "\n");
 }
 
+static int dump_reftable(const char *tablename)
+{
+	struct reftable_block_source src = { NULL };
+	int err = reftable_block_source_from_file(&src, tablename);
+	struct reftable_reader *r = NULL;
+	struct reftable_table tab = { NULL };
+	if (err < 0)
+		goto done;
+
+	err = reftable_new_reader(&r, &src, tablename);
+	if (err < 0)
+		goto done;
+
+	reftable_table_from_reader(&tab, r);
+	err = reftable_table_print(&tab);
+done:
+	reftable_reader_free(r);
+	return err;
+}
+
 int cmd__dump_reftable(int argc, const char **argv)
 {
 	int err = 0;
@@ -64,7 +85,7 @@ int cmd__dump_reftable(int argc, const char **argv)
 	if (opt_dump_blocks) {
 		err = reftable_reader_print_blocks(arg);
 	} else if (opt_dump_table) {
-		err = reftable_reader_print_file(arg);
+		err = dump_reftable(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);
 	}
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#10 lore raw

PATCH v2 10/15 t/helper: inline `reftable_stack_print_directory()`

In reply to Patrick Steinhardt

Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/reftable-stack.h |  3 ---
 reftable/stack.c          | 20 --------------------
 reftable/stack_test.c     |  7 -------
 t/helper/test-reftable.c  | 23 ++++++++++++++++++++++-
 4 files changed, 22 insertions(+), 31 deletions(-)
reftable/reftable-stack.h +0 -3
index 09e97c9991..f4f8cabc7f 100644
@@ -140,7 +140,4 @@ struct reftable_compaction_stats {
 struct reftable_compaction_stats *
 reftable_stack_compaction_stats(struct reftable_stack *st);
 
-/* print the entire stack represented by the directory */
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);
-
 #endif
reftable/stack.c +0 -20
index d08ec00959..bedd503e7e 100644
@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)
 	reftable_addition_destroy(add);
 	return err;
 }
-
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { .hash_id = hash_id };
-	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = reftable_table_print(&table);
-done:
-	if (stack)
-		reftable_stack_destroy(stack);
-	return err;
-}
reftable/stack_test.c +0 -7
index dbca9eaf4a..42044ed8a3 100644
@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)
 	EXPECT(0 == strcmp("master", dest.value.symref));
 	EXPECT(st->readers_len > 0);
 
-	printf("testing print functionality:\n");
-	err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
-
-	err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
-
 #ifndef GIT_WINDOWS_NATIVE
 	strbuf_addstr(&scratch, dir);
 	strbuf_addstr(&scratch, "/tables.list");
t/helper/test-reftable.c +22 -1
index 19367c25f9..db62ea8dc3 100644
@@ -1,6 +1,7 @@
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
 #include "reftable/reftable-tests.h"
@@ -29,6 +30,26 @@ static void print_help(void)
 	       "\n");
 }
 
+static int dump_stack(const char *stackdir, uint32_t hash_id)
+{
+	struct reftable_stack *stack = NULL;
+	struct reftable_write_options opts = { .hash_id = hash_id };
+	struct reftable_merged_table *merged = NULL;
+	struct reftable_table table = { NULL };
+
+	int err = reftable_new_stack(&stack, stackdir, &opts);
+	if (err < 0)
+		goto done;
+
+	merged = reftable_stack_merged_table(stack);
+	reftable_table_from_merged_table(&table, merged);
+	err = reftable_table_print(&table);
+done:
+	if (stack)
+		reftable_stack_destroy(stack);
+	return err;
+}
+
 static int dump_reftable(const char *tablename)
 {
 	struct reftable_block_source src = { NULL };
@@ -87,7 +108,7 @@ int cmd__dump_reftable(int argc, const char **argv)
 	} else if (opt_dump_table) {
 		err = dump_reftable(arg);
 	} else if (opt_dump_stack) {
-		err = reftable_stack_print_directory(arg, opt_hash_id);
+		err = dump_stack(arg, opt_hash_id);
 	}
 
 	if (err < 0) {
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#13 lore raw

PATCH v2 11/15 t/helper: inline `reftable_table_print()`

In reply to Patrick Steinhardt

Move reftable_table_print() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/generic.c          | 47 ---------------------------------
 reftable/reftable-generic.h |  3 ---
 t/helper/test-reftable.c    | 52 +++++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 52 deletions(-)
reftable/generic.c +0 -47
index 6ecf9b880f..495ee9af6b 100644
@@ -61,53 +61,6 @@ int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 	return err;
 }
 
-int reftable_table_print(struct reftable_table *tab) {
-	struct reftable_iterator it = { NULL };
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_log_record log = { NULL };
-	uint32_t hash_id = reftable_table_hash_id(tab);
-	int err;
-
-	reftable_table_init_ref_iter(tab, &it);
-
-	err = reftable_iterator_seek_ref(&it, "");
-	if (err < 0)
-		return err;
-
-	while (1) {
-		err = reftable_iterator_next_ref(&it, &ref);
-		if (err > 0) {
-			break;
-		}
-		if (err < 0) {
-			return err;
-		}
-		reftable_ref_record_print(&ref, hash_id);
-	}
-	reftable_iterator_destroy(&it);
-	reftable_ref_record_release(&ref);
-
-	reftable_table_init_log_iter(tab, &it);
-
-	err = reftable_iterator_seek_log(&it, "");
-	if (err < 0)
-		return err;
-
-	while (1) {
-		err = reftable_iterator_next_log(&it, &log);
-		if (err > 0) {
-			break;
-		}
-		if (err < 0) {
-			return err;
-		}
-		reftable_log_record_print(&log, hash_id);
-	}
-	reftable_iterator_destroy(&it);
-	reftable_log_record_release(&log);
-	return 0;
-}
-
 uint64_t reftable_table_max_update_index(struct reftable_table *tab)
 {
 	return tab->ops->max_update_index(tab->table_arg);
reftable/reftable-generic.h +0 -3
index 65670ea093..b8b1323a33 100644
@@ -41,7 +41,4 @@ uint64_t reftable_table_min_update_index(struct reftable_table *tab);
 int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 			    struct reftable_ref_record *ref);
 
-/* dump table contents onto stdout for debugging */
-int reftable_table_print(struct reftable_table *tab);
-
 #endif
t/helper/test-reftable.c +50 -2

70 lines not on this page · raw message

Signature · 1 line
2.46.0.46.g406f326d27.dirty
#14 lore raw

PATCH v2 12/15 t/helper: inline printing of reftable records

In reply to Patrick Steinhardt

Move printing of reftable records into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/record.c          | 127 -------------------------------------
 reftable/record.h          |   1 -
 reftable/reftable-record.h |   8 ---
 t/helper/test-reftable.c   |  77 +++++++++++++++++++---
 4 files changed, 69 insertions(+), 144 deletions(-)
reftable/record.c +0 -127
index a2cba5ef74..e26bd4bc8d 100644
@@ -259,58 +259,6 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
 	}
 }
 
-static char hexdigit(int c)
-{
-	if (c <= 9)
-		return '0' + c;
-	return 'a' + (c - 10);
-}
-
-static void hex_format(char *dest, const unsigned char *src, int hash_size)
-{
-	assert(hash_size > 0);
-	if (src) {
-		int i = 0;
-		for (i = 0; i < hash_size; i++) {
-			dest[2 * i] = hexdigit(src[i] >> 4);
-			dest[2 * i + 1] = hexdigit(src[i] & 0xf);
-		}
-		dest[2 * hash_size] = 0;
-	}
-}
-
-static void reftable_ref_record_print_sz(const struct reftable_ref_record *ref,
-					 int hash_size)
-{
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
-	printf("ref{%s(%" PRIu64 ") ", ref->refname, ref->update_index);
-	switch (ref->value_type) {
-	case REFTABLE_REF_SYMREF:
-		printf("=> %s", ref->value.symref);
-		break;
-	case REFTABLE_REF_VAL2:
-		hex_format(hex, ref->value.val2.value, hash_size);
-		printf("val 2 %s", hex);
-		hex_format(hex, ref->value.val2.target_value,
-			   hash_size);
-		printf("(T %s)", hex);
-		break;
-	case REFTABLE_REF_VAL1:
-		hex_format(hex, ref->value.val1, hash_size);
-		printf("val 1 %s", hex);
-		break;
-	case REFTABLE_REF_DELETION:
-		printf("delete");
-		break;
-	}
-	printf("}\n");
-}
-
-void reftable_ref_record_print(const struct reftable_ref_record *ref,
-			       uint32_t hash_id) {
-	reftable_ref_record_print_sz(ref, hash_size(hash_id));
-}
-
 static void reftable_ref_record_release_void(void *rec)
 {
 	reftable_ref_record_release(rec);
@@ -480,12 +428,6 @@ static int reftable_ref_record_cmp_void(const void *_a, const void *_b)
 	return strcmp(a->refname, b->refname);
 }
 
-static void reftable_ref_record_print_void(const void *rec,
-					   int hash_size)
-{
-	reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size);
-}
-
 static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.key = &reftable_ref_record_key,
 	.type = BLOCK_TYPE_REF,
@@ -497,7 +439,6 @@ static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.is_deletion = &reftable_ref_record_is_deletion_void,
 	.equal = &reftable_ref_record_equal_void,
 	.cmp = &reftable_ref_record_cmp_void,
-	.print = &reftable_ref_record_print_void,
 };
 
 static void reftable_obj_record_key(const void *r, struct strbuf *dest)
@@ -516,21 +457,6 @@ static void reftable_obj_record_release(void *rec)
 	memset(obj, 0, sizeof(struct reftable_obj_record));
 }
 
-static void reftable_obj_record_print(const void *rec, int hash_size)
-{
-	const struct reftable_obj_record *obj = rec;
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-	struct strbuf offset_str = STRBUF_INIT;
-	int i;
-
-	for (i = 0; i < obj->offset_len; i++)
-		strbuf_addf(&offset_str, "%" PRIu64 " ", obj->offsets[i]);
-	hex_format(hex, obj->hash_prefix, obj->hash_prefix_len);
-	printf("prefix %s (len %d), offsets [%s]\n",
-	       hex, obj->hash_prefix_len, offset_str.buf);
-	strbuf_release(&offset_str);
-}
-
 static void reftable_obj_record_copy_from(void *rec, const void *src_rec,
 					  int hash_size)
 {
@@ -701,41 +627,8 @@ static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_obj_record_equal_void,
 	.cmp = &reftable_obj_record_cmp_void,
-	.print = &reftable_obj_record_print,
 };
 
-static void reftable_log_record_print_sz(struct reftable_log_record *log,
-					 int hash_size)
-{
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-
-	switch (log->value_type) {
-	case REFTABLE_LOG_DELETION:
-		printf("log{%s(%" PRIu64 ") delete\n", log->refname,
-		       log->update_index);
-		break;
-	case REFTABLE_LOG_UPDATE:
-		printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
-		       log->refname, log->update_index,
-		       log->value.update.name ? log->value.update.name : "",
-		       log->value.update.email ? log->value.update.email : "",
-		       log->value.update.time,
-		       log->value.update.tz_offset);
-		hex_format(hex, log->value.update.old_hash, hash_size);
-		printf("%s => ", hex);
-		hex_format(hex, log->value.update.new_hash, hash_size);
-		printf("%s\n\n%s\n}\n", hex,
-		       log->value.update.message ? log->value.update.message : "");
-		break;
-	}
-}
-
-void reftable_log_record_print(struct reftable_log_record *log,
-				      uint32_t hash_id)
-{
-	reftable_log_record_print_sz(log, hash_size(hash_id));
-}
-
 static void reftable_log_record_key(const void *r, struct strbuf *dest)
 {
 	const struct reftable_log_record *rec =
@@ -1039,11 +932,6 @@ static int reftable_log_record_is_deletion_void(const void *p)
 		(const struct reftable_log_record *)p);
 }
 
-static void reftable_log_record_print_void(const void *rec, int hash_size)
-{
-	reftable_log_record_print_sz((struct reftable_log_record*)rec, hash_size);
-}
-
 static struct reftable_record_vtable reftable_log_record_vtable = {
 	.key = &reftable_log_record_key,
 	.type = BLOCK_TYPE_LOG,
@@ -1055,7 +943,6 @@ static struct reftable_record_vtable reftable_log_record_vtable = {
 	.is_deletion = &reftable_log_record_is_deletion_void,
 	.equal = &reftable_log_record_equal_void,
 	.cmp = &reftable_log_record_cmp_void,
-	.print = &reftable_log_record_print_void,
 };
 
 static void reftable_index_record_key(const void *r, struct strbuf *dest)
@@ -1137,13 +1024,6 @@ static int reftable_index_record_cmp(const void *_a, const void *_b)
 	return strbuf_cmp(&a->last_key, &b->last_key);
 }
 
-static void reftable_index_record_print(const void *rec, int hash_size)
-{
-	const struct reftable_index_record *idx = rec;
-	/* TODO: escape null chars? */
-	printf("\"%s\" %" PRIu64 "\n", idx->last_key.buf, idx->offset);
-}
-
 static struct reftable_record_vtable reftable_index_record_vtable = {
 	.key = &reftable_index_record_key,
 	.type = BLOCK_TYPE_INDEX,
@@ -1155,7 +1035,6 @@ static struct reftable_record_vtable reftable_index_record_vtable = {
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_index_record_equal,
 	.cmp = &reftable_index_record_cmp,
-	.print = &reftable_index_record_print,
 };
 
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
@@ -1334,9 +1213,3 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ)
 		BUG("unhandled record type");
 	}
 }
-
-void reftable_record_print(struct reftable_record *rec, int hash_size)
-{
-	printf("'%c': ", rec->type);
-	reftable_record_vtable(rec)->print(reftable_record_data(rec), hash_size);
-}
reftable/record.h +0 -1
index d778133e6e..5003bacdb0 100644
@@ -136,7 +136,6 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);
 /* see struct record_vtable */
 int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);
 int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
-void reftable_record_print(struct reftable_record *rec, int hash_size);
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest);
 void reftable_record_copy_from(struct reftable_record *rec,
 			       struct reftable_record *src, int hash_size);
reftable/reftable-record.h +0 -8
index ff486eb1f7..2d42463c58 100644
@@ -60,10 +60,6 @@ const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *
 /* returns whether 'ref' represents a deletion */
 int reftable_ref_record_is_deletion(const struct reftable_ref_record *ref);
 
-/* prints a reftable_ref_record onto stdout. Useful for debugging. */
-void reftable_ref_record_print(const struct reftable_ref_record *ref,
-			       uint32_t hash_id);
-
 /* frees and nulls all pointer values inside `ref`. */
 void reftable_ref_record_release(struct reftable_ref_record *ref);
 
@@ -111,8 +107,4 @@ void reftable_log_record_release(struct reftable_log_record *log);
 int reftable_log_record_equal(const struct reftable_log_record *a,
 			      const struct reftable_log_record *b, int hash_size);
 
-/* dumps a reftable_log_record on stdout, for debugging/testing. */
-void reftable_log_record_print(struct reftable_log_record *log,
-			       uint32_t hash_id);
-
 #endif
t/helper/test-reftable.c +69 -8

109 lines not on this page · raw message

Signature · 1 line
2.46.0.46.g406f326d27.dirty
#15 lore raw

PATCH v2 13/15 t/helper: use `hash_to_hex_algop()` to print hashes

In reply to Patrick Steinhardt

The "reftable" test helper uses a hand-crafted version to convert from a raw hash to its hex variant. This was done because this code used to be part of the reftable library, where we do not use most functions from the Git core.

Now that the code is integrated into the "dump-reftable" helper though, that limitation went away. Let's thus use hash_to_hex_algop() instead.

---
 t/helper/test-reftable.c | 48 +++++++++-------------------------------
 1 file changed, 11 insertions(+), 37 deletions(-)
t/helper/test-reftable.c +11 -37
index cb22d7537a..234fb80010 100644
@@ -1,3 +1,6 @@
+#include "git-compat-util.h"
+#include "hash.h"
+#include "hex.h"
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
@@ -30,33 +33,12 @@ static void print_help(void)
 	       "\n");
 }
 
-static char hexdigit(int c)
-{
-	if (c <= 9)
-		return '0' + c;
-	return 'a' + (c - 10);
-}
-
-static void hex_format(char *dest, const unsigned char *src, int hash_size)
-{
-	assert(hash_size > 0);
-	if (src) {
-		int i = 0;
-		for (i = 0; i < hash_size; i++) {
-			dest[2 * i] = hexdigit(src[i] >> 4);
-			dest[2 * i + 1] = hexdigit(src[i] & 0xf);
-		}
-		dest[2 * hash_size] = 0;
-	}
-}
-
 static int dump_table(struct reftable_table *tab)
 {
 	struct reftable_iterator it = { NULL };
 	struct reftable_ref_record ref = { NULL };
 	struct reftable_log_record log = { NULL };
-	uint32_t hash_id = reftable_table_hash_id(tab);
-	int hash_len = hash_size(hash_id);
+	const struct git_hash_algo *algop;
 	int err;
 
 	reftable_table_init_ref_iter(tab, &it);
@@ -65,9 +47,9 @@ static int dump_table(struct reftable_table *tab)
 	if (err < 0)
 		return err;
 
-	while (1) {
-		char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
+	algop = &hash_algos[hash_algo_by_id(reftable_table_hash_id(tab))];
 
+	while (1) {
 		err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -80,15 +62,11 @@ static int dump_table(struct reftable_table *tab)
 			printf("=> %s", ref.value.symref);
 			break;
 		case REFTABLE_REF_VAL2:
-			hex_format(hex, ref.value.val2.value, hash_len);
-			printf("val 2 %s", hex);
-			hex_format(hex, ref.value.val2.target_value,
-				   hash_len);
-			printf("(T %s)", hex);
+			printf("val 2 %s", hash_to_hex_algop(ref.value.val2.value, algop));
+			printf("(T %s)", hash_to_hex_algop(ref.value.val2.target_value, algop));
 			break;
 		case REFTABLE_REF_VAL1:
-			hex_format(hex, ref.value.val1, hash_len);
-			printf("val 1 %s", hex);
+			printf("val 1 %s", hash_to_hex_algop(ref.value.val1, algop));
 			break;
 		case REFTABLE_REF_DELETION:
 			printf("delete");
@@ -106,8 +84,6 @@ static int dump_table(struct reftable_table *tab)
 		return err;
 
 	while (1) {
-		char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-
 		err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -126,10 +102,8 @@ static int dump_table(struct reftable_table *tab)
 			       log.value.update.email ? log.value.update.email : "",
 			       log.value.update.time,
 			       log.value.update.tz_offset);
-			hex_format(hex, log.value.update.old_hash, hash_len);
-			printf("%s => ", hex);
-			hex_format(hex, log.value.update.new_hash, hash_len);
-			printf("%s\n\n%s\n}\n", hex,
+			printf("%s => ", hash_to_hex_algop(log.value.update.old_hash, algop));
+			printf("%s\n\n%s\n}\n", hash_to_hex_algop(log.value.update.new_hash, algop),
 			       log.value.update.message ? log.value.update.message : "");
 			break;
 		}
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#16 lore raw

PATCH v2 14/15 t/helper: refactor to not use `struct reftable_table`

In reply to Patrick Steinhardt

The struct reftable_table interface in our "reftable" test helper gets used such that we can easily print either a single table, or a merged stack. This generic interface is about to go away.

Prepare the code for this change by using merged tables instead. When printing the stack we've already got one. When using a single table, we can create a merged table from it to adapt.

This removes the last user of the generic interface.

---
 t/helper/test-reftable.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
t/helper/test-reftable.c +18 -15
index 234fb80010..c1942156b5 100644
@@ -3,7 +3,6 @@
 #include "hex.h"
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
-#include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
@@ -33,7 +32,7 @@ static void print_help(void)
 	       "\n");
 }
 
-static int dump_table(struct reftable_table *tab)
+static int dump_table(struct reftable_merged_table *mt)
 {
 	struct reftable_iterator it = { NULL };
 	struct reftable_ref_record ref = { NULL };
@@ -41,13 +40,12 @@ static int dump_table(struct reftable_table *tab)
 	const struct git_hash_algo *algop;
 	int err;
 
-	reftable_table_init_ref_iter(tab, &it);
-
+	reftable_merged_table_init_ref_iterator(mt, &it);
 	err = reftable_iterator_seek_ref(&it, "");
 	if (err < 0)
 		return err;
 
-	algop = &hash_algos[hash_algo_by_id(reftable_table_hash_id(tab))];
+	algop = &hash_algos[hash_algo_by_id(reftable_merged_table_hash_id(mt))];
 
 	while (1) {
 		err = reftable_iterator_next_ref(&it, &ref);
@@ -77,8 +75,7 @@ static int dump_table(struct reftable_table *tab)
 	reftable_iterator_destroy(&it);
 	reftable_ref_record_release(&ref);
 
-	reftable_table_init_log_iter(tab, &it);
-
+	reftable_merged_table_init_log_iterator(mt, &it);
 	err = reftable_iterator_seek_log(&it, "");
 	if (err < 0)
 		return err;
@@ -118,15 +115,13 @@ static int dump_stack(const char *stackdir, uint32_t hash_id)
 	struct reftable_stack *stack = NULL;
 	struct reftable_write_options opts = { .hash_id = hash_id };
 	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
 
 	int err = reftable_new_stack(&stack, stackdir, &opts);
 	if (err < 0)
 		goto done;
 
 	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = dump_table(&table);
+	err = dump_table(merged);
 done:
 	if (stack)
 		reftable_stack_destroy(stack);
@@ -135,10 +130,12 @@ static int dump_stack(const char *stackdir, uint32_t hash_id)
 
 static int dump_reftable(const char *tablename)
 {
-	struct reftable_block_source src = { NULL };
-	int err = reftable_block_source_from_file(&src, tablename);
+	struct reftable_block_source src = { 0 };
+	struct reftable_merged_table *mt = NULL;
 	struct reftable_reader *r = NULL;
-	struct reftable_table tab = { NULL };
+	int err;
+
+	err = reftable_block_source_from_file(&src, tablename);
 	if (err < 0)
 		goto done;
 
@@ -146,9 +143,15 @@ static int dump_reftable(const char *tablename)
 	if (err < 0)
 		goto done;
 
-	reftable_table_from_reader(&tab, r);
-	err = dump_table(&tab);
+	err = reftable_merged_table_new(&mt, &r, 1,
+					reftable_reader_hash_id(r));
+	if (err < 0)
+		goto done;
+
+	err = dump_table(mt);
+
 done:
+	reftable_merged_table_free(mt);
 	reftable_reader_free(r);
 	return err;
 }
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#17 lore raw

PATCH v2 15/15 reftable/generic: drop interface

In reply to Patrick Steinhardt

The reftable_table interface provides a generic infrastructure that can abstract away whether the underlying table is a single table, or a merged table. This abstraction can make it rather hard to reason about the code. We didn't ever use it to implement the reftable backend, and with the preceding patches in this patch series we in fact don't use it at all anymore. Furthermore, it became somewhat useless with the recent refactorings that made it possible to seek reftable iterators multiple times, as these now provide generic access to tables for us. The interface is thus redundant and only brings unnecessary complexity with it.

Remove the struct reftable_table interface and its associated functions.

---
 Makefile                         |  1 -
 reftable/generic.c               | 77 --------------------------------
 reftable/generic.h               | 27 -----------
 reftable/iter.c                  |  1 -
 reftable/iter.h                  |  1 -
 reftable/merged.c                | 38 ----------------
 reftable/reader.c                | 41 -----------------
 reftable/reftable-generic.h      | 44 ------------------
 reftable/reftable-merged.h       |  6 ---
 reftable/reftable-reader.h       |  7 ---
 reftable/stack.c                 |  1 -
 t/unit-tests/t-reftable-merged.c |  1 -
 12 files changed, 245 deletions(-)
 delete mode 100644 reftable/generic.c
 delete mode 100644 reftable/generic.h
 delete mode 100644 reftable/reftable-generic.h
Makefile +0 -1
index 343f19a488..41dfa0bad2 100644
@@ -2674,7 +2674,6 @@ REFTABLE_OBJS += reftable/merged.o
 REFTABLE_OBJS += reftable/pq.o
 REFTABLE_OBJS += reftable/reader.o
 REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/generic.o
 REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
reftable/generic.c deleted +0 -77

77 lines not on this page · raw message

reftable/generic.h deleted +0 -27
deleted file mode 100644
index 837fbb8df2..0000000000
@@ -1,27 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#ifndef GENERIC_H
-#define GENERIC_H
-
-#include "record.h"
-#include "reftable-generic.h"
-
-/* generic interface to reftables */
-struct reftable_table_vtable {
-	void (*init_iter)(void *tab, struct reftable_iterator *it, uint8_t typ);
-	uint32_t (*hash_id)(void *tab);
-	uint64_t (*min_update_index)(void *tab);
-	uint64_t (*max_update_index)(void *tab);
-};
-
-void table_init_iter(struct reftable_table *tab,
-		     struct reftable_iterator *it,
-		     uint8_t typ);
-
-#endif
reftable/iter.c +0 -1
index 225feb7871..97a4642ed5 100644
@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at
 #include "system.h"
 
 #include "block.h"
-#include "generic.h"
 #include "constants.h"
 #include "reader.h"
 #include "reftable-error.h"
reftable/iter.h +0 -1
index 3b401f1259..befc4597df 100644
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at
 #include "record.h"
 
 #include "reftable-iterator.h"
-#include "reftable-generic.h"
 
 /*
  * The virtual function table for implementing generic reftable iterators.
reftable/merged.c +0 -38
index 2e72eab306..128a810c55 100644
@@ -13,7 +13,6 @@ license that can be found in the LICENSE file or at
 #include "pq.h"
 #include "reader.h"
 #include "record.h"
-#include "generic.h"
 #include "reftable-merged.h"
 #include "reftable-error.h"
 #include "system.h"
@@ -270,40 +269,3 @@ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)
 {
 	return mt->hash_id;
 }
-
-static void reftable_merged_table_init_iter_void(void *tab,
-						 struct reftable_iterator *it,
-						 uint8_t typ)
-{
-	merged_table_init_iter(tab, it, typ);
-}
-
-static uint32_t reftable_merged_table_hash_id_void(void *tab)
-{
-	return reftable_merged_table_hash_id(tab);
-}
-
-static uint64_t reftable_merged_table_min_update_index_void(void *tab)
-{
-	return reftable_merged_table_min_update_index(tab);
-}
-
-static uint64_t reftable_merged_table_max_update_index_void(void *tab)
-{
-	return reftable_merged_table_max_update_index(tab);
-}
-
-static struct reftable_table_vtable merged_table_vtable = {
-	.init_iter = reftable_merged_table_init_iter_void,
-	.hash_id = reftable_merged_table_hash_id_void,
-	.min_update_index = reftable_merged_table_min_update_index_void,
-	.max_update_index = reftable_merged_table_max_update_index_void,
-};
-
-void reftable_table_from_merged_table(struct reftable_table *tab,
-				      struct reftable_merged_table *merged)
-{
-	assert(!tab->ops);
-	tab->ops = &merged_table_vtable;
-	tab->table_arg = merged;
-}
reftable/reader.c +0 -41

56 lines not on this page · raw message

reftable/reftable-generic.h deleted +0 -44

44 lines not on this page · raw message

reftable/reftable-merged.h +0 -6
index 03c2619c0f..16d19f8df2 100644
@@ -26,8 +26,6 @@ license that can be found in the LICENSE file or at
 /* A merged table is implements seeking/iterating over a stack of tables. */
 struct reftable_merged_table;
 
-/* A generic reftable; see below. */
-struct reftable_table;
 struct reftable_reader;
 
 /*
@@ -60,8 +58,4 @@ void reftable_merged_table_free(struct reftable_merged_table *m);
 /* return the hash ID of the merged table. */
 uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *m);
 
-/* create a generic table from reftable_merged_table */
-void reftable_table_from_merged_table(struct reftable_table *tab,
-				      struct reftable_merged_table *table);
-
 #endif
reftable/reftable-reader.h +0 -7
index 7c7d171651..69621c5b0f 100644
@@ -23,9 +23,6 @@
 /* The reader struct is a handle to an open reftable file. */
 struct reftable_reader;
 
-/* Generic table. */
-struct reftable_table;
-
 /* reftable_new_reader opens a reftable for reading. If successful,
  * returns 0 code and sets pp. The name is used for creating a
  * stack. Typically, it is the basename of the file. The block source
@@ -60,10 +57,6 @@ uint64_t reftable_reader_max_update_index(struct reftable_reader *r);
 /* return the min_update_index for a table */
 uint64_t reftable_reader_min_update_index(struct reftable_reader *r);
 
-/* creates a generic table from a file reader. */
-void reftable_table_from_reader(struct reftable_table *tab,
-				struct reftable_reader *reader);
-
 /* print blocks onto stdout for debugging. */
 int reftable_reader_print_blocks(const char *tablename);
 
reftable/stack.c +0 -1
index bedd503e7e..d3a95d2f1d 100644
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at
 #include "merged.h"
 #include "reader.h"
 #include "reftable-error.h"
-#include "reftable-generic.h"
 #include "reftable-record.h"
 #include "reftable-merged.h"
 #include "writer.h"
t/unit-tests/t-reftable-merged.c +0 -1
index 577b1a5be8..93345c6c8b 100644
@@ -12,7 +12,6 @@ license that can be found in the LICENSE file or at
 #include "reftable/merged.h"
 #include "reftable/reader.h"
 #include "reftable/reftable-error.h"
-#include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
 
Signature · 1 line
2.46.0.46.g406f326d27.dirty
#18 lore raw
In reply to Patrick Steinhardt
Quoted 11 lines · Patrick Steinhardt

Hi,

this is the second version of my patch series that gets rid of the generic reftable_table interface. It made it way harder to understand the reftable code base and is not really required nowadays anymore where we have generic re-seekable reftable iterators.

Changes compared to v1:

- Fix some commit message typos.

Quoted 8 lines

- Remove some braces while at it.

- Drop the "-c" switch from the test-tool's help output.

- Restore printing-related functionality, but move all of it into the test-helper. It has no reason to exist in the reftable library.

Thanks!

Oops, forgot to link this to v1, sorry! You can find it at [1].

Patrick

[1]: https://lore.kernel.org/git/[email protected]/

#20 lore raw
Quoted 9 lines · Junio C Hamano
Quoted 5 lines · Patrick Steinhardt

Oops, forgot to link this to v1, sorry! You can find it at [1].

Patrick

[1]: https://lore.kernel.org/git/[email protected]/

And on what commit are these patches supposed to build on?

Whenever I was out of office I had the nagging feeling that I forgot to mention the base for one of my patch series, but always forgot to double check when I returned. Seems like that feeling was right after all, sorry!

This builds on top of 25673b1c47 (The third batch, 2024-08-07) with ps/reftable-stack-compaction at f234df07f6 (reftable/stack: handle locked tables during auto-compaction, 2024-08-08) merged into it.

Patrick

#11 lore raw

Re: [PATCH v2 10/15] t/helper: inline `reftable_stack_print_directory()`

In reply to Patrick Steinhardt
Quoted 57 lines · Patrick Steinhardt

Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/reftable-stack.h |  3 ---
 reftable/stack.c          | 20 --------------------
 reftable/stack_test.c     |  7 -------
 t/helper/test-reftable.c  | 23 ++++++++++++++++++++++-
 4 files changed, 22 insertions(+), 31 deletions(-)
reftable/reftable-stack.h +0 -3
index 09e97c9991..f4f8cabc7f 100644
@@ -140,7 +140,4 @@ struct reftable_compaction_stats {
 struct reftable_compaction_stats *
 reftable_stack_compaction_stats(struct reftable_stack *st);
 
-/* print the entire stack represented by the directory */
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);
-
 #endif
reftable/stack.c +0 -20
index d08ec00959..bedd503e7e 100644
@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)
 	reftable_addition_destroy(add);
 	return err;
 }
-
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { .hash_id = hash_id };
-	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = reftable_table_print(&table);
-done:
-	if (stack)
-		reftable_stack_destroy(stack);
-	return err;
-}
reftable/stack_test.c +0 -0
index dbca9eaf4a..42044ed8a3 100644
@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)
 	EXPECT(0 == strcmp("master", dest.value.symref));

reftable/stack_test.c static void test_reftable_stack_add_one(void) · line 182

Quoted 10 lines
reftable/stack_test.c +0 -7
 	EXPECT(st->readers_len > 0);
 
-	printf("testing print functionality:\n");
-	err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
-
-	err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
-
 

We loose this test due to the movement. It is okay, because the code that it is testing, is now only available in the testing section and is a test-helper. But it would be nice to mention this in the commit message.

[snip]

#21 lore raw
In reply to Patrick Steinhardt
Quoted 13 lines · Patrick Steinhardt

Hi,

this is the second version of my patch series that gets rid of the generic reftable_table interface. It made it way harder to understand the reftable code base and is not really required nowadays anymore where we have generic re-seekable reftable iterators.

Changes compared to v1:

- Fix some commit message typos.

- Remove some braces while at it.

Quoted 9 lines

- Drop the "-c" switch from the test-tool's help output.

- Restore printing-related functionality, but move all of it into the test-helper. It has no reason to exist in the reftable library.

Thanks!

Patrick

This version looks inline with the review I did on the previous version. It looks great to me. Thanks

[snip]

#12 lore raw

Re: [PATCH v2 10/15] t/helper: inline `reftable_stack_print_directory()`

In reply to karthik nayak
Quoted 61 lines · karthik nayak
Quoted 59 lines · Patrick Steinhardt

Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/reftable-stack.h |  3 ---
 reftable/stack.c          | 20 --------------------
 reftable/stack_test.c     |  7 -------
 t/helper/test-reftable.c  | 23 ++++++++++++++++++++++-
 4 files changed, 22 insertions(+), 31 deletions(-)
reftable/reftable-stack.h +0 -3
index 09e97c9991..f4f8cabc7f 100644
@@ -140,7 +140,4 @@ struct reftable_compaction_stats {
 struct reftable_compaction_stats *
 reftable_stack_compaction_stats(struct reftable_stack *st);
 
-/* print the entire stack represented by the directory */
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);
-
 #endif
reftable/stack.c +0 -20
index d08ec00959..bedd503e7e 100644
@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)
 	reftable_addition_destroy(add);
 	return err;
 }
-
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { .hash_id = hash_id };
-	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = reftable_table_print(&table);
-done:
-	if (stack)
-		reftable_stack_destroy(stack);
-	return err;
-}
reftable/stack_test.c +0 -0
index dbca9eaf4a..42044ed8a3 100644
@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)
 	EXPECT(0 == strcmp("master", dest.value.symref));
 	EXPECT(st->readers_len > 0);
 
Quoted 13 lines

reftable/stack_test.c static void test_reftable_stack_add_one(void)

Quoted 8 lines
reftable/stack_test.c +0 -7
-	printf("testing print functionality:\n");
-	err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
-
-	err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
-
 

We loose this test due to the movement. It is okay, because the code that it is testing, is now only available in the testing section and is a test-helper. But it would be nice to mention this in the commit message.

Ah, good point, I'll mention that. The test was basically worthless anyway as we didn't very anything -- only that it doesn't crash.

Patrick

#22 lore raw

v3 cover reftable: drop generic `reftable_table` interface

In reply to Patrick Steinhardt

Hi,

this is the third version of my patch series that gets rid of the generic reftable_table interface. It made it way harder to understand the reftable code base and is not really required nowadays anymore where we have generic re-seekable reftable iterators.

There is only a single change compared to v2, which updates one of the commit messages to explain why it is fine to drop tests for the printing functionality.

This patch series continues to build on top of 25673b1c47 (The third batch, 2024-08-07) with Junio's ps/reftable-stack-compaction at f234df07f6 (reftable/stack: handle locked tables during auto-compaction, 2024-08-08) merged into it.

Thanks!

Patrick

Patrick Steinhardt (15):
reftable/merged: expose functions to initialize iterators reftable/merged: rename reftable_new_merged_table() reftable/merged: stop using generic tables in the merged table reftable/stack: open-code reading refs
reftable/iter: drop double-checking logic reftable/generic: move generic iterator code into iterator interface reftable/dump: drop unused compact_stack()
t/helper: inline reftable_dump_main()
t/helper: inline reftable_reader_print_file()
t/helper: inline reftable_stack_print_directory() t/helper: inline reftable_table_print()
t/helper: inline printing of reftable records
t/helper: use hash_to_hex_algop() to print hashes t/helper: refactor to not use struct reftable_table reftable/generic: drop interface

 Makefile                         |   2 -
 reftable/dump.c                  | 111 ---------------
 reftable/generic.c               | 229 -------------------------------
 reftable/generic.h               |  37 -----
 reftable/iter.c                  | 126 ++++++++++++++---
 reftable/iter.h                  |  30 +++-
 reftable/merged.c                |  72 ++++------
 reftable/merged.h                |   4 +-
 reftable/reader.c                |  70 +---------
 reftable/reader.h                |   4 +
 reftable/record.c                | 127 -----------------
 reftable/record.h                |   1 -
 reftable/reftable-generic.h      |  47 -------
 reftable/reftable-merged.h       |  26 ++--
 reftable/reftable-reader.h       |   9 --
 reftable/reftable-record.h       |   8 --
 reftable/reftable-stack.h        |   3 -
 reftable/reftable-tests.h        |   1 -
 reftable/stack.c                 |  94 ++++++-------
 reftable/stack_test.c            |  29 ++--
 t/helper/test-reftable.c         | 189 ++++++++++++++++++++++++-
 t/unit-tests/t-reftable-merged.c |  17 +--
 22 files changed, 422 insertions(+), 814 deletions(-)
 delete mode 100644 reftable/dump.c
 delete mode 100644 reftable/generic.c
 delete mode 100644 reftable/generic.h
 delete mode 100644 reftable/reftable-generic.h

Range-diff against v2:

 1:  472c169b501 =  1:  472c169b501 reftable/merged: expose functions to initialize iterators
 2:  bc6f1cd8c1b =  2:  bc6f1cd8c1b reftable/merged: rename `reftable_new_merged_table()`
 3:  58e91ab4b34 =  3:  58e91ab4b34 reftable/merged: stop using generic tables in the merged table
 4:  6ba3fcee411 =  4:  6ba3fcee411 reftable/stack: open-code reading refs
 5:  cac08a934c5 =  5:  cac08a934c5 reftable/iter: drop double-checking logic
 6:  103262dc79c =  6:  103262dc79c reftable/generic: move generic iterator code into iterator interface
 7:  4011fa65d81 =  7:  4011fa65d81 reftable/dump: drop unused `compact_stack()`
 8:  ceaa296bfd4 =  8:  ceaa296bfd4 t/helper: inline `reftable_dump_main()`
 9:  a62e4612e97 =  9:  a62e4612e97 t/helper: inline `reftable_reader_print_file()`
10:  7acfe4fecc5 ! 10:  242c179df5f t/helper: inline `reftable_stack_print_directory()`

@@ Commit message
Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

+ Note that this requires us to remove the tests for this functionality in
+ reftable/stack_test.c. The test does not really add much anyway,
+ because all it verifies is that we do not crash or run into an error,
+ and it specifically doesn't check the outputted data. Also, as the code
+ is now part of the test helper, it doesn't make much sense to have a
+ unit test for it in the first place.
+

## reftable/reftable-stack.h ##

11:  8bd53a1a656 = 11:  a05e2060996 t/helper: inline `reftable_table_print()`
12:  c50aabbb804 = 12:  ee22a08e11e t/helper: inline printing of reftable records
13:  5498395872c = 13:  0a3c619e842 t/helper: use `hash_to_hex_algop()` to print hashes
14:  5390be75c37 = 14:  8eab399dfc6 t/helper: refactor to not use `struct reftable_table`
15:  5aeab8ee077 = 15:  b5d7b5679b5 reftable/generic: drop interface
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#23 lore raw

PATCH v3 1/15 reftable/merged: expose functions to initialize iterators

We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic reftable_table interface, which is somewhat roundabout.

Implement two new functions to initialize iterators for ref and log records to plug this gap.

---
 reftable/merged.c          | 12 ++++++++++++
 reftable/reftable-merged.h |  8 ++++++++
 2 files changed, 20 insertions(+)
reftable/merged.c +12 -0
index 6adce44f4b6..8d78b3da719 100644
@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,
 	iterator_from_merged_iter(it, mi);
 }
 
+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it)
+{
+	merged_table_init_iter(mt, it, BLOCK_TYPE_REF);
+}
+
+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it)
+{
+	merged_table_init_iter(mt, it, BLOCK_TYPE_LOG);
+}
+
 uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)
 {
 	return mt->hash_id;
reftable/reftable-merged.h +8 -0
index 14d5fc9f05c..4deb0ad22e1 100644
@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id);
 
+/* Initialize a merged table iterator for reading refs. */
+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it);
+
+/* Initialize a merged table iterator for reading logs. */
+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,
+					     struct reftable_iterator *it);
+
 /* returns the max update_index covered by this merged table. */
 uint64_t
 reftable_merged_table_max_update_index(struct reftable_merged_table *mt);
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#24 lore raw

PATCH v3 2/15 reftable/merged: rename `reftable_new_merged_table()`

In reply to Patrick Steinhardt

Rename reftable_new_merged_table() to reftable_merged_table_new() such that the name matches our coding style.

---
 reftable/merged.c                | 2 +-
 reftable/reftable-merged.h       | 9 +++++----
 reftable/stack.c                 | 4 ++--
 t/unit-tests/t-reftable-merged.c | 8 ++++----
 4 files changed, 12 insertions(+), 11 deletions(-)
reftable/merged.c +1 -1
index 8d78b3da719..25d414ec415 100644
@@ -192,7 +192,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
 	it->ops = &merged_iter_vtable;
 }
 
-int reftable_new_merged_table(struct reftable_merged_table **dest,
+int reftable_merged_table_new(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id)
 {
reftable/reftable-merged.h +5 -4
index 4deb0ad22e1..72762483b9e 100644
@@ -29,10 +29,11 @@ struct reftable_merged_table;
 /* A generic reftable; see below. */
 struct reftable_table;
 
-/* reftable_new_merged_table creates a new merged table. It takes ownership of
-   the stack array.
-*/
-int reftable_new_merged_table(struct reftable_merged_table **dest,
+/*
+ * reftable_merged_table_new creates a new merged table. It takes ownership of
+ * the stack array.
+ */
+int reftable_merged_table_new(struct reftable_merged_table **dest,
 			      struct reftable_table *stack, size_t n,
 			      uint32_t hash_id);
 
reftable/stack.c +2 -2
index 2071e428a80..64c7fdf8c49 100644
@@ -272,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 	}
 
 	/* success! */
-	err = reftable_new_merged_table(&new_merged, new_tables,
+	err = reftable_merged_table_new(&new_merged, new_tables,
 					new_readers_len, st->opts.hash_id);
 	if (err < 0)
 		goto done;
@@ -924,7 +924,7 @@ static int stack_write_compact(struct reftable_stack *st,
 	reftable_writer_set_limits(wr, st->readers[first]->min_update_index,
 				   st->readers[last]->max_update_index);
 
-	err = reftable_new_merged_table(&mt, subtabs, subtabs_len,
+	err = reftable_merged_table_new(&mt, subtabs, subtabs_len,
 					st->opts.hash_id);
 	if (err < 0) {
 		reftable_free(subtabs);
t/unit-tests/t-reftable-merged.c +4 -4
index b6263ee8b5a..210603e8c78 100644
@@ -111,7 +111,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -289,7 +289,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -441,9 +441,9 @@ static void t_default_write_opts(void)
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
 	reftable_table_from_reader(&tab[0], rd);
-	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
 	check_int(err, ==, REFTABLE_FORMAT_ERROR);
-	err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
 	reftable_reader_free(rd);
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#25 lore raw

PATCH v3 3/15 reftable/merged: stop using generic tables in the merged table

In reply to Patrick Steinhardt

The merged table provides access to a reftable stack by merging the contents of those tables into a virtual table. These subtables are being tracked via struct reftable_table, which is a generic interface for accessing either a single reftable or a merged reftable. So in theory, it would be possible for the merged table to merge together other merged tables.

This is somewhat nonsensical though: we only ever set up a merged table over normal reftables, and there is no reason to do otherwise. This generic interface thus makes the code way harder to follow and reason about than really necessary. The abstraction layer may also have an impact on performance, even though the extra set of vtable function calls probably doesn't really matter.

Refactor the merged tables to use a struct reftable_reader for each of the subtables instead, which gives us direct access to the underlying tables. Adjust names accordingly.

---
 reftable/merged.c                | 28 +++++++++----------
 reftable/merged.h                |  4 +--
 reftable/reader.c                |  6 ++--
 reftable/reader.h                |  4 +++
 reftable/reftable-merged.h       |  7 +++--
 reftable/stack.c                 | 48 ++++++++++++--------------------
 reftable/stack_test.c            | 22 +++++++--------
 t/unit-tests/t-reftable-merged.c | 16 +++--------
 8 files changed, 60 insertions(+), 75 deletions(-)
reftable/merged.c +14 -14
index 25d414ec415..2e72eab3069 100644
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
 #include "constants.h"
 #include "iter.h"
 #include "pq.h"
+#include "reader.h"
 #include "record.h"
 #include "generic.h"
 #include "reftable-merged.h"
@@ -25,7 +26,7 @@ struct merged_subiter {
 struct merged_iter {
 	struct merged_subiter *subiters;
 	struct merged_iter_pqueue pq;
-	size_t stack_len;
+	size_t subiters_len;
 	int suppress_deletions;
 	ssize_t advance_index;
 };
@@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi,
 	mi->advance_index = -1;
 	mi->suppress_deletions = mt->suppress_deletions;
 
-	REFTABLE_CALLOC_ARRAY(mi->subiters, mt->stack_len);
-	for (size_t i = 0; i < mt->stack_len; i++) {
+	REFTABLE_CALLOC_ARRAY(mi->subiters, mt->readers_len);
+	for (size_t i = 0; i < mt->readers_len; i++) {
 		reftable_record_init(&mi->subiters[i].rec, typ);
-		table_init_iter(&mt->stack[i], &mi->subiters[i].iter, typ);
+		reader_init_iter(mt->readers[i], &mi->subiters[i].iter, typ);
 	}
-	mi->stack_len = mt->stack_len;
+	mi->subiters_len = mt->readers_len;
 }
 
 static void merged_iter_close(void *p)
@@ -51,7 +52,7 @@ static void merged_iter_close(void *p)
 	struct merged_iter *mi = p;
 
 	merged_iter_pqueue_release(&mi->pq);
-	for (size_t i = 0; i < mi->stack_len; i++) {
+	for (size_t i = 0; i < mi->subiters_len; i++) {
 		reftable_iterator_destroy(&mi->subiters[i].iter);
 		reftable_record_release(&mi->subiters[i].rec);
 	}
@@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want
 
 	mi->advance_index = -1;
 
-	for (size_t i = 0; i < mi->stack_len; i++) {
+	for (size_t i = 0; i < mi->subiters_len; i++) {
 		err = iterator_seek(&mi->subiters[i].iter, want);
 		if (err < 0)
 			return err;
@@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
 }
 
 int reftable_merged_table_new(struct reftable_merged_table **dest,
-			      struct reftable_table *stack, size_t n,
+			      struct reftable_reader **readers, size_t n,
 			      uint32_t hash_id)
 {
 	struct reftable_merged_table *m = NULL;
@@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
 	uint64_t first_min = 0;
 
 	for (size_t i = 0; i < n; i++) {
-		uint64_t min = reftable_table_min_update_index(&stack[i]);
-		uint64_t max = reftable_table_max_update_index(&stack[i]);
+		uint64_t min = reftable_reader_min_update_index(readers[i]);
+		uint64_t max = reftable_reader_max_update_index(readers[i]);
 
-		if (reftable_table_hash_id(&stack[i]) != hash_id) {
+		if (reftable_reader_hash_id(readers[i]) != hash_id) {
 			return REFTABLE_FORMAT_ERROR;
 		}
 		if (i == 0 || min < first_min) {
@@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
 	}
 
 	REFTABLE_CALLOC_ARRAY(m, 1);
-	m->stack = stack;
-	m->stack_len = n;
+	m->readers = readers;
+	m->readers_len = n;
 	m->min = first_min;
 	m->max = last_max;
 	m->hash_id = hash_id;
@@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt)
 {
 	if (!mt)
 		return;
-	FREE_AND_NULL(mt->stack);
 	reftable_free(mt);
 }
 
reftable/merged.h +2 -2
index 2efe571da67..de5fd33f010 100644
@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at
 #include "system.h"
 
 struct reftable_merged_table {
-	struct reftable_table *stack;
-	size_t stack_len;
+	struct reftable_reader **readers;
+	size_t readers_len;
 	uint32_t hash_id;
 
 	/* If unset, produce deletions. This is useful for compaction. For the
reftable/reader.c +3 -3
index 29c99e22694..f7ae35da728 100644
@@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it,
 	it->ops = &table_iter_vtable;
 }
 
-static void reader_init_iter(struct reftable_reader *r,
-			     struct reftable_iterator *it,
-			     uint8_t typ)
+void reader_init_iter(struct reftable_reader *r,
+		      struct reftable_iterator *it,
+		      uint8_t typ)
 {
 	struct reftable_reader_offsets *offs = reader_offsets_for(r, typ);
 
reftable/reader.h +4 -0
index e869165f234..a2c204d523c 100644
@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,
 void reader_close(struct reftable_reader *r);
 const char *reader_name(struct reftable_reader *r);
 
+void reader_init_iter(struct reftable_reader *r,
+		      struct reftable_iterator *it,
+		      uint8_t typ);
+
 /* initialize a block reader to read from `r` */
 int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
 			     uint64_t next_off, uint8_t want_typ);
reftable/reftable-merged.h +4 -3
index 72762483b9e..03c2619c0ff 100644
@@ -28,13 +28,14 @@ struct reftable_merged_table;
 
 /* A generic reftable; see below. */
 struct reftable_table;
+struct reftable_reader;
 
 /*
- * reftable_merged_table_new creates a new merged table. It takes ownership of
- * the stack array.
+ * reftable_merged_table_new creates a new merged table. The readers must be
+ * kept alive as long as the merged table is still in use.
  */
 int reftable_merged_table_new(struct reftable_merged_table **dest,
-			      struct reftable_table *stack, size_t n,
+			      struct reftable_reader **readers, size_t n,
 			      uint32_t hash_id);
 
 /* Initialize a merged table iterator for reading refs. */
reftable/stack.c +18 -30

146 lines not on this page · raw message

reftable/stack_test.c +11 -11
index 8c36590ff0f..dbca9eaf4a8 100644
@@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
 		 * all tables in the stack.
 		 */
 		if (i != n)
-			EXPECT(st->merged->stack_len == i + 1);
+			EXPECT(st->merged->readers_len == i + 1);
 		else
-			EXPECT(st->merged->stack_len == 1);
+			EXPECT(st->merged->readers_len == 1);
 	}
 
 	reftable_stack_destroy(st);
@@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
 
 	err = reftable_stack_add(st, write_test_ref, &ref);
 	EXPECT_ERR(err);
-	EXPECT(st->merged->stack_len == 1);
+	EXPECT(st->merged->readers_len == 1);
 	EXPECT(st->stats.attempts == 0);
 	EXPECT(st->stats.failures == 0);
 
@@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
 	ref.update_index = 2;
 	err = reftable_stack_add(st, write_test_ref, &ref);
 	EXPECT_ERR(err);
-	EXPECT(st->merged->stack_len == 2);
+	EXPECT(st->merged->readers_len == 2);
 	EXPECT(st->stats.attempts == 1);
 	EXPECT(st->stats.failures == 1);
 
@@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void)
 
 		err = reftable_stack_auto_compact(st);
 		EXPECT_ERR(err);
-		EXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));
+		EXPECT(i < 3 || st->merged->readers_len < 2 * fastlog2(i));
 	}
 
 	EXPECT(reftable_stack_compaction_stats(st)->entries_written <
@@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
 	EXPECT_ERR(err);
 
 	write_n_ref_tables(st, 5);
-	EXPECT(st->merged->stack_len == 5);
+	EXPECT(st->merged->readers_len == 5);
 
 	/*
 	 * Given that all tables we have written should be roughly the same
@@ -925,7 +925,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
 	err = reftable_stack_auto_compact(st);
 	EXPECT_ERR(err);
 	EXPECT(st->stats.failures == 0);
-	EXPECT(st->merged->stack_len == 4);
+	EXPECT(st->merged->readers_len == 4);
 
 	reftable_stack_destroy(st);
 	strbuf_release(&buf);
@@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void)
 		 * all tables in the stack.
 		 */
 		if (i != n)
-			EXPECT(st->merged->stack_len == i + 1);
+			EXPECT(st->merged->readers_len == i + 1);
 		else
-			EXPECT(st->merged->stack_len == 1);
+			EXPECT(st->merged->readers_len == 1);
 	}
 
 	reftable_stack_destroy(st);
@@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
 	EXPECT_ERR(err);
 
 	write_n_ref_tables(st, 3);
-	EXPECT(st->merged->stack_len == 3);
+	EXPECT(st->merged->readers_len == 3);
 
 	/* Lock one of the tables that we're about to compact. */
 	strbuf_reset(&buf);
@@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
 	err = reftable_stack_compact_all(st, NULL);
 	EXPECT(err == REFTABLE_LOCK_ERROR);
 	EXPECT(st->stats.failures == 1);
-	EXPECT(st->merged->stack_len == 3);
+	EXPECT(st->merged->readers_len == 3);
 
 	reftable_stack_destroy(st);
 	strbuf_release(&buf);
t/unit-tests/t-reftable-merged.c +4 -12
index 210603e8c78..577b1a5be87 100644
@@ -94,10 +94,8 @@ merged_table_from_records(struct reftable_ref_record **refs,
 			  struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
-	struct reftable_table *tabs;
 	int err;
 
-	REFTABLE_CALLOC_ARRAY(tabs, n);
 	REFTABLE_CALLOC_ARRAY(*readers, n);
 	REFTABLE_CALLOC_ARRAY(*source, n);
 
@@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs,
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
 		check(!err);
-		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 			      struct strbuf *buf, const size_t n)
 {
 	struct reftable_merged_table *mt = NULL;
-	struct reftable_table *tabs;
 	int err;
 
-	REFTABLE_CALLOC_ARRAY(tabs, n);
 	REFTABLE_CALLOC_ARRAY(*readers, n);
 	REFTABLE_CALLOC_ARRAY(*source, n);
 
@@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs,
 		err = reftable_new_reader(&(*readers)[i], &(*source)[i],
 					  "name");
 		check(!err);
-		reftable_table_from_reader(&tabs[i], (*readers)[i]);
 	}
 
-	err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
 	check(!err);
 	return mt;
 }
@@ -418,7 +412,6 @@ static void t_default_write_opts(void)
 	};
 	int err;
 	struct reftable_block_source source = { 0 };
-	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
 	struct reftable_merged_table *merged = NULL;
@@ -440,10 +433,9 @@ static void t_default_write_opts(void)
 	hash_id = reftable_reader_hash_id(rd);
 	check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
 
-	reftable_table_from_reader(&tab[0], rd);
-	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA256_FORMAT_ID);
 	check_int(err, ==, REFTABLE_FORMAT_ERROR);
-	err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
+	err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);
 	check(!err);
 
 	reftable_reader_free(rd);
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#26 lore raw

PATCH v3 4/15 reftable/stack: open-code reading refs

In reply to Patrick Steinhardt

To read a reference for the reftable stack, we first create a generic reftable_table from the merged table and then read the reference via a convenience function. We are about to remove these generic interfaces, so let's instead open-code the logic to prepare for this removal.

---
 reftable/stack.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
reftable/stack.c +22 -3
index 7f4e267ea98..d08ec009590 100644
@@ -1468,9 +1468,28 @@ reftable_stack_compaction_stats(struct reftable_stack *st)
 int reftable_stack_read_ref(struct reftable_stack *st, const char *refname,
 			    struct reftable_ref_record *ref)
 {
-	struct reftable_table tab = { NULL };
-	reftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));
-	return reftable_table_read_ref(&tab, refname, ref);
+	struct reftable_iterator it = { 0 };
+	int ret;
+
+	reftable_merged_table_init_ref_iterator(st->merged, &it);
+	ret = reftable_iterator_seek_ref(&it, refname);
+	if (ret)
+		goto out;
+
+	ret = reftable_iterator_next_ref(&it, ref);
+	if (ret)
+		goto out;
+
+	if (strcmp(ref->refname, refname) ||
+	    reftable_ref_record_is_deletion(ref)) {
+		reftable_ref_record_release(ref);
+		ret = 1;
+		goto out;
+	}
+
+out:
+	reftable_iterator_destroy(&it);
+	return ret;
 }
 
 int reftable_stack_read_log(struct reftable_stack *st, const char *refname,
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#27 lore raw

PATCH v3 5/15 reftable/iter: drop double-checking logic

In reply to Patrick Steinhardt

The filtering ref iterator can be used to only yield refs which are not in a specific skip list. This iterator has an option to double-check the results it returns, which causes us to seek the reference we are about to yield via a separate table such that we detect whether the reference that the first iterator has yielded actually exists.

The value of this is somewhat dubious, and I cannot think of any usecase where this functionality should be required. Furthermore, this option is never set in our codebase, which means that it is essentially untested. And last but not least, the struct reftable_table that is used to implement it is about to go away.

So while we could refactor the code to not use a reftable_table, it very much feels like a wasted effort. Let's just drop this code.

---
 reftable/iter.c   | 20 --------------------
 reftable/iter.h   |  2 --
 reftable/reader.c |  2 --
 3 files changed, 24 deletions(-)
reftable/iter.c +0 -20
index fddea31e517..a7484aba60d 100644
@@ -42,26 +42,6 @@ static int filtering_ref_iterator_next(void *iter_arg,
 			break;
 		}
 
-		if (fri->double_check) {
-			struct reftable_iterator it = { NULL };
-
-			reftable_table_init_ref_iter(&fri->tab, &it);
-
-			err = reftable_iterator_seek_ref(&it, ref->refname);
-			if (err == 0)
-				err = reftable_iterator_next_ref(&it, ref);
-
-			reftable_iterator_destroy(&it);
-
-			if (err < 0) {
-				break;
-			}
-
-			if (err > 0) {
-				continue;
-			}
-		}
-
 		if (ref->value_type == REFTABLE_REF_VAL2 &&
 		    (!memcmp(fri->oid.buf, ref->value.val2.target_value,
 			     fri->oid.len) ||
reftable/iter.h +0 -2
index 537431baba0..b75d7ac2ac0 100644
@@ -18,8 +18,6 @@ license that can be found in the LICENSE file or at
 
 /* iterator that produces only ref records that point to `oid` */
 struct filtering_ref_iterator {
-	int double_check;
-	struct reftable_table tab;
 	struct strbuf oid;
 	struct reftable_iterator it;
 };
reftable/reader.c +0 -2
index f7ae35da728..e3f58542297 100644
@@ -735,8 +735,6 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,
 	*filter = empty;
 
 	strbuf_add(&filter->oid, oid, oid_len);
-	reftable_table_from_reader(&filter->tab, r);
-	filter->double_check = 0;
 	iterator_from_table_iter(&filter->it, ti);
 
 	iterator_from_filtering_ref_iterator(it, filter);
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#28 lore raw

PATCH v3 6/15 reftable/generic: move generic iterator code into iterator interface

In reply to Patrick Steinhardt

Move functions relating to the reftable iterator from "generic.c" into "iter.c". This prepares for the removal of the former subsystem.

While at it, remove some unneeded braces to conform to our coding style.

---
 reftable/generic.c | 107 +--------------------------------------------
 reftable/generic.h |  10 -----
 reftable/iter.c    | 105 ++++++++++++++++++++++++++++++++++++++++++++
 reftable/iter.h    |  27 ++++++++++++
 4 files changed, 133 insertions(+), 116 deletions(-)
reftable/generic.c +1 -106
index 28ae26145e6..6ecf9b880f7 100644
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
 #include "constants.h"
 #include "record.h"
 #include "generic.h"
+#include "iter.h"
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
@@ -32,37 +33,6 @@ void reftable_table_init_log_iter(struct reftable_table *tab,
 	table_init_iter(tab, it, BLOCK_TYPE_LOG);
 }
 
-int reftable_iterator_seek_ref(struct reftable_iterator *it,
-			       const char *name)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_REF,
-		.u.ref = {
-			.refname = (char *)name,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log_at(struct reftable_iterator *it,
-				  const char *name, uint64_t update_index)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_LOG,
-		.u.log = {
-			.refname = (char *)name,
-			.update_index = update_index,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log(struct reftable_iterator *it,
-			       const char *name)
-{
-	return reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));
-}
-
 int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 			    struct reftable_ref_record *ref)
 {
@@ -152,78 +122,3 @@ uint32_t reftable_table_hash_id(struct reftable_table *tab)
 {
 	return tab->ops->hash_id(tab->table_arg);
 }
-
-void reftable_iterator_destroy(struct reftable_iterator *it)
-{
-	if (!it->ops) {
-		return;
-	}
-	it->ops->close(it->iter_arg);
-	it->ops = NULL;
-	FREE_AND_NULL(it->iter_arg);
-}
-
-int reftable_iterator_next_ref(struct reftable_iterator *it,
-			       struct reftable_ref_record *ref)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_REF,
-		.u = {
-			.ref = *ref
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*ref = rec.u.ref;
-	return err;
-}
-
-int reftable_iterator_next_log(struct reftable_iterator *it,
-			       struct reftable_log_record *log)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_LOG,
-		.u = {
-			.log = *log,
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*log = rec.u.log;
-	return err;
-}
-
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)
-{
-	return it->ops->seek(it->iter_arg, want);
-}
-
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
-{
-	return it->ops->next(it->iter_arg, rec);
-}
-
-static int empty_iterator_seek(void *arg, struct reftable_record *want)
-{
-	return 0;
-}
-
-static int empty_iterator_next(void *arg, struct reftable_record *rec)
-{
-	return 1;
-}
-
-static void empty_iterator_close(void *arg)
-{
-}
-
-static struct reftable_iterator_vtable empty_vtable = {
-	.seek = &empty_iterator_seek,
-	.next = &empty_iterator_next,
-	.close = &empty_iterator_close,
-};
-
-void iterator_set_empty(struct reftable_iterator *it)
-{
-	assert(!it->ops);
-	it->iter_arg = NULL;
-	it->ops = &empty_vtable;
-}
reftable/generic.h +0 -10
index 8341fa570e0..837fbb8df20 100644
@@ -24,14 +24,4 @@ void table_init_iter(struct reftable_table *tab,
 		     struct reftable_iterator *it,
 		     uint8_t typ);
 
-struct reftable_iterator_vtable {
-	int (*seek)(void *iter_arg, struct reftable_record *want);
-	int (*next)(void *iter_arg, struct reftable_record *rec);
-	void (*close)(void *iter_arg);
-};
-
-void iterator_set_empty(struct reftable_iterator *it);
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
-
 #endif
reftable/iter.c +105 -0

114 lines not on this page · raw message

reftable/iter.h +27 -0
index b75d7ac2ac0..3b401f12590 100644
@@ -16,6 +16,33 @@ license that can be found in the LICENSE file or at
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
+/*
+ * The virtual function table for implementing generic reftable iterators.
+ */
+struct reftable_iterator_vtable {
+	int (*seek)(void *iter_arg, struct reftable_record *want);
+	int (*next)(void *iter_arg, struct reftable_record *rec);
+	void (*close)(void *iter_arg);
+};
+
+/*
+ * Position the iterator at the wanted record such that a call to
+ * `iterator_next()` would return that record, if it exists.
+ */
+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
+
+/*
+ * Yield the next record and advance the iterator. Returns <0 on error, 0 when
+ * a record was yielded, and >0 when the iterator hit an error.
+ */
+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
+
+/*
+ * Set up the iterator such that it behaves the same as an iterator with no
+ * entries.
+ */
+void iterator_set_empty(struct reftable_iterator *it);
+
 /* iterator that produces only ref records that point to `oid` */
 struct filtering_ref_iterator {
 	struct strbuf oid;
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#29 lore raw

PATCH v3 7/15 reftable/dump: drop unused `compact_stack()`

In reply to Patrick Steinhardt

The compact_stack() function is exposed via reftable_dump_main(), which ultimately ends up being wired into "test-tool reftable". It is never used by our tests though, and nowadays we have wired up support for stack compaction into git-pack-refs(1).

Remove the code.

---
 reftable/dump.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)
reftable/dump.c +1 -26
index dd65d9e8bb7..391d93de6a4 100644
@@ -24,30 +24,10 @@ license that can be found in the LICENSE file or at
 #include <unistd.h>
 #include <string.h>
 
-static int compact_stack(const char *stackdir)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { 0 };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	err = reftable_stack_compact_all(stack, NULL);
-	if (err < 0)
-		goto done;
-done:
-	if (stack) {
-		reftable_stack_destroy(stack);
-	}
-	return err;
-}
-
 static void print_help(void)
 {
-	printf("usage: dump [-cst] arg\n\n"
+	printf("usage: dump [-st] arg\n\n"
 	       "options: \n"
-	       "  -c compact\n"
 	       "  -b dump blocks\n"
 	       "  -t dump table\n"
 	       "  -s dump stack\n"
@@ -62,7 +42,6 @@ int reftable_dump_main(int argc, char *const *argv)
 	int opt_dump_blocks = 0;
 	int opt_dump_table = 0;
 	int opt_dump_stack = 0;
-	int opt_compact = 0;
 	uint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;
 	const char *arg = NULL, *argv0 = argv[0];
 
@@ -77,8 +56,6 @@ int reftable_dump_main(int argc, char *const *argv)
 			opt_hash_id = GIT_SHA256_FORMAT_ID;
 		else if (!strcmp("-s", argv[1]))
 			opt_dump_stack = 1;
-		else if (!strcmp("-c", argv[1]))
-			opt_compact = 1;
 		else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {
 			print_help();
 			return 2;
@@ -98,8 +75,6 @@ int reftable_dump_main(int argc, char *const *argv)
 		err = reftable_reader_print_file(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);
-	} else if (opt_compact) {
-		err = compact_stack(arg);
 	}
 
 	if (err < 0) {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#30 lore raw

PATCH v3 8/15 t/helper: inline `reftable_dump_main()`

In reply to Patrick Steinhardt

The printing functionality part of reftable/dump.c is really only used by our "dump-reftable" test helper. It is certainly not generic logic that is useful to anybody outside of Git, and the format it generates is quite specific. Still, parts of it are used in our test suite and the output may be useful to take a peek into reftable stacks, tables and blocks. So while it does not make sense to expose this as part of the reftable library, it does make sense to keep it around.

Inline the reftable_dump_main() function into the "dump-reftable" test helper. This clarifies that its format is subject to change and not part of our public interface. Furthermore, this allows us to iterate on the implementation in subsequent patches.

---
 Makefile                  |  1 -
 reftable/dump.c           | 86 ---------------------------------------
 reftable/reftable-tests.h |  1 -
 t/helper/test-reftable.c  | 61 ++++++++++++++++++++++++++-
 4 files changed, 60 insertions(+), 89 deletions(-)
 delete mode 100644 reftable/dump.c
Makefile +0 -1
index 3863e60b664..343f19a488b 100644
@@ -2680,7 +2680,6 @@ REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
 
 REFTABLE_TEST_OBJS += reftable/block_test.o
-REFTABLE_TEST_OBJS += reftable/dump.o
 REFTABLE_TEST_OBJS += reftable/pq_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
reftable/dump.c deleted +0 -86

86 lines not on this page · raw message

reftable/reftable-tests.h +0 -1
index d5e03dcc1b6..d005a8bb9e1 100644
@@ -16,6 +16,5 @@ int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
 int tree_test_main(int argc, const char **argv);
-int reftable_dump_main(int argc, char *const *argv);
 
 #endif
t/helper/test-reftable.c +60 -1

71 lines not on this page · raw message

Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#31 lore raw

PATCH v3 9/15 t/helper: inline `reftable_reader_print_file()`

In reply to Patrick Steinhardt

Move reftable_reader_print_file() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/reader.c          | 21 ---------------------
 reftable/reftable-reader.h |  2 --
 t/helper/test-reftable.c   | 23 ++++++++++++++++++++++-
 3 files changed, 22 insertions(+), 24 deletions(-)
reftable/reader.c +0 -21
index e3f58542297..fbd93b88dff 100644
@@ -798,27 +798,6 @@ void reftable_table_from_reader(struct reftable_table *tab,
 	tab->table_arg = reader;
 }
 
-
-int reftable_reader_print_file(const char *tablename)
-{
-	struct reftable_block_source src = { NULL };
-	int err = reftable_block_source_from_file(&src, tablename);
-	struct reftable_reader *r = NULL;
-	struct reftable_table tab = { NULL };
-	if (err < 0)
-		goto done;
-
-	err = reftable_new_reader(&r, &src, tablename);
-	if (err < 0)
-		goto done;
-
-	reftable_table_from_reader(&tab, r);
-	err = reftable_table_print(&tab);
-done:
-	reftable_reader_free(r);
-	return err;
-}
-
 int reftable_reader_print_blocks(const char *tablename)
 {
 	struct {
reftable/reftable-reader.h +0 -2
index a32f31d6486..7c7d1716516 100644
@@ -64,8 +64,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r);
 void reftable_table_from_reader(struct reftable_table *tab,
 				struct reftable_reader *reader);
 
-/* print table onto stdout for debugging. */
-int reftable_reader_print_file(const char *tablename);
 /* print blocks onto stdout for debugging. */
 int reftable_reader_print_blocks(const char *tablename);
 
t/helper/test-reftable.c +22 -1
index 7f37d0cd34b..19367c25f9a 100644
@@ -1,5 +1,6 @@
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
+#include "reftable/reftable-generic.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
 #include "reftable/reftable-tests.h"
@@ -28,6 +29,26 @@ static void print_help(void)
 	       "\n");
 }
 
+static int dump_reftable(const char *tablename)
+{
+	struct reftable_block_source src = { NULL };
+	int err = reftable_block_source_from_file(&src, tablename);
+	struct reftable_reader *r = NULL;
+	struct reftable_table tab = { NULL };
+	if (err < 0)
+		goto done;
+
+	err = reftable_new_reader(&r, &src, tablename);
+	if (err < 0)
+		goto done;
+
+	reftable_table_from_reader(&tab, r);
+	err = reftable_table_print(&tab);
+done:
+	reftable_reader_free(r);
+	return err;
+}
+
 int cmd__dump_reftable(int argc, const char **argv)
 {
 	int err = 0;
@@ -64,7 +85,7 @@ int cmd__dump_reftable(int argc, const char **argv)
 	if (opt_dump_blocks) {
 		err = reftable_reader_print_blocks(arg);
 	} else if (opt_dump_table) {
-		err = reftable_reader_print_file(arg);
+		err = dump_reftable(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);
 	}
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#32 lore raw

PATCH v3 10/15 t/helper: inline `reftable_stack_print_directory()`

In reply to Patrick Steinhardt

Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

Note that this requires us to remove the tests for this functionality in reftable/stack_test.c. The test does not really add much anyway, because all it verifies is that we do not crash or run into an error, and it specifically doesn't check the outputted data. Also, as the code is now part of the test helper, it doesn't make much sense to have a unit test for it in the first place.

---
 reftable/reftable-stack.h |  3 ---
 reftable/stack.c          | 20 --------------------
 reftable/stack_test.c     |  7 -------
 t/helper/test-reftable.c  | 23 ++++++++++++++++++++++-
 4 files changed, 22 insertions(+), 31 deletions(-)
reftable/reftable-stack.h +0 -3
index 09e97c99915..f4f8cabc7fb 100644
@@ -140,7 +140,4 @@ struct reftable_compaction_stats {
 struct reftable_compaction_stats *
 reftable_stack_compaction_stats(struct reftable_stack *st);
 
-/* print the entire stack represented by the directory */
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);
-
 #endif
reftable/stack.c +0 -20
index d08ec009590..bedd503e7e1 100644
@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)
 	reftable_addition_destroy(add);
 	return err;
 }
-
-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
-{
-	struct reftable_stack *stack = NULL;
-	struct reftable_write_options opts = { .hash_id = hash_id };
-	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
-
-	int err = reftable_new_stack(&stack, stackdir, &opts);
-	if (err < 0)
-		goto done;
-
-	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = reftable_table_print(&table);
-done:
-	if (stack)
-		reftable_stack_destroy(stack);
-	return err;
-}
reftable/stack_test.c +0 -7
index dbca9eaf4a8..42044ed8a3e 100644
@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)
 	EXPECT(0 == strcmp("master", dest.value.symref));
 	EXPECT(st->readers_len > 0);
 
-	printf("testing print functionality:\n");
-	err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);
-	EXPECT_ERR(err);
-
-	err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);
-	EXPECT(err == REFTABLE_FORMAT_ERROR);
-
 #ifndef GIT_WINDOWS_NATIVE
 	strbuf_addstr(&scratch, dir);
 	strbuf_addstr(&scratch, "/tables.list");
t/helper/test-reftable.c +22 -1
index 19367c25f9a..db62ea8dc3b 100644
@@ -1,6 +1,7 @@
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
+#include "reftable/reftable-merged.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
 #include "reftable/reftable-tests.h"
@@ -29,6 +30,26 @@ static void print_help(void)
 	       "\n");
 }
 
+static int dump_stack(const char *stackdir, uint32_t hash_id)
+{
+	struct reftable_stack *stack = NULL;
+	struct reftable_write_options opts = { .hash_id = hash_id };
+	struct reftable_merged_table *merged = NULL;
+	struct reftable_table table = { NULL };
+
+	int err = reftable_new_stack(&stack, stackdir, &opts);
+	if (err < 0)
+		goto done;
+
+	merged = reftable_stack_merged_table(stack);
+	reftable_table_from_merged_table(&table, merged);
+	err = reftable_table_print(&table);
+done:
+	if (stack)
+		reftable_stack_destroy(stack);
+	return err;
+}
+
 static int dump_reftable(const char *tablename)
 {
 	struct reftable_block_source src = { NULL };
@@ -87,7 +108,7 @@ int cmd__dump_reftable(int argc, const char **argv)
 	} else if (opt_dump_table) {
 		err = dump_reftable(arg);
 	} else if (opt_dump_stack) {
-		err = reftable_stack_print_directory(arg, opt_hash_id);
+		err = dump_stack(arg, opt_hash_id);
 	}
 
 	if (err < 0) {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#33 lore raw

PATCH v3 11/15 t/helper: inline `reftable_table_print()`

In reply to Patrick Steinhardt

Move reftable_table_print() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/generic.c          | 47 ---------------------------------
 reftable/reftable-generic.h |  3 ---
 t/helper/test-reftable.c    | 52 +++++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 52 deletions(-)
reftable/generic.c +0 -47
index 6ecf9b880f7..495ee9af6b0 100644
@@ -61,53 +61,6 @@ int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 	return err;
 }
 
-int reftable_table_print(struct reftable_table *tab) {
-	struct reftable_iterator it = { NULL };
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_log_record log = { NULL };
-	uint32_t hash_id = reftable_table_hash_id(tab);
-	int err;
-
-	reftable_table_init_ref_iter(tab, &it);
-
-	err = reftable_iterator_seek_ref(&it, "");
-	if (err < 0)
-		return err;
-
-	while (1) {
-		err = reftable_iterator_next_ref(&it, &ref);
-		if (err > 0) {
-			break;
-		}
-		if (err < 0) {
-			return err;
-		}
-		reftable_ref_record_print(&ref, hash_id);
-	}
-	reftable_iterator_destroy(&it);
-	reftable_ref_record_release(&ref);
-
-	reftable_table_init_log_iter(tab, &it);
-
-	err = reftable_iterator_seek_log(&it, "");
-	if (err < 0)
-		return err;
-
-	while (1) {
-		err = reftable_iterator_next_log(&it, &log);
-		if (err > 0) {
-			break;
-		}
-		if (err < 0) {
-			return err;
-		}
-		reftable_log_record_print(&log, hash_id);
-	}
-	reftable_iterator_destroy(&it);
-	reftable_log_record_release(&log);
-	return 0;
-}
-
 uint64_t reftable_table_max_update_index(struct reftable_table *tab)
 {
 	return tab->ops->max_update_index(tab->table_arg);
reftable/reftable-generic.h +0 -3
index 65670ea093b..b8b1323a331 100644
@@ -41,7 +41,4 @@ uint64_t reftable_table_min_update_index(struct reftable_table *tab);
 int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 			    struct reftable_ref_record *ref);
 
-/* dump table contents onto stdout for debugging */
-int reftable_table_print(struct reftable_table *tab);
-
 #endif
t/helper/test-reftable.c +50 -2

70 lines not on this page · raw message

Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#34 lore raw

PATCH v3 12/15 t/helper: inline printing of reftable records

In reply to Patrick Steinhardt

Move printing of reftable records into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

---
 reftable/record.c          | 127 -------------------------------------
 reftable/record.h          |   1 -
 reftable/reftable-record.h |   8 ---
 t/helper/test-reftable.c   |  77 +++++++++++++++++++---
 4 files changed, 69 insertions(+), 144 deletions(-)
reftable/record.c +0 -127
index a2cba5ef747..e26bd4bc8d9 100644
@@ -259,58 +259,6 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
 	}
 }
 
-static char hexdigit(int c)
-{
-	if (c <= 9)
-		return '0' + c;
-	return 'a' + (c - 10);
-}
-
-static void hex_format(char *dest, const unsigned char *src, int hash_size)
-{
-	assert(hash_size > 0);
-	if (src) {
-		int i = 0;
-		for (i = 0; i < hash_size; i++) {
-			dest[2 * i] = hexdigit(src[i] >> 4);
-			dest[2 * i + 1] = hexdigit(src[i] & 0xf);
-		}
-		dest[2 * hash_size] = 0;
-	}
-}
-
-static void reftable_ref_record_print_sz(const struct reftable_ref_record *ref,
-					 int hash_size)
-{
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
-	printf("ref{%s(%" PRIu64 ") ", ref->refname, ref->update_index);
-	switch (ref->value_type) {
-	case REFTABLE_REF_SYMREF:
-		printf("=> %s", ref->value.symref);
-		break;
-	case REFTABLE_REF_VAL2:
-		hex_format(hex, ref->value.val2.value, hash_size);
-		printf("val 2 %s", hex);
-		hex_format(hex, ref->value.val2.target_value,
-			   hash_size);
-		printf("(T %s)", hex);
-		break;
-	case REFTABLE_REF_VAL1:
-		hex_format(hex, ref->value.val1, hash_size);
-		printf("val 1 %s", hex);
-		break;
-	case REFTABLE_REF_DELETION:
-		printf("delete");
-		break;
-	}
-	printf("}\n");
-}
-
-void reftable_ref_record_print(const struct reftable_ref_record *ref,
-			       uint32_t hash_id) {
-	reftable_ref_record_print_sz(ref, hash_size(hash_id));
-}
-
 static void reftable_ref_record_release_void(void *rec)
 {
 	reftable_ref_record_release(rec);
@@ -480,12 +428,6 @@ static int reftable_ref_record_cmp_void(const void *_a, const void *_b)
 	return strcmp(a->refname, b->refname);
 }
 
-static void reftable_ref_record_print_void(const void *rec,
-					   int hash_size)
-{
-	reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size);
-}
-
 static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.key = &reftable_ref_record_key,
 	.type = BLOCK_TYPE_REF,
@@ -497,7 +439,6 @@ static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.is_deletion = &reftable_ref_record_is_deletion_void,
 	.equal = &reftable_ref_record_equal_void,
 	.cmp = &reftable_ref_record_cmp_void,
-	.print = &reftable_ref_record_print_void,
 };
 
 static void reftable_obj_record_key(const void *r, struct strbuf *dest)
@@ -516,21 +457,6 @@ static void reftable_obj_record_release(void *rec)
 	memset(obj, 0, sizeof(struct reftable_obj_record));
 }
 
-static void reftable_obj_record_print(const void *rec, int hash_size)
-{
-	const struct reftable_obj_record *obj = rec;
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-	struct strbuf offset_str = STRBUF_INIT;
-	int i;
-
-	for (i = 0; i < obj->offset_len; i++)
-		strbuf_addf(&offset_str, "%" PRIu64 " ", obj->offsets[i]);
-	hex_format(hex, obj->hash_prefix, obj->hash_prefix_len);
-	printf("prefix %s (len %d), offsets [%s]\n",
-	       hex, obj->hash_prefix_len, offset_str.buf);
-	strbuf_release(&offset_str);
-}
-
 static void reftable_obj_record_copy_from(void *rec, const void *src_rec,
 					  int hash_size)
 {
@@ -701,41 +627,8 @@ static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_obj_record_equal_void,
 	.cmp = &reftable_obj_record_cmp_void,
-	.print = &reftable_obj_record_print,
 };
 
-static void reftable_log_record_print_sz(struct reftable_log_record *log,
-					 int hash_size)
-{
-	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-
-	switch (log->value_type) {
-	case REFTABLE_LOG_DELETION:
-		printf("log{%s(%" PRIu64 ") delete\n", log->refname,
-		       log->update_index);
-		break;
-	case REFTABLE_LOG_UPDATE:
-		printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
-		       log->refname, log->update_index,
-		       log->value.update.name ? log->value.update.name : "",
-		       log->value.update.email ? log->value.update.email : "",
-		       log->value.update.time,
-		       log->value.update.tz_offset);
-		hex_format(hex, log->value.update.old_hash, hash_size);
-		printf("%s => ", hex);
-		hex_format(hex, log->value.update.new_hash, hash_size);
-		printf("%s\n\n%s\n}\n", hex,
-		       log->value.update.message ? log->value.update.message : "");
-		break;
-	}
-}
-
-void reftable_log_record_print(struct reftable_log_record *log,
-				      uint32_t hash_id)
-{
-	reftable_log_record_print_sz(log, hash_size(hash_id));
-}
-
 static void reftable_log_record_key(const void *r, struct strbuf *dest)
 {
 	const struct reftable_log_record *rec =
@@ -1039,11 +932,6 @@ static int reftable_log_record_is_deletion_void(const void *p)
 		(const struct reftable_log_record *)p);
 }
 
-static void reftable_log_record_print_void(const void *rec, int hash_size)
-{
-	reftable_log_record_print_sz((struct reftable_log_record*)rec, hash_size);
-}
-
 static struct reftable_record_vtable reftable_log_record_vtable = {
 	.key = &reftable_log_record_key,
 	.type = BLOCK_TYPE_LOG,
@@ -1055,7 +943,6 @@ static struct reftable_record_vtable reftable_log_record_vtable = {
 	.is_deletion = &reftable_log_record_is_deletion_void,
 	.equal = &reftable_log_record_equal_void,
 	.cmp = &reftable_log_record_cmp_void,
-	.print = &reftable_log_record_print_void,
 };
 
 static void reftable_index_record_key(const void *r, struct strbuf *dest)
@@ -1137,13 +1024,6 @@ static int reftable_index_record_cmp(const void *_a, const void *_b)
 	return strbuf_cmp(&a->last_key, &b->last_key);
 }
 
-static void reftable_index_record_print(const void *rec, int hash_size)
-{
-	const struct reftable_index_record *idx = rec;
-	/* TODO: escape null chars? */
-	printf("\"%s\" %" PRIu64 "\n", idx->last_key.buf, idx->offset);
-}
-
 static struct reftable_record_vtable reftable_index_record_vtable = {
 	.key = &reftable_index_record_key,
 	.type = BLOCK_TYPE_INDEX,
@@ -1155,7 +1035,6 @@ static struct reftable_record_vtable reftable_index_record_vtable = {
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_index_record_equal,
 	.cmp = &reftable_index_record_cmp,
-	.print = &reftable_index_record_print,
 };
 
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
@@ -1334,9 +1213,3 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ)
 		BUG("unhandled record type");
 	}
 }
-
-void reftable_record_print(struct reftable_record *rec, int hash_size)
-{
-	printf("'%c': ", rec->type);
-	reftable_record_vtable(rec)->print(reftable_record_data(rec), hash_size);
-}
reftable/record.h +0 -1
index d778133e6ec..5003bacdb0c 100644
@@ -136,7 +136,6 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);
 /* see struct record_vtable */
 int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);
 int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
-void reftable_record_print(struct reftable_record *rec, int hash_size);
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest);
 void reftable_record_copy_from(struct reftable_record *rec,
 			       struct reftable_record *src, int hash_size);
reftable/reftable-record.h +0 -8
index ff486eb1f75..2d42463c581 100644
@@ -60,10 +60,6 @@ const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *
 /* returns whether 'ref' represents a deletion */
 int reftable_ref_record_is_deletion(const struct reftable_ref_record *ref);
 
-/* prints a reftable_ref_record onto stdout. Useful for debugging. */
-void reftable_ref_record_print(const struct reftable_ref_record *ref,
-			       uint32_t hash_id);
-
 /* frees and nulls all pointer values inside `ref`. */
 void reftable_ref_record_release(struct reftable_ref_record *ref);
 
@@ -111,8 +107,4 @@ void reftable_log_record_release(struct reftable_log_record *log);
 int reftable_log_record_equal(const struct reftable_log_record *a,
 			      const struct reftable_log_record *b, int hash_size);
 
-/* dumps a reftable_log_record on stdout, for debugging/testing. */
-void reftable_log_record_print(struct reftable_log_record *log,
-			       uint32_t hash_id);
-
 #endif
t/helper/test-reftable.c +69 -8

109 lines not on this page · raw message

Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#35 lore raw

PATCH v3 13/15 t/helper: use `hash_to_hex_algop()` to print hashes

In reply to Patrick Steinhardt

The "reftable" test helper uses a hand-crafted version to convert from a raw hash to its hex variant. This was done because this code used to be part of the reftable library, where we do not use most functions from the Git core.

Now that the code is integrated into the "dump-reftable" helper though, that limitation went away. Let's thus use hash_to_hex_algop() instead.

---
 t/helper/test-reftable.c | 48 +++++++++-------------------------------
 1 file changed, 11 insertions(+), 37 deletions(-)
t/helper/test-reftable.c +11 -37
index cb22d7537a9..234fb80010f 100644
@@ -1,3 +1,6 @@
+#include "git-compat-util.h"
+#include "hash.h"
+#include "hex.h"
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
 #include "reftable/reftable-generic.h"
@@ -30,33 +33,12 @@ static void print_help(void)
 	       "\n");
 }
 
-static char hexdigit(int c)
-{
-	if (c <= 9)
-		return '0' + c;
-	return 'a' + (c - 10);
-}
-
-static void hex_format(char *dest, const unsigned char *src, int hash_size)
-{
-	assert(hash_size > 0);
-	if (src) {
-		int i = 0;
-		for (i = 0; i < hash_size; i++) {
-			dest[2 * i] = hexdigit(src[i] >> 4);
-			dest[2 * i + 1] = hexdigit(src[i] & 0xf);
-		}
-		dest[2 * hash_size] = 0;
-	}
-}
-
 static int dump_table(struct reftable_table *tab)
 {
 	struct reftable_iterator it = { NULL };
 	struct reftable_ref_record ref = { NULL };
 	struct reftable_log_record log = { NULL };
-	uint32_t hash_id = reftable_table_hash_id(tab);
-	int hash_len = hash_size(hash_id);
+	const struct git_hash_algo *algop;
 	int err;
 
 	reftable_table_init_ref_iter(tab, &it);
@@ -65,9 +47,9 @@ static int dump_table(struct reftable_table *tab)
 	if (err < 0)
 		return err;
 
-	while (1) {
-		char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
+	algop = &hash_algos[hash_algo_by_id(reftable_table_hash_id(tab))];
 
+	while (1) {
 		err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -80,15 +62,11 @@ static int dump_table(struct reftable_table *tab)
 			printf("=> %s", ref.value.symref);
 			break;
 		case REFTABLE_REF_VAL2:
-			hex_format(hex, ref.value.val2.value, hash_len);
-			printf("val 2 %s", hex);
-			hex_format(hex, ref.value.val2.target_value,
-				   hash_len);
-			printf("(T %s)", hex);
+			printf("val 2 %s", hash_to_hex_algop(ref.value.val2.value, algop));
+			printf("(T %s)", hash_to_hex_algop(ref.value.val2.target_value, algop));
 			break;
 		case REFTABLE_REF_VAL1:
-			hex_format(hex, ref.value.val1, hash_len);
-			printf("val 1 %s", hex);
+			printf("val 1 %s", hash_to_hex_algop(ref.value.val1, algop));
 			break;
 		case REFTABLE_REF_DELETION:
 			printf("delete");
@@ -106,8 +84,6 @@ static int dump_table(struct reftable_table *tab)
 		return err;
 
 	while (1) {
-		char hex[GIT_MAX_HEXSZ + 1] = { 0 };
-
 		err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -126,10 +102,8 @@ static int dump_table(struct reftable_table *tab)
 			       log.value.update.email ? log.value.update.email : "",
 			       log.value.update.time,
 			       log.value.update.tz_offset);
-			hex_format(hex, log.value.update.old_hash, hash_len);
-			printf("%s => ", hex);
-			hex_format(hex, log.value.update.new_hash, hash_len);
-			printf("%s\n\n%s\n}\n", hex,
+			printf("%s => ", hash_to_hex_algop(log.value.update.old_hash, algop));
+			printf("%s\n\n%s\n}\n", hash_to_hex_algop(log.value.update.new_hash, algop),
 			       log.value.update.message ? log.value.update.message : "");
 			break;
 		}
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#36 lore raw

PATCH v3 14/15 t/helper: refactor to not use `struct reftable_table`

In reply to Patrick Steinhardt

The struct reftable_table interface in our "reftable" test helper gets used such that we can easily print either a single table, or a merged stack. This generic interface is about to go away.

Prepare the code for this change by using merged tables instead. When printing the stack we've already got one. When using a single table, we can create a merged table from it to adapt.

This removes the last user of the generic interface.

---
 t/helper/test-reftable.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
t/helper/test-reftable.c +18 -15
index 234fb80010f..c1942156b50 100644
@@ -3,7 +3,6 @@
 #include "hex.h"
 #include "reftable/system.h"
 #include "reftable/reftable-error.h"
-#include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-reader.h"
 #include "reftable/reftable-stack.h"
@@ -33,7 +32,7 @@ static void print_help(void)
 	       "\n");
 }
 
-static int dump_table(struct reftable_table *tab)
+static int dump_table(struct reftable_merged_table *mt)
 {
 	struct reftable_iterator it = { NULL };
 	struct reftable_ref_record ref = { NULL };
@@ -41,13 +40,12 @@ static int dump_table(struct reftable_table *tab)
 	const struct git_hash_algo *algop;
 	int err;
 
-	reftable_table_init_ref_iter(tab, &it);
-
+	reftable_merged_table_init_ref_iterator(mt, &it);
 	err = reftable_iterator_seek_ref(&it, "");
 	if (err < 0)
 		return err;
 
-	algop = &hash_algos[hash_algo_by_id(reftable_table_hash_id(tab))];
+	algop = &hash_algos[hash_algo_by_id(reftable_merged_table_hash_id(mt))];
 
 	while (1) {
 		err = reftable_iterator_next_ref(&it, &ref);
@@ -77,8 +75,7 @@ static int dump_table(struct reftable_table *tab)
 	reftable_iterator_destroy(&it);
 	reftable_ref_record_release(&ref);
 
-	reftable_table_init_log_iter(tab, &it);
-
+	reftable_merged_table_init_log_iterator(mt, &it);
 	err = reftable_iterator_seek_log(&it, "");
 	if (err < 0)
 		return err;
@@ -118,15 +115,13 @@ static int dump_stack(const char *stackdir, uint32_t hash_id)
 	struct reftable_stack *stack = NULL;
 	struct reftable_write_options opts = { .hash_id = hash_id };
 	struct reftable_merged_table *merged = NULL;
-	struct reftable_table table = { NULL };
 
 	int err = reftable_new_stack(&stack, stackdir, &opts);
 	if (err < 0)
 		goto done;
 
 	merged = reftable_stack_merged_table(stack);
-	reftable_table_from_merged_table(&table, merged);
-	err = dump_table(&table);
+	err = dump_table(merged);
 done:
 	if (stack)
 		reftable_stack_destroy(stack);
@@ -135,10 +130,12 @@ static int dump_stack(const char *stackdir, uint32_t hash_id)
 
 static int dump_reftable(const char *tablename)
 {
-	struct reftable_block_source src = { NULL };
-	int err = reftable_block_source_from_file(&src, tablename);
+	struct reftable_block_source src = { 0 };
+	struct reftable_merged_table *mt = NULL;
 	struct reftable_reader *r = NULL;
-	struct reftable_table tab = { NULL };
+	int err;
+
+	err = reftable_block_source_from_file(&src, tablename);
 	if (err < 0)
 		goto done;
 
@@ -146,9 +143,15 @@ static int dump_reftable(const char *tablename)
 	if (err < 0)
 		goto done;
 
-	reftable_table_from_reader(&tab, r);
-	err = dump_table(&tab);
+	err = reftable_merged_table_new(&mt, &r, 1,
+					reftable_reader_hash_id(r));
+	if (err < 0)
+		goto done;
+
+	err = dump_table(mt);
+
 done:
+	reftable_merged_table_free(mt);
 	reftable_reader_free(r);
 	return err;
 }
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#37 lore raw

PATCH v3 15/15 reftable/generic: drop interface

In reply to Patrick Steinhardt

The reftable_table interface provides a generic infrastructure that can abstract away whether the underlying table is a single table, or a merged table. This abstraction can make it rather hard to reason about the code. We didn't ever use it to implement the reftable backend, and with the preceding patches in this patch series we in fact don't use it at all anymore. Furthermore, it became somewhat useless with the recent refactorings that made it possible to seek reftable iterators multiple times, as these now provide generic access to tables for us. The interface is thus redundant and only brings unnecessary complexity with it.

Remove the struct reftable_table interface and its associated functions.

---
 Makefile                         |  1 -
 reftable/generic.c               | 77 --------------------------------
 reftable/generic.h               | 27 -----------
 reftable/iter.c                  |  1 -
 reftable/iter.h                  |  1 -
 reftable/merged.c                | 38 ----------------
 reftable/reader.c                | 41 -----------------
 reftable/reftable-generic.h      | 44 ------------------
 reftable/reftable-merged.h       |  6 ---
 reftable/reftable-reader.h       |  7 ---
 reftable/stack.c                 |  1 -
 t/unit-tests/t-reftable-merged.c |  1 -
 12 files changed, 245 deletions(-)
 delete mode 100644 reftable/generic.c
 delete mode 100644 reftable/generic.h
 delete mode 100644 reftable/reftable-generic.h
Makefile +0 -1
index 343f19a488b..41dfa0bad2c 100644
@@ -2674,7 +2674,6 @@ REFTABLE_OBJS += reftable/merged.o
 REFTABLE_OBJS += reftable/pq.o
 REFTABLE_OBJS += reftable/reader.o
 REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/generic.o
 REFTABLE_OBJS += reftable/stack.o
 REFTABLE_OBJS += reftable/tree.o
 REFTABLE_OBJS += reftable/writer.o
reftable/generic.c deleted +0 -77

77 lines not on this page · raw message

reftable/generic.h deleted +0 -27
deleted file mode 100644
index 837fbb8df20..00000000000
@@ -1,27 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#ifndef GENERIC_H
-#define GENERIC_H
-
-#include "record.h"
-#include "reftable-generic.h"
-
-/* generic interface to reftables */
-struct reftable_table_vtable {
-	void (*init_iter)(void *tab, struct reftable_iterator *it, uint8_t typ);
-	uint32_t (*hash_id)(void *tab);
-	uint64_t (*min_update_index)(void *tab);
-	uint64_t (*max_update_index)(void *tab);
-};
-
-void table_init_iter(struct reftable_table *tab,
-		     struct reftable_iterator *it,
-		     uint8_t typ);
-
-#endif
reftable/iter.c +0 -1
index 225feb78714..97a4642ed57 100644
@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at
 #include "system.h"
 
 #include "block.h"
-#include "generic.h"
 #include "constants.h"
 #include "reader.h"
 #include "reftable-error.h"
reftable/iter.h +0 -1
index 3b401f12590..befc4597df1 100644
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at
 #include "record.h"
 
 #include "reftable-iterator.h"
-#include "reftable-generic.h"
 
 /*
  * The virtual function table for implementing generic reftable iterators.
reftable/merged.c +0 -38
index 2e72eab3069..128a810c55d 100644
@@ -13,7 +13,6 @@ license that can be found in the LICENSE file or at
 #include "pq.h"
 #include "reader.h"
 #include "record.h"
-#include "generic.h"
 #include "reftable-merged.h"
 #include "reftable-error.h"
 #include "system.h"
@@ -270,40 +269,3 @@ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)
 {
 	return mt->hash_id;
 }
-
-static void reftable_merged_table_init_iter_void(void *tab,
-						 struct reftable_iterator *it,
-						 uint8_t typ)
-{
-	merged_table_init_iter(tab, it, typ);
-}
-
-static uint32_t reftable_merged_table_hash_id_void(void *tab)
-{
-	return reftable_merged_table_hash_id(tab);
-}
-
-static uint64_t reftable_merged_table_min_update_index_void(void *tab)
-{
-	return reftable_merged_table_min_update_index(tab);
-}
-
-static uint64_t reftable_merged_table_max_update_index_void(void *tab)
-{
-	return reftable_merged_table_max_update_index(tab);
-}
-
-static struct reftable_table_vtable merged_table_vtable = {
-	.init_iter = reftable_merged_table_init_iter_void,
-	.hash_id = reftable_merged_table_hash_id_void,
-	.min_update_index = reftable_merged_table_min_update_index_void,
-	.max_update_index = reftable_merged_table_max_update_index_void,
-};
-
-void reftable_table_from_merged_table(struct reftable_table *tab,
-				      struct reftable_merged_table *merged)
-{
-	assert(!tab->ops);
-	tab->ops = &merged_table_vtable;
-	tab->table_arg = merged;
-}
reftable/reader.c +0 -41

56 lines not on this page · raw message

reftable/reftable-generic.h deleted +0 -44

44 lines not on this page · raw message

reftable/reftable-merged.h +0 -6
index 03c2619c0ff..16d19f8df20 100644
@@ -26,8 +26,6 @@ license that can be found in the LICENSE file or at
 /* A merged table is implements seeking/iterating over a stack of tables. */
 struct reftable_merged_table;
 
-/* A generic reftable; see below. */
-struct reftable_table;
 struct reftable_reader;
 
 /*
@@ -60,8 +58,4 @@ void reftable_merged_table_free(struct reftable_merged_table *m);
 /* return the hash ID of the merged table. */
 uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *m);
 
-/* create a generic table from reftable_merged_table */
-void reftable_table_from_merged_table(struct reftable_table *tab,
-				      struct reftable_merged_table *table);
-
 #endif
reftable/reftable-reader.h +0 -7
index 7c7d1716516..69621c5b0fc 100644
@@ -23,9 +23,6 @@
 /* The reader struct is a handle to an open reftable file. */
 struct reftable_reader;
 
-/* Generic table. */
-struct reftable_table;
-
 /* reftable_new_reader opens a reftable for reading. If successful,
  * returns 0 code and sets pp. The name is used for creating a
  * stack. Typically, it is the basename of the file. The block source
@@ -60,10 +57,6 @@ uint64_t reftable_reader_max_update_index(struct reftable_reader *r);
 /* return the min_update_index for a table */
 uint64_t reftable_reader_min_update_index(struct reftable_reader *r);
 
-/* creates a generic table from a file reader. */
-void reftable_table_from_reader(struct reftable_table *tab,
-				struct reftable_reader *reader);
-
 /* print blocks onto stdout for debugging. */
 int reftable_reader_print_blocks(const char *tablename);
 
reftable/stack.c +0 -1
index bedd503e7e1..d3a95d2f1d7 100644
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at
 #include "merged.h"
 #include "reader.h"
 #include "reftable-error.h"
-#include "reftable-generic.h"
 #include "reftable-record.h"
 #include "reftable-merged.h"
 #include "writer.h"
t/unit-tests/t-reftable-merged.c +0 -1
index 577b1a5be87..93345c6c8be 100644
@@ -12,7 +12,6 @@ license that can be found in the LICENSE file or at
 #include "reftable/merged.h"
 #include "reftable/reader.h"
 #include "reftable/reftable-error.h"
-#include "reftable/reftable-generic.h"
 #include "reftable/reftable-merged.h"
 #include "reftable/reftable-writer.h"
 
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
#38 lore raw
In reply to Patrick Steinhardt

Patrick Steinhardt <[email protected]> writes:

[snip]

Quoted 24 lines

Range-diff against v2:

 1:  472c169b501 =  1:  472c169b501 reftable/merged: expose functions to initialize iterators
 2:  bc6f1cd8c1b =  2:  bc6f1cd8c1b reftable/merged: rename `reftable_new_merged_table()`
 3:  58e91ab4b34 =  3:  58e91ab4b34 reftable/merged: stop using generic tables in the merged table
 4:  6ba3fcee411 =  4:  6ba3fcee411 reftable/stack: open-code reading refs
 5:  cac08a934c5 =  5:  cac08a934c5 reftable/iter: drop double-checking logic
 6:  103262dc79c =  6:  103262dc79c reftable/generic: move generic iterator code into iterator interface
 7:  4011fa65d81 =  7:  4011fa65d81 reftable/dump: drop unused `compact_stack()`
 8:  ceaa296bfd4 =  8:  ceaa296bfd4 t/helper: inline `reftable_dump_main()`
 9:  a62e4612e97 =  9:  a62e4612e97 t/helper: inline `reftable_reader_print_file()`
10:  7acfe4fecc5 ! 10:  242c179df5f t/helper: inline `reftable_stack_print_directory()`

@@ Commit message
Move reftable_stack_print_directory() into the "dump-reftable" helper. This follows the same reasoning as the preceding commit.

+ Note that this requires us to remove the tests for this functionality in
+ reftable/stack_test.c. The test does not really add much anyway,
+ because all it verifies is that we do not crash or run into an error,
+ and it specifically doesn't check the outputted data. Also, as the code
+ is now part of the test helper, it doesn't make much sense to have a
+ unit test for it in the first place.
+

Quoted 8 lines

## reftable/reftable-stack.h ##

11:  8bd53a1a656 = 11:  a05e2060996 t/helper: inline `reftable_table_print()`
12:  c50aabbb804 = 12:  ee22a08e11e t/helper: inline printing of reftable records
13:  5498395872c = 13:  0a3c619e842 t/helper: use `hash_to_hex_algop()` to print hashes
14:  5390be75c37 = 14:  8eab399dfc6 t/helper: refactor to not use `struct reftable_table`
15:  5aeab8ee077 = 15:  b5d7b5679b5 reftable/generic: drop interface

--
2.46.0.164.g477ce5ccd6.dirty

This was the only change I requested in the previous version. So looks good to me now!

Thanks
Karthik

#39 lore raw
In reply to Patrick Steinhardt
Quoted 4 lines · Patrick Steinhardt

this is the third version of my patch series that gets rid of the generic reftable_table interface. It made it way harder to understand the reftable code base and is not really required nowadays anymore where we have generic re-seekable reftable iterators.

Looking good. Let me mark it for 'next'. Thanks.