{"thread":{"id":"61939","subject":"[PATCH 01/10] reftable/merged: expose functions to initialize iterators","startedAt":"2024-08-13T06:24:06Z","lastAt":"2024-08-20T12:00:56Z","messageCount":25,"participants":["Patrick Steinhardt","Eric Sunshine","karthik nayak","Junio C Hamano","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"500676","messageId":"404d64effde3bee424e338d858ed507ff83dff20.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 01/10] reftable/merged: expose functions to initialize iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:04Z","receivedAt":"2024-08-13T06:24:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not expose any functions via our public headers that would allow a\ncaller to initialize a reftable iterator from a merged table. Instead,\nthey are expected to go via the generic `reftable_table` interface,\nwhich is somewhat roundabout.\n\nImplement two new functions to initialize iterators for ref and log\nrecords to plug this gap.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/merged.c          | 12 ++++++++++++\n reftable/reftable-merged.h |  8 ++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 6adce44f4b..8d78b3da71 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,\n \titerator_from_merged_iter(it, mi);\n }\n \n+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n+\t\t\t\t\t     struct reftable_iterator *it)\n+{\n+\tmerged_table_init_iter(mt, it, BLOCK_TYPE_REF);\n+}\n+\n+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n+\t\t\t\t\t     struct reftable_iterator *it)\n+{\n+\tmerged_table_init_iter(mt, it, BLOCK_TYPE_LOG);\n+}\n+\n uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)\n {\n \treturn mt->hash_id;\ndiff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\nindex 14d5fc9f05..4deb0ad22e 100644\n--- a/reftable/reftable-merged.h\n+++ b/reftable/reftable-merged.h\n@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,\n \t\t\t      struct reftable_table *stack, size_t n,\n \t\t\t      uint32_t hash_id);\n \n+/* Initialize a merged table iterator for reading refs. */\n+void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n+\t\t\t\t\t     struct reftable_iterator *it);\n+\n+/* Initialize a merged table iterator for reading logs. */\n+void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n+\t\t\t\t\t     struct reftable_iterator *it);\n+\n /* returns the max update_index covered by this merged table. */\n uint64_t\n reftable_merged_table_max_update_index(struct reftable_merged_table *mt);\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500677","messageId":"cover.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":null,"subject":"[PATCH 00/10] reftable: drop generic `reftable_table` interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:00Z","receivedAt":"2024-08-13T06:24:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series performs some simplification of the reftable library\nby dropping the generic `reftable_table` interface. The intent of this\ninterface is to abstract away whether the underlying table is either a\nsingle reftable, or a merged reftable. Arguably though, it is not needed\nanymore starting with my recent refactorings [1] that made the reftable\niterator itself seekable, as that now provides such a generic interface\nalready.\n\nSo this patch series rips out the `reftable_table` interface, both to\nremove unused clutter, but more importantly to make the reftable library\neasier to understand overall. There is also a tiny performance gain of\n1-3% by requiring one less vtable function call. But while that speedup\nis consistent, I didn't include any benchmarks as it is rather close to\nnoise and not the primary motivation of this patch series.\n\nPatrick\n\n[1]: <cover.1715166175.git.ps@pks.im>\n\nPatrick Steinhardt (10):\n  reftable/merged: expose functions to initialize iterators\n  reftable/merged: rename `reftable_new_merged_table()`\n  reftable/merged: stop using generic tables in the merged table\n  reftable/stack: open-code reading refs\n  reftable/iter: drop double-checking logic\n  reftable/generic: move generic iterator code into iterator interface\n  reftable/dump: drop unused `compact_stack()`\n  reftable/dump: drop unused printing functionality\n  reftable/dump: move code into \"t/helper/test-reftable.c\"\n  reftable/generic: drop interface\n\n Makefile                         |   2 -\n reftable/dump.c                  | 111 ---------------\n reftable/generic.c               | 229 -------------------------------\n reftable/generic.h               |  37 -----\n reftable/iter.c                  | 127 ++++++++++++++---\n reftable/iter.h                  |  30 +++-\n reftable/merged.c                |  72 ++++------\n reftable/merged.h                |   4 +-\n reftable/reader.c                |  70 +---------\n reftable/reader.h                |   4 +\n reftable/record.c                | 127 -----------------\n reftable/record.h                |   4 -\n reftable/reftable-generic.h      |  47 -------\n reftable/reftable-merged.h       |  26 ++--\n reftable/reftable-reader.h       |   9 --\n reftable/reftable-record.h       |   8 --\n reftable/reftable-stack.h        |   3 -\n reftable/stack.c                 |  94 ++++++-------\n reftable/stack_test.c            |  29 ++--\n t/helper/test-reftable.c         |  47 ++++++-\n t/unit-tests/t-reftable-merged.c |  17 +--\n 21 files changed, 281 insertions(+), 816 deletions(-)\n delete mode 100644 reftable/dump.c\n delete mode 100644 reftable/generic.c\n delete mode 100644 reftable/generic.h\n delete mode 100644 reftable/reftable-generic.h\n\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500678","messageId":"511416fb73f2ab42dac08966ca6e82ced0f91e50.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 02/10] reftable/merged: rename `reftable_new_merged_table()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:07Z","receivedAt":"2024-08-13T06:24:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename `reftable_new_merged_table()` to `reftable_merged_table_new()`\nsuch that the name matches our coding style.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/merged.c                | 2 +-\n reftable/reftable-merged.h       | 9 +++++----\n reftable/stack.c                 | 4 ++--\n t/unit-tests/t-reftable-merged.c | 8 ++++----\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 8d78b3da71..25d414ec41 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -192,7 +192,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,\n \tit->ops = &merged_iter_vtable;\n }\n \n-int reftable_new_merged_table(struct reftable_merged_table **dest,\n+int reftable_merged_table_new(struct reftable_merged_table **dest,\n \t\t\t      struct reftable_table *stack, size_t n,\n \t\t\t      uint32_t hash_id)\n {\ndiff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\nindex 4deb0ad22e..72762483b9 100644\n--- a/reftable/reftable-merged.h\n+++ b/reftable/reftable-merged.h\n@@ -29,10 +29,11 @@ struct reftable_merged_table;\n /* A generic reftable; see below. */\n struct reftable_table;\n \n-/* reftable_new_merged_table creates a new merged table. It takes ownership of\n-   the stack array.\n-*/\n-int reftable_new_merged_table(struct reftable_merged_table **dest,\n+/*\n+ * reftable_merged_table_new creates a new merged table. It takes ownership of\n+ * the stack array.\n+ */\n+int reftable_merged_table_new(struct reftable_merged_table **dest,\n \t\t\t      struct reftable_table *stack, size_t n,\n \t\t\t      uint32_t hash_id);\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2071e428a8..64c7fdf8c4 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -272,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t}\n \n \t/* success! */\n-\terr = reftable_new_merged_table(&new_merged, new_tables,\n+\terr = reftable_merged_table_new(&new_merged, new_tables,\n \t\t\t\t\tnew_readers_len, st->opts.hash_id);\n \tif (err < 0)\n \t\tgoto done;\n@@ -924,7 +924,7 @@ static int stack_write_compact(struct reftable_stack *st,\n \treftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n \t\t\t\t   st->readers[last]->max_update_index);\n \n-\terr = reftable_new_merged_table(&mt, subtabs, subtabs_len,\n+\terr = reftable_merged_table_new(&mt, subtabs, subtabs_len,\n \t\t\t\t\tst->opts.hash_id);\n \tif (err < 0) {\n \t\treftable_free(subtabs);\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex b6263ee8b5..210603e8c7 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -111,7 +111,7 @@ merged_table_from_records(struct reftable_ref_record **refs,\n \t\treftable_table_from_reader(&tabs[i], (*readers)[i]);\n \t}\n \n-\terr = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \treturn mt;\n }\n@@ -289,7 +289,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \t\treftable_table_from_reader(&tabs[i], (*readers)[i]);\n \t}\n \n-\terr = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \treturn mt;\n }\n@@ -441,9 +441,9 @@ static void t_default_write_opts(void)\n \tcheck_int(hash_id, ==, GIT_SHA1_FORMAT_ID);\n \n \treftable_table_from_reader(&tab[0], rd);\n-\terr = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID);\n+\terr = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);\n \tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n-\terr = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \n \treftable_reader_free(rd);\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500679","messageId":"86e2f9f5dc64eee97510e597820791287032ba29.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 03/10] reftable/merged: stop using generic tables in the merged table","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:09Z","receivedAt":"2024-08-13T06:24:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The merged table provides access to a reftable stack by merging the\ncontents of those tables into a virtual table. These subtables are being\ntracked via `struct reftable_table`, which is a generic interface for\naccessing either a single reftable or a merged reftable. So in theory,\nit would be possible for the merged table to merge together other merged\ntables.\n\nThis is somewhat nonsensical though: we only ever set up a merged table\nover normal reftables, and there is no reason to do otherwise. This\ngeneric interface thus makes the code way harder to follow and reason\nabout than really necessary. The abstraction layer may also have an\nimpact on performance, even though the extra set of vtable function\ncalls probably doesn't really matter.\n\nRefactor the merged tables to use a `struct reftable_reader` for each of\nthe subtables instead, which gives us direct access to the underlying\ntables. Adjust names accordingly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/merged.c                | 28 +++++++++----------\n reftable/merged.h                |  4 +--\n reftable/reader.c                |  6 ++--\n reftable/reader.h                |  4 +++\n reftable/reftable-merged.h       |  7 +++--\n reftable/stack.c                 | 48 ++++++++++++--------------------\n reftable/stack_test.c            | 22 +++++++--------\n t/unit-tests/t-reftable-merged.c | 16 +++--------\n 8 files changed, 60 insertions(+), 75 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 25d414ec41..2e72eab306 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at\n #include \"constants.h\"\n #include \"iter.h\"\n #include \"pq.h\"\n+#include \"reader.h\"\n #include \"record.h\"\n #include \"generic.h\"\n #include \"reftable-merged.h\"\n@@ -25,7 +26,7 @@ struct merged_subiter {\n struct merged_iter {\n \tstruct merged_subiter *subiters;\n \tstruct merged_iter_pqueue pq;\n-\tsize_t stack_len;\n+\tsize_t subiters_len;\n \tint suppress_deletions;\n \tssize_t advance_index;\n };\n@@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi,\n \tmi->advance_index = -1;\n \tmi->suppress_deletions = mt->suppress_deletions;\n \n-\tREFTABLE_CALLOC_ARRAY(mi->subiters, mt->stack_len);\n-\tfor (size_t i = 0; i < mt->stack_len; i++) {\n+\tREFTABLE_CALLOC_ARRAY(mi->subiters, mt->readers_len);\n+\tfor (size_t i = 0; i < mt->readers_len; i++) {\n \t\treftable_record_init(&mi->subiters[i].rec, typ);\n-\t\ttable_init_iter(&mt->stack[i], &mi->subiters[i].iter, typ);\n+\t\treader_init_iter(mt->readers[i], &mi->subiters[i].iter, typ);\n \t}\n-\tmi->stack_len = mt->stack_len;\n+\tmi->subiters_len = mt->readers_len;\n }\n \n static void merged_iter_close(void *p)\n@@ -51,7 +52,7 @@ static void merged_iter_close(void *p)\n \tstruct merged_iter *mi = p;\n \n \tmerged_iter_pqueue_release(&mi->pq);\n-\tfor (size_t i = 0; i < mi->stack_len; i++) {\n+\tfor (size_t i = 0; i < mi->subiters_len; i++) {\n \t\treftable_iterator_destroy(&mi->subiters[i].iter);\n \t\treftable_record_release(&mi->subiters[i].rec);\n \t}\n@@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want\n \n \tmi->advance_index = -1;\n \n-\tfor (size_t i = 0; i < mi->stack_len; i++) {\n+\tfor (size_t i = 0; i < mi->subiters_len; i++) {\n \t\terr = iterator_seek(&mi->subiters[i].iter, want);\n \t\tif (err < 0)\n \t\t\treturn err;\n@@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,\n }\n \n int reftable_merged_table_new(struct reftable_merged_table **dest,\n-\t\t\t      struct reftable_table *stack, size_t n,\n+\t\t\t      struct reftable_reader **readers, size_t n,\n \t\t\t      uint32_t hash_id)\n {\n \tstruct reftable_merged_table *m = NULL;\n@@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,\n \tuint64_t first_min = 0;\n \n \tfor (size_t i = 0; i < n; i++) {\n-\t\tuint64_t min = reftable_table_min_update_index(&stack[i]);\n-\t\tuint64_t max = reftable_table_max_update_index(&stack[i]);\n+\t\tuint64_t min = reftable_reader_min_update_index(readers[i]);\n+\t\tuint64_t max = reftable_reader_max_update_index(readers[i]);\n \n-\t\tif (reftable_table_hash_id(&stack[i]) != hash_id) {\n+\t\tif (reftable_reader_hash_id(readers[i]) != hash_id) {\n \t\t\treturn REFTABLE_FORMAT_ERROR;\n \t\t}\n \t\tif (i == 0 || min < first_min) {\n@@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,\n \t}\n \n \tREFTABLE_CALLOC_ARRAY(m, 1);\n-\tm->stack = stack;\n-\tm->stack_len = n;\n+\tm->readers = readers;\n+\tm->readers_len = n;\n \tm->min = first_min;\n \tm->max = last_max;\n \tm->hash_id = hash_id;\n@@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt)\n {\n \tif (!mt)\n \t\treturn;\n-\tFREE_AND_NULL(mt->stack);\n \treftable_free(mt);\n }\n \ndiff --git a/reftable/merged.h b/reftable/merged.h\nindex 2efe571da6..de5fd33f01 100644\n--- a/reftable/merged.h\n+++ b/reftable/merged.h\n@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n \n struct reftable_merged_table {\n-\tstruct reftable_table *stack;\n-\tsize_t stack_len;\n+\tstruct reftable_reader **readers;\n+\tsize_t readers_len;\n \tuint32_t hash_id;\n \n \t/* If unset, produce deletions. This is useful for compaction. For the\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 29c99e2269..f7ae35da72 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it,\n \tit->ops = &table_iter_vtable;\n }\n \n-static void reader_init_iter(struct reftable_reader *r,\n-\t\t\t     struct reftable_iterator *it,\n-\t\t\t     uint8_t typ)\n+void reader_init_iter(struct reftable_reader *r,\n+\t\t      struct reftable_iterator *it,\n+\t\t      uint8_t typ)\n {\n \tstruct reftable_reader_offsets *offs = reader_offsets_for(r, typ);\n \ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex e869165f23..a2c204d523 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\n \n+void reader_init_iter(struct reftable_reader *r,\n+\t\t      struct reftable_iterator *it,\n+\t\t      uint8_t typ);\n+\n /* initialize a block reader to read from `r` */\n int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,\n \t\t\t     uint64_t next_off, uint8_t want_typ);\ndiff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\nindex 72762483b9..03c2619c0f 100644\n--- a/reftable/reftable-merged.h\n+++ b/reftable/reftable-merged.h\n@@ -28,13 +28,14 @@ struct reftable_merged_table;\n \n /* A generic reftable; see below. */\n struct reftable_table;\n+struct reftable_reader;\n \n /*\n- * reftable_merged_table_new creates a new merged table. It takes ownership of\n- * the stack array.\n+ * reftable_merged_table_new creates a new merged table. The readers must be\n+ * kept alive as long as the merged table is still in use.\n  */\n int reftable_merged_table_new(struct reftable_merged_table **dest,\n-\t\t\t      struct reftable_table *stack, size_t n,\n+\t\t\t      struct reftable_reader **readers, size_t n,\n \t\t\t      uint32_t hash_id);\n \n /* Initialize a merged table iterator for reading refs. */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 64c7fdf8c4..7f4e267ea9 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -225,13 +225,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\t\t      const char **names,\n \t\t\t\t      int reuse_open)\n {\n-\tsize_t cur_len = !st->merged ? 0 : st->merged->stack_len;\n+\tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n \tstruct reftable_reader **cur = stack_copy_readers(st, cur_len);\n \tsize_t names_len = names_length(names);\n \tstruct reftable_reader **new_readers =\n \t\treftable_calloc(names_len, sizeof(*new_readers));\n-\tstruct reftable_table *new_tables =\n-\t\treftable_calloc(names_len, sizeof(*new_tables));\n \tsize_t new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n \tstruct strbuf table_path = STRBUF_INIT;\n@@ -267,17 +265,15 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t}\n \n \t\tnew_readers[new_readers_len] = rd;\n-\t\treftable_table_from_reader(&new_tables[new_readers_len], rd);\n \t\tnew_readers_len++;\n \t}\n \n \t/* success! */\n-\terr = reftable_merged_table_new(&new_merged, new_tables,\n+\terr = reftable_merged_table_new(&new_merged, new_readers,\n \t\t\t\t\tnew_readers_len, st->opts.hash_id);\n \tif (err < 0)\n \t\tgoto done;\n \n-\tnew_tables = NULL;\n \tst->readers_len = new_readers_len;\n \tif (st->merged)\n \t\treftable_merged_table_free(st->merged);\n@@ -309,7 +305,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\treftable_reader_free(new_readers[i]);\n \t}\n \treftable_free(new_readers);\n-\treftable_free(new_tables);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n \treturn err;\n@@ -520,7 +515,7 @@ static int stack_uptodate(struct reftable_stack *st)\n \t\t}\n \t}\n \n-\tif (names[st->merged->stack_len]) {\n+\tif (names[st->merged->readers_len]) {\n \t\terr = 1;\n \t\tgoto done;\n \t}\n@@ -659,7 +654,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tif (add->new_tables_len == 0)\n \t\tgoto done;\n \n-\tfor (i = 0; i < add->stack->merged->stack_len; i++) {\n+\tfor (i = 0; i < add->stack->merged->readers_len; i++) {\n \t\tstrbuf_addstr(&table_list, add->stack->readers[i]->name);\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n@@ -839,7 +834,7 @@ int reftable_addition_add(struct reftable_addition *add,\n \n uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n {\n-\tint sz = st->merged->stack_len;\n+\tint sz = st->merged->readers_len;\n \tif (sz > 0)\n \t\treturn reftable_reader_max_update_index(st->readers[sz - 1]) +\n \t\t       1;\n@@ -906,30 +901,23 @@ static int stack_write_compact(struct reftable_stack *st,\n \t\t\t       size_t first, size_t last,\n \t\t\t       struct reftable_log_expiry_config *config)\n {\n-\tsize_t subtabs_len = last - first + 1;\n-\tstruct reftable_table *subtabs = reftable_calloc(\n-\t\tlast - first + 1, sizeof(*subtabs));\n \tstruct reftable_merged_table *mt = NULL;\n \tstruct reftable_iterator it = { NULL };\n \tstruct reftable_ref_record ref = { NULL };\n \tstruct reftable_log_record log = { NULL };\n+\tsize_t subtabs_len = last - first + 1;\n \tuint64_t entries = 0;\n \tint err = 0;\n \n-\tfor (size_t i = first, j = 0; i <= last; i++) {\n-\t\tstruct reftable_reader *t = st->readers[i];\n-\t\treftable_table_from_reader(&subtabs[j++], t);\n-\t\tst->stats.bytes += t->size;\n-\t}\n+\tfor (size_t i = first; i <= last; i++)\n+\t\tst->stats.bytes += st->readers[i]->size;\n \treftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n \t\t\t\t   st->readers[last]->max_update_index);\n \n-\terr = reftable_merged_table_new(&mt, subtabs, subtabs_len,\n+\terr = reftable_merged_table_new(&mt, st->readers + first, subtabs_len,\n \t\t\t\t\tst->opts.hash_id);\n-\tif (err < 0) {\n-\t\treftable_free(subtabs);\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n \n \tmerged_table_init_iter(mt, &it, BLOCK_TYPE_REF);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n@@ -1207,7 +1195,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t * have compacted them.\n \t\t\t */\n \t\t\tfor (size_t j = 1; j < last - first + 1; j++) {\n-\t\t\t\tconst char *old = first + j < st->merged->stack_len ?\n+\t\t\t\tconst char *old = first + j < st->merged->readers_len ?\n \t\t\t\t\tst->readers[first + j]->name : NULL;\n \t\t\t\tconst char *new = names[i + j];\n \n@@ -1248,10 +1236,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t * `fd_read_lines()` uses a `NULL` sentinel to indicate that\n \t\t * the array is at its end. As we use `free_names()` to free\n \t\t * the array, we need to include this sentinel value here and\n-\t\t * thus have to allocate `stack_len + 1` many entries.\n+\t\t * thus have to allocate `readers_len + 1` many entries.\n \t\t */\n-\t\tREFTABLE_CALLOC_ARRAY(names, st->merged->stack_len + 1);\n-\t\tfor (size_t i = 0; i < st->merged->stack_len; i++)\n+\t\tREFTABLE_CALLOC_ARRAY(names, st->merged->readers_len + 1);\n+\t\tfor (size_t i = 0; i < st->merged->readers_len; i++)\n \t\t\tnames[i] = xstrdup(st->readers[i]->name);\n \t\tfirst_to_replace = first;\n \t\tlast_to_replace = last;\n@@ -1358,7 +1346,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,\n int reftable_stack_compact_all(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *config)\n {\n-\tsize_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0;\n+\tsize_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0;\n \treturn stack_compact_range_stats(st, 0, last, config, 0);\n }\n \n@@ -1449,9 +1437,9 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n \tint overhead = header_size(version) - 1;\n \tuint64_t *sizes;\n \n-\tREFTABLE_CALLOC_ARRAY(sizes, st->merged->stack_len);\n+\tREFTABLE_CALLOC_ARRAY(sizes, st->merged->readers_len);\n \n-\tfor (size_t i = 0; i < st->merged->stack_len; i++)\n+\tfor (size_t i = 0; i < st->merged->readers_len; i++)\n \t\tsizes[i] = st->readers[i]->size - overhead;\n \n \treturn sizes;\n@@ -1461,7 +1449,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n {\n \tuint64_t *sizes = stack_table_sizes_for_compaction(st);\n \tstruct segment seg =\n-\t\tsuggest_compaction_segment(sizes, st->merged->stack_len,\n+\t\tsuggest_compaction_segment(sizes, st->merged->readers_len,\n \t\t\t\t\t   st->opts.auto_compaction_factor);\n \treftable_free(sizes);\n \tif (segment_size(&seg) > 0)\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 8c36590ff0..dbca9eaf4a 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n \t\t * all tables in the stack.\n \t\t */\n \t\tif (i != n)\n-\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\t\tEXPECT(st->merged->readers_len == i + 1);\n \t\telse\n-\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t\t\tEXPECT(st->merged->readers_len == 1);\n \t}\n \n \treftable_stack_destroy(st);\n@@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n \n \terr = reftable_stack_add(st, write_test_ref, &ref);\n \tEXPECT_ERR(err);\n-\tEXPECT(st->merged->stack_len == 1);\n+\tEXPECT(st->merged->readers_len == 1);\n \tEXPECT(st->stats.attempts == 0);\n \tEXPECT(st->stats.failures == 0);\n \n@@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n \tref.update_index = 2;\n \terr = reftable_stack_add(st, write_test_ref, &ref);\n \tEXPECT_ERR(err);\n-\tEXPECT(st->merged->stack_len == 2);\n+\tEXPECT(st->merged->readers_len == 2);\n \tEXPECT(st->stats.attempts == 1);\n \tEXPECT(st->stats.failures == 1);\n \n@@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void)\n \n \t\terr = reftable_stack_auto_compact(st);\n \t\tEXPECT_ERR(err);\n-\t\tEXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));\n+\t\tEXPECT(i < 3 || st->merged->readers_len < 2 * fastlog2(i));\n \t}\n \n \tEXPECT(reftable_stack_compaction_stats(st)->entries_written <\n@@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)\n \tEXPECT_ERR(err);\n \n \twrite_n_ref_tables(st, 5);\n-\tEXPECT(st->merged->stack_len == 5);\n+\tEXPECT(st->merged->readers_len == 5);\n \n \t/*\n \t * Given that all tables we have written should be roughly the same\n@@ -925,7 +925,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)\n \terr = reftable_stack_auto_compact(st);\n \tEXPECT_ERR(err);\n \tEXPECT(st->stats.failures == 0);\n-\tEXPECT(st->merged->stack_len == 4);\n+\tEXPECT(st->merged->readers_len == 4);\n \n \treftable_stack_destroy(st);\n \tstrbuf_release(&buf);\n@@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void)\n \t\t * all tables in the stack.\n \t\t */\n \t\tif (i != n)\n-\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\t\tEXPECT(st->merged->readers_len == i + 1);\n \t\telse\n-\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t\t\tEXPECT(st->merged->readers_len == 1);\n \t}\n \n \treftable_stack_destroy(st);\n@@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)\n \tEXPECT_ERR(err);\n \n \twrite_n_ref_tables(st, 3);\n-\tEXPECT(st->merged->stack_len == 3);\n+\tEXPECT(st->merged->readers_len == 3);\n \n \t/* Lock one of the tables that we're about to compact. */\n \tstrbuf_reset(&buf);\n@@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)\n \terr = reftable_stack_compact_all(st, NULL);\n \tEXPECT(err == REFTABLE_LOCK_ERROR);\n \tEXPECT(st->stats.failures == 1);\n-\tEXPECT(st->merged->stack_len == 3);\n+\tEXPECT(st->merged->readers_len == 3);\n \n \treftable_stack_destroy(st);\n \tstrbuf_release(&buf);\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 210603e8c7..577b1a5be8 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -94,10 +94,8 @@ merged_table_from_records(struct reftable_ref_record **refs,\n \t\t\t  struct strbuf *buf, const size_t n)\n {\n \tstruct reftable_merged_table *mt = NULL;\n-\tstruct reftable_table *tabs;\n \tint err;\n \n-\tREFTABLE_CALLOC_ARRAY(tabs, n);\n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n@@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs,\n \t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n-\t\treftable_table_from_reader(&tabs[i], (*readers)[i]);\n \t}\n \n-\terr = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \treturn mt;\n }\n@@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \t\t\t      struct strbuf *buf, const size_t n)\n {\n \tstruct reftable_merged_table *mt = NULL;\n-\tstruct reftable_table *tabs;\n \tint err;\n \n-\tREFTABLE_CALLOC_ARRAY(tabs, n);\n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n@@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n-\t\treftable_table_from_reader(&tabs[i], (*readers)[i]);\n \t}\n \n-\terr = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \treturn mt;\n }\n@@ -418,7 +412,6 @@ static void t_default_write_opts(void)\n \t};\n \tint err;\n \tstruct reftable_block_source source = { 0 };\n-\tstruct reftable_table *tab = reftable_calloc(1, sizeof(*tab));\n \tuint32_t hash_id;\n \tstruct reftable_reader *rd = NULL;\n \tstruct reftable_merged_table *merged = NULL;\n@@ -440,10 +433,9 @@ static void t_default_write_opts(void)\n \thash_id = reftable_reader_hash_id(rd);\n \tcheck_int(hash_id, ==, GIT_SHA1_FORMAT_ID);\n \n-\treftable_table_from_reader(&tab[0], rd);\n-\terr = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);\n+\terr = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA256_FORMAT_ID);\n \tcheck_int(err, ==, REFTABLE_FORMAT_ERROR);\n-\terr = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);\n+\terr = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \n \treftable_reader_free(rd);\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500680","messageId":"d5d24e03bd9d901e83ff3cd0f580b66e3601482a.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 04/10] reftable/stack: open-code reading refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:12Z","receivedAt":"2024-08-13T06:24:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"To read a reference for the reftable stack, we first create a generic\n`reftable_table` from the merged table and then read the reference via a\nconvenience function. We are about to remove these generic interfaces,\nso let's instead open-code the logic to prepare for this removal.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 7f4e267ea9..d08ec00959 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1468,9 +1468,28 @@ reftable_stack_compaction_stats(struct reftable_stack *st)\n int reftable_stack_read_ref(struct reftable_stack *st, const char *refname,\n \t\t\t    struct reftable_ref_record *ref)\n {\n-\tstruct reftable_table tab = { NULL };\n-\treftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));\n-\treturn reftable_table_read_ref(&tab, refname, ref);\n+\tstruct reftable_iterator it = { 0 };\n+\tint ret;\n+\n+\treftable_merged_table_init_ref_iterator(st->merged, &it);\n+\tret = reftable_iterator_seek_ref(&it, refname);\n+\tif (ret)\n+\t\tgoto out;\n+\n+\tret = reftable_iterator_next_ref(&it, ref);\n+\tif (ret)\n+\t\tgoto out;\n+\n+\tif (strcmp(ref->refname, refname) ||\n+\t    reftable_ref_record_is_deletion(ref)) {\n+\t\treftable_ref_record_release(ref);\n+\t\tret = 1;\n+\t\tgoto out;\n+\t}\n+\n+out:\n+\treftable_iterator_destroy(&it);\n+\treturn ret;\n }\n \n int reftable_stack_read_log(struct reftable_stack *st, const char *refname,\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500681","messageId":"ab7538d0bbb8b3c02b5db71a421163fbf87a45f6.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 05/10] reftable/iter: drop double-checking logic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:26Z","receivedAt":"2024-08-13T06:24:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The filtering ref iterator can be used to only yield refs which are not\nin a specific skip list. This iterator has an option to double-check the\nresults it returns, which causes us to seek tho reference we are about\nto yield via a separate table such that we detect whether the reference\nthat the first iterator has yielded actually exists.\n\nThe value of this is somewhat dubious, and I cannot think of any usecase\nwhere this functionality should be required. Furthermore, this option is\nnever set in our codebase, which means that it is essentially untested.\nAnd last but not least, the `struct reftable_table` that is used to\nimplement it is about to go away.\n\nSo while we could refactor the code to not use a `reftable_table`, it\nvery much feels like a wasted effort. Let's just drop this code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/iter.c   | 20 --------------------\n reftable/iter.h   |  2 --\n reftable/reader.c |  2 --\n 3 files changed, 24 deletions(-)\n\ndiff --git a/reftable/iter.c b/reftable/iter.c\nindex fddea31e51..a7484aba60 100644\n--- a/reftable/iter.c\n+++ b/reftable/iter.c\n@@ -42,26 +42,6 @@ static int filtering_ref_iterator_next(void *iter_arg,\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (fri->double_check) {\n-\t\t\tstruct reftable_iterator it = { NULL };\n-\n-\t\t\treftable_table_init_ref_iter(&fri->tab, &it);\n-\n-\t\t\terr = reftable_iterator_seek_ref(&it, ref->refname);\n-\t\t\tif (err == 0)\n-\t\t\t\terr = reftable_iterator_next_ref(&it, ref);\n-\n-\t\t\treftable_iterator_destroy(&it);\n-\n-\t\t\tif (err < 0) {\n-\t\t\t\tbreak;\n-\t\t\t}\n-\n-\t\t\tif (err > 0) {\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n-\n \t\tif (ref->value_type == REFTABLE_REF_VAL2 &&\n \t\t    (!memcmp(fri->oid.buf, ref->value.val2.target_value,\n \t\t\t     fri->oid.len) ||\ndiff --git a/reftable/iter.h b/reftable/iter.h\nindex 537431baba..b75d7ac2ac 100644\n--- a/reftable/iter.h\n+++ b/reftable/iter.h\n@@ -18,8 +18,6 @@ license that can be found in the LICENSE file or at\n \n /* iterator that produces only ref records that point to `oid` */\n struct filtering_ref_iterator {\n-\tint double_check;\n-\tstruct reftable_table tab;\n \tstruct strbuf oid;\n \tstruct reftable_iterator it;\n };\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex f7ae35da72..e3f5854229 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -735,8 +735,6 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,\n \t*filter = empty;\n \n \tstrbuf_add(&filter->oid, oid, oid_len);\n-\treftable_table_from_reader(&filter->tab, r);\n-\tfilter->double_check = 0;\n \titerator_from_table_iter(&filter->it, ti);\n \n \titerator_from_filtering_ref_iterator(it, filter);\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500682","messageId":"14924604cebe20ac30d291399b0200016fa8b4e3.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 06/10] reftable/generic: move generic iterator code into iterator interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:29Z","receivedAt":"2024-08-13T06:24:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move functions relating to the reftable iterator from \"generic.c\" into\n\"iter.c\". This prepares for the removal of the former subsystem.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/generic.c | 107 +--------------------------------------------\n reftable/generic.h |  10 -----\n reftable/iter.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++\n reftable/iter.h    |  27 ++++++++++++\n 4 files changed, 134 insertions(+), 116 deletions(-)\n\ndiff --git a/reftable/generic.c b/reftable/generic.c\nindex 28ae26145e..6ecf9b880f 100644\n--- a/reftable/generic.c\n+++ b/reftable/generic.c\n@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at\n #include \"constants.h\"\n #include \"record.h\"\n #include \"generic.h\"\n+#include \"iter.h\"\n #include \"reftable-iterator.h\"\n #include \"reftable-generic.h\"\n \n@@ -32,37 +33,6 @@ void reftable_table_init_log_iter(struct reftable_table *tab,\n \ttable_init_iter(tab, it, BLOCK_TYPE_LOG);\n }\n \n-int reftable_iterator_seek_ref(struct reftable_iterator *it,\n-\t\t\t       const char *name)\n-{\n-\tstruct reftable_record want = {\n-\t\t.type = BLOCK_TYPE_REF,\n-\t\t.u.ref = {\n-\t\t\t.refname = (char *)name,\n-\t\t},\n-\t};\n-\treturn it->ops->seek(it->iter_arg, &want);\n-}\n-\n-int reftable_iterator_seek_log_at(struct reftable_iterator *it,\n-\t\t\t\t  const char *name, uint64_t update_index)\n-{\n-\tstruct reftable_record want = {\n-\t\t.type = BLOCK_TYPE_LOG,\n-\t\t.u.log = {\n-\t\t\t.refname = (char *)name,\n-\t\t\t.update_index = update_index,\n-\t\t},\n-\t};\n-\treturn it->ops->seek(it->iter_arg, &want);\n-}\n-\n-int reftable_iterator_seek_log(struct reftable_iterator *it,\n-\t\t\t       const char *name)\n-{\n-\treturn reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));\n-}\n-\n int reftable_table_read_ref(struct reftable_table *tab, const char *name,\n \t\t\t    struct reftable_ref_record *ref)\n {\n@@ -152,78 +122,3 @@ uint32_t reftable_table_hash_id(struct reftable_table *tab)\n {\n \treturn tab->ops->hash_id(tab->table_arg);\n }\n-\n-void reftable_iterator_destroy(struct reftable_iterator *it)\n-{\n-\tif (!it->ops) {\n-\t\treturn;\n-\t}\n-\tit->ops->close(it->iter_arg);\n-\tit->ops = NULL;\n-\tFREE_AND_NULL(it->iter_arg);\n-}\n-\n-int reftable_iterator_next_ref(struct reftable_iterator *it,\n-\t\t\t       struct reftable_ref_record *ref)\n-{\n-\tstruct reftable_record rec = {\n-\t\t.type = BLOCK_TYPE_REF,\n-\t\t.u = {\n-\t\t\t.ref = *ref\n-\t\t},\n-\t};\n-\tint err = iterator_next(it, &rec);\n-\t*ref = rec.u.ref;\n-\treturn err;\n-}\n-\n-int reftable_iterator_next_log(struct reftable_iterator *it,\n-\t\t\t       struct reftable_log_record *log)\n-{\n-\tstruct reftable_record rec = {\n-\t\t.type = BLOCK_TYPE_LOG,\n-\t\t.u = {\n-\t\t\t.log = *log,\n-\t\t},\n-\t};\n-\tint err = iterator_next(it, &rec);\n-\t*log = rec.u.log;\n-\treturn err;\n-}\n-\n-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)\n-{\n-\treturn it->ops->seek(it->iter_arg, want);\n-}\n-\n-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)\n-{\n-\treturn it->ops->next(it->iter_arg, rec);\n-}\n-\n-static int empty_iterator_seek(void *arg, struct reftable_record *want)\n-{\n-\treturn 0;\n-}\n-\n-static int empty_iterator_next(void *arg, struct reftable_record *rec)\n-{\n-\treturn 1;\n-}\n-\n-static void empty_iterator_close(void *arg)\n-{\n-}\n-\n-static struct reftable_iterator_vtable empty_vtable = {\n-\t.seek = &empty_iterator_seek,\n-\t.next = &empty_iterator_next,\n-\t.close = &empty_iterator_close,\n-};\n-\n-void iterator_set_empty(struct reftable_iterator *it)\n-{\n-\tassert(!it->ops);\n-\tit->iter_arg = NULL;\n-\tit->ops = &empty_vtable;\n-}\ndiff --git a/reftable/generic.h b/reftable/generic.h\nindex 8341fa570e..837fbb8df2 100644\n--- a/reftable/generic.h\n+++ b/reftable/generic.h\n@@ -24,14 +24,4 @@ void table_init_iter(struct reftable_table *tab,\n \t\t     struct reftable_iterator *it,\n \t\t     uint8_t typ);\n \n-struct reftable_iterator_vtable {\n-\tint (*seek)(void *iter_arg, struct reftable_record *want);\n-\tint (*next)(void *iter_arg, struct reftable_record *rec);\n-\tvoid (*close)(void *iter_arg);\n-};\n-\n-void iterator_set_empty(struct reftable_iterator *it);\n-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);\n-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);\n-\n #endif\ndiff --git a/reftable/iter.c b/reftable/iter.c\nindex a7484aba60..844853fad2 100644\n--- a/reftable/iter.c\n+++ b/reftable/iter.c\n@@ -16,6 +16,43 @@ license that can be found in the LICENSE file or at\n #include \"reader.h\"\n #include \"reftable-error.h\"\n \n+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)\n+{\n+\treturn it->ops->seek(it->iter_arg, want);\n+}\n+\n+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)\n+{\n+\treturn it->ops->next(it->iter_arg, rec);\n+}\n+\n+static int empty_iterator_seek(void *arg, struct reftable_record *want)\n+{\n+\treturn 0;\n+}\n+\n+static int empty_iterator_next(void *arg, struct reftable_record *rec)\n+{\n+\treturn 1;\n+}\n+\n+static void empty_iterator_close(void *arg)\n+{\n+}\n+\n+static struct reftable_iterator_vtable empty_vtable = {\n+\t.seek = &empty_iterator_seek,\n+\t.next = &empty_iterator_next,\n+\t.close = &empty_iterator_close,\n+};\n+\n+void iterator_set_empty(struct reftable_iterator *it)\n+{\n+\tassert(!it->ops);\n+\tit->iter_arg = NULL;\n+\tit->ops = &empty_vtable;\n+}\n+\n static void filtering_ref_iterator_close(void *iter_arg)\n {\n \tstruct filtering_ref_iterator *fri = iter_arg;\n@@ -181,3 +218,72 @@ void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it,\n \tit->iter_arg = itr;\n \tit->ops = &indexed_table_ref_iter_vtable;\n }\n+\n+void reftable_iterator_destroy(struct reftable_iterator *it)\n+{\n+\tif (!it->ops) {\n+\t\treturn;\n+\t}\n+\tit->ops->close(it->iter_arg);\n+\tit->ops = NULL;\n+\tFREE_AND_NULL(it->iter_arg);\n+}\n+\n+int reftable_iterator_seek_ref(struct reftable_iterator *it,\n+\t\t\t       const char *name)\n+{\n+\tstruct reftable_record want = {\n+\t\t.type = BLOCK_TYPE_REF,\n+\t\t.u.ref = {\n+\t\t\t.refname = (char *)name,\n+\t\t},\n+\t};\n+\treturn it->ops->seek(it->iter_arg, &want);\n+}\n+\n+int reftable_iterator_next_ref(struct reftable_iterator *it,\n+\t\t\t       struct reftable_ref_record *ref)\n+{\n+\tstruct reftable_record rec = {\n+\t\t.type = BLOCK_TYPE_REF,\n+\t\t.u = {\n+\t\t\t.ref = *ref\n+\t\t},\n+\t};\n+\tint err = iterator_next(it, &rec);\n+\t*ref = rec.u.ref;\n+\treturn err;\n+}\n+\n+int reftable_iterator_seek_log_at(struct reftable_iterator *it,\n+\t\t\t\t  const char *name, uint64_t update_index)\n+{\n+\tstruct reftable_record want = {\n+\t\t.type = BLOCK_TYPE_LOG,\n+\t\t.u.log = {\n+\t\t\t.refname = (char *)name,\n+\t\t\t.update_index = update_index,\n+\t\t},\n+\t};\n+\treturn it->ops->seek(it->iter_arg, &want);\n+}\n+\n+int reftable_iterator_seek_log(struct reftable_iterator *it,\n+\t\t\t       const char *name)\n+{\n+\treturn reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));\n+}\n+\n+int reftable_iterator_next_log(struct reftable_iterator *it,\n+\t\t\t       struct reftable_log_record *log)\n+{\n+\tstruct reftable_record rec = {\n+\t\t.type = BLOCK_TYPE_LOG,\n+\t\t.u = {\n+\t\t\t.log = *log,\n+\t\t},\n+\t};\n+\tint err = iterator_next(it, &rec);\n+\t*log = rec.u.log;\n+\treturn err;\n+}\ndiff --git a/reftable/iter.h b/reftable/iter.h\nindex b75d7ac2ac..3b401f1259 100644\n--- a/reftable/iter.h\n+++ b/reftable/iter.h\n@@ -16,6 +16,33 @@ license that can be found in the LICENSE file or at\n #include \"reftable-iterator.h\"\n #include \"reftable-generic.h\"\n \n+/*\n+ * The virtual function table for implementing generic reftable iterators.\n+ */\n+struct reftable_iterator_vtable {\n+\tint (*seek)(void *iter_arg, struct reftable_record *want);\n+\tint (*next)(void *iter_arg, struct reftable_record *rec);\n+\tvoid (*close)(void *iter_arg);\n+};\n+\n+/*\n+ * Position the iterator at the wanted record such that a call to\n+ * `iterator_next()` would return that record, if it exists.\n+ */\n+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);\n+\n+/*\n+ * Yield the next record and advance the iterator. Returns <0 on error, 0 when\n+ * a record was yielded, and >0 when the iterator hit an error.\n+ */\n+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);\n+\n+/*\n+ * Set up the iterator such that it behaves the same as an iterator with no\n+ * entries.\n+ */\n+void iterator_set_empty(struct reftable_iterator *it);\n+\n /* iterator that produces only ref records that point to `oid` */\n struct filtering_ref_iterator {\n \tstruct strbuf oid;\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500683","messageId":"0aa718606767dc4890fee5af16bb9a0f01a688c1.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 07/10] reftable/dump: drop unused `compact_stack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:32Z","receivedAt":"2024-08-13T06:24:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `compact_stack()` function is exposed via `reftable_dump_main()`,\nwhich ultimately ends up being wired into \"test-tool reftable\". It is\nnever used by our tests though, and nowadays we have wired up support\nfor stack compaction into git-pack-refs(1).\n\nRemove the code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/dump.c | 24 ------------------------\n 1 file changed, 24 deletions(-)\n\ndiff --git a/reftable/dump.c b/reftable/dump.c\nindex dd65d9e8bb..2953e0a83a 100644\n--- a/reftable/dump.c\n+++ b/reftable/dump.c\n@@ -24,25 +24,6 @@ license that can be found in the LICENSE file or at\n #include <unistd.h>\n #include <string.h>\n \n-static int compact_stack(const char *stackdir)\n-{\n-\tstruct reftable_stack *stack = NULL;\n-\tstruct reftable_write_options opts = { 0 };\n-\n-\tint err = reftable_new_stack(&stack, stackdir, &opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_stack_compact_all(stack, NULL);\n-\tif (err < 0)\n-\t\tgoto done;\n-done:\n-\tif (stack) {\n-\t\treftable_stack_destroy(stack);\n-\t}\n-\treturn err;\n-}\n-\n static void print_help(void)\n {\n \tprintf(\"usage: dump [-cst] arg\\n\\n\"\n@@ -62,7 +43,6 @@ int reftable_dump_main(int argc, char *const *argv)\n \tint opt_dump_blocks = 0;\n \tint opt_dump_table = 0;\n \tint opt_dump_stack = 0;\n-\tint opt_compact = 0;\n \tuint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;\n \tconst char *arg = NULL, *argv0 = argv[0];\n \n@@ -77,8 +57,6 @@ int reftable_dump_main(int argc, char *const *argv)\n \t\t\topt_hash_id = GIT_SHA256_FORMAT_ID;\n \t\telse if (!strcmp(\"-s\", argv[1]))\n \t\t\topt_dump_stack = 1;\n-\t\telse if (!strcmp(\"-c\", argv[1]))\n-\t\t\topt_compact = 1;\n \t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n \t\t\tprint_help();\n \t\t\treturn 2;\n@@ -98,8 +76,6 @@ int reftable_dump_main(int argc, char *const *argv)\n \t\terr = reftable_reader_print_file(arg);\n \t} else if (opt_dump_stack) {\n \t\terr = reftable_stack_print_directory(arg, opt_hash_id);\n-\t} else if (opt_compact) {\n-\t\terr = compact_stack(arg);\n \t}\n \n \tif (err < 0) {\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500684","messageId":"1f211e514d049379ada609e6a4e8cf985e721271.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 08/10] reftable/dump: drop unused printing functionality","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:35Z","receivedAt":"2024-08-13T06:24:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a bunch of infrastructure wired up that allows us to print\nreftable records, tables and stacks. While this functionality is wired\nup via various \"test-tool reftable\" options, it is never used. It also\nfeels kind of dubious whether any other eventual user of the reftable\nlibrary should use it as it very much feels like a debugging aid rather\nthan something sensible. The format itself is somewhat inscrutable and\nthe infrastructure is non-extensible.\n\nDrop this code. The only remaining function in this context is\n`reftable_reader_print_blocks()`, which we do use in our tests.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/dump.c             |  16 +----\n reftable/generic.c          |  47 -------------\n reftable/reader.c           |  21 ------\n reftable/record.c           | 127 ------------------------------------\n reftable/record.h           |   4 --\n reftable/reftable-generic.h |   3 -\n reftable/reftable-reader.h  |   2 -\n reftable/reftable-record.h  |   8 ---\n reftable/reftable-stack.h   |   3 -\n reftable/stack.c            |  20 ------\n reftable/stack_test.c       |   7 --\n 11 files changed, 1 insertion(+), 257 deletions(-)\n\ndiff --git a/reftable/dump.c b/reftable/dump.c\nindex 2953e0a83a..35a1731da9 100644\n--- a/reftable/dump.c\n+++ b/reftable/dump.c\n@@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv)\n {\n \tint err = 0;\n \tint opt_dump_blocks = 0;\n-\tint opt_dump_table = 0;\n-\tint opt_dump_stack = 0;\n-\tuint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;\n \tconst char *arg = NULL, *argv0 = argv[0];\n \n \tfor (; argc > 1; argv++, argc--)\n@@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)\n \t\t\tbreak;\n \t\telse if (!strcmp(\"-b\", argv[1]))\n \t\t\topt_dump_blocks = 1;\n-\t\telse if (!strcmp(\"-t\", argv[1]))\n-\t\t\topt_dump_table = 1;\n-\t\telse if (!strcmp(\"-6\", argv[1]))\n-\t\t\topt_hash_id = GIT_SHA256_FORMAT_ID;\n-\t\telse if (!strcmp(\"-s\", argv[1]))\n-\t\t\topt_dump_stack = 1;\n \t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n \t\t\tprint_help();\n \t\t\treturn 2;\n@@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)\n \n \targ = argv[1];\n \n-\tif (opt_dump_blocks) {\n+\tif (opt_dump_blocks)\n \t\terr = reftable_reader_print_blocks(arg);\n-\t} else if (opt_dump_table) {\n-\t\terr = reftable_reader_print_file(arg);\n-\t} else if (opt_dump_stack) {\n-\t\terr = reftable_stack_print_directory(arg, opt_hash_id);\n-\t}\n \n \tif (err < 0) {\n \t\tfprintf(stderr, \"%s: %s: %s\\n\", argv0, arg,\ndiff --git a/reftable/generic.c b/reftable/generic.c\nindex 6ecf9b880f..495ee9af6b 100644\n--- a/reftable/generic.c\n+++ b/reftable/generic.c\n@@ -61,53 +61,6 @@ int reftable_table_read_ref(struct reftable_table *tab, const char *name,\n \treturn err;\n }\n \n-int reftable_table_print(struct reftable_table *tab) {\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_ref_record ref = { NULL };\n-\tstruct reftable_log_record log = { NULL };\n-\tuint32_t hash_id = reftable_table_hash_id(tab);\n-\tint err;\n-\n-\treftable_table_init_ref_iter(tab, &it);\n-\n-\terr = reftable_iterator_seek_ref(&it, \"\");\n-\tif (err < 0)\n-\t\treturn err;\n-\n-\twhile (1) {\n-\t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tif (err > 0) {\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n-\t\treftable_ref_record_print(&ref, hash_id);\n-\t}\n-\treftable_iterator_destroy(&it);\n-\treftable_ref_record_release(&ref);\n-\n-\treftable_table_init_log_iter(tab, &it);\n-\n-\terr = reftable_iterator_seek_log(&it, \"\");\n-\tif (err < 0)\n-\t\treturn err;\n-\n-\twhile (1) {\n-\t\terr = reftable_iterator_next_log(&it, &log);\n-\t\tif (err > 0) {\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n-\t\treftable_log_record_print(&log, hash_id);\n-\t}\n-\treftable_iterator_destroy(&it);\n-\treftable_log_record_release(&log);\n-\treturn 0;\n-}\n-\n uint64_t reftable_table_max_update_index(struct reftable_table *tab)\n {\n \treturn tab->ops->max_update_index(tab->table_arg);\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex e3f5854229..fbd93b88df 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -798,27 +798,6 @@ void reftable_table_from_reader(struct reftable_table *tab,\n \ttab->table_arg = reader;\n }\n \n-\n-int reftable_reader_print_file(const char *tablename)\n-{\n-\tstruct reftable_block_source src = { NULL };\n-\tint err = reftable_block_source_from_file(&src, tablename);\n-\tstruct reftable_reader *r = NULL;\n-\tstruct reftable_table tab = { NULL };\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_new_reader(&r, &src, tablename);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\treftable_table_from_reader(&tab, r);\n-\terr = reftable_table_print(&tab);\n-done:\n-\treftable_reader_free(r);\n-\treturn err;\n-}\n-\n int reftable_reader_print_blocks(const char *tablename)\n {\n \tstruct {\ndiff --git a/reftable/record.c b/reftable/record.c\nindex a2cba5ef74..e26bd4bc8d 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -259,58 +259,6 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,\n \t}\n }\n \n-static char hexdigit(int c)\n-{\n-\tif (c <= 9)\n-\t\treturn '0' + c;\n-\treturn 'a' + (c - 10);\n-}\n-\n-static void hex_format(char *dest, const unsigned char *src, int hash_size)\n-{\n-\tassert(hash_size > 0);\n-\tif (src) {\n-\t\tint i = 0;\n-\t\tfor (i = 0; i < hash_size; i++) {\n-\t\t\tdest[2 * i] = hexdigit(src[i] >> 4);\n-\t\t\tdest[2 * i + 1] = hexdigit(src[i] & 0xf);\n-\t\t}\n-\t\tdest[2 * hash_size] = 0;\n-\t}\n-}\n-\n-static void reftable_ref_record_print_sz(const struct reftable_ref_record *ref,\n-\t\t\t\t\t int hash_size)\n-{\n-\tchar hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */\n-\tprintf(\"ref{%s(%\" PRIu64 \") \", ref->refname, ref->update_index);\n-\tswitch (ref->value_type) {\n-\tcase REFTABLE_REF_SYMREF:\n-\t\tprintf(\"=> %s\", ref->value.symref);\n-\t\tbreak;\n-\tcase REFTABLE_REF_VAL2:\n-\t\thex_format(hex, ref->value.val2.value, hash_size);\n-\t\tprintf(\"val 2 %s\", hex);\n-\t\thex_format(hex, ref->value.val2.target_value,\n-\t\t\t   hash_size);\n-\t\tprintf(\"(T %s)\", hex);\n-\t\tbreak;\n-\tcase REFTABLE_REF_VAL1:\n-\t\thex_format(hex, ref->value.val1, hash_size);\n-\t\tprintf(\"val 1 %s\", hex);\n-\t\tbreak;\n-\tcase REFTABLE_REF_DELETION:\n-\t\tprintf(\"delete\");\n-\t\tbreak;\n-\t}\n-\tprintf(\"}\\n\");\n-}\n-\n-void reftable_ref_record_print(const struct reftable_ref_record *ref,\n-\t\t\t       uint32_t hash_id) {\n-\treftable_ref_record_print_sz(ref, hash_size(hash_id));\n-}\n-\n static void reftable_ref_record_release_void(void *rec)\n {\n \treftable_ref_record_release(rec);\n@@ -480,12 +428,6 @@ static int reftable_ref_record_cmp_void(const void *_a, const void *_b)\n \treturn strcmp(a->refname, b->refname);\n }\n \n-static void reftable_ref_record_print_void(const void *rec,\n-\t\t\t\t\t   int hash_size)\n-{\n-\treftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size);\n-}\n-\n static struct reftable_record_vtable reftable_ref_record_vtable = {\n \t.key = &reftable_ref_record_key,\n \t.type = BLOCK_TYPE_REF,\n@@ -497,7 +439,6 @@ static struct reftable_record_vtable reftable_ref_record_vtable = {\n \t.is_deletion = &reftable_ref_record_is_deletion_void,\n \t.equal = &reftable_ref_record_equal_void,\n \t.cmp = &reftable_ref_record_cmp_void,\n-\t.print = &reftable_ref_record_print_void,\n };\n \n static void reftable_obj_record_key(const void *r, struct strbuf *dest)\n@@ -516,21 +457,6 @@ static void reftable_obj_record_release(void *rec)\n \tmemset(obj, 0, sizeof(struct reftable_obj_record));\n }\n \n-static void reftable_obj_record_print(const void *rec, int hash_size)\n-{\n-\tconst struct reftable_obj_record *obj = rec;\n-\tchar hex[GIT_MAX_HEXSZ + 1] = { 0 };\n-\tstruct strbuf offset_str = STRBUF_INIT;\n-\tint i;\n-\n-\tfor (i = 0; i < obj->offset_len; i++)\n-\t\tstrbuf_addf(&offset_str, \"%\" PRIu64 \" \", obj->offsets[i]);\n-\thex_format(hex, obj->hash_prefix, obj->hash_prefix_len);\n-\tprintf(\"prefix %s (len %d), offsets [%s]\\n\",\n-\t       hex, obj->hash_prefix_len, offset_str.buf);\n-\tstrbuf_release(&offset_str);\n-}\n-\n static void reftable_obj_record_copy_from(void *rec, const void *src_rec,\n \t\t\t\t\t  int hash_size)\n {\n@@ -701,41 +627,8 @@ static struct reftable_record_vtable reftable_obj_record_vtable = {\n \t.is_deletion = &not_a_deletion,\n \t.equal = &reftable_obj_record_equal_void,\n \t.cmp = &reftable_obj_record_cmp_void,\n-\t.print = &reftable_obj_record_print,\n };\n \n-static void reftable_log_record_print_sz(struct reftable_log_record *log,\n-\t\t\t\t\t int hash_size)\n-{\n-\tchar hex[GIT_MAX_HEXSZ + 1] = { 0 };\n-\n-\tswitch (log->value_type) {\n-\tcase REFTABLE_LOG_DELETION:\n-\t\tprintf(\"log{%s(%\" PRIu64 \") delete\\n\", log->refname,\n-\t\t       log->update_index);\n-\t\tbreak;\n-\tcase REFTABLE_LOG_UPDATE:\n-\t\tprintf(\"log{%s(%\" PRIu64 \") %s <%s> %\" PRIu64 \" %04d\\n\",\n-\t\t       log->refname, log->update_index,\n-\t\t       log->value.update.name ? log->value.update.name : \"\",\n-\t\t       log->value.update.email ? log->value.update.email : \"\",\n-\t\t       log->value.update.time,\n-\t\t       log->value.update.tz_offset);\n-\t\thex_format(hex, log->value.update.old_hash, hash_size);\n-\t\tprintf(\"%s => \", hex);\n-\t\thex_format(hex, log->value.update.new_hash, hash_size);\n-\t\tprintf(\"%s\\n\\n%s\\n}\\n\", hex,\n-\t\t       log->value.update.message ? log->value.update.message : \"\");\n-\t\tbreak;\n-\t}\n-}\n-\n-void reftable_log_record_print(struct reftable_log_record *log,\n-\t\t\t\t      uint32_t hash_id)\n-{\n-\treftable_log_record_print_sz(log, hash_size(hash_id));\n-}\n-\n static void reftable_log_record_key(const void *r, struct strbuf *dest)\n {\n \tconst struct reftable_log_record *rec =\n@@ -1039,11 +932,6 @@ static int reftable_log_record_is_deletion_void(const void *p)\n \t\t(const struct reftable_log_record *)p);\n }\n \n-static void reftable_log_record_print_void(const void *rec, int hash_size)\n-{\n-\treftable_log_record_print_sz((struct reftable_log_record*)rec, hash_size);\n-}\n-\n static struct reftable_record_vtable reftable_log_record_vtable = {\n \t.key = &reftable_log_record_key,\n \t.type = BLOCK_TYPE_LOG,\n@@ -1055,7 +943,6 @@ static struct reftable_record_vtable reftable_log_record_vtable = {\n \t.is_deletion = &reftable_log_record_is_deletion_void,\n \t.equal = &reftable_log_record_equal_void,\n \t.cmp = &reftable_log_record_cmp_void,\n-\t.print = &reftable_log_record_print_void,\n };\n \n static void reftable_index_record_key(const void *r, struct strbuf *dest)\n@@ -1137,13 +1024,6 @@ static int reftable_index_record_cmp(const void *_a, const void *_b)\n \treturn strbuf_cmp(&a->last_key, &b->last_key);\n }\n \n-static void reftable_index_record_print(const void *rec, int hash_size)\n-{\n-\tconst struct reftable_index_record *idx = rec;\n-\t/* TODO: escape null chars? */\n-\tprintf(\"\\\"%s\\\" %\" PRIu64 \"\\n\", idx->last_key.buf, idx->offset);\n-}\n-\n static struct reftable_record_vtable reftable_index_record_vtable = {\n \t.key = &reftable_index_record_key,\n \t.type = BLOCK_TYPE_INDEX,\n@@ -1155,7 +1035,6 @@ static struct reftable_record_vtable reftable_index_record_vtable = {\n \t.is_deletion = &not_a_deletion,\n \t.equal = &reftable_index_record_equal,\n \t.cmp = &reftable_index_record_cmp,\n-\t.print = &reftable_index_record_print,\n };\n \n void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)\n@@ -1334,9 +1213,3 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ)\n \t\tBUG(\"unhandled record type\");\n \t}\n }\n-\n-void reftable_record_print(struct reftable_record *rec, int hash_size)\n-{\n-\tprintf(\"'%c': \", rec->type);\n-\treftable_record_vtable(rec)->print(reftable_record_data(rec), hash_size);\n-}\ndiff --git a/reftable/record.h b/reftable/record.h\nindex d778133e6e..5edd46ec75 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -72,9 +72,6 @@ struct reftable_record_vtable {\n \t * the same type.\n \t */\n \tint (*cmp)(const void *a, const void *b);\n-\n-\t/* Print on stdout, for debugging. */\n-\tvoid (*print)(const void *rec, int hash_size);\n };\n \n /* returns true for recognized block types. Block start with the block type. */\n@@ -136,7 +133,6 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);\n /* see struct record_vtable */\n int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);\n int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);\n-void reftable_record_print(struct reftable_record *rec, int hash_size);\n void reftable_record_key(struct reftable_record *rec, struct strbuf *dest);\n void reftable_record_copy_from(struct reftable_record *rec,\n \t\t\t       struct reftable_record *src, int hash_size);\ndiff --git a/reftable/reftable-generic.h b/reftable/reftable-generic.h\nindex 65670ea093..b8b1323a33 100644\n--- a/reftable/reftable-generic.h\n+++ b/reftable/reftable-generic.h\n@@ -41,7 +41,4 @@ uint64_t reftable_table_min_update_index(struct reftable_table *tab);\n int reftable_table_read_ref(struct reftable_table *tab, const char *name,\n \t\t\t    struct reftable_ref_record *ref);\n \n-/* dump table contents onto stdout for debugging */\n-int reftable_table_print(struct reftable_table *tab);\n-\n #endif\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex a32f31d648..7c7d171651 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -64,8 +64,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r);\n void reftable_table_from_reader(struct reftable_table *tab,\n \t\t\t\tstruct reftable_reader *reader);\n \n-/* print table onto stdout for debugging. */\n-int reftable_reader_print_file(const char *tablename);\n /* print blocks onto stdout for debugging. */\n int reftable_reader_print_blocks(const char *tablename);\n \ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex ff486eb1f7..2d42463c58 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -60,10 +60,6 @@ const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *\n /* returns whether 'ref' represents a deletion */\n int reftable_ref_record_is_deletion(const struct reftable_ref_record *ref);\n \n-/* prints a reftable_ref_record onto stdout. Useful for debugging. */\n-void reftable_ref_record_print(const struct reftable_ref_record *ref,\n-\t\t\t       uint32_t hash_id);\n-\n /* frees and nulls all pointer values inside `ref`. */\n void reftable_ref_record_release(struct reftable_ref_record *ref);\n \n@@ -111,8 +107,4 @@ void reftable_log_record_release(struct reftable_log_record *log);\n int reftable_log_record_equal(const struct reftable_log_record *a,\n \t\t\t      const struct reftable_log_record *b, int hash_size);\n \n-/* dumps a reftable_log_record on stdout, for debugging/testing. */\n-void reftable_log_record_print(struct reftable_log_record *log,\n-\t\t\t       uint32_t hash_id);\n-\n #endif\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 09e97c9991..f4f8cabc7f 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -140,7 +140,4 @@ struct reftable_compaction_stats {\n struct reftable_compaction_stats *\n reftable_stack_compaction_stats(struct reftable_stack *st);\n \n-/* print the entire stack represented by the directory */\n-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id);\n-\n #endif\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex d08ec00959..bedd503e7e 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)\n \treftable_addition_destroy(add);\n \treturn err;\n }\n-\n-int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)\n-{\n-\tstruct reftable_stack *stack = NULL;\n-\tstruct reftable_write_options opts = { .hash_id = hash_id };\n-\tstruct reftable_merged_table *merged = NULL;\n-\tstruct reftable_table table = { NULL };\n-\n-\tint err = reftable_new_stack(&stack, stackdir, &opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\tmerged = reftable_stack_merged_table(stack);\n-\treftable_table_from_merged_table(&table, merged);\n-\terr = reftable_table_print(&table);\n-done:\n-\tif (stack)\n-\t\treftable_stack_destroy(stack);\n-\treturn err;\n-}\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex dbca9eaf4a..42044ed8a3 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)\n \tEXPECT(0 == strcmp(\"master\", dest.value.symref));\n \tEXPECT(st->readers_len > 0);\n \n-\tprintf(\"testing print functionality:\\n\");\n-\terr = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID);\n-\tEXPECT_ERR(err);\n-\n-\terr = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID);\n-\tEXPECT(err == REFTABLE_FORMAT_ERROR);\n-\n #ifndef GIT_WINDOWS_NATIVE\n \tstrbuf_addstr(&scratch, dir);\n \tstrbuf_addstr(&scratch, \"/tables.list\");\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500685","messageId":"4184f46f92c16654ebc2523028a203eb2e1e4f7a.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 09/10] reftable/dump: move code into \"t/helper/test-reftable.c\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:37Z","receivedAt":"2024-08-13T06:24:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The code in \"reftable/dump.c\" is only ever used by our test-tool to\nimplement the \"reftable\" subcommand. It also feels very unlikely that it\nwill ever be useful to any other potential user of the reftable library,\nwhich makes it a weird candidate to have in the library interface.\n\nInline the code into \"t/helper/test-reftable.c\".\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Makefile                 |  1 -\n reftable/dump.c          | 73 ----------------------------------------\n t/helper/test-reftable.c | 47 +++++++++++++++++++++++++-\n 3 files changed, 46 insertions(+), 75 deletions(-)\n delete mode 100644 reftable/dump.c\n\ndiff --git a/Makefile b/Makefile\nindex 3863e60b66..343f19a488 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2680,7 +2680,6 @@ REFTABLE_OBJS += reftable/tree.o\n REFTABLE_OBJS += reftable/writer.o\n \n REFTABLE_TEST_OBJS += reftable/block_test.o\n-REFTABLE_TEST_OBJS += reftable/dump.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\ndiff --git a/reftable/dump.c b/reftable/dump.c\ndeleted file mode 100644\nindex 35a1731da9..0000000000\n--- a/reftable/dump.c\n+++ /dev/null\n@@ -1,73 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"git-compat-util.h\"\n-#include \"hash.h\"\n-\n-#include \"reftable-blocksource.h\"\n-#include \"reftable-error.h\"\n-#include \"reftable-record.h\"\n-#include \"reftable-tests.h\"\n-#include \"reftable-writer.h\"\n-#include \"reftable-iterator.h\"\n-#include \"reftable-reader.h\"\n-#include \"reftable-stack.h\"\n-\n-#include <stddef.h>\n-#include <stdio.h>\n-#include <stdlib.h>\n-#include <unistd.h>\n-#include <string.h>\n-\n-static void print_help(void)\n-{\n-\tprintf(\"usage: dump [-cst] arg\\n\\n\"\n-\t       \"options: \\n\"\n-\t       \"  -c compact\\n\"\n-\t       \"  -b dump blocks\\n\"\n-\t       \"  -t dump table\\n\"\n-\t       \"  -s dump stack\\n\"\n-\t       \"  -6 sha256 hash format\\n\"\n-\t       \"  -h this help\\n\"\n-\t       \"\\n\");\n-}\n-\n-int reftable_dump_main(int argc, char *const *argv)\n-{\n-\tint err = 0;\n-\tint opt_dump_blocks = 0;\n-\tconst char *arg = NULL, *argv0 = argv[0];\n-\n-\tfor (; argc > 1; argv++, argc--)\n-\t\tif (*argv[1] != '-')\n-\t\t\tbreak;\n-\t\telse if (!strcmp(\"-b\", argv[1]))\n-\t\t\topt_dump_blocks = 1;\n-\t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n-\t\t\tprint_help();\n-\t\t\treturn 2;\n-\t\t}\n-\n-\tif (argc != 2) {\n-\t\tfprintf(stderr, \"need argument\\n\");\n-\t\tprint_help();\n-\t\treturn 2;\n-\t}\n-\n-\targ = argv[1];\n-\n-\tif (opt_dump_blocks)\n-\t\terr = reftable_reader_print_blocks(arg);\n-\n-\tif (err < 0) {\n-\t\tfprintf(stderr, \"%s: %s: %s\\n\", argv0, arg,\n-\t\t\treftable_error_str(err));\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 9d378427da..cf93d30910 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -1,4 +1,6 @@\n #include \"reftable/system.h\"\n+#include \"reftable/reftable-error.h\"\n+#include \"reftable/reftable-reader.h\"\n #include \"reftable/reftable-tests.h\"\n #include \"test-tool.h\"\n \n@@ -13,7 +15,50 @@ int cmd__reftable(int argc, const char **argv)\n \treturn 0;\n }\n \n+static void print_help(void)\n+{\n+\tprintf(\"usage: dump [-cst] arg\\n\\n\"\n+\t       \"options: \\n\"\n+\t       \"  -c compact\\n\"\n+\t       \"  -b dump blocks\\n\"\n+\t       \"  -t dump table\\n\"\n+\t       \"  -s dump stack\\n\"\n+\t       \"  -6 sha256 hash format\\n\"\n+\t       \"  -h this help\\n\"\n+\t       \"\\n\");\n+}\n+\n int cmd__dump_reftable(int argc, const char **argv)\n {\n-\treturn reftable_dump_main(argc, (char *const *)argv);\n+\tint err = 0;\n+\tint opt_dump_blocks = 0;\n+\tconst char *arg = NULL, *argv0 = argv[0];\n+\n+\tfor (; argc > 1; argv++, argc--)\n+\t\tif (*argv[1] != '-')\n+\t\t\tbreak;\n+\t\telse if (!strcmp(\"-b\", argv[1]))\n+\t\t\topt_dump_blocks = 1;\n+\t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n+\t\t\tprint_help();\n+\t\t\treturn 2;\n+\t\t}\n+\n+\tif (argc != 2) {\n+\t\tfprintf(stderr, \"need argument\\n\");\n+\t\tprint_help();\n+\t\treturn 2;\n+\t}\n+\n+\targ = argv[1];\n+\n+\tif (opt_dump_blocks)\n+\t\terr = reftable_reader_print_blocks(arg);\n+\n+\tif (err < 0) {\n+\t\tfprintf(stderr, \"%s: %s: %s\\n\", argv0, arg,\n+\t\t\treftable_error_str(err));\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n }\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500686","messageId":"723495adf624cbc483b9c5d9fa4b5b6078e46168.1723528765.git.ps@pks.im","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"[PATCH 10/10] reftable/generic: drop interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T06:24:41Z","receivedAt":"2024-08-13T06:24:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_table` interface provides a generic infrastructure that\ncan abstract away whether the underlying table is a single table, or a\nmerged table. This abstraction can make it rather hard to reason about\nthe code. We didn't ever use it to implement the reftable backend, and\nwith the preceding patches in this patch series we in fact don't use it\nat all anymore. Furthermore, it became somewhat useless with the recent\nrefactorings that made it possible to seek reftable iterators multiple\ntimes, as these now provide generic access to tables for us. The\ninterface is thus redundant and only brings unnecessary complexity with\nit.\n\nRemove the `struct reftable_table` interface and its associated\nfunctions.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Makefile                         |  1 -\n reftable/generic.c               | 77 --------------------------------\n reftable/generic.h               | 27 -----------\n reftable/iter.c                  |  1 -\n reftable/iter.h                  |  1 -\n reftable/merged.c                | 38 ----------------\n reftable/reader.c                | 41 -----------------\n reftable/reftable-generic.h      | 44 ------------------\n reftable/reftable-merged.h       |  6 ---\n reftable/reftable-reader.h       |  7 ---\n reftable/stack.c                 |  1 -\n t/unit-tests/t-reftable-merged.c |  1 -\n 12 files changed, 245 deletions(-)\n delete mode 100644 reftable/generic.c\n delete mode 100644 reftable/generic.h\n delete mode 100644 reftable/reftable-generic.h\n\ndiff --git a/Makefile b/Makefile\nindex 343f19a488..41dfa0bad2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2674,7 +2674,6 @@ REFTABLE_OBJS += reftable/merged.o\n REFTABLE_OBJS += reftable/pq.o\n REFTABLE_OBJS += reftable/reader.o\n REFTABLE_OBJS += reftable/record.o\n-REFTABLE_OBJS += reftable/generic.o\n REFTABLE_OBJS += reftable/stack.o\n REFTABLE_OBJS += reftable/tree.o\n REFTABLE_OBJS += reftable/writer.o\ndiff --git a/reftable/generic.c b/reftable/generic.c\ndeleted file mode 100644\nindex 495ee9af6b..0000000000\n--- a/reftable/generic.c\n+++ /dev/null\n@@ -1,77 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"constants.h\"\n-#include \"record.h\"\n-#include \"generic.h\"\n-#include \"iter.h\"\n-#include \"reftable-iterator.h\"\n-#include \"reftable-generic.h\"\n-\n-void table_init_iter(struct reftable_table *tab,\n-\t\t     struct reftable_iterator *it,\n-\t\t     uint8_t typ)\n-{\n-\n-\ttab->ops->init_iter(tab->table_arg, it, typ);\n-}\n-\n-void reftable_table_init_ref_iter(struct reftable_table *tab,\n-\t\t\t\t  struct reftable_iterator *it)\n-{\n-\ttable_init_iter(tab, it, BLOCK_TYPE_REF);\n-}\n-\n-void reftable_table_init_log_iter(struct reftable_table *tab,\n-\t\t\t\t  struct reftable_iterator *it)\n-{\n-\ttable_init_iter(tab, it, BLOCK_TYPE_LOG);\n-}\n-\n-int reftable_table_read_ref(struct reftable_table *tab, const char *name,\n-\t\t\t    struct reftable_ref_record *ref)\n-{\n-\tstruct reftable_iterator it = { NULL };\n-\tint err;\n-\n-\treftable_table_init_ref_iter(tab, &it);\n-\n-\terr = reftable_iterator_seek_ref(&it, name);\n-\tif (err)\n-\t\tgoto done;\n-\n-\terr = reftable_iterator_next_ref(&it, ref);\n-\tif (err)\n-\t\tgoto done;\n-\n-\tif (strcmp(ref->refname, name) ||\n-\t    reftable_ref_record_is_deletion(ref)) {\n-\t\treftable_ref_record_release(ref);\n-\t\terr = 1;\n-\t\tgoto done;\n-\t}\n-\n-done:\n-\treftable_iterator_destroy(&it);\n-\treturn err;\n-}\n-\n-uint64_t reftable_table_max_update_index(struct reftable_table *tab)\n-{\n-\treturn tab->ops->max_update_index(tab->table_arg);\n-}\n-\n-uint64_t reftable_table_min_update_index(struct reftable_table *tab)\n-{\n-\treturn tab->ops->min_update_index(tab->table_arg);\n-}\n-\n-uint32_t reftable_table_hash_id(struct reftable_table *tab)\n-{\n-\treturn tab->ops->hash_id(tab->table_arg);\n-}\ndiff --git a/reftable/generic.h b/reftable/generic.h\ndeleted file mode 100644\nindex 837fbb8df2..0000000000\n--- a/reftable/generic.h\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#ifndef GENERIC_H\n-#define GENERIC_H\n-\n-#include \"record.h\"\n-#include \"reftable-generic.h\"\n-\n-/* generic interface to reftables */\n-struct reftable_table_vtable {\n-\tvoid (*init_iter)(void *tab, struct reftable_iterator *it, uint8_t typ);\n-\tuint32_t (*hash_id)(void *tab);\n-\tuint64_t (*min_update_index)(void *tab);\n-\tuint64_t (*max_update_index)(void *tab);\n-};\n-\n-void table_init_iter(struct reftable_table *tab,\n-\t\t     struct reftable_iterator *it,\n-\t\t     uint8_t typ);\n-\n-#endif\ndiff --git a/reftable/iter.c b/reftable/iter.c\nindex 844853fad2..4eb7fafd2f 100644\n--- a/reftable/iter.c\n+++ b/reftable/iter.c\n@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n \n #include \"block.h\"\n-#include \"generic.h\"\n #include \"constants.h\"\n #include \"reader.h\"\n #include \"reftable-error.h\"\ndiff --git a/reftable/iter.h b/reftable/iter.h\nindex 3b401f1259..befc4597df 100644\n--- a/reftable/iter.h\n+++ b/reftable/iter.h\n@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at\n #include \"record.h\"\n \n #include \"reftable-iterator.h\"\n-#include \"reftable-generic.h\"\n \n /*\n  * The virtual function table for implementing generic reftable iterators.\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 2e72eab306..128a810c55 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -13,7 +13,6 @@ license that can be found in the LICENSE file or at\n #include \"pq.h\"\n #include \"reader.h\"\n #include \"record.h\"\n-#include \"generic.h\"\n #include \"reftable-merged.h\"\n #include \"reftable-error.h\"\n #include \"system.h\"\n@@ -270,40 +269,3 @@ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt)\n {\n \treturn mt->hash_id;\n }\n-\n-static void reftable_merged_table_init_iter_void(void *tab,\n-\t\t\t\t\t\t struct reftable_iterator *it,\n-\t\t\t\t\t\t uint8_t typ)\n-{\n-\tmerged_table_init_iter(tab, it, typ);\n-}\n-\n-static uint32_t reftable_merged_table_hash_id_void(void *tab)\n-{\n-\treturn reftable_merged_table_hash_id(tab);\n-}\n-\n-static uint64_t reftable_merged_table_min_update_index_void(void *tab)\n-{\n-\treturn reftable_merged_table_min_update_index(tab);\n-}\n-\n-static uint64_t reftable_merged_table_max_update_index_void(void *tab)\n-{\n-\treturn reftable_merged_table_max_update_index(tab);\n-}\n-\n-static struct reftable_table_vtable merged_table_vtable = {\n-\t.init_iter = reftable_merged_table_init_iter_void,\n-\t.hash_id = reftable_merged_table_hash_id_void,\n-\t.min_update_index = reftable_merged_table_min_update_index_void,\n-\t.max_update_index = reftable_merged_table_max_update_index_void,\n-};\n-\n-void reftable_table_from_merged_table(struct reftable_table *tab,\n-\t\t\t\t      struct reftable_merged_table *merged)\n-{\n-\tassert(!tab->ops);\n-\ttab->ops = &merged_table_vtable;\n-\ttab->table_arg = merged;\n-}\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex fbd93b88df..082cf00b60 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -11,11 +11,9 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"block.h\"\n #include \"constants.h\"\n-#include \"generic.h\"\n #include \"iter.h\"\n #include \"record.h\"\n #include \"reftable-error.h\"\n-#include \"reftable-generic.h\"\n \n uint64_t block_source_size(struct reftable_block_source *source)\n {\n@@ -759,45 +757,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r)\n \treturn r->min_update_index;\n }\n \n-/* generic table interface. */\n-\n-static void reftable_reader_init_iter_void(void *tab,\n-\t\t\t\t\t   struct reftable_iterator *it,\n-\t\t\t\t\t   uint8_t typ)\n-{\n-\treader_init_iter(tab, it, typ);\n-}\n-\n-static uint32_t reftable_reader_hash_id_void(void *tab)\n-{\n-\treturn reftable_reader_hash_id(tab);\n-}\n-\n-static uint64_t reftable_reader_min_update_index_void(void *tab)\n-{\n-\treturn reftable_reader_min_update_index(tab);\n-}\n-\n-static uint64_t reftable_reader_max_update_index_void(void *tab)\n-{\n-\treturn reftable_reader_max_update_index(tab);\n-}\n-\n-static struct reftable_table_vtable reader_vtable = {\n-\t.init_iter = reftable_reader_init_iter_void,\n-\t.hash_id = reftable_reader_hash_id_void,\n-\t.min_update_index = reftable_reader_min_update_index_void,\n-\t.max_update_index = reftable_reader_max_update_index_void,\n-};\n-\n-void reftable_table_from_reader(struct reftable_table *tab,\n-\t\t\t\tstruct reftable_reader *reader)\n-{\n-\tassert(!tab->ops);\n-\ttab->ops = &reader_vtable;\n-\ttab->table_arg = reader;\n-}\n-\n int reftable_reader_print_blocks(const char *tablename)\n {\n \tstruct {\ndiff --git a/reftable/reftable-generic.h b/reftable/reftable-generic.h\ndeleted file mode 100644\nindex b8b1323a33..0000000000\n--- a/reftable/reftable-generic.h\n+++ /dev/null\n@@ -1,44 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#ifndef REFTABLE_GENERIC_H\n-#define REFTABLE_GENERIC_H\n-\n-#include \"reftable-iterator.h\"\n-\n-struct reftable_table_vtable;\n-\n-/*\n- * Provides a unified API for reading tables, either merged tables, or single\n- * readers. */\n-struct reftable_table {\n-\tstruct reftable_table_vtable *ops;\n-\tvoid *table_arg;\n-};\n-\n-void reftable_table_init_ref_iter(struct reftable_table *tab,\n-\t\t\t\t  struct reftable_iterator *it);\n-\n-void reftable_table_init_log_iter(struct reftable_table *tab,\n-\t\t\t\t  struct reftable_iterator *it);\n-\n-/* returns the hash ID from a generic reftable_table */\n-uint32_t reftable_table_hash_id(struct reftable_table *tab);\n-\n-/* returns the max update_index covered by this table. */\n-uint64_t reftable_table_max_update_index(struct reftable_table *tab);\n-\n-/* returns the min update_index covered by this table. */\n-uint64_t reftable_table_min_update_index(struct reftable_table *tab);\n-\n-/* convenience function to read a single ref. Returns < 0 for error, 0\n-   for success, and 1 if ref not found. */\n-int reftable_table_read_ref(struct reftable_table *tab, const char *name,\n-\t\t\t    struct reftable_ref_record *ref);\n-\n-#endif\ndiff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\nindex 03c2619c0f..16d19f8df2 100644\n--- a/reftable/reftable-merged.h\n+++ b/reftable/reftable-merged.h\n@@ -26,8 +26,6 @@ license that can be found in the LICENSE file or at\n /* A merged table is implements seeking/iterating over a stack of tables. */\n struct reftable_merged_table;\n \n-/* A generic reftable; see below. */\n-struct reftable_table;\n struct reftable_reader;\n \n /*\n@@ -60,8 +58,4 @@ void reftable_merged_table_free(struct reftable_merged_table *m);\n /* return the hash ID of the merged table. */\n uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *m);\n \n-/* create a generic table from reftable_merged_table */\n-void reftable_table_from_merged_table(struct reftable_table *tab,\n-\t\t\t\t      struct reftable_merged_table *table);\n-\n #endif\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex 7c7d171651..69621c5b0f 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -23,9 +23,6 @@\n /* The reader struct is a handle to an open reftable file. */\n struct reftable_reader;\n \n-/* Generic table. */\n-struct reftable_table;\n-\n /* reftable_new_reader opens a reftable for reading. If successful,\n  * returns 0 code and sets pp. The name is used for creating a\n  * stack. Typically, it is the basename of the file. The block source\n@@ -60,10 +57,6 @@ uint64_t reftable_reader_max_update_index(struct reftable_reader *r);\n /* return the min_update_index for a table */\n uint64_t reftable_reader_min_update_index(struct reftable_reader *r);\n \n-/* creates a generic table from a file reader. */\n-void reftable_table_from_reader(struct reftable_table *tab,\n-\t\t\t\tstruct reftable_reader *reader);\n-\n /* print blocks onto stdout for debugging. */\n int reftable_reader_print_blocks(const char *tablename);\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex bedd503e7e..d3a95d2f1d 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at\n #include \"merged.h\"\n #include \"reader.h\"\n #include \"reftable-error.h\"\n-#include \"reftable-generic.h\"\n #include \"reftable-record.h\"\n #include \"reftable-merged.h\"\n #include \"writer.h\"\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 577b1a5be8..93345c6c8b 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -12,7 +12,6 @@ license that can be found in the LICENSE file or at\n #include \"reftable/merged.h\"\n #include \"reftable/reader.h\"\n #include \"reftable/reftable-error.h\"\n-#include \"reftable/reftable-generic.h\"\n #include \"reftable/reftable-merged.h\"\n #include \"reftable/reftable-writer.h\"\n \n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500691","messageId":"CAPig+cQ7+mHQNL8Y5m_8puM769qyTJTLOhA5NHnFLapO9P-T1A@mail.gmail.com","threadId":"61939","inReplyTo":"ab7538d0bbb8b3c02b5db71a421163fbf87a45f6.1723528765.git.ps@pks.im","subject":"Re: [PATCH 05/10] reftable/iter: drop double-checking logic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-13T06:57:10Z","receivedAt":"2024-08-13T06:57:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 13, 2024 at 2:24 AM Patrick Steinhardt <ps@pks.im> wrote:\n> The filtering ref iterator can be used to only yield refs which are not\n> in a specific skip list. This iterator has an option to double-check the\n> results it returns, which causes us to seek tho reference we are about\n\ns/tho/the/\n\n> to yield via a separate table such that we detect whether the reference\n> that the first iterator has yielded actually exists.\n>\n> The value of this is somewhat dubious, and I cannot think of any usecase\n> where this functionality should be required. Furthermore, this option is\n> never set in our codebase, which means that it is essentially untested.\n> And last but not least, the `struct reftable_table` that is used to\n> implement it is about to go away.\n>\n> So while we could refactor the code to not use a `reftable_table`, it\n> very much feels like a wasted effort. Let's just drop this code.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n"},{"id":"500759","messageId":"CAOLa=ZSY8EWuXAK_tF4sSYGHs2Mv6nE-YtVtVkau4k5h=hOEdA@mail.gmail.com","threadId":"61939","inReplyTo":"404d64effde3bee424e338d858ed507ff83dff20.1723528765.git.ps@pks.im","subject":"Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-13T09:36:27Z","receivedAt":"2024-08-13T09:36:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> We do not expose any functions via our public headers that would allow a\n> caller to initialize a reftable iterator from a merged table. Instead,\n> they are expected to go via the generic `reftable_table` interface,\n> which is somewhat roundabout.\n>\n> Implement two new functions to initialize iterators for ref and log\n> records to plug this gap.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/merged.c          | 12 ++++++++++++\n>  reftable/reftable-merged.h |  8 ++++++++\n>  2 files changed, 20 insertions(+)\n>\n> diff --git a/reftable/merged.c b/reftable/merged.c\n> index 6adce44f4b..8d78b3da71 100644\n> --- a/reftable/merged.c\n> +++ b/reftable/merged.c\n> @@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,\n>  \titerator_from_merged_iter(it, mi);\n>  }\n>\n> +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n> +\t\t\t\t\t     struct reftable_iterator *it)\n> +{\n> +\tmerged_table_init_iter(mt, it, BLOCK_TYPE_REF);\n> +}\n> +\n> +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n> +\t\t\t\t\t     struct reftable_iterator *it)\n> +{\n> +\tmerged_table_init_iter(mt, it, BLOCK_TYPE_LOG);\n> +}\n> +\n\nThese too look similar to `static void\nreftable_merged_table_init_iter_void` defined a little below, I wonder\nif we could have simply exposed that? Perhaps we remove it in the\nupcoming patches.\n\n[snip]\n"},{"id":"500762","messageId":"CAOLa=ZSe75TW88omzYXc1t2iUxLDiY4haBGigA7VP2=ARsDObA@mail.gmail.com","threadId":"61939","inReplyTo":"86e2f9f5dc64eee97510e597820791287032ba29.1723528765.git.ps@pks.im","subject":"Re: [PATCH 03/10] reftable/merged: stop using generic tables in the merged table","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-13T09:58:29Z","receivedAt":"2024-08-13T09:58:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n[snip]\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 64c7fdf8c4..7f4e267ea9 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -225,13 +225,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n>  \t\t\t\t      const char **names,\n>  \t\t\t\t      int reuse_open)\n>  {\n> -\tsize_t cur_len = !st->merged ? 0 : st->merged->stack_len;\n> +\tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n>  \tstruct reftable_reader **cur = stack_copy_readers(st, cur_len);\n>  \tsize_t names_len = names_length(names);\n>  \tstruct reftable_reader **new_readers =\n>  \t\treftable_calloc(names_len, sizeof(*new_readers));\n> -\tstruct reftable_table *new_tables =\n> -\t\treftable_calloc(names_len, sizeof(*new_tables));\n>\n\nBefore we had both `reftable_reader` and `reftable_table` here and we\nused to set `reftable_table` from the reader and use it to create a\nmerged table. Now we will simply use the readers directly.\n\n>  \tsize_t new_readers_len = 0;\n>  \tstruct reftable_merged_table *new_merged = NULL;\n>  \tstruct strbuf table_path = STRBUF_INIT;\n> @@ -267,17 +265,15 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n>  \t\t}\n>\n>  \t\tnew_readers[new_readers_len] = rd;\n> -\t\treftable_table_from_reader(&new_tables[new_readers_len], rd);\n\nThis being the part where we convert `reftable_reader` array to\n`reftable_table` array.\n\n>  \t\tnew_readers_len++;\n>  \t}\n>\n>  \t/* success! */\n> -\terr = reftable_merged_table_new(&new_merged, new_tables,\n> +\terr = reftable_merged_table_new(&new_merged, new_readers,\n>  \t\t\t\t\tnew_readers_len, st->opts.hash_id);\n>\n\nAnd this being the part where we create the merged table.\n\n>  \tif (err < 0)\n>  \t\tgoto done;\n>\n> -\tnew_tables = NULL;\n>  \tst->readers_len = new_readers_len;\n>  \tif (st->merged)\n>  \t\treftable_merged_table_free(st->merged);\n> @@ -309,7 +305,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n\n[snip]\n\nThe entire patch is along the lines of what I explained above, so looks\ngood!\n"},{"id":"500764","messageId":"CAOLa=ZQ15h8bFRGGhzJUPNwkCrRmc2Y26n-0x+L_V_06xWgd7g@mail.gmail.com","threadId":"61939","inReplyTo":"14924604cebe20ac30d291399b0200016fa8b4e3.1723528765.git.ps@pks.im","subject":"Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-13T10:04:30Z","receivedAt":"2024-08-13T10:04:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n[snip]\n\n> +\n> +void reftable_iterator_destroy(struct reftable_iterator *it)\n> +{\n> +\tif (!it->ops) {\n> +\t\treturn;\n> +\t}\n>\n\nI know this commit is to move, but I couldn't help noticing that we\nshould remove the curly braces here.\n\nSeems like the CI caught it too [1].\n\n> +\tit->ops->close(it->iter_arg);\n> +\tit->ops = NULL;\n> +\tFREE_AND_NULL(it->iter_arg);\n> +}\n> +\n\n[snip]\n\n[1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943\n"},{"id":"500765","messageId":"CAOLa=ZScKRvAWsa4D+_BCyAbxQ9580RBnJ+==DfCvrGV6C2ETg@mail.gmail.com","threadId":"61939","inReplyTo":"1f211e514d049379ada609e6a4e8cf985e721271.1723528765.git.ps@pks.im","subject":"Re: [PATCH 08/10] reftable/dump: drop unused printing functionality","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-13T10:14:33Z","receivedAt":"2024-08-13T10:14:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> We have a bunch of infrastructure wired up that allows us to print\n> reftable records, tables and stacks. While this functionality is wired\n> up via various \"test-tool reftable\" options, it is never used. It also\n> feels kind of dubious whether any other eventual user of the reftable\n> library should use it as it very much feels like a debugging aid rather\n> than something sensible. The format itself is somewhat inscrutable and\n> the infrastructure is non-extensible.\n>\n> Drop this code. The only remaining function in this context is\n> `reftable_reader_print_blocks()`, which we do use in our tests.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/dump.c             |  16 +----\n>  reftable/generic.c          |  47 -------------\n>  reftable/reader.c           |  21 ------\n>  reftable/record.c           | 127 ------------------------------------\n>  reftable/record.h           |   4 --\n>  reftable/reftable-generic.h |   3 -\n>  reftable/reftable-reader.h  |   2 -\n>  reftable/reftable-record.h  |   8 ---\n>  reftable/reftable-stack.h   |   3 -\n>  reftable/stack.c            |  20 ------\n>  reftable/stack_test.c       |   7 --\n>  11 files changed, 1 insertion(+), 257 deletions(-)\n>\n> diff --git a/reftable/dump.c b/reftable/dump.c\n> index 2953e0a83a..35a1731da9 100644\n> --- a/reftable/dump.c\n> +++ b/reftable/dump.c\n> @@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv)\n>  {\n>  \tint err = 0;\n>  \tint opt_dump_blocks = 0;\n> -\tint opt_dump_table = 0;\n> -\tint opt_dump_stack = 0;\n> -\tuint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;\n>  \tconst char *arg = NULL, *argv0 = argv[0];\n>\n>  \tfor (; argc > 1; argv++, argc--)\n> @@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)\n>  \t\t\tbreak;\n>  \t\telse if (!strcmp(\"-b\", argv[1]))\n>  \t\t\topt_dump_blocks = 1;\n> -\t\telse if (!strcmp(\"-t\", argv[1]))\n> -\t\t\topt_dump_table = 1;\n> -\t\telse if (!strcmp(\"-6\", argv[1]))\n> -\t\t\topt_hash_id = GIT_SHA256_FORMAT_ID;\n> -\t\telse if (!strcmp(\"-s\", argv[1]))\n> -\t\t\topt_dump_stack = 1;\n>  \t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n>  \t\t\tprint_help();\n>  \t\t\treturn 2;\n> @@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)\n\nI'm a bit skeptical about this change because I definitely have used the\n`-t` and `-s` options a bunch of times to understand what a table holds.\nSince the reftable format is binary, this is the only tooling we have\nwhich allows us to read this format from a plumbing point of view. I'd\nkeep them. I guess the stack printing just iterates over the tables and\nprints them and could be removed, but I'd keep the option to dump a\ntable.\n\nAlso this patch misses cleaning up `print_help` if we go down this\nroute.\n\n[snip]\n"},{"id":"500766","messageId":"CAOLa=ZREWViEAxBbVTGfTtb3ZwxO8NiVc6GkQEbgyKBEheFH6g@mail.gmail.com","threadId":"61939","inReplyTo":"cover.1723528765.git.ps@pks.im","subject":"Re: [PATCH 00/10] reftable: drop generic `reftable_table` interface","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-13T10:17:32Z","receivedAt":"2024-08-13T10:17:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> this patch series performs some simplification of the reftable library\n> by dropping the generic `reftable_table` interface. The intent of this\n> interface is to abstract away whether the underlying table is either a\n> single reftable, or a merged reftable. Arguably though, it is not needed\n> anymore starting with my recent refactorings [1] that made the reftable\n> iterator itself seekable, as that now provides such a generic interface\n> already.\n>\n> So this patch series rips out the `reftable_table` interface, both to\n> remove unused clutter, but more importantly to make the reftable library\n> easier to understand overall. There is also a tiny performance gain of\n> 1-3% by requiring one less vtable function call. But while that speedup\n> is consistent, I didn't include any benchmarks as it is rather close to\n> noise and not the primary motivation of this patch series.\n>\n> Patrick\n>\n\nThis is a very welcome change, the indirection was a cause for confusion\nbefore and this clears it up.\n\nMy only concern is the loss of dumping reftables for debug purposes in\npatch 8, I suggest we keep them until we have alternatives. Apart from\nthat everything looks great.\n\nThanks!\n"},{"id":"500887","messageId":"ZryphS1KICCYWi9z@tanuki","threadId":"61939","inReplyTo":"CAOLa=ZSY8EWuXAK_tF4sSYGHs2Mv6nE-YtVtVkau4k5h=hOEdA@mail.gmail.com","subject":"Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-14T12:56:48Z","receivedAt":"2024-08-14T12:56:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 13, 2024 at 05:36:27AM -0400, karthik nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > We do not expose any functions via our public headers that would allow a\n> > caller to initialize a reftable iterator from a merged table. Instead,\n> > they are expected to go via the generic `reftable_table` interface,\n> > which is somewhat roundabout.\n> >\n> > Implement two new functions to initialize iterators for ref and log\n> > records to plug this gap.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  reftable/merged.c          | 12 ++++++++++++\n> >  reftable/reftable-merged.h |  8 ++++++++\n> >  2 files changed, 20 insertions(+)\n> >\n> > diff --git a/reftable/merged.c b/reftable/merged.c\n> > index 6adce44f4b..8d78b3da71 100644\n> > --- a/reftable/merged.c\n> > +++ b/reftable/merged.c\n> > @@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,\n> >  \titerator_from_merged_iter(it, mi);\n> >  }\n> >\n> > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n> > +\t\t\t\t\t     struct reftable_iterator *it)\n> > +{\n> > +\tmerged_table_init_iter(mt, it, BLOCK_TYPE_REF);\n> > +}\n> > +\n> > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n> > +\t\t\t\t\t     struct reftable_iterator *it)\n> > +{\n> > +\tmerged_table_init_iter(mt, it, BLOCK_TYPE_LOG);\n> > +}\n> > +\n> \n> These too look similar to `static void\n> reftable_merged_table_init_iter_void` defined a little below, I wonder\n> if we could have simply exposed that? Perhaps we remove it in the\n> upcoming patches.\n\nYup, `reftable_merged_table_init_iter_void()` is about to go away. But\nregardless of that, even if it didn't it would make sense to have both\nfunctions. The `iter_void()` one takes an arbitrary block type as input,\nwhich we never expose via our public interfaces. So it has to stay an\nimplementation detail. The new ones added here do not expose the block\ntype.\n\nPatrick\n"},{"id":"500888","messageId":"ZryplD3NsINRMxY1@tanuki","threadId":"61939","inReplyTo":"CAOLa=ZQ15h8bFRGGhzJUPNwkCrRmc2Y26n-0x+L_V_06xWgd7g@mail.gmail.com","subject":"Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-14T12:56:52Z","receivedAt":"2024-08-14T12:56:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 13, 2024 at 06:04:30AM -0400, karthik nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> [snip]\n> \n> > +\n> > +void reftable_iterator_destroy(struct reftable_iterator *it)\n> > +{\n> > +\tif (!it->ops) {\n> > +\t\treturn;\n> > +\t}\n> >\n> \n> I know this commit is to move, but I couldn't help noticing that we\n> should remove the curly braces here.\n> \n> Seems like the CI caught it too [1].\n\nFair enough, let's remove them.\n\nPatrick\n"},{"id":"500889","messageId":"ZrypmQOthfGS1JFn@tanuki","threadId":"61939","inReplyTo":"CAOLa=ZScKRvAWsa4D+_BCyAbxQ9580RBnJ+==DfCvrGV6C2ETg@mail.gmail.com","subject":"Re: [PATCH 08/10] reftable/dump: drop unused printing functionality","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-14T12:56:57Z","receivedAt":"2024-08-14T12:57:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 13, 2024 at 06:14:33AM -0400, karthik nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > We have a bunch of infrastructure wired up that allows us to print\n> > reftable records, tables and stacks. While this functionality is wired\n> > up via various \"test-tool reftable\" options, it is never used. It also\n> > feels kind of dubious whether any other eventual user of the reftable\n> > library should use it as it very much feels like a debugging aid rather\n> > than something sensible. The format itself is somewhat inscrutable and\n> > the infrastructure is non-extensible.\n> >\n> > Drop this code. The only remaining function in this context is\n> > `reftable_reader_print_blocks()`, which we do use in our tests.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  reftable/dump.c             |  16 +----\n> >  reftable/generic.c          |  47 -------------\n> >  reftable/reader.c           |  21 ------\n> >  reftable/record.c           | 127 ------------------------------------\n> >  reftable/record.h           |   4 --\n> >  reftable/reftable-generic.h |   3 -\n> >  reftable/reftable-reader.h  |   2 -\n> >  reftable/reftable-record.h  |   8 ---\n> >  reftable/reftable-stack.h   |   3 -\n> >  reftable/stack.c            |  20 ------\n> >  reftable/stack_test.c       |   7 --\n> >  11 files changed, 1 insertion(+), 257 deletions(-)\n> >\n> > diff --git a/reftable/dump.c b/reftable/dump.c\n> > index 2953e0a83a..35a1731da9 100644\n> > --- a/reftable/dump.c\n> > +++ b/reftable/dump.c\n> > @@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv)\n> >  {\n> >  \tint err = 0;\n> >  \tint opt_dump_blocks = 0;\n> > -\tint opt_dump_table = 0;\n> > -\tint opt_dump_stack = 0;\n> > -\tuint32_t opt_hash_id = GIT_SHA1_FORMAT_ID;\n> >  \tconst char *arg = NULL, *argv0 = argv[0];\n> >\n> >  \tfor (; argc > 1; argv++, argc--)\n> > @@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)\n> >  \t\t\tbreak;\n> >  \t\telse if (!strcmp(\"-b\", argv[1]))\n> >  \t\t\topt_dump_blocks = 1;\n> > -\t\telse if (!strcmp(\"-t\", argv[1]))\n> > -\t\t\topt_dump_table = 1;\n> > -\t\telse if (!strcmp(\"-6\", argv[1]))\n> > -\t\t\topt_hash_id = GIT_SHA256_FORMAT_ID;\n> > -\t\telse if (!strcmp(\"-s\", argv[1]))\n> > -\t\t\topt_dump_stack = 1;\n> >  \t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n> >  \t\t\tprint_help();\n> >  \t\t\treturn 2;\n> > @@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)\n> \n> I'm a bit skeptical about this change because I definitely have used the\n> `-t` and `-s` options a bunch of times to understand what a table holds.\n> Since the reftable format is binary, this is the only tooling we have\n> which allows us to read this format from a plumbing point of view. I'd\n> keep them. I guess the stack printing just iterates over the tables and\n> prints them and could be removed, but I'd keep the option to dump a\n> table.\n\nWell, I have to say that I find the output to be rather useless. But\nyou're making a valid point: we don't have anything else to peek at the\ntable's contents.\n\nI'll keep this in v2, but make it an internal implementation detail of\nthe test-tool. It certainly has no place in the reftable library in my\nopinion.\n\n> Also this patch misses cleaning up `print_help` if we go down this\n> route.\n\nIndeed, will fix.\n\nPatrick\n"},{"id":"500917","messageId":"xmqqh6bn1391.fsf@gitster.g","threadId":"61939","inReplyTo":"CAOLa=ZQ15h8bFRGGhzJUPNwkCrRmc2Y26n-0x+L_V_06xWgd7g@mail.gmail.com","subject":"Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-14T16:24:42Z","receivedAt":"2024-08-14T16:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karthik nayak <karthik.188@gmail.com> writes:\n\n> Seems like the CI caught it too [1].\n>\n>> +\tit->ops->close(it->iter_arg);\n>> +\tit->ops = NULL;\n>> +\tFREE_AND_NULL(it->iter_arg);\n>> +}\n>> +\n>\n> [snip]\n>\n> [1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943\n\nLooking at it, the suggestions the CI job makes look hit-and-miss.\n\nFor examle, this one\n\n-static int stack_compact_range_stats(struct reftable_stack *st,\n-\t\t\t\t     size_t first, size_t last,\n+static int stack_compact_range_stats(struct reftable_stack *st, size_t first,\n+\t\t\t\t     size_t last,\n \t\t\t\t     struct reftable_log_expiry_config *config,\n \t\t\t\t     unsigned int flags)\n\nis a degradation of readability.  \"first\" and \"last\" pretty\nmuch act as a pair and they read better sitting together next to\neach other.  The rewrite is reducing neither the total number of\nlines or the maximum column width.\n\nBut this one\n\n-static void write_n_ref_tables(struct reftable_stack *st,\n-\t\t\t       size_t n)\n+static void write_n_ref_tables(struct reftable_stack *st, size_t n)\n\nis certainly an improvement.\n\nThis one\n\n \tstruct reftable_record rec = {\n \t\t.type = BLOCK_TYPE_REF,\n-\t\t.u = {\n-\t\t\t.ref = *ref\n-\t\t},\n+\t\t.u = { .ref = *ref },\n \t};\n\nis hard to generalize but in this case it made a good suggestion.\n\nIf we _expect_ that we will enumerate more members of .u, then the\nway originally written (plus trailing comma after the \".ref = *ref\")\nwould be easier to maintain.  But since .u is a union, we won't be\nchoosing more than one member to initialize by definition, so what\nwas suggested by the check-style linter is certainly much better.\n\nThanks.\n"},{"id":"500969","messageId":"CAOLa=ZQ=6y2jRNUt_0YyoEHTSgOc6y+ECEnLccXnntbqyU_J6Q@mail.gmail.com","threadId":"61939","inReplyTo":"xmqqh6bn1391.fsf@gitster.g","subject":"Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-15T11:04:09Z","receivedAt":"2024-08-15T11:04:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> karthik nayak <karthik.188@gmail.com> writes:\n>\n>> Seems like the CI caught it too [1].\n>>\n>>> +\tit->ops->close(it->iter_arg);\n>>> +\tit->ops = NULL;\n>>> +\tFREE_AND_NULL(it->iter_arg);\n>>> +}\n>>> +\n>>\n>> [snip]\n>>\n>> [1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943\n>\n> Looking at it, the suggestions the CI job makes look hit-and-miss.\n>\n\nYup, this is kinda what I was expecting and why I also wanted it to be\nallowed to fail. So we have data points on what we should change to make\nit better.\n\n> For examle, this one\n>\n> -static int stack_compact_range_stats(struct reftable_stack *st,\n> -\t\t\t\t     size_t first, size_t last,\n> +static int stack_compact_range_stats(struct reftable_stack *st, size_t first,\n> +\t\t\t\t     size_t last,\n>  \t\t\t\t     struct reftable_log_expiry_config *config,\n>  \t\t\t\t     unsigned int flags)\n>\n> is a degradation of readability.  \"first\" and \"last\" pretty\n> much act as a pair and they read better sitting together next to\n> each other.  The rewrite is reducing neither the total number of\n> lines or the maximum column width.\n>\n\nTotally agreed, This is set by `ColumnLimit: 80` and I think most of the\nmisses I see are because of this. This works with the `Penalties` we set\nat the bottom. We need to see what combination works best for us since\nour focus would be more on the readability front.\n\n> But this one\n>\n> -static void write_n_ref_tables(struct reftable_stack *st,\n> -\t\t\t       size_t n)\n> +static void write_n_ref_tables(struct reftable_stack *st, size_t n)\n>\n> is certainly an improvement.\n>\n> This one\n>\n>  \tstruct reftable_record rec = {\n>  \t\t.type = BLOCK_TYPE_REF,\n> -\t\t.u = {\n> -\t\t\t.ref = *ref\n> -\t\t},\n> +\t\t.u = { .ref = *ref },\n>  \t};\n>\n> is hard to generalize but in this case it made a good suggestion.\n>\n> If we _expect_ that we will enumerate more members of .u, then the\n> way originally written (plus trailing comma after the \".ref = *ref\")\n> would be easier to maintain.  But since .u is a union, we won't be\n> choosing more than one member to initialize by definition, so what\n> was suggested by the check-style linter is certainly much better.\n>\n> Thanks.\n\nYup these two do look much nicer indeed. There are weird bugs in\nclang-format however and I was able to fool it to accept\n\n\tstruct reftable_record rec = {\n\t\t.type = BLOCK_TYPE_REF,\n\t\t.u = {\n\t\t\t.ref = *ref },\n\t};\n\nBut this could also be an issue with our config too.\n\nI'll try playing around for an hour today to see if I can find some\nimprovements we can make to the formatter config.\n\nThanks\n"},{"id":"501287","messageId":"gzhbfjhyhhmje53s3zp4crzvwf7dfkcixl3uxw3h4fvsv32x37@6rvvt7svtrjg","threadId":"61939","inReplyTo":"86e2f9f5dc64eee97510e597820791287032ba29.1723528765.git.ps@pks.im","subject":"Re: [PATCH 03/10] reftable/merged: stop using generic tables in the merged table","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-08-19T16:47:06Z","receivedAt":"2024-08-19T16:48:01Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/08/13 08:24AM, Patrick Steinhardt wrote:\n> The merged table provides access to a reftable stack by merging the\n> contents of those tables into a virtual table. These subtables are being\n> tracked via `struct reftable_table`, which is a generic interface for\n> accessing either a single reftable or a merged reftable. So in theory,\n> it would be possible for the merged table to merge together other merged\n> tables.\n> \n> This is somewhat nonsensical though: we only ever set up a merged table\n> over normal reftables, and there is no reason to do otherwise. This\n> generic interface thus makes the code way harder to follow and reason\n> about than really necessary. The abstraction layer may also have an\n> impact on performance, even though the extra set of vtable function\n> calls probably doesn't really matter.\n\nAgreed! When I was reading through the reftable code for the first time\nI remember being rather confused by this interface. It left me wondering\nif merge tables could also be composed of other merge tables, but I\ncouldn't think of a valid reason to ever do so.\n\n> Refactor the merged tables to use a `struct reftable_reader` for each of\n> the subtables instead, which gives us direct access to the underlying\n> tables. Adjust names accordingly.\n\nUsing `struct reftable_reader` directly instead of the generic `struct\nreftable_table` sounds like the right decision and the flexibility\nit provids is unneeded and only adds complexity.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n[snip] \n> diff --git a/reftable/merged.h b/reftable/merged.h\n> index 2efe571da6..de5fd33f01 100644\n> --- a/reftable/merged.h\n> +++ b/reftable/merged.h\n> @@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at\n>  #include \"system.h\"\n>  \n>  struct reftable_merged_table {\n> -\tstruct reftable_table *stack;\n> -\tsize_t stack_len;\n> +\tstruct reftable_reader **readers;\n> +\tsize_t readers_len;\n\nThe merged table is being made to directly reference the table readers\ninstead of going through the generic `struct reftable_table`. Makes\nsense.\n\n>  \tuint32_t hash_id;\n>  \n>  \t/* If unset, produce deletions. This is useful for compaction. For the\n> diff --git a/reftable/reader.c b/reftable/reader.c\n> index 29c99e2269..f7ae35da72 100644\n> --- a/reftable/reader.c\n> +++ b/reftable/reader.c\n> @@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it,\n>  \tit->ops = &table_iter_vtable;\n>  }\n>  \n> -static void reader_init_iter(struct reftable_reader *r,\n> -\t\t\t     struct reftable_iterator *it,\n> -\t\t\t     uint8_t typ)\n> +void reader_init_iter(struct reftable_reader *r,\n> +\t\t      struct reftable_iterator *it,\n> +\t\t      uint8_t typ)\n>  {\n>  \tstruct reftable_reader_offsets *offs = reader_offsets_for(r, typ);\n>  \n> diff --git a/reftable/reader.h b/reftable/reader.h\n> index e869165f23..a2c204d523 100644\n> --- a/reftable/reader.h\n> +++ b/reftable/reader.h\n> @@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n>  void reader_close(struct reftable_reader *r);\n>  const char *reader_name(struct reftable_reader *r);\n>  \n> +void reader_init_iter(struct reftable_reader *r,\n> +\t\t      struct reftable_iterator *it,\n> +\t\t      uint8_t typ);\n\nAt first I was wondering if we should prefix the function name with\n`reftable_`, but this is only exposed as part of the internal reftable\ninterface and matches the format of other \"reftable/reader.h\" functions.\n"},{"id":"501290","messageId":"wb5nteglnm4bfu5pe3xqtdatsv4byyah4py6m2imp6wsh2xspo@bft2kvapv6vh","threadId":"61939","inReplyTo":"404d64effde3bee424e338d858ed507ff83dff20.1723528765.git.ps@pks.im","subject":"Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-08-19T16:55:16Z","receivedAt":"2024-08-19T16:56:08Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/08/13 08:24AM, Patrick Steinhardt wrote:\n> We do not expose any functions via our public headers that would allow a\n> caller to initialize a reftable iterator from a merged table. Instead,\n> they are expected to go via the generic `reftable_table` interface,\n> which is somewhat roundabout.\n> \n> Implement two new functions to initialize iterators for ref and log\n> records to plug this gap.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n[snip]\n> diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\n> index 14d5fc9f05..4deb0ad22e 100644\n> --- a/reftable/reftable-merged.h\n> +++ b/reftable/reftable-merged.h\n> @@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,\n>  \t\t\t      struct reftable_table *stack, size_t n,\n>  \t\t\t      uint32_t hash_id);\n>  \n> +/* Initialize a merged table iterator for reading refs. */\n> +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n> +\t\t\t\t\t     struct reftable_iterator *it);\n> +\n> +/* Initialize a merged table iterator for reading logs. */\n> +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n> +\t\t\t\t\t     struct reftable_iterator *it);\n\nI might have missed it, but I do not see \n`reftable_merged_table_init_log_iterator()` used anywhere in the later\npatches. Does this need to be added? Or are we just adding it because we\nwant a companion function to the ref iterator to be more consistent? \n\n-Justin\n"},{"id":"501333","messageId":"ZsSFcXr1NQqHYWi9@tanuki","threadId":"61939","inReplyTo":"wb5nteglnm4bfu5pe3xqtdatsv4byyah4py6m2imp6wsh2xspo@bft2kvapv6vh","subject":"Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-20T12:00:49Z","receivedAt":"2024-08-20T12:00:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 19, 2024 at 11:55:16AM -0500, Justin Tobler wrote:\n> On 24/08/13 08:24AM, Patrick Steinhardt wrote:\n> > We do not expose any functions via our public headers that would allow a\n> > caller to initialize a reftable iterator from a merged table. Instead,\n> > they are expected to go via the generic `reftable_table` interface,\n> > which is somewhat roundabout.\n> > \n> > Implement two new functions to initialize iterators for ref and log\n> > records to plug this gap.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> [snip]\n> > diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h\n> > index 14d5fc9f05..4deb0ad22e 100644\n> > --- a/reftable/reftable-merged.h\n> > +++ b/reftable/reftable-merged.h\n> > @@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,\n> >  \t\t\t      struct reftable_table *stack, size_t n,\n> >  \t\t\t      uint32_t hash_id);\n> >  \n> > +/* Initialize a merged table iterator for reading refs. */\n> > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt,\n> > +\t\t\t\t\t     struct reftable_iterator *it);\n> > +\n> > +/* Initialize a merged table iterator for reading logs. */\n> > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt,\n> > +\t\t\t\t\t     struct reftable_iterator *it);\n> \n> I might have missed it, but I do not see \n> `reftable_merged_table_init_log_iterator()` used anywhere in the later\n> patches. Does this need to be added? Or are we just adding it because we\n> want a companion function to the ref iterator to be more consistent? \n\nThe latter. It really should exist to make the public interface\ncomplete.\n\nPatrick\n"}]}