{"thread":{"id":"62084","subject":"[PATCH 0/6] refs/reftable: wire up exclude patterns","startedAt":"2024-09-09T11:31:35Z","lastAt":"2024-09-17T17:33:42Z","messageCount":36,"participants":["Patrick Steinhardt","karthik nayak","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"502432","messageId":"cover.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":null,"subject":"[PATCH 0/6] refs/reftable: wire up exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:30Z","receivedAt":"2024-09-09T11:31:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series wires up exclude patterns for the reftable backend.\nExclude patterns allow the backend to skip references internally that\nmatch a specific pattern. This avoids having to read references that the\ncaller would discard immediately anyway.\n\nThe series is structured as follows:\n\n - Patches 1 and 2 fix two separate bugs in how we currently handle\n exclude patterns in combination with namespaces. We didn't happen to\n stumble over these bugs before because exclude patterns are only\n implemented for the \"packed\" backend. But once you start to pack\n refs we exclude references based on their full name instead of the\n name with the prefixed stripped. For the reftable backend we'd\n always hit those bugs because it always uses exclude patterns when\n passed.\n\n - Patches 3 to 5 wire up proper re-seeking of reftable iterators and\n adds some tests to demonstrate that this works as expected. This is\n a prerequisite for handling exclude patterns.\n\n - Patch 6 wires up exclude patterns in the reftable backend by\n re-seeking iterators once we hit an excluded reference.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (6):\n refs: properly apply exclude patterns to namespaced refs\n builtin/receive-pack: fix exclude patterns when announcing refs\n Makefile: stop listing test library objects twice\n t/unit-tests: introduce reftable library\n reftable/reader: make table iterator reseekable\n refs/reftable: wire up support for exclude patterns\n\n Makefile | 8 +-\n builtin/receive-pack.c | 18 ++-\n refs.c | 35 +++++-\n refs.h | 9 ++\n refs/reftable-backend.c | 125 ++++++++++++++++++++-\n reftable/reader.c | 1 +\n t/t1419-exclude-refs.sh | 33 ++++--\n t/t5509-fetch-push-namespaces.sh | 9 ++\n t/unit-tests/lib-reftable.c | 93 ++++++++++++++++\n t/unit-tests/lib-reftable.h | 20 ++++\n t/unit-tests/t-reftable-merged.c | 163 +++++++++++++++-------------\n t/unit-tests/t-reftable-reader.c | 96 ++++++++++++++++\n t/unit-tests/t-reftable-readwrite.c | 130 +++++++---------------\n t/unit-tests/t-reftable-stack.c | 25 ++---\n trace2.h | 1 +\n trace2/tr2_ctr.c | 5 +\n 16 files changed, 570 insertions(+), 201 deletions(-)\n create mode 100644 t/unit-tests/lib-reftable.c\n create mode 100644 t/unit-tests/lib-reftable.h\n create mode 100644 t/unit-tests/t-reftable-reader.c\n\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502433","messageId":"8d347bc5599e2a679d50fed073e0f09ffdad85c4.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:32Z","receivedAt":"2024-09-09T11:31:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reference namespaces allow commands like git-upload-pack(1) to serve\ndifferent sets of references to the client depending on which namespace\nis enabled, which is for example useful in fork networks. Namespaced\nrefs are stored with a `refs/namespaces/$namespace` prefix, but all the\nuser will ultimately see is a stripped version where that prefix is\nremoved.\n\nThe way that this interacts with \"transfer.hideRefs\" is not immediately\nobvious: the hidden refs can either apply to the stripped references, or\nto the non-stripped ones that still have the namespace prefix. In fact,\nthe \"transfer.hideRefs\" machinery does the former and applies to the\nstripped reference by default, but rules can have \"^\" prefixed to switch\nthis behaviour to iinstead match against the rull reference name.\n\nNamespaces are exclusively handled at the generic \"refs\" layer, the\nrespective backends have no clue that such a thing even exists. This\nalso has the consequence that they cannot handle hiding references as\nsoon as reference namespaces come into play because they neither know\nwhether a namespace is active, nor do they know how to strip references\nif they are active.\n\nHandling such exclude patterns in `refs_for_each_namespaced_ref()` and\n`refs_for_each_fullref_in_prefixes()` is broken though, as both support\nthat the user passes both namespaces and exclude patterns. In the case\nwhere both are set we will exclude references with unstripped names,\neven though we really wanted to exclude references based on their\nstripped names.\n\nThis only surfaces when:\n\n - A repository uses reference namespaces.\n\n - \"transfer.hideRefs\" is active.\n\n - The namespaced references are packed into the \"packed-refs\" file.\n\nNone of our tests exercise this scenario, and thus we haven't ever hit\nit. While t5509 exercises both (1) and (2), it does not happen to hit\n(3). It is trivial to demonstrate the bug though by explicitly packing\nrefs in the tests, and then we indeed surface the breakage.\n\nFix this bug by prefixing exclude patterns with the namespace in the\ngeneric layer.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 35 ++++++++++++++++++++++++++++----\n refs.h | 9 ++++++++\n t/t5509-fetch-push-namespaces.sh | 1 +\n 3 files changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ceb72d4bd74..b3a367ea12c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n \treturn hide_refs->v;\n }\n \n+const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n+\t\t\t\t\t const char *namespace,\n+\t\t\t\t\t struct strvec *out)\n+{\n+\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n+\t\treturn exclude_patterns;\n+\n+\tfor (size_t i = 0; exclude_patterns[i]; i++)\n+\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n+\n+\treturn out->v;\n+}\n+\n const char *find_descendant_ref(const char *dirname,\n \t\t\t\tconst struct string_list *extras,\n \t\t\t\tconst struct string_list *skip)\n@@ -1634,11 +1647,19 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \t\t\t\t const char **exclude_patterns,\n \t\t\t\t each_ref_fn fn, void *cb_data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n+\tstruct strbuf prefix = STRBUF_INIT;\n \tint ret;\n-\tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n-\tret = do_for_each_ref(refs, buf.buf, exclude_patterns, fn, 0, 0, cb_data);\n-\tstrbuf_release(&buf);\n+\n+\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n+\t\t\t\t\t\t\t get_git_namespace(),\n+\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n+\n+\tstrbuf_addf(&prefix, \"%srefs/\", get_git_namespace());\n+\tret = do_for_each_ref(refs, prefix.buf, exclude_patterns, fn, 0, 0, cb_data);\n+\n+\tstrvec_clear(&namespaced_exclude_patterns);\n+\tstrbuf_release(&prefix);\n \treturn ret;\n }\n \n@@ -1719,6 +1740,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\t\t\t const char **exclude_patterns,\n \t\t\t\t each_ref_fn fn, void *cb_data)\n {\n+\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *prefix;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1730,6 +1752,10 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\tstrbuf_addstr(&buf, namespace);\n \tnamespace_len = buf.len;\n \n+\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n+\t\t\t\t\t\t\t namespace,\n+\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n+\n \tfor_each_string_list_item(prefix, &prefixes) {\n \t\tstrbuf_addstr(&buf, prefix->string);\n \t\tret = refs_for_each_fullref_in(ref_store, buf.buf,\n@@ -1739,6 +1765,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\tstrbuf_setlen(&buf, namespace_len);\n \t}\n \n+\tstrvec_clear(&namespaced_exclude_patterns);\n \tstring_list_clear(&prefixes, 0);\n \tstrbuf_release(&buf);\n \treturn ret;\ndiff --git a/refs.h b/refs.h\nindex f8b919a1388..3f774e96d18 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -859,6 +859,15 @@ int ref_is_hidden(const char *, const char *, const struct strvec *);\n */\n const char **hidden_refs_to_excludes(const struct strvec *hide_refs);\n \n+/*\n+ * Prefix all exclude patterns with the namespace, if any. This is required\n+ * because exclude patterns apply to the stripped reference name, not the full\n+ * reference name with the namespace.\n+ */\n+const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n+\t\t\t\t\t const char *namespace,\n+\t\t\t\t\t struct strvec *out);\n+\n /* Is this a per-worktree ref living in the refs/ namespace? */\n int is_per_worktree_ref(const char *refname);\n \ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 05090feaf92..98e8352b6cc 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -96,6 +96,7 @@ test_expect_success 'hide namespaced refs with transfer.hideRefs' '\n '\n \n test_expect_success 'check that transfer.hideRefs does not match unstripped refs' '\n+\tgit -C pushee pack-refs --all &&\n \tGIT_NAMESPACE=namespace \\\n \t\tgit -C pushee -c transfer.hideRefs=refs/namespaces/namespace/refs/tags \\\n \t\tls-remote \"ext::git %s .\" >actual &&\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502434","messageId":"0317a5a7edeab29b7cad31d11140bd99f459144f.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 2/6] builtin/receive-pack: fix exclude patterns when announcing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:35Z","receivedAt":"2024-09-09T11:31:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `write_head_info()` we announce references to the remote client. We\nneed to honor \"transfer.hideRefs\" here so that we do not announce any\nreferences that the client shouldn't be able to learn about. This is\ndone via two separate mechanisms:\n\n - We hand over exclude patterns to the reference backend. We can only\n honor \"plain\" exclude patterns here that do not have prefixes with\n special meaning such as \"^\" or \"!\". Filtering down the references is\n handled by `hidden_refs_to_excludes()`.\n\n - In `show_ref_cb()` we perform a second check against hidden refs.\n For one this is done such that we can handle those special prefixes.\n And second, handling exclude patterns in ref backends is optional,\n so we also have to handle \"normal\" patterns.\n\nThe special-meaning \"^\" prefix alters whether a hidden ref applies to\nthe namespace-stripped reference name or the full name. So while we\nwould usually call `refs_for_each_namespaced_ref()` to only get those\nreferences in the current namespace, we can't because we'd get the\nalready-rewritten reference names. Instead, we are forced to use\n`refs_for_each_fullref_in()` and then manually strip away the namespace\nprefix such that we have access to both names.\n\nBut this also means that we do not get namespace handling for exclude\npatterns, which `refs_for_each_namespaced_ref()` brings for free. This\nresults in a bug because we potentially end up hiding away references\nbased on their namespaced name and not on the stripped name as we really\nshould be doing.\n\nFix this by manually rewriting the exclude patterns to their namespaced\nvariants.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/receive-pack.c | 18 ++++++++++++++++--\n t/t5509-fetch-push-namespaces.sh | 8 ++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 3f35140e489..478c62ca836 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -339,12 +339,26 @@ static void show_one_alternate_ref(const struct object_id *oid,\n static void write_head_info(void)\n {\n \tstatic struct oidset seen = OIDSET_INIT;\n+\tstruct strvec excludes_vector = STRVEC_INIT;\n+\tconst char **exclude_patterns;\n+\n+\t/*\n+\t * We need access to the reference names both with and without their\n+\t * namespace and thus cannot use `refs_for_each_namespaced_ref()`. We\n+\t * thus have to adapt exclude patterns to carry the namespace prefix\n+\t * ourselves.\n+\t */\n+\texclude_patterns = get_namespaced_exclude_patterns(\n+\t\thidden_refs_to_excludes(&hidden_refs),\n+\t\tget_git_namespace(), &excludes_vector);\n \n \trefs_for_each_fullref_in(get_main_ref_store(the_repository), \"\",\n-\t\t\t\t hidden_refs_to_excludes(&hidden_refs),\n-\t\t\t\t show_ref_cb, &seen);\n+\t\t\t\t exclude_patterns, show_ref_cb, &seen);\n \tfor_each_alternate_ref(show_one_alternate_ref, &seen);\n+\n \toidset_clear(&seen);\n+\tstrvec_clear(&excludes_vector);\n+\n \tif (!sent_capabilities)\n \t\tshow_ref(\"capabilities^{}\", null_oid());\n \ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 98e8352b6cc..f029ae0d286 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -124,6 +124,14 @@ test_expect_success 'try to update a ref that is not hidden' '\n \tgit -C original push pushee-namespaced main\n '\n \n+test_expect_success 'git-receive-pack(1) with transfer.hideRefs does not match unstripped refs during advertisement' '\n+\tgit -C pushee update-ref refs/namespaces/namespace/refs/heads/foo/1 refs/namespaces/namespace/refs/heads/main &&\n+\tgit -C pushee pack-refs --all &&\n+\ttest_config -C pushee transfer.hideRefs refs/namespaces/namespace/refs/heads/foo &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C original push pushee-namespaced main &&\n+\ttest_grep refs/heads/foo/1 trace\n+'\n+\n test_expect_success 'try to update a hidden full ref' '\n \ttest_config -C pushee transfer.hideRefs \"^refs/namespaces/namespace/refs/heads/main\" &&\n \ttest_must_fail git -C original push pushee-namespaced main\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502435","messageId":"503c44e6cab5b574c02175154a5234974f6da439.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 3/6] Makefile: stop listing test library objects twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:38Z","receivedAt":"2024-09-09T11:31:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever one adds another test library compilation unit one has to wire\nit up twice in the Makefile: once to append it to `UNIT_TEST_OBJS`, and\nonce to append it to the `UNIT_TEST_PROGS` target. Ideally, we'd just\nreuse the `UNIT_TEST_OBJS` variable in the target so that we can avoid\nthe duplication. But it also contains all the objects for our test\nprograms, each of which contains a `cmd_main()`, and thus we cannot link\nthem all into the target executable.\n\nRefactor the code such that `UNIT_TEST_OBJS` does not contain the unit\ntest program objects anymore, which we can instead manually append to\nthe `OBJECTS` variable. Like this, the former variable now only contains\nobjects for test libraries and can thus be reused.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex bdea061971a..4ed5f1f50a8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1356,7 +1356,6 @@ UNIT_TEST_PROGRAMS += t-strvec\n UNIT_TEST_PROGRAMS += t-trailer\n UNIT_TEST_PROGRAMS += t-urlmatch-normalization\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n-UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n \n@@ -2715,6 +2714,7 @@ OBJECTS += $(XDIFF_OBJS)\n OBJECTS += $(FUZZ_OBJS)\n OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)\n OBJECTS += $(UNIT_TEST_OBJS)\n+OBJECTS += $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n \n ifndef NO_CURL\n \tOBJECTS += http.o http-walker.o remote-curl.o\n@@ -3852,9 +3852,7 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \\\n-\t$(UNIT_TEST_DIR)/test-lib.o \\\n-\t$(UNIT_TEST_DIR)/lib-oid.o \\\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \\\n \t$(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502436","messageId":"3df4040dd3ca5b63fc22cc76ec6e09c332d23bbe.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 4/6] t/unit-tests: introduce reftable library","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:40Z","receivedAt":"2024-09-09T11:31:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently migrated all of the reftable unit tests that were part\nof the reftable library into our own unit testing framework. As part of\nthat migration we have duplicated some of the functionality that was\npart of the reftable test framework into each of the migrated test\nsuites. This was a sensible decision to not have all of the migrations\ndependent on each other, but now that the migration is done it makes\nsense to deduplicate the functionality again.\n\nIntroduce a new reftable test library that hosts some shared code and\nadapt tests to use it.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 1 +\n t/unit-tests/lib-reftable.c | 93 ++++++++++++++++++++\n t/unit-tests/lib-reftable.h | 20 +++++\n t/unit-tests/t-reftable-merged.c | 87 +++----------------\n t/unit-tests/t-reftable-readwrite.c | 130 +++++++++-------------------\n t/unit-tests/t-reftable-stack.c | 25 +++---\n 6 files changed, 177 insertions(+), 179 deletions(-)\n create mode 100644 t/unit-tests/lib-reftable.c\n create mode 100644 t/unit-tests/lib-reftable.h\n\ndiff --git a/Makefile b/Makefile\nindex 4ed5f1f50a8..9460a80d0dd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1358,6 +1358,7 @@ UNIT_TEST_PROGRAMS += t-urlmatch-normalization\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n+UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o\n \n # xdiff and reftable libs may in turn depend on what is in libgit.a\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)\ndiff --git a/t/unit-tests/lib-reftable.c b/t/unit-tests/lib-reftable.c\nnew file mode 100644\nindex 00000000000..ab1fa44a282\n--- /dev/null\n+++ b/t/unit-tests/lib-reftable.c\n@@ -0,0 +1,93 @@\n+#include \"lib-reftable.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/constants.h\"\n+#include \"reftable/writer.h\"\n+\n+void t_reftable_set_hash(uint8_t *p, int i, uint32_t id)\n+{\n+\tmemset(p, (uint8_t)i, hash_size(id));\n+}\n+\n+static ssize_t strbuf_writer_write(void *b, const void *data, size_t sz)\n+{\n+\tstrbuf_add(b, data, sz);\n+\treturn sz;\n+}\n+\n+static int strbuf_writer_flush(void *arg UNUSED)\n+{\n+\treturn 0;\n+}\n+\n+struct reftable_writer *t_reftable_strbuf_writer(struct strbuf *buf,\n+\t\t\t\t\t\t struct reftable_write_options *opts)\n+{\n+\treturn reftable_new_writer(&strbuf_writer_write,\n+\t\t\t\t &strbuf_writer_flush,\n+\t\t\t\t buf, opts);\n+}\n+\n+void t_reftable_write_to_buf(struct strbuf *buf,\n+\t\t\t struct reftable_ref_record *refs,\n+\t\t\t size_t nrefs,\n+\t\t\t struct reftable_log_record *logs,\n+\t\t\t size_t nlogs,\n+\t\t\t struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tconst struct reftable_stats *stats;\n+\tstruct reftable_writer *writer;\n+\tuint64_t min = 0xffffffff;\n+\tuint64_t max = 0;\n+\tint ret;\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\n+\tfor (size_t i = 0; i < nrefs; i++) {\n+\t\tuint64_t ui = refs[i].update_index;\n+\t\tif (ui > max)\n+\t\t\tmax = ui;\n+\t\tif (ui < min)\n+\t\t\tmin = ui;\n+\t}\n+\tfor (size_t i = 0; i < nlogs; i++) {\n+\t\tuint64_t ui = logs[i].update_index;\n+\t\tif (ui > max)\n+\t\t\tmax = ui;\n+\t\tif (ui < min)\n+\t\t\tmin = ui;\n+\t}\n+\n+\twriter = t_reftable_strbuf_writer(buf, &opts);\n+\treftable_writer_set_limits(writer, min, max);\n+\n+\tif (nrefs) {\n+\t\tret = reftable_writer_add_refs(writer, refs, nrefs);\n+\t\tcheck_int(ret, ==, 0);\n+\t}\n+\n+\tif (nlogs) {\n+\t\tret = reftable_writer_add_logs(writer, logs, nlogs);\n+\t\tcheck_int(ret, ==, 0);\n+\t}\n+\n+\tret = reftable_writer_close(writer);\n+\tcheck_int(ret, ==, 0);\n+\n+\tstats = reftable_writer_stats(writer);\n+\tfor (size_t i = 0; i < stats->ref_stats.blocks; i++) {\n+\t\tsize_t off = i * (opts.block_size ? opts.block_size\n+\t\t\t\t\t\t : DEFAULT_BLOCK_SIZE);\n+\t\tif (!off)\n+\t\t\toff = header_size(opts.hash_id == GIT_SHA256_FORMAT_ID ? 2 : 1);\n+\t\tcheck_char(buf->buf[off], ==, 'r');\n+\t}\n+\n+\tif (nrefs)\n+\t\tcheck_int(stats->ref_stats.blocks, >, 0);\n+\tif (nlogs)\n+\t\tcheck_int(stats->log_stats.blocks, >, 0);\n+\n+\treftable_writer_free(writer);\n+}\ndiff --git a/t/unit-tests/lib-reftable.h b/t/unit-tests/lib-reftable.h\nnew file mode 100644\nindex 00000000000..d1154190847\n--- /dev/null\n+++ b/t/unit-tests/lib-reftable.h\n@@ -0,0 +1,20 @@\n+#ifndef LIB_REFTABLE_H\n+#define LIB_REFTABLE_H\n+\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"reftable/reftable-writer.h\"\n+\n+void t_reftable_set_hash(uint8_t *p, int i, uint32_t id);\n+\n+struct reftable_writer *t_reftable_strbuf_writer(struct strbuf *buf,\n+\t\t\t\t\t\t struct reftable_write_options *opts);\n+\n+void t_reftable_write_to_buf(struct strbuf *buf,\n+\t\t\t struct reftable_ref_record *refs,\n+\t\t\t size_t nrecords,\n+\t\t\t struct reftable_log_record *logs,\n+\t\t\t size_t nlogs,\n+\t\t\t struct reftable_write_options *opts);\n+\n+#endif\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex e9d100a01ea..b8c92fdb365 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/blocksource.h\"\n #include \"reftable/constants.h\"\n #include \"reftable/merged.h\"\n@@ -15,77 +16,6 @@ license that can be found in the LICENSE file or at\n #include \"reftable/reftable-merged.h\"\n #include \"reftable/reftable-writer.h\"\n \n-static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)\n-{\n-\tstrbuf_add(b, data, sz);\n-\treturn sz;\n-}\n-\n-static int noop_flush(void *arg UNUSED)\n-{\n-\treturn 0;\n-}\n-\n-static void write_test_table(struct strbuf *buf,\n-\t\t\t struct reftable_ref_record refs[], const size_t n)\n-{\n-\tuint64_t min = 0xffffffff;\n-\tuint64_t max = 0;\n-\tsize_t i;\n-\tint err;\n-\n-\tstruct reftable_write_options opts = {\n-\t\t.block_size = 256,\n-\t};\n-\tstruct reftable_writer *w = NULL;\n-\tfor (i = 0; i < n; i++) {\n-\t\tuint64_t ui = refs[i].update_index;\n-\t\tif (ui > max)\n-\t\t\tmax = ui;\n-\t\tif (ui < min)\n-\t\t\tmin = ui;\n-\t}\n-\n-\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\treftable_writer_set_limits(w, min, max);\n-\n-\tfor (i = 0; i < n; i++) {\n-\t\tuint64_t before = refs[i].update_index;\n-\t\tint n = reftable_writer_add_ref(w, &refs[i]);\n-\t\tcheck_int(n, ==, 0);\n-\t\tcheck_int(before, ==, refs[i].update_index);\n-\t}\n-\n-\terr = reftable_writer_close(w);\n-\tcheck(!err);\n-\n-\treftable_writer_free(w);\n-}\n-\n-static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],\n-\t\t\t\t const size_t n, const uint64_t update_index)\n-{\n-\tint err;\n-\n-\tstruct reftable_write_options opts = {\n-\t\t.block_size = 256,\n-\t\t.exact_log_message = 1,\n-\t};\n-\tstruct reftable_writer *w = NULL;\n-\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\treftable_writer_set_limits(w, update_index, update_index);\n-\n-\tfor (size_t i = 0; i < n; i++) {\n-\t\tint err = reftable_writer_add_log(w, &logs[i]);\n-\t\tcheck(!err);\n-\t}\n-\n-\terr = reftable_writer_close(w);\n-\tcheck(!err);\n-\n-\treftable_writer_free(w);\n-}\n-\n static struct reftable_merged_table *\n merged_table_from_records(struct reftable_ref_record **refs,\n \t\t\t struct reftable_block_source **source,\n@@ -93,13 +23,16 @@ 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_write_options opts = {\n+\t\t.block_size = 256,\n+\t};\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n \tfor (size_t i = 0; i < n; i++) {\n-\t\twrite_test_table(&buf[i], refs[i], sizes[i]);\n+\t\tt_reftable_write_to_buf(&buf[i], refs[i], sizes[i], NULL, 0, &opts);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n \t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n@@ -268,13 +201,17 @@ 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_write_options opts = {\n+\t\t.block_size = 256,\n+\t\t.exact_log_message = 1,\n+\t};\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n \tfor (size_t i = 0; i < n; i++) {\n-\t\twrite_test_log_table(&buf[i], logs[i], sizes[i], i + 1);\n+\t\tt_reftable_write_to_buf(&buf[i], NULL, 0, logs[i], sizes[i], &opts);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n \t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n@@ -402,9 +339,7 @@ static void t_default_write_opts(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record rec = {\n \t\t.refname = (char *) \"master\",\n \t\t.update_index = 1,\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 82bfaf32874..e1b235a5f13 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/basics.h\"\n #include \"reftable/blocksource.h\"\n #include \"reftable/reader.h\"\n@@ -15,22 +16,6 @@ license that can be found in the LICENSE file or at\n \n static const int update_index = 5;\n \n-static void set_test_hash(uint8_t *p, int i)\n-{\n-\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n-}\n-\n-static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n-{\n-\tstrbuf_add(b, data, sz);\n-\treturn sz;\n-}\n-\n-static int noop_flush(void *arg UNUSED)\n-{\n-\treturn 0;\n-}\n-\n static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -62,61 +47,34 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t.block_size = block_size,\n \t\t.hash_id = hash_id,\n \t};\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\tstruct reftable_ref_record ref = { 0 };\n-\tint i = 0, n;\n-\tstruct reftable_log_record log = { 0 };\n-\tconst struct reftable_stats *stats = NULL;\n+\tstruct reftable_ref_record *refs;\n+\tstruct reftable_log_record *logs;\n+\tint i;\n \n \tREFTABLE_CALLOC_ARRAY(*names, N + 1);\n+\tREFTABLE_CALLOC_ARRAY(refs, N);\n+\tREFTABLE_CALLOC_ARRAY(logs, N);\n \n-\treftable_writer_set_limits(w, update_index, update_index);\n \tfor (i = 0; i < N; i++) {\n-\t\tchar name[100];\n-\t\tint n;\n-\n-\t\tsnprintf(name, sizeof(name), \"refs/heads/branch%02d\", i);\n-\n-\t\tref.refname = name;\n-\t\tref.update_index = update_index;\n-\t\tref.value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(ref.value.val1, i);\n-\t\t(*names)[i] = xstrdup(name);\n-\n-\t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tcheck_int(n, ==, 0);\n+\t\trefs[i].refname = (*names)[i] = xstrfmt(\"refs/heads/branch%02d\", i);\n+\t\trefs[i].update_index = update_index;\n+\t\trefs[i].value_type = REFTABLE_REF_VAL1;\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tchar name[100];\n-\t\tint n;\n-\n-\t\tsnprintf(name, sizeof(name), \"refs/heads/branch%02d\", i);\n-\n-\t\tlog.refname = name;\n-\t\tlog.update_index = update_index;\n-\t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tset_test_hash(log.value.update.new_hash, i);\n-\t\tlog.value.update.message = (char *) \"message\";\n-\n-\t\tn = reftable_writer_add_log(w, &log);\n-\t\tcheck_int(n, ==, 0);\n+\t\tlogs[i].refname = (*names)[i];\n+\t\tlogs[i].update_index = update_index;\n+\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tlogs[i].value.update.message = (char *) \"message\";\n \t}\n \n-\tn = reftable_writer_close(w);\n-\tcheck_int(n, ==, 0);\n-\n-\tstats = reftable_writer_stats(w);\n-\tfor (i = 0; i < stats->ref_stats.blocks; i++) {\n-\t\tint off = i * opts.block_size;\n-\t\tif (!off)\n-\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n-\t\tcheck_char(buf->buf[off], ==, 'r');\n-\t}\n+\tt_reftable_write_to_buf(buf, refs, N, logs, N, &opts);\n \n-\tcheck_int(stats->log_stats.blocks, >, 0);\n-\treftable_writer_free(w);\n+\tfree(refs);\n+\tfree(logs);\n }\n \n static void t_log_buffer_size(void)\n@@ -138,8 +96,7 @@ static void t_log_buffer_size(void)\n \t\t\t\t\t .time = 0x5e430672,\n \t\t\t\t\t .message = (char *) \"commit: 9\\n\",\n \t\t\t\t } } };\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \n \t/* This tests buffer extension for log compression. Must use a random\n \t hash, to ensure that the compressed part is larger than the original.\n@@ -181,8 +138,7 @@ static void t_log_overflow(void)\n \t\t\t},\n \t\t},\n \t};\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \n \tmemset(msg, 'x', sizeof(msg) - 1);\n \treftable_writer_set_limits(w, update_index, update_index);\n@@ -208,8 +164,7 @@ static void t_log_write_read(void)\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n@@ -229,8 +184,10 @@ static void t_log_write_read(void)\n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n \t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tset_test_hash(log.value.update.old_hash, i);\n-\t\tset_test_hash(log.value.update.new_hash, i + 1);\n+\t\tt_reftable_set_hash(log.value.update.old_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tt_reftable_set_hash(log.value.update.new_hash, i + 1,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \n \t\terr = reftable_writer_add_log(w, &log);\n \t\tcheck(!err);\n@@ -297,8 +254,7 @@ static void t_log_zlib_corruption(void)\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \tchar message[100] = { 0 };\n \tint err, i, n;\n@@ -528,15 +484,12 @@ static void t_table_refs_for(int indexed)\n \tint err;\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n-\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_iterator it = { 0 };\n \tint j;\n \n-\tset_test_hash(want_hash, 4);\n+\tt_reftable_set_hash(want_hash, 4, GIT_SHA1_FORMAT_ID);\n \n \tfor (i = 0; i < N; i++) {\n \t\tuint8_t hash[GIT_SHA1_RAWSZ];\n@@ -552,8 +505,10 @@ static void t_table_refs_for(int indexed)\n \t\tref.refname = name;\n \n \t\tref.value_type = REFTABLE_REF_VAL2;\n-\t\tset_test_hash(ref.value.val2.value, i / 4);\n-\t\tset_test_hash(ref.value.val2.target_value, 3 + i / 4);\n+\t\tt_reftable_set_hash(ref.value.val2.value, i / 4,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tt_reftable_set_hash(ref.value.val2.target_value, 3 + i / 4,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \n \t\t/* 80 bytes / entry, so 3 entries per block. Yields 17\n \t\t */\n@@ -618,8 +573,7 @@ static void t_write_empty_table(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_block_source source = { 0 };\n \tstruct reftable_reader *rd = NULL;\n \tstruct reftable_ref_record rec = { 0 };\n@@ -657,8 +611,7 @@ static void t_write_object_id_min_length(void)\n \t\t.block_size = 75,\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -692,8 +645,7 @@ static void t_write_object_id_length(void)\n \t\t.block_size = 75,\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -726,8 +678,7 @@ static void t_write_empty_key(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.refname = (char *) \"\",\n \t\t.update_index = 1,\n@@ -749,8 +700,7 @@ static void t_write_key_order(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record refs[2] = {\n \t\t{\n \t\t\t.refname = (char *) \"b\",\n@@ -798,7 +748,7 @@ static void t_write_multiple_indices(void)\n \tstruct reftable_reader *reader;\n \tint err, i;\n \n-\twriter = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);\n+\twriter = t_reftable_strbuf_writer(&writer_buf, &opts);\n \treftable_writer_set_limits(writer, 1, 1);\n \tfor (i = 0; i < 100; i++) {\n \t\tstruct reftable_ref_record ref = {\n@@ -876,7 +826,7 @@ static void t_write_multi_level_index(void)\n \tstruct reftable_reader *reader;\n \tint err;\n \n-\twriter = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);\n+\twriter = t_reftable_strbuf_writer(&writer_buf, &opts);\n \treftable_writer_set_limits(writer, 1, 1);\n \tfor (size_t i = 0; i < 200; i++) {\n \t\tstruct reftable_ref_record ref = {\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..65e513d5ec8 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -7,17 +7,13 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/merged.h\"\n #include \"reftable/reader.h\"\n #include \"reftable/reftable-error.h\"\n #include \"reftable/stack.h\"\n #include \n \n-static void set_test_hash(uint8_t *p, int i)\n-{\n-\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n-}\n-\n static void clear_dir(const char *dirname)\n {\n \tstruct strbuf path = STRBUF_INIT;\n@@ -125,7 +121,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"refs/heads/branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf.buf;\n-\t\tset_test_hash(ref.value.val1, i);\n+\t\tt_reftable_set_hash(ref.value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n \t\tcheck(!err);\n@@ -470,13 +466,13 @@ static void t_reftable_stack_add(void)\n \t\trefs[i].refname = xstrdup(buf);\n \t\trefs[i].update_index = i + 1;\n \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(refs[i].value.val1, i);\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\tlogs[i].refname = xstrdup(buf);\n \t\tlogs[i].update_index = N + i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -562,14 +558,14 @@ static void t_reftable_stack_iterator(void)\n \t\trefs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\trefs[i].update_index = i + 1;\n \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(refs[i].value.val1, i);\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\tlogs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\tlogs[i].update_index = i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.email = xstrdup(\"johndoe@invalid\");\n \t\tlogs[i].value.update.message = xstrdup(\"commit\\n\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -704,7 +700,8 @@ static void t_reftable_stack_tombstone(void)\n \t\trefs[i].update_index = i + 1;\n \t\tif (i % 2 == 0) {\n \t\t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\t\tset_test_hash(refs[i].value.val1, i);\n+\t\t\tt_reftable_set_hash(refs[i].value.val1, i,\n+\t\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t\t}\n \n \t\tlogs[i].refname = xstrdup(buf);\n@@ -712,7 +709,8 @@ static void t_reftable_stack_tombstone(void)\n \t\tlogs[i].update_index = 42;\n \t\tif (i % 2 == 0) {\n \t\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n-\t\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t\t\tlogs[i].value.update.email =\n \t\t\t\txstrdup(\"identity@invalid\");\n \t\t}\n@@ -844,7 +842,8 @@ static void t_reflog_expire(void)\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.time = i;\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 1; i <= N; i++) {\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502437","messageId":"a281f936a2b3a697b32f57652a2120afd54f7e4f.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 5/6] reftable/reader: make table iterator reseekable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:44Z","receivedAt":"2024-09-09T11:31:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 67ce50ba26 (Merge branch 'ps/reftable-reusable-iterator', 2024-05-30)\nwe have refactored the interface of reftable iterators such that they\ncan be reused in theory. This patch series only landed the required\nchanges on the interface level, but didn't yet implement the actual\nlogic to make iterators reusable.\n\nAs it turns out almost all of the infrastructure already does support\nre-seeking. The only exception is the table iterator, which does not\nreset its `is_finished` bit. Do so and add a couple of tests that verify\nthat we can re-seek iterators.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 1 +\n reftable/reader.c | 1 +\n t/unit-tests/t-reftable-merged.c | 76 +++++++++++++++++++++++++\n t/unit-tests/t-reftable-reader.c | 96 ++++++++++++++++++++++++++++++++\n 4 files changed, 174 insertions(+)\n create mode 100644 t/unit-tests/t-reftable-reader.c\n\ndiff --git a/Makefile b/Makefile\nindex 9460a80d0dd..4039e355b09 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1346,6 +1346,7 @@ UNIT_TEST_PROGRAMS += t-reftable-basics\n UNIT_TEST_PROGRAMS += t-reftable-block\n UNIT_TEST_PROGRAMS += t-reftable-merged\n UNIT_TEST_PROGRAMS += t-reftable-pq\n+UNIT_TEST_PROGRAMS += t-reftable-reader\n UNIT_TEST_PROGRAMS += t-reftable-readwrite\n UNIT_TEST_PROGRAMS += t-reftable-record\n UNIT_TEST_PROGRAMS += t-reftable-stack\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex f8770990876..6494ce2e327 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -328,6 +328,7 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)\n \tti->typ = block_reader_type(&ti->br);\n \tti->block_off = off;\n \tblock_iter_seek_start(&ti->bi, &ti->br);\n+\tti->is_finished = 0;\n \treturn 0;\n }\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex b8c92fdb365..19e54bdfb8b 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -194,6 +194,81 @@ static void t_merged_refs(void)\n \treftable_free(bs);\n }\n \n+static void t_merged_seek_multiple_times(void)\n+{\n+\tstruct reftable_ref_record r1[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 1 },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"c\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 2 },\n+\t\t}\n+\t};\n+\tstruct reftable_ref_record r2[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 3 },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"d\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 4 },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record *refs[] = {\n+\t\tr1, r2,\n+\t};\n+\tsize_t sizes[] = {\n+\t\tARRAY_SIZE(r1), ARRAY_SIZE(r2),\n+\t};\n+\tstruct strbuf bufs[] = {\n+\t\tSTRBUF_INIT, STRBUF_INIT,\n+\t};\n+\tstruct reftable_block_source *sources = NULL;\n+\tstruct reftable_reader **readers = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_merged_table *mt;\n+\n+\tmt = merged_table_from_records(refs, &sources, &readers, sizes, bufs, 2);\n+\tmerged_table_init_iter(mt, &it, BLOCK_TYPE_REF);\n+\n+\tfor (size_t i = 0; i < 5; i++) {\n+\t\tint err = reftable_iterator_seek_ref(&it, \"c\");\n+\t\tcheck(!err);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(!err);\n+\t\terr = reftable_ref_record_equal(&rec, &r1[1], GIT_SHA1_RAWSZ);\n+\t\tcheck(err == 1);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(!err);\n+\t\terr = reftable_ref_record_equal(&rec, &r2[1], GIT_SHA1_RAWSZ);\n+\t\tcheck(err == 1);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(err > 0);\n+\t}\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(bufs); i++)\n+\t\tstrbuf_release(&bufs[i]);\n+\treaders_destroy(readers, ARRAY_SIZE(refs));\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_merged_table_free(mt);\n+\treftable_free(sources);\n+}\n+\n static struct reftable_merged_table *\n merged_table_from_log_records(struct reftable_log_record **logs,\n \t\t\t struct reftable_block_source **source,\n@@ -383,6 +458,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_default_write_opts(), \"merged table with default write opts\");\n \tTEST(t_merged_logs(), \"merged table with multiple log updates for same ref\");\n \tTEST(t_merged_refs(), \"merged table with multiple updates to same ref\");\n+\tTEST(t_merged_seek_multiple_times(), \"merged table can seek multiple times\");\n \tTEST(t_merged_single_record(), \"ref ocurring in only one record can be fetched\");\n \n \treturn test_done();\ndiff --git a/t/unit-tests/t-reftable-reader.c b/t/unit-tests/t-reftable-reader.c\nnew file mode 100644\nindex 00000000000..7a46580b6f1\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-reader.c\n@@ -0,0 +1,96 @@\n+#include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n+#include \"reftable/blocksource.h\"\n+#include \"reftable/reader.h\"\n+\n+static int t_reader_seek_once(void)\n+{\n+\tstruct reftable_ref_record records[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/main\",\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 42 },\n+\t\t},\n+\t};\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_ref_record ref = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader *reader;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret;\n+\n+\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n+\tblock_source_from_strbuf(&source, &buf);\n+\n+\tret = reftable_reader_new(&reader, &source, \"name\");\n+\tcheck_int(ret, ==, 0);\n+\n+\treftable_reader_init_ref_iterator(reader, &it);\n+\tret = reftable_iterator_seek_ref(&it, \"\");\n+\tcheck_int(ret, ==, 0);\n+\tret = reftable_iterator_next_ref(&it, &ref);\n+\tcheck_int(ret, ==, 0);\n+\n+\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n+\tcheck_int(ret, ==, 1);\n+\n+\tret = reftable_iterator_next_ref(&it, &ref);\n+\tcheck_int(ret, ==, 1);\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_iterator_destroy(&it);\n+\treftable_reader_decref(reader);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+static int t_reader_reseek(void)\n+{\n+\tstruct reftable_ref_record records[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/main\",\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 42 },\n+\t\t},\n+\t};\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_ref_record ref = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader *reader;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret;\n+\n+\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n+\tblock_source_from_strbuf(&source, &buf);\n+\n+\tret = reftable_reader_new(&reader, &source, \"name\");\n+\tcheck_int(ret, ==, 0);\n+\n+\treftable_reader_init_ref_iterator(reader, &it);\n+\n+\tfor (size_t i = 0; i < 5; i++) {\n+\t\tret = reftable_iterator_seek_ref(&it, \"\");\n+\t\tcheck_int(ret, ==, 0);\n+\t\tret = reftable_iterator_next_ref(&it, &ref);\n+\t\tcheck_int(ret, ==, 0);\n+\n+\t\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n+\t\tcheck_int(ret, ==, 1);\n+\n+\t\tret = reftable_iterator_next_ref(&it, &ref);\n+\t\tcheck_int(ret, ==, 1);\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_iterator_destroy(&it);\n+\treftable_reader_decref(reader);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n+{\n+\tTEST(t_reader_seek_once(), \"reader can seek once\");\n+\tTEST(t_reader_reseek(), \"reader can reseek multiple times\");\n+\treturn test_done();\n+}\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502438","messageId":"f3922b81db69cd3bbdddcfbe02c99613448fd9ed.1725881266.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-09T11:31:49Z","receivedAt":"2024-09-09T11:31:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Exclude patterns can be used by reference backends to skip over blocks\nof references that are uninteresting to the caller. Reference backends\ndo not have to wire up support for them, and all callers are expected to\nbehave as if the backend didn't support them. In fact, the only backend\nthat supports exclude patterns right now is the \"packed\" backend.\n\nExclude patterns can be quite an important performance optimization in\nrepositories that have loads of references. The patterns are set up in\ncase \"transfer.hideRefs\" and friends are configured during a fetch, so\nhandling these patterns becomes important once there are lots of hidden\nrefs in a served repository.\n\nNow that we have properly re-seekable reftable iterators we can also\nwire up support for these patterns in the \"reftable\" backend. Doing so\nis conceptually simple: once we hit a reference whose prefix matches the\ncurrent exclude pattern we re-seek the iterator to the first reference\nthat doesn't match the pattern anymore. This schema only works for\ntrivial patterns that do not have any globbing characters in them, but\nthis restriction also applies do the \"packed\" backend.\n\nThis makes t1419 work with the \"reftable\" backend with some slight\nmodifications. Of course it also speeds up listing of references with\nhidden refs. The following benchmark prints one reference with 1 million\nhidden references:\n\n Benchmark 1: HEAD~\n Time (mean ± σ): 93.3 ms ± 2.1 ms [User: 90.3 ms, System: 2.5 ms]\n Range (min … max): 89.8 ms … 97.2 ms 33 runs\n\n Benchmark 2: HEAD\n Time (mean ± σ): 4.2 ms ± 0.6 ms [User: 2.2 ms, System: 1.8 ms]\n Range (min … max): 3.1 ms … 8.1 ms 765 runs\n\n Summary\n HEAD ran\n 22.15 ± 3.19 times faster than HEAD~\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 125 +++++++++++++++++++++++++++++++++++++++-\n t/t1419-exclude-refs.sh | 33 ++++++++---\n trace2.h | 1 +\n trace2/tr2_ctr.c | 5 ++\n 4 files changed, 152 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..5c241097a4e 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -21,6 +21,7 @@\n #include \"../reftable/reftable-iterator.h\"\n #include \"../setup.h\"\n #include \"../strmap.h\"\n+#include \"../trace2.h\"\n #include \"parse.h\"\n #include \"refs-internal.h\"\n \n@@ -447,10 +448,81 @@ struct reftable_ref_iterator {\n \n \tconst char *prefix;\n \tsize_t prefix_len;\n+\tchar **exclude_patterns;\n+\tsize_t exclude_patterns_index;\n+\tsize_t exclude_patterns_strlen;\n \tunsigned int flags;\n \tint err;\n };\n \n+/*\n+ * Handle exclude patterns. Returns either `1`, which tells the caller that the\n+ * current reference shall not be shown. Or `0`, which indicates that it should\n+ * be shown.\n+ */\n+static int should_exclude_current_ref(struct reftable_ref_iterator *iter)\n+{\n+\twhile (iter->exclude_patterns[iter->exclude_patterns_index]) {\n+\t\tconst char *pattern = iter->exclude_patterns[iter->exclude_patterns_index];\n+\t\tchar *ref_after_pattern;\n+\t\tint cmp;\n+\n+\t\t/*\n+\t\t * Lazily cache the pattern length so that we don't have to\n+\t\t * recompute it every time this function is called.\n+\t\t */\n+\t\tif (!iter->exclude_patterns_strlen)\n+\t\t\titer->exclude_patterns_strlen = strlen(pattern);\n+\n+\t\t/*\n+\t\t * When the reference name is lexicographically bigger than the\n+\t\t * current exclude pattern we know that it won't ever match any\n+\t\t * of the following references, either. We thus advance to the\n+\t\t * next pattern and re-check whether it matches.\n+\t\t *\n+\t\t * Otherwise, if it's smaller, then we do not have a match and\n+\t\t * thus want to show the current reference.\n+\t\t */\n+\t\tcmp = strncmp(iter->ref.refname, pattern,\n+\t\t\t iter->exclude_patterns_strlen);\n+\t\tif (cmp > 0) {\n+\t\t\titer->exclude_patterns_index++;\n+\t\t\titer->exclude_patterns_strlen = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (cmp < 0)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * The reference shares a prefix with the exclude pattern and\n+\t\t * shall thus be omitted. We skip all references that match the\n+\t\t * pattern by seeking to the first reference after the block of\n+\t\t * matches.\n+\t\t *\n+\t\t * This is done by appending the highest possible character to\n+\t\t * the pattern. Consequently, all references that have the\n+\t\t * pattern as prefix and whose suffix starts with anything in\n+\t\t * the range [0x00, 0xfe] are skipped. And given that 0xff is a\n+\t\t * non-printable character that shouldn't ever be in a ref name,\n+\t\t * we'd not yield any such record, either.\n+\t\t *\n+\t\t * Note that the seeked-to reference may also be excluded. This\n+\t\t * is not handled here though, but the caller is expected to\n+\t\t * loop and re-verify the next reference for us.\n+\t\t */\n+\t\tref_after_pattern = xstrfmt(\"%s%c\", pattern, 0xff);\n+\t\titer->err = reftable_iterator_seek_ref(&iter->iter, ref_after_pattern);\n+\t\titer->exclude_patterns_index++;\n+\t\titer->exclude_patterns_strlen = 0;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_REFTABLE_RESEEKS, 1);\n+\n+\t\tfree(ref_after_pattern);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n {\n \tstruct reftable_ref_iterator *iter =\n@@ -481,6 +553,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\tbreak;\n \t\t}\n \n+\t\tif (iter->exclude_patterns && should_exclude_current_ref(iter))\n+\t\t\tcontinue;\n+\n \t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n \t\t parse_worktree_ref(iter->ref.refname, NULL, NULL, NULL) !=\n \t\t\t REF_WORKTREE_CURRENT)\n@@ -570,6 +645,11 @@ static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_ref_iterator *)ref_iterator;\n \treftable_ref_record_release(&iter->ref);\n \treftable_iterator_destroy(&iter->iter);\n+\tif (iter->exclude_patterns) {\n+\t\tfor (size_t i = 0; iter->exclude_patterns[i]; i++)\n+\t\t\tfree(iter->exclude_patterns[i]);\n+\t\tfree(iter->exclude_patterns);\n+\t}\n \tfree(iter);\n \treturn ITER_DONE;\n }\n@@ -580,9 +660,45 @@ static struct ref_iterator_vtable reftable_ref_iterator_vtable = {\n \t.abort = reftable_ref_iterator_abort\n };\n \n+static char **filter_exclude_patterns(const char **exclude_patterns)\n+{\n+\tsize_t filtered_size = 0, filtered_alloc = 0;\n+\tchar **filtered = NULL;\n+\n+\tif (!exclude_patterns)\n+\t\treturn NULL;\n+\n+\tfor (size_t i = 0; ; i++) {\n+\t\tconst char *exclude_pattern = exclude_patterns[i];\n+\t\tint has_glob = 0;\n+\n+\t\tif (!exclude_pattern)\n+\t\t\tbreak;\n+\n+\t\tfor (const char *p = exclude_pattern; *p; p++) {\n+\t\t\thas_glob = is_glob_special(*p);\n+\t\t\tif (has_glob)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (has_glob)\n+\t\t\tcontinue;\n+\n+\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n+\t\tfiltered[filtered_size++] = xstrdup(exclude_pattern);\n+\t}\n+\n+\tif (filtered_size) {\n+\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n+\t\tfiltered[filtered_size++] = NULL;\n+\t}\n+\n+\treturn filtered;\n+}\n+\n static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_store *refs,\n \t\t\t\t\t\t\t struct reftable_stack *stack,\n \t\t\t\t\t\t\t const char *prefix,\n+\t\t\t\t\t\t\t const char **exclude_patterns,\n \t\t\t\t\t\t\t int flags)\n {\n \tstruct reftable_ref_iterator *iter;\n@@ -595,6 +711,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \titer->base.oid = &iter->oid;\n \titer->flags = flags;\n \titer->refs = refs;\n+\titer->exclude_patterns = filter_exclude_patterns(exclude_patterns);\n \n \tret = refs->err;\n \tif (ret)\n@@ -616,7 +733,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \n static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_store,\n \t\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t\t const char **exclude_patterns UNUSED,\n+\t\t\t\t\t\t const char **exclude_patterns,\n \t\t\t\t\t\t unsigned int flags)\n {\n \tstruct reftable_ref_iterator *main_iter, *worktree_iter;\n@@ -627,7 +744,8 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t\trequired_flags |= REF_STORE_ODB;\n \trefs = reftable_be_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n-\tmain_iter = ref_iterator_for_stack(refs, refs->main_stack, prefix, flags);\n+\tmain_iter = ref_iterator_for_stack(refs, refs->main_stack, prefix,\n+\t\t\t\t\t exclude_patterns, flags);\n \n \t/*\n \t * The worktree stack is only set when we're in an actual worktree\n@@ -641,7 +759,8 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t * Otherwise we merge both the common and the per-worktree refs into a\n \t * single iterator.\n \t */\n-\tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n+\tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix,\n+\t\t\t\t\t exclude_patterns, flags);\n \treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\tref_iterator_select, NULL);\n }\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 13595744190..8c7b69a9b34 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,12 +8,6 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-if test_have_prereq !REFFILES\n-then\n-\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n-\ttest_done\n-fi\n-\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n@@ -28,7 +22,14 @@ assert_jumps () {\n \tlocal nr=\"$1\"\n \tlocal trace=\"$2\"\n \n-\tgrep -q \"name:jumps_made value:$nr$\" $trace\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tgrep -q \"name:jumps_made value:$nr$\" $trace;;\n+\treftable)\n+\t\tgrep -q \"name:reseeks_made value:$nr$\" $trace;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n }\n \n assert_no_jumps () {\n@@ -89,7 +90,14 @@ test_expect_success 'adjacent, non-overlapping excluded regions' '\n \tfor_each_ref refs/heads/foo refs/heads/quux >expect &&\n \n \ttest_cmp expect actual &&\n-\tassert_jumps 1 perf\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tassert_jumps 1 perf;;\n+\treftable)\n+\t\tassert_jumps 2 perf;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n '\n \n test_expect_success 'overlapping excluded regions' '\n@@ -106,7 +114,14 @@ test_expect_success 'several overlapping excluded regions' '\n \tfor_each_ref refs/heads/quux >expect &&\n \n \ttest_cmp expect actual &&\n-\tassert_jumps 1 perf\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tassert_jumps 1 perf;;\n+\treftable)\n+\t\tassert_jumps 3 perf;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n '\n \n test_expect_success 'non-matching excluded section' '\ndiff --git a/trace2.h b/trace2.h\nindex 19e04bf040f..901f39253a6 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -554,6 +554,7 @@ enum trace2_counter_id {\n \tTRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */\n \n \tTRACE2_COUNTER_ID_PACKED_REFS_JUMPS, /* counts number of jumps */\n+\tTRACE2_COUNTER_ID_REFTABLE_RESEEKS, /* counts number of re-seeks */\n \n \t/* counts number of fsyncs */\n \tTRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,\ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex d3a33715c14..036b643578b 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -31,6 +31,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n \t\t.name = \"jumps_made\",\n \t\t.want_per_thread_events = 0,\n \t},\n+\t[TRACE2_COUNTER_ID_REFTABLE_RESEEKS] = {\n+\t\t.category = \"reftable\",\n+\t\t.name = \"reseeks_made\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n \t[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {\n \t\t.category = \"fsync\",\n \t\t.name = \"writeout-only\",\n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502749","messageId":"CAOLa=ZTM9N0i+8jDp24pp1DdU1mmwU02L4vOP6GOpGW-=SJUoQ@mail.gmail.com","threadId":"62084","inReplyTo":"8d347bc5599e2a679d50fed073e0f09ffdad85c4.1725881266.git.ps@pks.im","subject":"Re: [PATCH 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-13T11:35:35Z","receivedAt":"2024-09-13T11:35:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Reference namespaces allow commands like git-upload-pack(1) to serve\n> different sets of references to the client depending on which namespace\n> is enabled, which is for example useful in fork networks. Namespaced\n> refs are stored with a `refs/namespaces/$namespace` prefix, but all the\n> user will ultimately see is a stripped version where that prefix is\n> removed.\n>\n> The way that this interacts with \"transfer.hideRefs\" is not immediately\n> obvious: the hidden refs can either apply to the stripped references, or\n> to the non-stripped ones that still have the namespace prefix. In fact,\n> the \"transfer.hideRefs\" machinery does the former and applies to the\n> stripped reference by default, but rules can have \"^\" prefixed to switch\n> this behaviour to iinstead match against the rull reference name.\n\ns/iinstead/instead\ns/rull/full\n\n> Namespaces are exclusively handled at the generic \"refs\" layer, the\n> respective backends have no clue that such a thing even exists. This\n> also has the consequence that they cannot handle hiding references as\n> soon as reference namespaces come into play because they neither know\n> whether a namespace is active, nor do they know how to strip references\n> if they are active.\n>\n> Handling such exclude patterns in `refs_for_each_namespaced_ref()` and\n> `refs_for_each_fullref_in_prefixes()` is broken though, as both support\n> that the user passes both namespaces and exclude patterns. In the case\n> where both are set we will exclude references with unstripped names,\n> even though we really wanted to exclude references based on their\n> stripped names.\n>\n> This only surfaces when:\n>\n> - A repository uses reference namespaces.\n>\n> - \"transfer.hideRefs\" is active.\n>\n> - The namespaced references are packed into the \"packed-refs\" file.\n>\n\nSo this is because we don't even apply exclude patterns to the loose\nrefs right?\n\nTo understand correctly, the transport layer passes on\n'transfer.hideRefs' as `exclude_refs` to the generic refs layer. This is\nmostly to optimize the reference backend to skip such refs. This is used\nby the packed-refs currently but not used for loose refs.\n\nThe transfer layer also uses this list in `mark_our_ref()` to skip refs\nas needed.\n\nSo all in all `exclude_refs` here is mostly for optimization.\n\n> None of our tests exercise this scenario, and thus we haven't ever hit\n> it. While t5509 exercises both (1) and (2), it does not happen to hit\n> (3). It is trivial to demonstrate the bug though by explicitly packing\n> refs in the tests, and then we indeed surface the breakage.\n\nNit: I know you're referring to the three points stated above, it would\nbe nice if they were numbered.\n\n> Fix this bug by prefixing exclude patterns with the namespace in the\n> generic layer.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs.c | 35 ++++++++++++++++++++++++++++----\n> refs.h | 9 ++++++++\n> t/t5509-fetch-push-namespaces.sh | 1 +\n> 3 files changed, 41 insertions(+), 4 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index ceb72d4bd74..b3a367ea12c 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> \treturn hide_refs->v;\n> }\n>\n> +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> +\t\t\t\t\t const char *namespace,\n> +\t\t\t\t\t struct strvec *out)\n> +{\n> +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n\nWhat scenario would `!*namespace` be possible?\n\n> +\t\treturn exclude_patterns;\n> +\n> +\tfor (size_t i = 0; exclude_patterns[i]; i++)\n> +\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n> +\n> +\treturn out->v;\n> +}\n> +\n> const char *find_descendant_ref(const char *dirname,\n> \t\t\t\tconst struct string_list *extras,\n> \t\t\t\tconst struct string_list *skip)\n> @@ -1634,11 +1647,19 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n> \t\t\t\t const char **exclude_patterns,\n> \t\t\t\t each_ref_fn fn, void *cb_data)\n> {\n> -\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n> +\tstruct strbuf prefix = STRBUF_INIT;\n> \tint ret;\n> -\tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n> -\tret = do_for_each_ref(refs, buf.buf, exclude_patterns, fn, 0, 0, cb_data);\n> -\tstrbuf_release(&buf);\n> +\n> +\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n> +\t\t\t\t\t\t\t get_git_namespace(),\n> +\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n> +\n> +\tstrbuf_addf(&prefix, \"%srefs/\", get_git_namespace());\n> +\tret = do_for_each_ref(refs, prefix.buf, exclude_patterns, fn, 0, 0, cb_data);\n> +\n> +\tstrvec_clear(&namespaced_exclude_patterns);\n> +\tstrbuf_release(&prefix);\n> \treturn ret;\n> }\n>\n> @@ -1719,6 +1740,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n> \t\t\t\t const char **exclude_patterns,\n> \t\t\t\t each_ref_fn fn, void *cb_data)\n> {\n> +\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n> \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n> \tstruct string_list_item *prefix;\n> \tstruct strbuf buf = STRBUF_INIT;\n> @@ -1730,6 +1752,10 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n> \t\tstrbuf_addstr(&buf, namespace);\n> \tnamespace_len = buf.len;\n>\n> +\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n> +\t\t\t\t\t\t\t namespace,\n> +\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n> +\n> \tfor_each_string_list_item(prefix, &prefixes) {\n> \t\tstrbuf_addstr(&buf, prefix->string);\n> \t\tret = refs_for_each_fullref_in(ref_store, buf.buf,\n> @@ -1739,6 +1765,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n> \t\tstrbuf_setlen(&buf, namespace_len);\n> \t}\n>\n> +\tstrvec_clear(&namespaced_exclude_patterns);\n> \tstring_list_clear(&prefixes, 0);\n> \tstrbuf_release(&buf);\n> \treturn ret;\n> diff --git a/refs.h b/refs.h\n> index f8b919a1388..3f774e96d18 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -859,6 +859,15 @@ int ref_is_hidden(const char *, const char *, const struct strvec *);\n> */\n> const char **hidden_refs_to_excludes(const struct strvec *hide_refs);\n>\n> +/*\n> + * Prefix all exclude patterns with the namespace, if any. This is required\n> + * because exclude patterns apply to the stripped reference name, not the full\n> + * reference name with the namespace.\n> + */\n> +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> +\t\t\t\t\t const char *namespace,\n> +\t\t\t\t\t struct strvec *out);\n> +\n\nDo we need to expose this? Can't it be made static?\n"},{"id":"502750","messageId":"CAOLa=ZSf01hsh+jX_ubRfKLAomQr4_Ub7U5Mvht-CZ9=kj-nNQ@mail.gmail.com","threadId":"62084","inReplyTo":"0317a5a7edeab29b7cad31d11140bd99f459144f.1725881266.git.ps@pks.im","subject":"Re: [PATCH 2/6] builtin/receive-pack: fix exclude patterns when announcing refs","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-13T11:50:04Z","receivedAt":"2024-09-13T11:50:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 3f35140e489..478c62ca836 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -339,12 +339,26 @@ static void show_one_alternate_ref(const struct object_id *oid,\n> static void write_head_info(void)\n> {\n> \tstatic struct oidset seen = OIDSET_INIT;\n> +\tstruct strvec excludes_vector = STRVEC_INIT;\n> +\tconst char **exclude_patterns;\n> +\n> +\t/*\n> +\t * We need access to the reference names both with and without their\n> +\t * namespace and thus cannot use `refs_for_each_namespaced_ref()`. We\n> +\t * thus have to adapt exclude patterns to carry the namespace prefix\n> +\t * ourselves.\n> +\t */\n> +\texclude_patterns = get_namespaced_exclude_patterns(\n> +\t\thidden_refs_to_excludes(&hidden_refs),\n> +\t\tget_git_namespace(), &excludes_vector);\n>\n\nNit: So we do use it here, might be worth pointing out in the previous commit.\n\n[snip]\n"},{"id":"502751","messageId":"CAOLa=ZRA=vFjnWZC7PQBAxOxDfD9X6767ddgBqwaen5n6JLt0w@mail.gmail.com","threadId":"62084","inReplyTo":"a281f936a2b3a697b32f57652a2120afd54f7e4f.1725881266.git.ps@pks.im","subject":"Re: [PATCH 5/6] reftable/reader: make table iterator reseekable","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-13T12:11:54Z","receivedAt":"2024-09-13T12:11:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\n> index b8c92fdb365..19e54bdfb8b 100644\n> --- a/t/unit-tests/t-reftable-merged.c\n> +++ b/t/unit-tests/t-reftable-merged.c\n> @@ -194,6 +194,81 @@ static void t_merged_refs(void)\n> \treftable_free(bs);\n> }\n>\n> +static void t_merged_seek_multiple_times(void)\n> +{\n> +\tstruct reftable_ref_record r1[] = {\n> +\t\t{\n> +\t\t\t.refname = (char *) \"a\",\n> +\t\t\t.update_index = 1,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { 1 },\n> +\t\t},\n> +\t\t{\n> +\t\t\t.refname = (char *) \"c\",\n> +\t\t\t.update_index = 1,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { 2 },\n> +\t\t}\n> +\t};\n> +\tstruct reftable_ref_record r2[] = {\n> +\t\t{\n> +\t\t\t.refname = (char *) \"b\",\n> +\t\t\t.update_index = 2,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { 3 },\n> +\t\t},\n> +\t\t{\n> +\t\t\t.refname = (char *) \"d\",\n> +\t\t\t.update_index = 2,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { 4 },\n> +\t\t},\n> +\t};\n> +\tstruct reftable_ref_record *refs[] = {\n> +\t\tr1, r2,\n> +\t};\n> +\tsize_t sizes[] = {\n> +\t\tARRAY_SIZE(r1), ARRAY_SIZE(r2),\n> +\t};\n> +\tstruct strbuf bufs[] = {\n> +\t\tSTRBUF_INIT, STRBUF_INIT,\n> +\t};\n> +\tstruct reftable_block_source *sources = NULL;\n> +\tstruct reftable_reader **readers = NULL;\n> +\tstruct reftable_ref_record rec = { 0 };\n> +\tstruct reftable_iterator it = { 0 };\n> +\tstruct reftable_merged_table *mt;\n> +\n> +\tmt = merged_table_from_records(refs, &sources, &readers, sizes, bufs, 2);\n> +\tmerged_table_init_iter(mt, &it, BLOCK_TYPE_REF);\n> +\n> +\tfor (size_t i = 0; i < 5; i++) {\n> +\t\tint err = reftable_iterator_seek_ref(&it, \"c\");\n> +\t\tcheck(!err);\n> +\n> +\t\terr = reftable_iterator_next_ref(&it, &rec);\n> +\t\tcheck(!err);\n> +\t\terr = reftable_ref_record_equal(&rec, &r1[1], GIT_SHA1_RAWSZ);\n> +\t\tcheck(err == 1);\n> +\n> +\t\terr = reftable_iterator_next_ref(&it, &rec);\n> +\t\tcheck(!err);\n> +\t\terr = reftable_ref_record_equal(&rec, &r2[1], GIT_SHA1_RAWSZ);\n> +\t\tcheck(err == 1);\n> +\n\nSo this skips r2[0] because when we seek, we seek all sub-iterators. So\nin r2 we seek to 'd' since that is the next alphabetical value.\n\n[snip]\n\n> diff --git a/t/unit-tests/t-reftable-reader.c b/t/unit-tests/t-reftable-reader.c\n> new file mode 100644\n> index 00000000000..7a46580b6f1\n> --- /dev/null\n> +++ b/t/unit-tests/t-reftable-reader.c\n> @@ -0,0 +1,96 @@\n> +#include \"test-lib.h\"\n> +#include \"lib-reftable.h\"\n> +#include \"reftable/blocksource.h\"\n> +#include \"reftable/reader.h\"\n> +\n> +static int t_reader_seek_once(void)\n> +{\n> +\tstruct reftable_ref_record records[] = {\n> +\t\t{\n> +\t\t\t.refname = (char *) \"refs/heads/main\",\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { 42 },\n> +\t\t},\n> +\t};\n> +\tstruct reftable_block_source source = { 0 };\n> +\tstruct reftable_ref_record ref = { 0 };\n> +\tstruct reftable_iterator it = { 0 };\n> +\tstruct reftable_reader *reader;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint ret;\n> +\n> +\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n> +\tblock_source_from_strbuf(&source, &buf);\n> +\n> +\tret = reftable_reader_new(&reader, &source, \"name\");\n> +\tcheck_int(ret, ==, 0);\n> +\n> +\treftable_reader_init_ref_iterator(reader, &it);\n> +\tret = reftable_iterator_seek_ref(&it, \"\");\n> +\tcheck_int(ret, ==, 0);\n> +\tret = reftable_iterator_next_ref(&it, &ref);\n> +\tcheck_int(ret, ==, 0);\n> +\n> +\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n\ns/20/GIT_SHA1_RAWSZ\n\nAlso here and elsewhere, shouldn't we just do\n`check(reftable_ref_record_equal(...))` or even\n`!check(reftable_iterator_seek_ref(...))` ?\n\n[snip]\n"},{"id":"502752","messageId":"CAOLa=ZQikyvJCRZ=mCfve+VWZfrvPL1bg55txB1q0Nh3SW_JJQ@mail.gmail.com","threadId":"62084","inReplyTo":"f3922b81db69cd3bbdddcfbe02c99613448fd9ed.1725881266.git.ps@pks.im","subject":"Re: [PATCH 6/6] refs/reftable: wire up support for exclude patterns","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-13T12:47:06Z","receivedAt":"2024-09-13T12:47:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> hidden refs. The following benchmark prints one reference with 1 million\n> hidden references:\n>\n> Benchmark 1: HEAD~\n> Time (mean ± σ): 93.3 ms ± 2.1 ms [User: 90.3 ms, System: 2.5 ms]\n> Range (min … max): 89.8 ms … 97.2 ms 33 runs\n>\n> Benchmark 2: HEAD\n> Time (mean ± σ): 4.2 ms ± 0.6 ms [User: 2.2 ms, System: 1.8 ms]\n> Range (min … max): 3.1 ms … 8.1 ms 765 runs\n>\n> Summary\n> HEAD ran\n> 22.15 ± 3.19 times faster than HEAD~\n>\n\nNice.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/reftable-backend.c | 125 +++++++++++++++++++++++++++++++++++++++-\n> t/t1419-exclude-refs.sh | 33 ++++++++---\n> trace2.h | 1 +\n> trace2/tr2_ctr.c | 5 ++\n> 4 files changed, 152 insertions(+), 12 deletions(-)\n>\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 1c4b19e737f..5c241097a4e 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -21,6 +21,7 @@\n> #include \"../reftable/reftable-iterator.h\"\n> #include \"../setup.h\"\n> #include \"../strmap.h\"\n> +#include \"../trace2.h\"\n> #include \"parse.h\"\n> #include \"refs-internal.h\"\n>\n> @@ -447,10 +448,81 @@ struct reftable_ref_iterator {\n>\n> \tconst char *prefix;\n> \tsize_t prefix_len;\n> +\tchar **exclude_patterns;\n> +\tsize_t exclude_patterns_index;\n> +\tsize_t exclude_patterns_strlen;\n> \tunsigned int flags;\n> \tint err;\n> };\n>\n> +/*\n> + * Handle exclude patterns. Returns either `1`, which tells the caller that the\n> + * current reference shall not be shown. Or `0`, which indicates that it should\n> + * be shown.\n> + */\n> +static int should_exclude_current_ref(struct reftable_ref_iterator *iter)\n> +{\n> +\twhile (iter->exclude_patterns[iter->exclude_patterns_index]) {\n> +\t\tconst char *pattern = iter->exclude_patterns[iter->exclude_patterns_index];\n> +\t\tchar *ref_after_pattern;\n> +\t\tint cmp;\n> +\n> +\t\t/*\n> +\t\t * Lazily cache the pattern length so that we don't have to\n> +\t\t * recompute it every time this function is called.\n> +\t\t */\n> +\t\tif (!iter->exclude_patterns_strlen)\n> +\t\t\titer->exclude_patterns_strlen = strlen(pattern);\n> +\n> +\t\t/*\n> +\t\t * When the reference name is lexicographically bigger than the\n> +\t\t * current exclude pattern we know that it won't ever match any\n> +\t\t * of the following references, either. We thus advance to the\n> +\t\t * next pattern and re-check whether it matches.\n\nSo this means that the exclude patterns were lexicographically sorted.\nOtherwise this would work.\n\n> +\t\t * Otherwise, if it's smaller, then we do not have a match and\n> +\t\t * thus want to show the current reference.\n> +\t\t */\n> +\t\tcmp = strncmp(iter->ref.refname, pattern,\n> +\t\t\t iter->exclude_patterns_strlen);\n> +\t\tif (cmp > 0) {\n> +\t\t\titer->exclude_patterns_index++;\n> +\t\t\titer->exclude_patterns_strlen = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (cmp < 0)\n> +\t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * The reference shares a prefix with the exclude pattern and\n> +\t\t * shall thus be omitted. We skip all references that match the\n> +\t\t * pattern by seeking to the first reference after the block of\n> +\t\t * matches.\n> +\t\t *\n> +\t\t * This is done by appending the highest possible character to\n> +\t\t * the pattern. Consequently, all references that have the\n> +\t\t * pattern as prefix and whose suffix starts with anything in\n> +\t\t * the range [0x00, 0xfe] are skipped. And given that 0xff is a\n> +\t\t * non-printable character that shouldn't ever be in a ref name,\n> +\t\t * we'd not yield any such record, either.\n> +\t\t *\n\nThis is simple yet clever.\n\n> +\t\t * Note that the seeked-to reference may also be excluded. This\n> +\t\t * is not handled here though, but the caller is expected to\n> +\t\t * loop and re-verify the next reference for us.\n> +\t\t */\n\nThe seeked-to reference here being the one with 0xff. We could get rid\nof this by doing something like this:\n\n int last_char_idx = iter->exclude_patterns_strlen - 1\n ref_after_pattern = xstrfmt(\"%s\", pattern);\n ref_after_pattern[last_char_idx] = ref_after_pattern[last_char_idx] + 1;\n\ninstead no?\n\n> +\t\tref_after_pattern = xstrfmt(\"%s%c\", pattern, 0xff);\n> +\t\titer->err = reftable_iterator_seek_ref(&iter->iter, ref_after_pattern);\n> +\t\titer->exclude_patterns_index++;\n> +\t\titer->exclude_patterns_strlen = 0;\n> +\t\ttrace2_counter_add(TRACE2_COUNTER_ID_REFTABLE_RESEEKS, 1);\n> +\n> +\t\tfree(ref_after_pattern);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n> {\n> \tstruct reftable_ref_iterator *iter =\n> @@ -481,6 +553,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\tif (iter->exclude_patterns && should_exclude_current_ref(iter))\n> +\t\t\tcontinue;\n> +\n> \t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n> \t\t parse_worktree_ref(iter->ref.refname, NULL, NULL, NULL) !=\n> \t\t\t REF_WORKTREE_CURRENT)\n> @@ -570,6 +645,11 @@ static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)\n> \t\t(struct reftable_ref_iterator *)ref_iterator;\n> \treftable_ref_record_release(&iter->ref);\n> \treftable_iterator_destroy(&iter->iter);\n> +\tif (iter->exclude_patterns) {\n> +\t\tfor (size_t i = 0; iter->exclude_patterns[i]; i++)\n> +\t\t\tfree(iter->exclude_patterns[i]);\n> +\t\tfree(iter->exclude_patterns);\n> +\t}\n> \tfree(iter);\n> \treturn ITER_DONE;\n> }\n> @@ -580,9 +660,45 @@ static struct ref_iterator_vtable reftable_ref_iterator_vtable = {\n> \t.abort = reftable_ref_iterator_abort\n> };\n>\n> +static char **filter_exclude_patterns(const char **exclude_patterns)\n> +{\n> +\tsize_t filtered_size = 0, filtered_alloc = 0;\n> +\tchar **filtered = NULL;\n> +\n> +\tif (!exclude_patterns)\n> +\t\treturn NULL;\n> +\n> +\tfor (size_t i = 0; ; i++) {\n> +\t\tconst char *exclude_pattern = exclude_patterns[i];\n> +\t\tint has_glob = 0;\n> +\n> +\t\tif (!exclude_pattern)\n> +\t\t\tbreak;\n> +\n> +\t\tfor (const char *p = exclude_pattern; *p; p++) {\n> +\t\t\thas_glob = is_glob_special(*p);\n> +\t\t\tif (has_glob)\n> +\t\t\t\tbreak;\n> +\t\t}\n\nWhy do we need to filter excludes here? Don't the callee's already do\nsomething like this?\n\n[snip]\n"},{"id":"502753","messageId":"CAOLa=ZS3XBG4_=D5rCkntJuU0tzV7t=rB95rkNvWgm++Q+xukg@mail.gmail.com","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"Re: [PATCH 0/6] refs/reftable: wire up exclude patterns","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-13T12:48:02Z","receivedAt":"2024-09-13T12:48:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this patch series wires up exclude patterns for the reftable backend.\n> Exclude patterns allow the backend to skip references internally that\n> match a specific pattern. This avoids having to read references that the\n> caller would discard immediately anyway.\n>\n> The series is structured as follows:\n>\n> - Patches 1 and 2 fix two separate bugs in how we currently handle\n> exclude patterns in combination with namespaces. We didn't happen to\n> stumble over these bugs before because exclude patterns are only\n> implemented for the \"packed\" backend. But once you start to pack\n> refs we exclude references based on their full name instead of the\n> name with the prefixed stripped. For the reftable backend we'd\n> always hit those bugs because it always uses exclude patterns when\n> passed.\n>\n> - Patches 3 to 5 wire up proper re-seeking of reftable iterators and\n> adds some tests to demonstrate that this works as expected. This is\n> a prerequisite for handling exclude patterns.\n>\n> - Patch 6 wires up exclude patterns in the reftable backend by\n> re-seeking iterators once we hit an excluded reference.\n>\n> Thanks!\n>\n> Patrick\n>\n\nThis was a bit more intensive so I took my time with the review. Overall\nI have some questions/comments. But the series looks good. Thanks!\n\nKarthik\n"},{"id":"502820","messageId":"ZufWilh3WupDnVWf@pks.im","threadId":"62084","inReplyTo":"CAOLa=ZTM9N0i+8jDp24pp1DdU1mmwU02L4vOP6GOpGW-=SJUoQ@mail.gmail.com","subject":"Re: [PATCH 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T06:56:10Z","receivedAt":"2024-09-16T06:56:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 13, 2024 at 04:35:35AM -0700, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> > Handling such exclude patterns in `refs_for_each_namespaced_ref()` and\n> > `refs_for_each_fullref_in_prefixes()` is broken though, as both support\n> > that the user passes both namespaces and exclude patterns. In the case\n> > where both are set we will exclude references with unstripped names,\n> > even though we really wanted to exclude references based on their\n> > stripped names.\n> >\n> > This only surfaces when:\n> >\n> > - A repository uses reference namespaces.\n> >\n> > - \"transfer.hideRefs\" is active.\n> >\n> > - The namespaced references are packed into the \"packed-refs\" file.\n> >\n> \n> So this is because we don't even apply exclude patterns to the loose\n> refs right?\n> \n> To understand correctly, the transport layer passes on\n> 'transfer.hideRefs' as `exclude_refs` to the generic refs layer. This is\n> mostly to optimize the reference backend to skip such refs. This is used\n> by the packed-refs currently but not used for loose refs.\n> \n> The transfer layer also uses this list in `mark_our_ref()` to skip refs\n> as needed.\n> \n> So all in all `exclude_refs` here is mostly for optimization.\n\nYup.\n\n> > diff --git a/refs.c b/refs.c\n> > index ceb72d4bd74..b3a367ea12c 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> > \treturn hide_refs->v;\n> > }\n> >\n> > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> > +\t\t\t\t\t const char *namespace,\n> > +\t\t\t\t\t struct strvec *out)\n> > +{\n> > +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n> \n> What scenario would `!*namespace` be possible?\n\nIt's the default value of `get_git_namespace()`.\n\n> > diff --git a/refs.h b/refs.h\n> > index f8b919a1388..3f774e96d18 100644\n> > --- a/refs.h\n> > +++ b/refs.h\n> > @@ -859,6 +859,15 @@ int ref_is_hidden(const char *, const char *, const struct strvec *);\n> > */\n> > const char **hidden_refs_to_excludes(const struct strvec *hide_refs);\n> >\n> > +/*\n> > + * Prefix all exclude patterns with the namespace, if any. This is required\n> > + * because exclude patterns apply to the stripped reference name, not the full\n> > + * reference name with the namespace.\n> > + */\n> > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> > +\t\t\t\t\t const char *namespace,\n> > +\t\t\t\t\t struct strvec *out);\n> > +\n> \n> Do we need to expose this? Can't it be made static?\n\nIt will be used by the next patch. I'll amen the commit message to point\nthis out.\n\nPatrick\n"},{"id":"502821","messageId":"ZufWjY053IKObF8T@pks.im","threadId":"62084","inReplyTo":"CAOLa=ZRA=vFjnWZC7PQBAxOxDfD9X6767ddgBqwaen5n6JLt0w@mail.gmail.com","subject":"Re: [PATCH 5/6] reftable/reader: make table iterator reseekable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T06:56:13Z","receivedAt":"2024-09-16T06:56:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 13, 2024 at 07:11:54AM -0500, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/t/unit-tests/t-reftable-reader.c b/t/unit-tests/t-reftable-reader.c\n> > new file mode 100644\n> > index 00000000000..7a46580b6f1\n> > --- /dev/null\n> > +++ b/t/unit-tests/t-reftable-reader.c\n> > @@ -0,0 +1,96 @@\n> > +#include \"test-lib.h\"\n> > +#include \"lib-reftable.h\"\n> > +#include \"reftable/blocksource.h\"\n> > +#include \"reftable/reader.h\"\n> > +\n> > +static int t_reader_seek_once(void)\n> > +{\n> > +\tstruct reftable_ref_record records[] = {\n> > +\t\t{\n> > +\t\t\t.refname = (char *) \"refs/heads/main\",\n> > +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> > +\t\t\t.value.val1 = { 42 },\n> > +\t\t},\n> > +\t};\n> > +\tstruct reftable_block_source source = { 0 };\n> > +\tstruct reftable_ref_record ref = { 0 };\n> > +\tstruct reftable_iterator it = { 0 };\n> > +\tstruct reftable_reader *reader;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tint ret;\n> > +\n> > +\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n> > +\tblock_source_from_strbuf(&source, &buf);\n> > +\n> > +\tret = reftable_reader_new(&reader, &source, \"name\");\n> > +\tcheck_int(ret, ==, 0);\n> > +\n> > +\treftable_reader_init_ref_iterator(reader, &it);\n> > +\tret = reftable_iterator_seek_ref(&it, \"\");\n> > +\tcheck_int(ret, ==, 0);\n> > +\tret = reftable_iterator_next_ref(&it, &ref);\n> > +\tcheck_int(ret, ==, 0);\n> > +\n> > +\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n> \n> s/20/GIT_SHA1_RAWSZ\n\nIndeed.\n\n> Also here and elsewhere, shouldn't we just do\n> `check(reftable_ref_record_equal(...))` or even\n> `!check(reftable_iterator_seek_ref(...))` ?\n\nI guess you mean `check(!reftable_iteraror_seek_ref())`, right?\n\nIn the case where we just expect a zero error code I can certainly adapt\nthe code to use `check(...)`. But the other cases shouldn't use\n`check(!...)` because it is important that the returnd error code is\npositive.\n\nPatrick\n"},{"id":"502822","messageId":"ZufWkLwn0RQeCCpD@pks.im","threadId":"62084","inReplyTo":"CAOLa=ZQikyvJCRZ=mCfve+VWZfrvPL1bg55txB1q0Nh3SW_JJQ@mail.gmail.com","subject":"Re: [PATCH 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T06:56:16Z","receivedAt":"2024-09-16T06:56:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 13, 2024 at 07:47:06AM -0500, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> > +/*\n> > + * Handle exclude patterns. Returns either `1`, which tells the caller that the\n> > + * current reference shall not be shown. Or `0`, which indicates that it should\n> > + * be shown.\n> > + */\n> > +static int should_exclude_current_ref(struct reftable_ref_iterator *iter)\n> > +{\n> > +\twhile (iter->exclude_patterns[iter->exclude_patterns_index]) {\n> > +\t\tconst char *pattern = iter->exclude_patterns[iter->exclude_patterns_index];\n> > +\t\tchar *ref_after_pattern;\n> > +\t\tint cmp;\n> > +\n> > +\t\t/*\n> > +\t\t * Lazily cache the pattern length so that we don't have to\n> > +\t\t * recompute it every time this function is called.\n> > +\t\t */\n> > +\t\tif (!iter->exclude_patterns_strlen)\n> > +\t\t\titer->exclude_patterns_strlen = strlen(pattern);\n> > +\n> > +\t\t/*\n> > +\t\t * When the reference name is lexicographically bigger than the\n> > +\t\t * current exclude pattern we know that it won't ever match any\n> > +\t\t * of the following references, either. We thus advance to the\n> > +\t\t * next pattern and re-check whether it matches.\n> \n> So this means that the exclude patterns were lexicographically sorted.\n> Otherwise this would work.\n\nIndeed. Good that you call out my assumption, as I in fact didn't verify\nthat it holds, and in fact it doesn't. It's not a correctness issue if\nit doesn't hold, because it would simply mean that we don't skip over\nsome references where we really could. But it certainly is a perfomance\nissue.\n\nWill fix and add a test for it.\n\n> > +\t\t * Note that the seeked-to reference may also be excluded. This\n> > +\t\t * is not handled here though, but the caller is expected to\n> > +\t\t * loop and re-verify the next reference for us.\n> > +\t\t */\n> \n> The seeked-to reference here being the one with 0xff. We could get rid\n> of this by doing something like this:\n> \n> int last_char_idx = iter->exclude_patterns_strlen - 1\n> ref_after_pattern = xstrfmt(\"%s\", pattern);\n> ref_after_pattern[last_char_idx] = ref_after_pattern[last_char_idx] + 1;\n> \n> instead no?\n\nSorry, I don't quite follow what you mean with \"get rid of this\". What\nexactly is \"this\"? Do you mean the re-looping?\n\nIf so then the above doesn't fix it, no. We'd have to repeat a whole lot\nof code here to also retrieve the next entry, store it into `iter->ref`,\ncheck whether it is an actual ref starting with \"refs/\" and so on.\nLooping once very much feels like the better thing to do.\n\n> > @@ -580,9 +660,45 @@ static struct ref_iterator_vtable reftable_ref_iterator_vtable = {\n> > \t.abort = reftable_ref_iterator_abort\n> > };\n> >\n> > +static char **filter_exclude_patterns(const char **exclude_patterns)\n> > +{\n> > +\tsize_t filtered_size = 0, filtered_alloc = 0;\n> > +\tchar **filtered = NULL;\n> > +\n> > +\tif (!exclude_patterns)\n> > +\t\treturn NULL;\n> > +\n> > +\tfor (size_t i = 0; ; i++) {\n> > +\t\tconst char *exclude_pattern = exclude_patterns[i];\n> > +\t\tint has_glob = 0;\n> > +\n> > +\t\tif (!exclude_pattern)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tfor (const char *p = exclude_pattern; *p; p++) {\n> > +\t\t\thas_glob = is_glob_special(*p);\n> > +\t\t\tif (has_glob)\n> > +\t\t\t\tbreak;\n> > +\t\t}\n> \n> Why do we need to filter excludes here? Don't the callee's already do\n> something like this?\n\nNo, it doesn't. The code for exclude patterns is structured in such a\nway that the responsibility is with the backend to decide what it can\nand cannot filter. In theory there could be a backend that can exclude\nrefs based on globs efficiently, even though neither the \"files\" nor the\n\"reftable\" backend can.\n\nPatrick\n"},{"id":"502823","messageId":"ZufWlEViXqgNqBLh@pks.im","threadId":"62084","inReplyTo":"CAOLa=ZS3XBG4_=D5rCkntJuU0tzV7t=rB95rkNvWgm++Q+xukg@mail.gmail.com","subject":"Re: [PATCH 0/6] refs/reftable: wire up exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T06:56:20Z","receivedAt":"2024-09-16T06:56:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 13, 2024 at 07:48:02AM -0500, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Hi,\n> >\n> > this patch series wires up exclude patterns for the reftable backend.\n> > Exclude patterns allow the backend to skip references internally that\n> > match a specific pattern. This avoids having to read references that the\n> > caller would discard immediately anyway.\n> >\n> > The series is structured as follows:\n> >\n> > - Patches 1 and 2 fix two separate bugs in how we currently handle\n> > exclude patterns in combination with namespaces. We didn't happen to\n> > stumble over these bugs before because exclude patterns are only\n> > implemented for the \"packed\" backend. But once you start to pack\n> > refs we exclude references based on their full name instead of the\n> > name with the prefixed stripped. For the reftable backend we'd\n> > always hit those bugs because it always uses exclude patterns when\n> > passed.\n> >\n> > - Patches 3 to 5 wire up proper re-seeking of reftable iterators and\n> > adds some tests to demonstrate that this works as expected. This is\n> > a prerequisite for handling exclude patterns.\n> >\n> > - Patch 6 wires up exclude patterns in the reftable backend by\n> > re-seeking iterators once we hit an excluded reference.\n> >\n> > Thanks!\n> >\n> > Patrick\n> >\n> \n> This was a bit more intensive so I took my time with the review. Overall\n> I have some questions/comments. But the series looks good. Thanks!\n\nThanks for your review!\n\nPatrick\n"},{"id":"502836","messageId":"cover.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1725881266.git.ps@pks.im","subject":"[PATCH v2 0/6] refs/reftable: wire up exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:49:58Z","receivedAt":"2024-09-16T08:50:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that fixes preexisting\nbugs with exclude patterns and wires them up for the reftable backend.\n\nChanges compared to v1:\n\n - Some typo fixes in commit messages.\n\n - Mention that the newly introduced function in patch 1 will be used\n in patch 2, which is why it's declared in \"refs.h\".\n\n - Use `check()` instead of `check_int(ret, ==, 0)`.\n\n - Sort exclude patterns lexicographically. This fixes a bug where we\n might not correctly skip over some refs when patterns are passed to\n the reftable backend in non-lexicographic order. Add a test for\n this.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (6):\n refs: properly apply exclude patterns to namespaced refs\n builtin/receive-pack: fix exclude patterns when announcing refs\n Makefile: stop listing test library objects twice\n t/unit-tests: introduce reftable library\n reftable/reader: make table iterator reseekable\n refs/reftable: wire up support for exclude patterns\n\n Makefile | 8 +-\n builtin/receive-pack.c | 18 ++-\n refs.c | 35 +++++-\n refs.h | 9 ++\n refs/reftable-backend.c | 133 ++++++++++++++++++++++-\n reftable/reader.c | 1 +\n t/t1419-exclude-refs.sh | 49 +++++++--\n t/t5509-fetch-push-namespaces.sh | 9 ++\n t/unit-tests/lib-reftable.c | 93 ++++++++++++++++\n t/unit-tests/lib-reftable.h | 20 ++++\n t/unit-tests/t-reftable-merged.c | 163 +++++++++++++++-------------\n t/unit-tests/t-reftable-reader.c | 96 ++++++++++++++++\n t/unit-tests/t-reftable-readwrite.c | 130 +++++++---------------\n t/unit-tests/t-reftable-stack.c | 25 ++---\n trace2.h | 1 +\n trace2/tr2_ctr.c | 5 +\n 16 files changed, 594 insertions(+), 201 deletions(-)\n create mode 100644 t/unit-tests/lib-reftable.c\n create mode 100644 t/unit-tests/lib-reftable.h\n create mode 100644 t/unit-tests/t-reftable-reader.c\n\nRange-diff against v1:\n1: 8d347bc5599 ! 1: 7497166422e refs: properly apply exclude patterns to namespaced refs\n @@ Commit message\n to the non-stripped ones that still have the namespace prefix. In fact,\n the \"transfer.hideRefs\" machinery does the former and applies to the\n stripped reference by default, but rules can have \"^\" prefixed to switch\n - this behaviour to iinstead match against the rull reference name.\n + this behaviour to instead match against the full reference name.\n \n Namespaces are exclusively handled at the generic \"refs\" layer, the\n respective backends have no clue that such a thing even exists. This\n @@ Commit message\n refs in the tests, and then we indeed surface the breakage.\n \n Fix this bug by prefixing exclude patterns with the namespace in the\n - generic layer.\n + generic layer. The newly introduced function will be used outside of\n + \"refs.c\" in the next patch, so we add a declaration to \"refs.h\".\n \n Signed-off-by: Patrick Steinhardt \n \n2: 0317a5a7ede = 2: 3dc6ae936c8 builtin/receive-pack: fix exclude patterns when announcing refs\n3: 503c44e6cab = 3: 4ba503520e6 Makefile: stop listing test library objects twice\n4: 3df4040dd3c = 4: 6747076420f t/unit-tests: introduce reftable library\n5: a281f936a2b ! 5: 3278cdf92fe reftable/reader: make table iterator reseekable\n @@ t/unit-tests/t-reftable-reader.c (new)\n +\tblock_source_from_strbuf(&source, &buf);\n +\n +\tret = reftable_reader_new(&reader, &source, \"name\");\n -+\tcheck_int(ret, ==, 0);\n ++\tcheck(!ret);\n +\n +\treftable_reader_init_ref_iterator(reader, &it);\n +\tret = reftable_iterator_seek_ref(&it, \"\");\n -+\tcheck_int(ret, ==, 0);\n ++\tcheck(!ret);\n +\tret = reftable_iterator_next_ref(&it, &ref);\n -+\tcheck_int(ret, ==, 0);\n ++\tcheck(!ret);\n +\n -+\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n ++\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n +\tcheck_int(ret, ==, 1);\n +\n +\tret = reftable_iterator_next_ref(&it, &ref);\n @@ t/unit-tests/t-reftable-reader.c (new)\n +\tblock_source_from_strbuf(&source, &buf);\n +\n +\tret = reftable_reader_new(&reader, &source, \"name\");\n -+\tcheck_int(ret, ==, 0);\n ++\tcheck(!ret);\n +\n +\treftable_reader_init_ref_iterator(reader, &it);\n +\n +\tfor (size_t i = 0; i < 5; i++) {\n +\t\tret = reftable_iterator_seek_ref(&it, \"\");\n -+\t\tcheck_int(ret, ==, 0);\n ++\t\tcheck(!ret);\n +\t\tret = reftable_iterator_next_ref(&it, &ref);\n -+\t\tcheck_int(ret, ==, 0);\n ++\t\tcheck(!ret);\n +\n +\t\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n +\t\tcheck_int(ret, ==, 1);\n6: f3922b81db6 ! 6: 050f4906393 refs/reftable: wire up support for exclude patterns\n @@ refs/reftable-backend.c: static struct ref_iterator_vtable reftable_ref_iterator\n \t.abort = reftable_ref_iterator_abort\n };\n \n ++static int qsort_strcmp(const void *va, const void *vb)\n ++{\n ++\tconst char *a = *(const char **)va;\n ++\tconst char *b = *(const char **)vb;\n ++\treturn strcmp(a, b);\n ++}\n ++\n +static char **filter_exclude_patterns(const char **exclude_patterns)\n +{\n +\tsize_t filtered_size = 0, filtered_alloc = 0;\n @@ refs/reftable-backend.c: static struct ref_iterator_vtable reftable_ref_iterator\n +\t}\n +\n +\tif (filtered_size) {\n ++\t\tQSORT(filtered, filtered_size, qsort_strcmp);\n +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n +\t\tfiltered[filtered_size++] = NULL;\n +\t}\n @@ t/t1419-exclude-refs.sh: test_expect_success 'several overlapping excluded regio\n +\t\tassert_jumps 3 perf;;\n +\t*)\n +\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n ++\tesac\n ++'\n ++\n ++test_expect_success 'unordered excludes' '\n ++\tfor_each_ref__exclude refs/heads \\\n ++\t\trefs/heads/foo refs/heads/baz >actual 2>perf &&\n ++\tfor_each_ref refs/heads/bar refs/heads/quux >expect &&\n ++\n ++\ttest_cmp expect actual &&\n ++\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n ++\tfiles)\n ++\t\tassert_jumps 1 perf;;\n ++\treftable)\n ++\t\tassert_jumps 2 perf;;\n ++\t*)\n ++\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n +\tesac\n '\n \n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502837","messageId":"7497166422ea702aabdf4159b0d7780f1422ba13.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:03Z","receivedAt":"2024-09-16T08:50:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reference namespaces allow commands like git-upload-pack(1) to serve\ndifferent sets of references to the client depending on which namespace\nis enabled, which is for example useful in fork networks. Namespaced\nrefs are stored with a `refs/namespaces/$namespace` prefix, but all the\nuser will ultimately see is a stripped version where that prefix is\nremoved.\n\nThe way that this interacts with \"transfer.hideRefs\" is not immediately\nobvious: the hidden refs can either apply to the stripped references, or\nto the non-stripped ones that still have the namespace prefix. In fact,\nthe \"transfer.hideRefs\" machinery does the former and applies to the\nstripped reference by default, but rules can have \"^\" prefixed to switch\nthis behaviour to instead match against the full reference name.\n\nNamespaces are exclusively handled at the generic \"refs\" layer, the\nrespective backends have no clue that such a thing even exists. This\nalso has the consequence that they cannot handle hiding references as\nsoon as reference namespaces come into play because they neither know\nwhether a namespace is active, nor do they know how to strip references\nif they are active.\n\nHandling such exclude patterns in `refs_for_each_namespaced_ref()` and\n`refs_for_each_fullref_in_prefixes()` is broken though, as both support\nthat the user passes both namespaces and exclude patterns. In the case\nwhere both are set we will exclude references with unstripped names,\neven though we really wanted to exclude references based on their\nstripped names.\n\nThis only surfaces when:\n\n - A repository uses reference namespaces.\n\n - \"transfer.hideRefs\" is active.\n\n - The namespaced references are packed into the \"packed-refs\" file.\n\nNone of our tests exercise this scenario, and thus we haven't ever hit\nit. While t5509 exercises both (1) and (2), it does not happen to hit\n(3). It is trivial to demonstrate the bug though by explicitly packing\nrefs in the tests, and then we indeed surface the breakage.\n\nFix this bug by prefixing exclude patterns with the namespace in the\ngeneric layer. The newly introduced function will be used outside of\n\"refs.c\" in the next patch, so we add a declaration to \"refs.h\".\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 35 ++++++++++++++++++++++++++++----\n refs.h | 9 ++++++++\n t/t5509-fetch-push-namespaces.sh | 1 +\n 3 files changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ceb72d4bd74..b3a367ea12c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n \treturn hide_refs->v;\n }\n \n+const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n+\t\t\t\t\t const char *namespace,\n+\t\t\t\t\t struct strvec *out)\n+{\n+\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n+\t\treturn exclude_patterns;\n+\n+\tfor (size_t i = 0; exclude_patterns[i]; i++)\n+\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n+\n+\treturn out->v;\n+}\n+\n const char *find_descendant_ref(const char *dirname,\n \t\t\t\tconst struct string_list *extras,\n \t\t\t\tconst struct string_list *skip)\n@@ -1634,11 +1647,19 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \t\t\t\t const char **exclude_patterns,\n \t\t\t\t each_ref_fn fn, void *cb_data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n+\tstruct strbuf prefix = STRBUF_INIT;\n \tint ret;\n-\tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n-\tret = do_for_each_ref(refs, buf.buf, exclude_patterns, fn, 0, 0, cb_data);\n-\tstrbuf_release(&buf);\n+\n+\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n+\t\t\t\t\t\t\t get_git_namespace(),\n+\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n+\n+\tstrbuf_addf(&prefix, \"%srefs/\", get_git_namespace());\n+\tret = do_for_each_ref(refs, prefix.buf, exclude_patterns, fn, 0, 0, cb_data);\n+\n+\tstrvec_clear(&namespaced_exclude_patterns);\n+\tstrbuf_release(&prefix);\n \treturn ret;\n }\n \n@@ -1719,6 +1740,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\t\t\t const char **exclude_patterns,\n \t\t\t\t each_ref_fn fn, void *cb_data)\n {\n+\tstruct strvec namespaced_exclude_patterns = STRVEC_INIT;\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *prefix;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1730,6 +1752,10 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\tstrbuf_addstr(&buf, namespace);\n \tnamespace_len = buf.len;\n \n+\texclude_patterns = get_namespaced_exclude_patterns(exclude_patterns,\n+\t\t\t\t\t\t\t namespace,\n+\t\t\t\t\t\t\t &namespaced_exclude_patterns);\n+\n \tfor_each_string_list_item(prefix, &prefixes) {\n \t\tstrbuf_addstr(&buf, prefix->string);\n \t\tret = refs_for_each_fullref_in(ref_store, buf.buf,\n@@ -1739,6 +1765,7 @@ int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store,\n \t\tstrbuf_setlen(&buf, namespace_len);\n \t}\n \n+\tstrvec_clear(&namespaced_exclude_patterns);\n \tstring_list_clear(&prefixes, 0);\n \tstrbuf_release(&buf);\n \treturn ret;\ndiff --git a/refs.h b/refs.h\nindex f8b919a1388..3f774e96d18 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -859,6 +859,15 @@ int ref_is_hidden(const char *, const char *, const struct strvec *);\n */\n const char **hidden_refs_to_excludes(const struct strvec *hide_refs);\n \n+/*\n+ * Prefix all exclude patterns with the namespace, if any. This is required\n+ * because exclude patterns apply to the stripped reference name, not the full\n+ * reference name with the namespace.\n+ */\n+const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n+\t\t\t\t\t const char *namespace,\n+\t\t\t\t\t struct strvec *out);\n+\n /* Is this a per-worktree ref living in the refs/ namespace? */\n int is_per_worktree_ref(const char *refname);\n \ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 05090feaf92..98e8352b6cc 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -96,6 +96,7 @@ test_expect_success 'hide namespaced refs with transfer.hideRefs' '\n '\n \n test_expect_success 'check that transfer.hideRefs does not match unstripped refs' '\n+\tgit -C pushee pack-refs --all &&\n \tGIT_NAMESPACE=namespace \\\n \t\tgit -C pushee -c transfer.hideRefs=refs/namespaces/namespace/refs/tags \\\n \t\tls-remote \"ext::git %s .\" >actual &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502838","messageId":"3dc6ae936c88d3105bc82daab3edd805c9b5c63b.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 2/6] builtin/receive-pack: fix exclude patterns when announcing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:05Z","receivedAt":"2024-09-16T08:50:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `write_head_info()` we announce references to the remote client. We\nneed to honor \"transfer.hideRefs\" here so that we do not announce any\nreferences that the client shouldn't be able to learn about. This is\ndone via two separate mechanisms:\n\n - We hand over exclude patterns to the reference backend. We can only\n honor \"plain\" exclude patterns here that do not have prefixes with\n special meaning such as \"^\" or \"!\". Filtering down the references is\n handled by `hidden_refs_to_excludes()`.\n\n - In `show_ref_cb()` we perform a second check against hidden refs.\n For one this is done such that we can handle those special prefixes.\n And second, handling exclude patterns in ref backends is optional,\n so we also have to handle \"normal\" patterns.\n\nThe special-meaning \"^\" prefix alters whether a hidden ref applies to\nthe namespace-stripped reference name or the full name. So while we\nwould usually call `refs_for_each_namespaced_ref()` to only get those\nreferences in the current namespace, we can't because we'd get the\nalready-rewritten reference names. Instead, we are forced to use\n`refs_for_each_fullref_in()` and then manually strip away the namespace\nprefix such that we have access to both names.\n\nBut this also means that we do not get namespace handling for exclude\npatterns, which `refs_for_each_namespaced_ref()` brings for free. This\nresults in a bug because we potentially end up hiding away references\nbased on their namespaced name and not on the stripped name as we really\nshould be doing.\n\nFix this by manually rewriting the exclude patterns to their namespaced\nvariants.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/receive-pack.c | 18 ++++++++++++++++--\n t/t5509-fetch-push-namespaces.sh | 8 ++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 3f35140e489..478c62ca836 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -339,12 +339,26 @@ static void show_one_alternate_ref(const struct object_id *oid,\n static void write_head_info(void)\n {\n \tstatic struct oidset seen = OIDSET_INIT;\n+\tstruct strvec excludes_vector = STRVEC_INIT;\n+\tconst char **exclude_patterns;\n+\n+\t/*\n+\t * We need access to the reference names both with and without their\n+\t * namespace and thus cannot use `refs_for_each_namespaced_ref()`. We\n+\t * thus have to adapt exclude patterns to carry the namespace prefix\n+\t * ourselves.\n+\t */\n+\texclude_patterns = get_namespaced_exclude_patterns(\n+\t\thidden_refs_to_excludes(&hidden_refs),\n+\t\tget_git_namespace(), &excludes_vector);\n \n \trefs_for_each_fullref_in(get_main_ref_store(the_repository), \"\",\n-\t\t\t\t hidden_refs_to_excludes(&hidden_refs),\n-\t\t\t\t show_ref_cb, &seen);\n+\t\t\t\t exclude_patterns, show_ref_cb, &seen);\n \tfor_each_alternate_ref(show_one_alternate_ref, &seen);\n+\n \toidset_clear(&seen);\n+\tstrvec_clear(&excludes_vector);\n+\n \tif (!sent_capabilities)\n \t\tshow_ref(\"capabilities^{}\", null_oid());\n \ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 98e8352b6cc..f029ae0d286 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -124,6 +124,14 @@ test_expect_success 'try to update a ref that is not hidden' '\n \tgit -C original push pushee-namespaced main\n '\n \n+test_expect_success 'git-receive-pack(1) with transfer.hideRefs does not match unstripped refs during advertisement' '\n+\tgit -C pushee update-ref refs/namespaces/namespace/refs/heads/foo/1 refs/namespaces/namespace/refs/heads/main &&\n+\tgit -C pushee pack-refs --all &&\n+\ttest_config -C pushee transfer.hideRefs refs/namespaces/namespace/refs/heads/foo &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C original push pushee-namespaced main &&\n+\ttest_grep refs/heads/foo/1 trace\n+'\n+\n test_expect_success 'try to update a hidden full ref' '\n \ttest_config -C pushee transfer.hideRefs \"^refs/namespaces/namespace/refs/heads/main\" &&\n \ttest_must_fail git -C original push pushee-namespaced main\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502839","messageId":"4ba503520e6d306b7330fc3f3a4b4ee0748abf82.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 3/6] Makefile: stop listing test library objects twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:08Z","receivedAt":"2024-09-16T08:50:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever one adds another test library compilation unit one has to wire\nit up twice in the Makefile: once to append it to `UNIT_TEST_OBJS`, and\nonce to append it to the `UNIT_TEST_PROGS` target. Ideally, we'd just\nreuse the `UNIT_TEST_OBJS` variable in the target so that we can avoid\nthe duplication. But it also contains all the objects for our test\nprograms, each of which contains a `cmd_main()`, and thus we cannot link\nthem all into the target executable.\n\nRefactor the code such that `UNIT_TEST_OBJS` does not contain the unit\ntest program objects anymore, which we can instead manually append to\nthe `OBJECTS` variable. Like this, the former variable now only contains\nobjects for test libraries and can thus be reused.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex bdea061971a..4ed5f1f50a8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1356,7 +1356,6 @@ UNIT_TEST_PROGRAMS += t-strvec\n UNIT_TEST_PROGRAMS += t-trailer\n UNIT_TEST_PROGRAMS += t-urlmatch-normalization\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n-UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n \n@@ -2715,6 +2714,7 @@ OBJECTS += $(XDIFF_OBJS)\n OBJECTS += $(FUZZ_OBJS)\n OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)\n OBJECTS += $(UNIT_TEST_OBJS)\n+OBJECTS += $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n \n ifndef NO_CURL\n \tOBJECTS += http.o http-walker.o remote-curl.o\n@@ -3852,9 +3852,7 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n \t\t-Wl,--allow-multiple-definition \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)\n \n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \\\n-\t$(UNIT_TEST_DIR)/test-lib.o \\\n-\t$(UNIT_TEST_DIR)/lib-oid.o \\\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \\\n \t$(GITLIBS) GIT-LDFLAGS\n \t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502840","messageId":"6747076420fe01cf741d44400114431070c3c54a.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 4/6] t/unit-tests: introduce reftable library","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:11Z","receivedAt":"2024-09-16T08:50:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently migrated all of the reftable unit tests that were part\nof the reftable library into our own unit testing framework. As part of\nthat migration we have duplicated some of the functionality that was\npart of the reftable test framework into each of the migrated test\nsuites. This was a sensible decision to not have all of the migrations\ndependent on each other, but now that the migration is done it makes\nsense to deduplicate the functionality again.\n\nIntroduce a new reftable test library that hosts some shared code and\nadapt tests to use it.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 1 +\n t/unit-tests/lib-reftable.c | 93 ++++++++++++++++++++\n t/unit-tests/lib-reftable.h | 20 +++++\n t/unit-tests/t-reftable-merged.c | 87 +++----------------\n t/unit-tests/t-reftable-readwrite.c | 130 +++++++++-------------------\n t/unit-tests/t-reftable-stack.c | 25 +++---\n 6 files changed, 177 insertions(+), 179 deletions(-)\n create mode 100644 t/unit-tests/lib-reftable.c\n create mode 100644 t/unit-tests/lib-reftable.h\n\ndiff --git a/Makefile b/Makefile\nindex 4ed5f1f50a8..9460a80d0dd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1358,6 +1358,7 @@ UNIT_TEST_PROGRAMS += t-urlmatch-normalization\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-oid.o\n+UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o\n \n # xdiff and reftable libs may in turn depend on what is in libgit.a\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)\ndiff --git a/t/unit-tests/lib-reftable.c b/t/unit-tests/lib-reftable.c\nnew file mode 100644\nindex 00000000000..ab1fa44a282\n--- /dev/null\n+++ b/t/unit-tests/lib-reftable.c\n@@ -0,0 +1,93 @@\n+#include \"lib-reftable.h\"\n+#include \"test-lib.h\"\n+#include \"reftable/constants.h\"\n+#include \"reftable/writer.h\"\n+\n+void t_reftable_set_hash(uint8_t *p, int i, uint32_t id)\n+{\n+\tmemset(p, (uint8_t)i, hash_size(id));\n+}\n+\n+static ssize_t strbuf_writer_write(void *b, const void *data, size_t sz)\n+{\n+\tstrbuf_add(b, data, sz);\n+\treturn sz;\n+}\n+\n+static int strbuf_writer_flush(void *arg UNUSED)\n+{\n+\treturn 0;\n+}\n+\n+struct reftable_writer *t_reftable_strbuf_writer(struct strbuf *buf,\n+\t\t\t\t\t\t struct reftable_write_options *opts)\n+{\n+\treturn reftable_new_writer(&strbuf_writer_write,\n+\t\t\t\t &strbuf_writer_flush,\n+\t\t\t\t buf, opts);\n+}\n+\n+void t_reftable_write_to_buf(struct strbuf *buf,\n+\t\t\t struct reftable_ref_record *refs,\n+\t\t\t size_t nrefs,\n+\t\t\t struct reftable_log_record *logs,\n+\t\t\t size_t nlogs,\n+\t\t\t struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tconst struct reftable_stats *stats;\n+\tstruct reftable_writer *writer;\n+\tuint64_t min = 0xffffffff;\n+\tuint64_t max = 0;\n+\tint ret;\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\n+\tfor (size_t i = 0; i < nrefs; i++) {\n+\t\tuint64_t ui = refs[i].update_index;\n+\t\tif (ui > max)\n+\t\t\tmax = ui;\n+\t\tif (ui < min)\n+\t\t\tmin = ui;\n+\t}\n+\tfor (size_t i = 0; i < nlogs; i++) {\n+\t\tuint64_t ui = logs[i].update_index;\n+\t\tif (ui > max)\n+\t\t\tmax = ui;\n+\t\tif (ui < min)\n+\t\t\tmin = ui;\n+\t}\n+\n+\twriter = t_reftable_strbuf_writer(buf, &opts);\n+\treftable_writer_set_limits(writer, min, max);\n+\n+\tif (nrefs) {\n+\t\tret = reftable_writer_add_refs(writer, refs, nrefs);\n+\t\tcheck_int(ret, ==, 0);\n+\t}\n+\n+\tif (nlogs) {\n+\t\tret = reftable_writer_add_logs(writer, logs, nlogs);\n+\t\tcheck_int(ret, ==, 0);\n+\t}\n+\n+\tret = reftable_writer_close(writer);\n+\tcheck_int(ret, ==, 0);\n+\n+\tstats = reftable_writer_stats(writer);\n+\tfor (size_t i = 0; i < stats->ref_stats.blocks; i++) {\n+\t\tsize_t off = i * (opts.block_size ? opts.block_size\n+\t\t\t\t\t\t : DEFAULT_BLOCK_SIZE);\n+\t\tif (!off)\n+\t\t\toff = header_size(opts.hash_id == GIT_SHA256_FORMAT_ID ? 2 : 1);\n+\t\tcheck_char(buf->buf[off], ==, 'r');\n+\t}\n+\n+\tif (nrefs)\n+\t\tcheck_int(stats->ref_stats.blocks, >, 0);\n+\tif (nlogs)\n+\t\tcheck_int(stats->log_stats.blocks, >, 0);\n+\n+\treftable_writer_free(writer);\n+}\ndiff --git a/t/unit-tests/lib-reftable.h b/t/unit-tests/lib-reftable.h\nnew file mode 100644\nindex 00000000000..d1154190847\n--- /dev/null\n+++ b/t/unit-tests/lib-reftable.h\n@@ -0,0 +1,20 @@\n+#ifndef LIB_REFTABLE_H\n+#define LIB_REFTABLE_H\n+\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"reftable/reftable-writer.h\"\n+\n+void t_reftable_set_hash(uint8_t *p, int i, uint32_t id);\n+\n+struct reftable_writer *t_reftable_strbuf_writer(struct strbuf *buf,\n+\t\t\t\t\t\t struct reftable_write_options *opts);\n+\n+void t_reftable_write_to_buf(struct strbuf *buf,\n+\t\t\t struct reftable_ref_record *refs,\n+\t\t\t size_t nrecords,\n+\t\t\t struct reftable_log_record *logs,\n+\t\t\t size_t nlogs,\n+\t\t\t struct reftable_write_options *opts);\n+\n+#endif\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex e9d100a01ea..b8c92fdb365 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/blocksource.h\"\n #include \"reftable/constants.h\"\n #include \"reftable/merged.h\"\n@@ -15,77 +16,6 @@ license that can be found in the LICENSE file or at\n #include \"reftable/reftable-merged.h\"\n #include \"reftable/reftable-writer.h\"\n \n-static ssize_t strbuf_add_void(void *b, const void *data, const size_t sz)\n-{\n-\tstrbuf_add(b, data, sz);\n-\treturn sz;\n-}\n-\n-static int noop_flush(void *arg UNUSED)\n-{\n-\treturn 0;\n-}\n-\n-static void write_test_table(struct strbuf *buf,\n-\t\t\t struct reftable_ref_record refs[], const size_t n)\n-{\n-\tuint64_t min = 0xffffffff;\n-\tuint64_t max = 0;\n-\tsize_t i;\n-\tint err;\n-\n-\tstruct reftable_write_options opts = {\n-\t\t.block_size = 256,\n-\t};\n-\tstruct reftable_writer *w = NULL;\n-\tfor (i = 0; i < n; i++) {\n-\t\tuint64_t ui = refs[i].update_index;\n-\t\tif (ui > max)\n-\t\t\tmax = ui;\n-\t\tif (ui < min)\n-\t\t\tmin = ui;\n-\t}\n-\n-\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\treftable_writer_set_limits(w, min, max);\n-\n-\tfor (i = 0; i < n; i++) {\n-\t\tuint64_t before = refs[i].update_index;\n-\t\tint n = reftable_writer_add_ref(w, &refs[i]);\n-\t\tcheck_int(n, ==, 0);\n-\t\tcheck_int(before, ==, refs[i].update_index);\n-\t}\n-\n-\terr = reftable_writer_close(w);\n-\tcheck(!err);\n-\n-\treftable_writer_free(w);\n-}\n-\n-static void write_test_log_table(struct strbuf *buf, struct reftable_log_record logs[],\n-\t\t\t\t const size_t n, const uint64_t update_index)\n-{\n-\tint err;\n-\n-\tstruct reftable_write_options opts = {\n-\t\t.block_size = 256,\n-\t\t.exact_log_message = 1,\n-\t};\n-\tstruct reftable_writer *w = NULL;\n-\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\treftable_writer_set_limits(w, update_index, update_index);\n-\n-\tfor (size_t i = 0; i < n; i++) {\n-\t\tint err = reftable_writer_add_log(w, &logs[i]);\n-\t\tcheck(!err);\n-\t}\n-\n-\terr = reftable_writer_close(w);\n-\tcheck(!err);\n-\n-\treftable_writer_free(w);\n-}\n-\n static struct reftable_merged_table *\n merged_table_from_records(struct reftable_ref_record **refs,\n \t\t\t struct reftable_block_source **source,\n@@ -93,13 +23,16 @@ 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_write_options opts = {\n+\t\t.block_size = 256,\n+\t};\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n \tfor (size_t i = 0; i < n; i++) {\n-\t\twrite_test_table(&buf[i], refs[i], sizes[i]);\n+\t\tt_reftable_write_to_buf(&buf[i], refs[i], sizes[i], NULL, 0, &opts);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n \t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n@@ -268,13 +201,17 @@ 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_write_options opts = {\n+\t\t.block_size = 256,\n+\t\t.exact_log_message = 1,\n+\t};\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*readers, n);\n \tREFTABLE_CALLOC_ARRAY(*source, n);\n \n \tfor (size_t i = 0; i < n; i++) {\n-\t\twrite_test_log_table(&buf[i], logs[i], sizes[i], i + 1);\n+\t\tt_reftable_write_to_buf(&buf[i], NULL, 0, logs[i], sizes[i], &opts);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n \t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n@@ -402,9 +339,7 @@ static void t_default_write_opts(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record rec = {\n \t\t.refname = (char *) \"master\",\n \t\t.update_index = 1,\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 82bfaf32874..e1b235a5f13 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/basics.h\"\n #include \"reftable/blocksource.h\"\n #include \"reftable/reader.h\"\n@@ -15,22 +16,6 @@ license that can be found in the LICENSE file or at\n \n static const int update_index = 5;\n \n-static void set_test_hash(uint8_t *p, int i)\n-{\n-\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n-}\n-\n-static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n-{\n-\tstrbuf_add(b, data, sz);\n-\treturn sz;\n-}\n-\n-static int noop_flush(void *arg UNUSED)\n-{\n-\treturn 0;\n-}\n-\n static void t_buffer(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -62,61 +47,34 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t.block_size = block_size,\n \t\t.hash_id = hash_id,\n \t};\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n-\tstruct reftable_ref_record ref = { 0 };\n-\tint i = 0, n;\n-\tstruct reftable_log_record log = { 0 };\n-\tconst struct reftable_stats *stats = NULL;\n+\tstruct reftable_ref_record *refs;\n+\tstruct reftable_log_record *logs;\n+\tint i;\n \n \tREFTABLE_CALLOC_ARRAY(*names, N + 1);\n+\tREFTABLE_CALLOC_ARRAY(refs, N);\n+\tREFTABLE_CALLOC_ARRAY(logs, N);\n \n-\treftable_writer_set_limits(w, update_index, update_index);\n \tfor (i = 0; i < N; i++) {\n-\t\tchar name[100];\n-\t\tint n;\n-\n-\t\tsnprintf(name, sizeof(name), \"refs/heads/branch%02d\", i);\n-\n-\t\tref.refname = name;\n-\t\tref.update_index = update_index;\n-\t\tref.value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(ref.value.val1, i);\n-\t\t(*names)[i] = xstrdup(name);\n-\n-\t\tn = reftable_writer_add_ref(w, &ref);\n-\t\tcheck_int(n, ==, 0);\n+\t\trefs[i].refname = (*names)[i] = xstrfmt(\"refs/heads/branch%02d\", i);\n+\t\trefs[i].update_index = update_index;\n+\t\trefs[i].value_type = REFTABLE_REF_VAL1;\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tchar name[100];\n-\t\tint n;\n-\n-\t\tsnprintf(name, sizeof(name), \"refs/heads/branch%02d\", i);\n-\n-\t\tlog.refname = name;\n-\t\tlog.update_index = update_index;\n-\t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tset_test_hash(log.value.update.new_hash, i);\n-\t\tlog.value.update.message = (char *) \"message\";\n-\n-\t\tn = reftable_writer_add_log(w, &log);\n-\t\tcheck_int(n, ==, 0);\n+\t\tlogs[i].refname = (*names)[i];\n+\t\tlogs[i].update_index = update_index;\n+\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tlogs[i].value.update.message = (char *) \"message\";\n \t}\n \n-\tn = reftable_writer_close(w);\n-\tcheck_int(n, ==, 0);\n-\n-\tstats = reftable_writer_stats(w);\n-\tfor (i = 0; i < stats->ref_stats.blocks; i++) {\n-\t\tint off = i * opts.block_size;\n-\t\tif (!off)\n-\t\t\toff = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);\n-\t\tcheck_char(buf->buf[off], ==, 'r');\n-\t}\n+\tt_reftable_write_to_buf(buf, refs, N, logs, N, &opts);\n \n-\tcheck_int(stats->log_stats.blocks, >, 0);\n-\treftable_writer_free(w);\n+\tfree(refs);\n+\tfree(logs);\n }\n \n static void t_log_buffer_size(void)\n@@ -138,8 +96,7 @@ static void t_log_buffer_size(void)\n \t\t\t\t\t .time = 0x5e430672,\n \t\t\t\t\t .message = (char *) \"commit: 9\\n\",\n \t\t\t\t } } };\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \n \t/* This tests buffer extension for log compression. Must use a random\n \t hash, to ensure that the compressed part is larger than the original.\n@@ -181,8 +138,7 @@ static void t_log_overflow(void)\n \t\t\t},\n \t\t},\n \t};\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \n \tmemset(msg, 'x', sizeof(msg) - 1);\n \treftable_writer_set_limits(w, update_index, update_index);\n@@ -208,8 +164,7 @@ static void t_log_write_read(void)\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n@@ -229,8 +184,10 @@ static void t_log_write_read(void)\n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n \t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tset_test_hash(log.value.update.old_hash, i);\n-\t\tset_test_hash(log.value.update.new_hash, i + 1);\n+\t\tt_reftable_set_hash(log.value.update.old_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tt_reftable_set_hash(log.value.update.new_hash, i + 1,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \n \t\terr = reftable_writer_add_log(w, &log);\n \t\tcheck(!err);\n@@ -297,8 +254,7 @@ static void t_log_zlib_corruption(void)\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \tchar message[100] = { 0 };\n \tint err, i, n;\n@@ -528,15 +484,12 @@ static void t_table_refs_for(int indexed)\n \tint err;\n \tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n-\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_iterator it = { 0 };\n \tint j;\n \n-\tset_test_hash(want_hash, 4);\n+\tt_reftable_set_hash(want_hash, 4, GIT_SHA1_FORMAT_ID);\n \n \tfor (i = 0; i < N; i++) {\n \t\tuint8_t hash[GIT_SHA1_RAWSZ];\n@@ -552,8 +505,10 @@ static void t_table_refs_for(int indexed)\n \t\tref.refname = name;\n \n \t\tref.value_type = REFTABLE_REF_VAL2;\n-\t\tset_test_hash(ref.value.val2.value, i / 4);\n-\t\tset_test_hash(ref.value.val2.target_value, 3 + i / 4);\n+\t\tt_reftable_set_hash(ref.value.val2.value, i / 4,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n+\t\tt_reftable_set_hash(ref.value.val2.target_value, 3 + i / 4,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \n \t\t/* 80 bytes / entry, so 3 entries per block. Yields 17\n \t\t */\n@@ -618,8 +573,7 @@ static void t_write_empty_table(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_block_source source = { 0 };\n \tstruct reftable_reader *rd = NULL;\n \tstruct reftable_ref_record rec = { 0 };\n@@ -657,8 +611,7 @@ static void t_write_object_id_min_length(void)\n \t\t.block_size = 75,\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -692,8 +645,7 @@ static void t_write_object_id_length(void)\n \t\t.block_size = 75,\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -726,8 +678,7 @@ static void t_write_empty_key(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.refname = (char *) \"\",\n \t\t.update_index = 1,\n@@ -749,8 +700,7 @@ static void t_write_key_order(void)\n {\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n \tstruct reftable_ref_record refs[2] = {\n \t\t{\n \t\t\t.refname = (char *) \"b\",\n@@ -798,7 +748,7 @@ static void t_write_multiple_indices(void)\n \tstruct reftable_reader *reader;\n \tint err, i;\n \n-\twriter = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);\n+\twriter = t_reftable_strbuf_writer(&writer_buf, &opts);\n \treftable_writer_set_limits(writer, 1, 1);\n \tfor (i = 0; i < 100; i++) {\n \t\tstruct reftable_ref_record ref = {\n@@ -876,7 +826,7 @@ static void t_write_multi_level_index(void)\n \tstruct reftable_reader *reader;\n \tint err;\n \n-\twriter = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);\n+\twriter = t_reftable_strbuf_writer(&writer_buf, &opts);\n \treftable_writer_set_limits(writer, 1, 1);\n \tfor (size_t i = 0; i < 200; i++) {\n \t\tstruct reftable_ref_record ref = {\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..65e513d5ec8 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -7,17 +7,13 @@ license that can be found in the LICENSE file or at\n */\n \n #include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n #include \"reftable/merged.h\"\n #include \"reftable/reader.h\"\n #include \"reftable/reftable-error.h\"\n #include \"reftable/stack.h\"\n #include \n \n-static void set_test_hash(uint8_t *p, int i)\n-{\n-\tmemset(p, (uint8_t)i, hash_size(GIT_SHA1_FORMAT_ID));\n-}\n-\n static void clear_dir(const char *dirname)\n {\n \tstruct strbuf path = STRBUF_INIT;\n@@ -125,7 +121,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"refs/heads/branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf.buf;\n-\t\tset_test_hash(ref.value.val1, i);\n+\t\tt_reftable_set_hash(ref.value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n \t\tcheck(!err);\n@@ -470,13 +466,13 @@ static void t_reftable_stack_add(void)\n \t\trefs[i].refname = xstrdup(buf);\n \t\trefs[i].update_index = i + 1;\n \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(refs[i].value.val1, i);\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\tlogs[i].refname = xstrdup(buf);\n \t\tlogs[i].update_index = N + i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -562,14 +558,14 @@ static void t_reftable_stack_iterator(void)\n \t\trefs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\trefs[i].update_index = i + 1;\n \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\tset_test_hash(refs[i].value.val1, i);\n+\t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \n \t\tlogs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\tlogs[i].update_index = i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.email = xstrdup(\"johndoe@invalid\");\n \t\tlogs[i].value.update.message = xstrdup(\"commit\\n\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i, GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 0; i < N; i++) {\n@@ -704,7 +700,8 @@ static void t_reftable_stack_tombstone(void)\n \t\trefs[i].update_index = i + 1;\n \t\tif (i % 2 == 0) {\n \t\t\trefs[i].value_type = REFTABLE_REF_VAL1;\n-\t\t\tset_test_hash(refs[i].value.val1, i);\n+\t\t\tt_reftable_set_hash(refs[i].value.val1, i,\n+\t\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t\t}\n \n \t\tlogs[i].refname = xstrdup(buf);\n@@ -712,7 +709,8 @@ static void t_reftable_stack_tombstone(void)\n \t\tlogs[i].update_index = 42;\n \t\tif (i % 2 == 0) {\n \t\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n-\t\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t\t\tlogs[i].value.update.email =\n \t\t\t\txstrdup(\"identity@invalid\");\n \t\t}\n@@ -844,7 +842,8 @@ static void t_reflog_expire(void)\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.time = i;\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n-\t\tset_test_hash(logs[i].value.update.new_hash, i);\n+\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n+\t\t\t\t GIT_SHA1_FORMAT_ID);\n \t}\n \n \tfor (i = 1; i <= N; i++) {\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502841","messageId":"3278cdf92fe7632b3798a732ad68044ab1d4adc2.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 5/6] reftable/reader: make table iterator reseekable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:14Z","receivedAt":"2024-09-16T08:50:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 67ce50ba26 (Merge branch 'ps/reftable-reusable-iterator', 2024-05-30)\nwe have refactored the interface of reftable iterators such that they\ncan be reused in theory. This patch series only landed the required\nchanges on the interface level, but didn't yet implement the actual\nlogic to make iterators reusable.\n\nAs it turns out almost all of the infrastructure already does support\nre-seeking. The only exception is the table iterator, which does not\nreset its `is_finished` bit. Do so and add a couple of tests that verify\nthat we can re-seek iterators.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 1 +\n reftable/reader.c | 1 +\n t/unit-tests/t-reftable-merged.c | 76 +++++++++++++++++++++++++\n t/unit-tests/t-reftable-reader.c | 96 ++++++++++++++++++++++++++++++++\n 4 files changed, 174 insertions(+)\n create mode 100644 t/unit-tests/t-reftable-reader.c\n\ndiff --git a/Makefile b/Makefile\nindex 9460a80d0dd..4039e355b09 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1346,6 +1346,7 @@ UNIT_TEST_PROGRAMS += t-reftable-basics\n UNIT_TEST_PROGRAMS += t-reftable-block\n UNIT_TEST_PROGRAMS += t-reftable-merged\n UNIT_TEST_PROGRAMS += t-reftable-pq\n+UNIT_TEST_PROGRAMS += t-reftable-reader\n UNIT_TEST_PROGRAMS += t-reftable-readwrite\n UNIT_TEST_PROGRAMS += t-reftable-record\n UNIT_TEST_PROGRAMS += t-reftable-stack\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex f8770990876..6494ce2e327 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -328,6 +328,7 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)\n \tti->typ = block_reader_type(&ti->br);\n \tti->block_off = off;\n \tblock_iter_seek_start(&ti->bi, &ti->br);\n+\tti->is_finished = 0;\n \treturn 0;\n }\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex b8c92fdb365..19e54bdfb8b 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -194,6 +194,81 @@ static void t_merged_refs(void)\n \treftable_free(bs);\n }\n \n+static void t_merged_seek_multiple_times(void)\n+{\n+\tstruct reftable_ref_record r1[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 1 },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"c\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 2 },\n+\t\t}\n+\t};\n+\tstruct reftable_ref_record r2[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 3 },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"d\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 4 },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record *refs[] = {\n+\t\tr1, r2,\n+\t};\n+\tsize_t sizes[] = {\n+\t\tARRAY_SIZE(r1), ARRAY_SIZE(r2),\n+\t};\n+\tstruct strbuf bufs[] = {\n+\t\tSTRBUF_INIT, STRBUF_INIT,\n+\t};\n+\tstruct reftable_block_source *sources = NULL;\n+\tstruct reftable_reader **readers = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_merged_table *mt;\n+\n+\tmt = merged_table_from_records(refs, &sources, &readers, sizes, bufs, 2);\n+\tmerged_table_init_iter(mt, &it, BLOCK_TYPE_REF);\n+\n+\tfor (size_t i = 0; i < 5; i++) {\n+\t\tint err = reftable_iterator_seek_ref(&it, \"c\");\n+\t\tcheck(!err);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(!err);\n+\t\terr = reftable_ref_record_equal(&rec, &r1[1], GIT_SHA1_RAWSZ);\n+\t\tcheck(err == 1);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(!err);\n+\t\terr = reftable_ref_record_equal(&rec, &r2[1], GIT_SHA1_RAWSZ);\n+\t\tcheck(err == 1);\n+\n+\t\terr = reftable_iterator_next_ref(&it, &rec);\n+\t\tcheck(err > 0);\n+\t}\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(bufs); i++)\n+\t\tstrbuf_release(&bufs[i]);\n+\treaders_destroy(readers, ARRAY_SIZE(refs));\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_merged_table_free(mt);\n+\treftable_free(sources);\n+}\n+\n static struct reftable_merged_table *\n merged_table_from_log_records(struct reftable_log_record **logs,\n \t\t\t struct reftable_block_source **source,\n@@ -383,6 +458,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_default_write_opts(), \"merged table with default write opts\");\n \tTEST(t_merged_logs(), \"merged table with multiple log updates for same ref\");\n \tTEST(t_merged_refs(), \"merged table with multiple updates to same ref\");\n+\tTEST(t_merged_seek_multiple_times(), \"merged table can seek multiple times\");\n \tTEST(t_merged_single_record(), \"ref ocurring in only one record can be fetched\");\n \n \treturn test_done();\ndiff --git a/t/unit-tests/t-reftable-reader.c b/t/unit-tests/t-reftable-reader.c\nnew file mode 100644\nindex 00000000000..eea86966c0d\n--- /dev/null\n+++ b/t/unit-tests/t-reftable-reader.c\n@@ -0,0 +1,96 @@\n+#include \"test-lib.h\"\n+#include \"lib-reftable.h\"\n+#include \"reftable/blocksource.h\"\n+#include \"reftable/reader.h\"\n+\n+static int t_reader_seek_once(void)\n+{\n+\tstruct reftable_ref_record records[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/main\",\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 42 },\n+\t\t},\n+\t};\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_ref_record ref = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader *reader;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret;\n+\n+\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n+\tblock_source_from_strbuf(&source, &buf);\n+\n+\tret = reftable_reader_new(&reader, &source, \"name\");\n+\tcheck(!ret);\n+\n+\treftable_reader_init_ref_iterator(reader, &it);\n+\tret = reftable_iterator_seek_ref(&it, \"\");\n+\tcheck(!ret);\n+\tret = reftable_iterator_next_ref(&it, &ref);\n+\tcheck(!ret);\n+\n+\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n+\tcheck_int(ret, ==, 1);\n+\n+\tret = reftable_iterator_next_ref(&it, &ref);\n+\tcheck_int(ret, ==, 1);\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_iterator_destroy(&it);\n+\treftable_reader_decref(reader);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+static int t_reader_reseek(void)\n+{\n+\tstruct reftable_ref_record records[] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/main\",\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { 42 },\n+\t\t},\n+\t};\n+\tstruct reftable_block_source source = { 0 };\n+\tstruct reftable_ref_record ref = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct reftable_reader *reader;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret;\n+\n+\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n+\tblock_source_from_strbuf(&source, &buf);\n+\n+\tret = reftable_reader_new(&reader, &source, \"name\");\n+\tcheck(!ret);\n+\n+\treftable_reader_init_ref_iterator(reader, &it);\n+\n+\tfor (size_t i = 0; i < 5; i++) {\n+\t\tret = reftable_iterator_seek_ref(&it, \"\");\n+\t\tcheck(!ret);\n+\t\tret = reftable_iterator_next_ref(&it, &ref);\n+\t\tcheck(!ret);\n+\n+\t\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n+\t\tcheck_int(ret, ==, 1);\n+\n+\t\tret = reftable_iterator_next_ref(&it, &ref);\n+\t\tcheck_int(ret, ==, 1);\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_iterator_destroy(&it);\n+\treftable_reader_decref(reader);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n+{\n+\tTEST(t_reader_seek_once(), \"reader can seek once\");\n+\tTEST(t_reader_reseek(), \"reader can reseek multiple times\");\n+\treturn test_done();\n+}\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502842","messageId":"050f4906393d1f9c58a6b6bc695b004695d219be.1726476401.git.ps@pks.im","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"[PATCH v2 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T08:50:16Z","receivedAt":"2024-09-16T08:50:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Exclude patterns can be used by reference backends to skip over blocks\nof references that are uninteresting to the caller. Reference backends\ndo not have to wire up support for them, and all callers are expected to\nbehave as if the backend didn't support them. In fact, the only backend\nthat supports exclude patterns right now is the \"packed\" backend.\n\nExclude patterns can be quite an important performance optimization in\nrepositories that have loads of references. The patterns are set up in\ncase \"transfer.hideRefs\" and friends are configured during a fetch, so\nhandling these patterns becomes important once there are lots of hidden\nrefs in a served repository.\n\nNow that we have properly re-seekable reftable iterators we can also\nwire up support for these patterns in the \"reftable\" backend. Doing so\nis conceptually simple: once we hit a reference whose prefix matches the\ncurrent exclude pattern we re-seek the iterator to the first reference\nthat doesn't match the pattern anymore. This schema only works for\ntrivial patterns that do not have any globbing characters in them, but\nthis restriction also applies do the \"packed\" backend.\n\nThis makes t1419 work with the \"reftable\" backend with some slight\nmodifications. Of course it also speeds up listing of references with\nhidden refs. The following benchmark prints one reference with 1 million\nhidden references:\n\n Benchmark 1: HEAD~\n Time (mean ± σ): 93.3 ms ± 2.1 ms [User: 90.3 ms, System: 2.5 ms]\n Range (min … max): 89.8 ms … 97.2 ms 33 runs\n\n Benchmark 2: HEAD\n Time (mean ± σ): 4.2 ms ± 0.6 ms [User: 2.2 ms, System: 1.8 ms]\n Range (min … max): 3.1 ms … 8.1 ms 765 runs\n\n Summary\n HEAD ran\n 22.15 ± 3.19 times faster than HEAD~\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 133 +++++++++++++++++++++++++++++++++++++++-\n t/t1419-exclude-refs.sh | 49 ++++++++++++---\n trace2.h | 1 +\n trace2/tr2_ctr.c | 5 ++\n 4 files changed, 176 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..3e63833ce41 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -21,6 +21,7 @@\n #include \"../reftable/reftable-iterator.h\"\n #include \"../setup.h\"\n #include \"../strmap.h\"\n+#include \"../trace2.h\"\n #include \"parse.h\"\n #include \"refs-internal.h\"\n \n@@ -447,10 +448,81 @@ struct reftable_ref_iterator {\n \n \tconst char *prefix;\n \tsize_t prefix_len;\n+\tchar **exclude_patterns;\n+\tsize_t exclude_patterns_index;\n+\tsize_t exclude_patterns_strlen;\n \tunsigned int flags;\n \tint err;\n };\n \n+/*\n+ * Handle exclude patterns. Returns either `1`, which tells the caller that the\n+ * current reference shall not be shown. Or `0`, which indicates that it should\n+ * be shown.\n+ */\n+static int should_exclude_current_ref(struct reftable_ref_iterator *iter)\n+{\n+\twhile (iter->exclude_patterns[iter->exclude_patterns_index]) {\n+\t\tconst char *pattern = iter->exclude_patterns[iter->exclude_patterns_index];\n+\t\tchar *ref_after_pattern;\n+\t\tint cmp;\n+\n+\t\t/*\n+\t\t * Lazily cache the pattern length so that we don't have to\n+\t\t * recompute it every time this function is called.\n+\t\t */\n+\t\tif (!iter->exclude_patterns_strlen)\n+\t\t\titer->exclude_patterns_strlen = strlen(pattern);\n+\n+\t\t/*\n+\t\t * When the reference name is lexicographically bigger than the\n+\t\t * current exclude pattern we know that it won't ever match any\n+\t\t * of the following references, either. We thus advance to the\n+\t\t * next pattern and re-check whether it matches.\n+\t\t *\n+\t\t * Otherwise, if it's smaller, then we do not have a match and\n+\t\t * thus want to show the current reference.\n+\t\t */\n+\t\tcmp = strncmp(iter->ref.refname, pattern,\n+\t\t\t iter->exclude_patterns_strlen);\n+\t\tif (cmp > 0) {\n+\t\t\titer->exclude_patterns_index++;\n+\t\t\titer->exclude_patterns_strlen = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (cmp < 0)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * The reference shares a prefix with the exclude pattern and\n+\t\t * shall thus be omitted. We skip all references that match the\n+\t\t * pattern by seeking to the first reference after the block of\n+\t\t * matches.\n+\t\t *\n+\t\t * This is done by appending the highest possible character to\n+\t\t * the pattern. Consequently, all references that have the\n+\t\t * pattern as prefix and whose suffix starts with anything in\n+\t\t * the range [0x00, 0xfe] are skipped. And given that 0xff is a\n+\t\t * non-printable character that shouldn't ever be in a ref name,\n+\t\t * we'd not yield any such record, either.\n+\t\t *\n+\t\t * Note that the seeked-to reference may also be excluded. This\n+\t\t * is not handled here though, but the caller is expected to\n+\t\t * loop and re-verify the next reference for us.\n+\t\t */\n+\t\tref_after_pattern = xstrfmt(\"%s%c\", pattern, 0xff);\n+\t\titer->err = reftable_iterator_seek_ref(&iter->iter, ref_after_pattern);\n+\t\titer->exclude_patterns_index++;\n+\t\titer->exclude_patterns_strlen = 0;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_REFTABLE_RESEEKS, 1);\n+\n+\t\tfree(ref_after_pattern);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n {\n \tstruct reftable_ref_iterator *iter =\n@@ -481,6 +553,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\tbreak;\n \t\t}\n \n+\t\tif (iter->exclude_patterns && should_exclude_current_ref(iter))\n+\t\t\tcontinue;\n+\n \t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n \t\t parse_worktree_ref(iter->ref.refname, NULL, NULL, NULL) !=\n \t\t\t REF_WORKTREE_CURRENT)\n@@ -570,6 +645,11 @@ static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_ref_iterator *)ref_iterator;\n \treftable_ref_record_release(&iter->ref);\n \treftable_iterator_destroy(&iter->iter);\n+\tif (iter->exclude_patterns) {\n+\t\tfor (size_t i = 0; iter->exclude_patterns[i]; i++)\n+\t\t\tfree(iter->exclude_patterns[i]);\n+\t\tfree(iter->exclude_patterns);\n+\t}\n \tfree(iter);\n \treturn ITER_DONE;\n }\n@@ -580,9 +660,53 @@ static struct ref_iterator_vtable reftable_ref_iterator_vtable = {\n \t.abort = reftable_ref_iterator_abort\n };\n \n+static int qsort_strcmp(const void *va, const void *vb)\n+{\n+\tconst char *a = *(const char **)va;\n+\tconst char *b = *(const char **)vb;\n+\treturn strcmp(a, b);\n+}\n+\n+static char **filter_exclude_patterns(const char **exclude_patterns)\n+{\n+\tsize_t filtered_size = 0, filtered_alloc = 0;\n+\tchar **filtered = NULL;\n+\n+\tif (!exclude_patterns)\n+\t\treturn NULL;\n+\n+\tfor (size_t i = 0; ; i++) {\n+\t\tconst char *exclude_pattern = exclude_patterns[i];\n+\t\tint has_glob = 0;\n+\n+\t\tif (!exclude_pattern)\n+\t\t\tbreak;\n+\n+\t\tfor (const char *p = exclude_pattern; *p; p++) {\n+\t\t\thas_glob = is_glob_special(*p);\n+\t\t\tif (has_glob)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (has_glob)\n+\t\t\tcontinue;\n+\n+\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n+\t\tfiltered[filtered_size++] = xstrdup(exclude_pattern);\n+\t}\n+\n+\tif (filtered_size) {\n+\t\tQSORT(filtered, filtered_size, qsort_strcmp);\n+\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n+\t\tfiltered[filtered_size++] = NULL;\n+\t}\n+\n+\treturn filtered;\n+}\n+\n static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_store *refs,\n \t\t\t\t\t\t\t struct reftable_stack *stack,\n \t\t\t\t\t\t\t const char *prefix,\n+\t\t\t\t\t\t\t const char **exclude_patterns,\n \t\t\t\t\t\t\t int flags)\n {\n \tstruct reftable_ref_iterator *iter;\n@@ -595,6 +719,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \titer->base.oid = &iter->oid;\n \titer->flags = flags;\n \titer->refs = refs;\n+\titer->exclude_patterns = filter_exclude_patterns(exclude_patterns);\n \n \tret = refs->err;\n \tif (ret)\n@@ -616,7 +741,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \n static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_store,\n \t\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t\t const char **exclude_patterns UNUSED,\n+\t\t\t\t\t\t const char **exclude_patterns,\n \t\t\t\t\t\t unsigned int flags)\n {\n \tstruct reftable_ref_iterator *main_iter, *worktree_iter;\n@@ -627,7 +752,8 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t\trequired_flags |= REF_STORE_ODB;\n \trefs = reftable_be_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n-\tmain_iter = ref_iterator_for_stack(refs, refs->main_stack, prefix, flags);\n+\tmain_iter = ref_iterator_for_stack(refs, refs->main_stack, prefix,\n+\t\t\t\t\t exclude_patterns, flags);\n \n \t/*\n \t * The worktree stack is only set when we're in an actual worktree\n@@ -641,7 +767,8 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t * Otherwise we merge both the common and the per-worktree refs into a\n \t * single iterator.\n \t */\n-\tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n+\tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix,\n+\t\t\t\t\t exclude_patterns, flags);\n \treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\tref_iterator_select, NULL);\n }\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 13595744190..3256e4462f9 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,12 +8,6 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-if test_have_prereq !REFFILES\n-then\n-\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n-\ttest_done\n-fi\n-\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n@@ -28,7 +22,14 @@ assert_jumps () {\n \tlocal nr=\"$1\"\n \tlocal trace=\"$2\"\n \n-\tgrep -q \"name:jumps_made value:$nr$\" $trace\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tgrep -q \"name:jumps_made value:$nr$\" $trace;;\n+\treftable)\n+\t\tgrep -q \"name:reseeks_made value:$nr$\" $trace;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n }\n \n assert_no_jumps () {\n@@ -89,7 +90,14 @@ test_expect_success 'adjacent, non-overlapping excluded regions' '\n \tfor_each_ref refs/heads/foo refs/heads/quux >expect &&\n \n \ttest_cmp expect actual &&\n-\tassert_jumps 1 perf\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tassert_jumps 1 perf;;\n+\treftable)\n+\t\tassert_jumps 2 perf;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n '\n \n test_expect_success 'overlapping excluded regions' '\n@@ -106,7 +114,30 @@ test_expect_success 'several overlapping excluded regions' '\n \tfor_each_ref refs/heads/quux >expect &&\n \n \ttest_cmp expect actual &&\n-\tassert_jumps 1 perf\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tassert_jumps 1 perf;;\n+\treftable)\n+\t\tassert_jumps 3 perf;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n+'\n+\n+test_expect_success 'unordered excludes' '\n+\tfor_each_ref__exclude refs/heads \\\n+\t\trefs/heads/foo refs/heads/baz >actual 2>perf &&\n+\tfor_each_ref refs/heads/bar refs/heads/quux >expect &&\n+\n+\ttest_cmp expect actual &&\n+\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n+\tfiles)\n+\t\tassert_jumps 1 perf;;\n+\treftable)\n+\t\tassert_jumps 2 perf;;\n+\t*)\n+\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n+\tesac\n '\n \n test_expect_success 'non-matching excluded section' '\ndiff --git a/trace2.h b/trace2.h\nindex 19e04bf040f..901f39253a6 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -554,6 +554,7 @@ enum trace2_counter_id {\n \tTRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */\n \n \tTRACE2_COUNTER_ID_PACKED_REFS_JUMPS, /* counts number of jumps */\n+\tTRACE2_COUNTER_ID_REFTABLE_RESEEKS, /* counts number of re-seeks */\n \n \t/* counts number of fsyncs */\n \tTRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,\ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex d3a33715c14..036b643578b 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -31,6 +31,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n \t\t.name = \"jumps_made\",\n \t\t.want_per_thread_events = 0,\n \t},\n+\t[TRACE2_COUNTER_ID_REFTABLE_RESEEKS] = {\n+\t\t.category = \"reftable\",\n+\t\t.name = \"reseeks_made\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n \t[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {\n \t\t.category = \"fsync\",\n \t\t.name = \"writeout-only\",\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502922","messageId":"ZulIB7k18+4CzwZb@nand.local","threadId":"62084","inReplyTo":"7497166422ea702aabdf4159b0d7780f1422ba13.1726476401.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:12:39Z","receivedAt":"2024-09-17T09:12:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 16, 2024 at 10:50:03AM +0200, Patrick Steinhardt wrote:\n> This only surfaces when:\n>\n> - A repository uses reference namespaces.\n>\n> - \"transfer.hideRefs\" is active.\n>\n> - The namespaced references are packed into the \"packed-refs\" file.\n>\n> None of our tests exercise this scenario, and thus we haven't ever hit\n> it. While t5509 exercises both (1) and (2), it does not happen to hit\n> (3). It is trivial to demonstrate the bug though by explicitly packing\n> refs in the tests, and then we indeed surface the breakage.\n>\n> Fix this bug by prefixing exclude patterns with the namespace in the\n> generic layer. The newly introduced function will be used outside of\n> \"refs.c\" in the next patch, so we add a declaration to \"refs.h\".\n\nThanks for finding and fixing this bug!\n\n> diff --git a/refs.c b/refs.c\n> index ceb72d4bd74..b3a367ea12c 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> \treturn hide_refs->v;\n> }\n>\n> +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> +\t\t\t\t\t const char *namespace,\n> +\t\t\t\t\t struct strvec *out)\n> +{\n> +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n> +\t\treturn exclude_patterns;\n> +\n> +\tfor (size_t i = 0; exclude_patterns[i]; i++)\n> +\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n> +\n> +\treturn out->v;\n> +}\n> +\n\nIs it safe to concatenate each exclude pattern with the specified\nnamespace? If I'm reading this correctly, I think we silently do the\nwrong thing for exclude patterns that start with '^'.\n\nI guess we reject such patterns in the hidden_refs_to_excludes()\nfunction, but perhaps we wouldn't have to if this function stripped\nthose prefixes for us when the caller does or doesn't specify exclude\npatterns with a '^'?\n\nThanks,\nTaylor\n"},{"id":"502923","messageId":"ZulI9Ha44ZwFPoiC@nand.local","threadId":"62084","inReplyTo":"3dc6ae936c88d3105bc82daab3edd805c9b5c63b.1726476401.git.ps@pks.im","subject":"Re: [PATCH v2 2/6] builtin/receive-pack: fix exclude patterns when announcing refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:16:36Z","receivedAt":"2024-09-17T09:16:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 16, 2024 at 10:50:05AM +0200, Patrick Steinhardt wrote:\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 3f35140e489..478c62ca836 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -339,12 +339,26 @@ static void show_one_alternate_ref(const struct object_id *oid,\n> static void write_head_info(void)\n> {\n> \tstatic struct oidset seen = OIDSET_INIT;\n> +\tstruct strvec excludes_vector = STRVEC_INIT;\n> +\tconst char **exclude_patterns;\n> +\n> +\t/*\n> +\t * We need access to the reference names both with and without their\n> +\t * namespace and thus cannot use `refs_for_each_namespaced_ref()`. We\n> +\t * thus have to adapt exclude patterns to carry the namespace prefix\n> +\t * ourselves.\n> +\t */\n> +\texclude_patterns = get_namespaced_exclude_patterns(\n> +\t\thidden_refs_to_excludes(&hidden_refs),\n> +\t\tget_git_namespace(), &excludes_vector);\n\nOK, so here we use the result of calling hidden_refs_to_excludes() as\nthe first argument to your new get_namespaced_exclude_patterns().\n\nBut I think that in this case when the caller specifies a pattern with\n'^', we still do not exclude any references in the backend, since\nhidden_refs_to_excludes() will return NULL when there is >1 pattern with\n'^' as the first character.\n\nI don't think that this results in broken behavior, since the callback\nto the refs API will still be expected to filter out references that it\ndoesn't want.\n\n> \trefs_for_each_fullref_in(get_main_ref_store(the_repository), \"\",\n> -\t\t\t\t hidden_refs_to_excludes(&hidden_refs),\n> -\t\t\t\t show_ref_cb, &seen);\n> +\t\t\t\t exclude_patterns, show_ref_cb, &seen);\n> \tfor_each_alternate_ref(show_one_alternate_ref, &seen);\n\nThanks,\nTaylor\n"},{"id":"502924","messageId":"ZulLWJWLgU7gL1MK@nand.local","threadId":"62084","inReplyTo":"050f4906393d1f9c58a6b6bc695b004695d219be.1726476401.git.ps@pks.im","subject":"Re: [PATCH v2 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:26:48Z","receivedAt":"2024-09-17T09:26:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 16, 2024 at 10:50:16AM +0200, Patrick Steinhardt wrote:\n> This makes t1419 work with the \"reftable\" backend with some slight\n> modifications. Of course it also speeds up listing of references with\n> hidden refs. The following benchmark prints one reference with 1 million\n> hidden references:\n>\n> Benchmark 1: HEAD~\n> Time (mean ± σ): 93.3 ms ± 2.1 ms [User: 90.3 ms, System: 2.5 ms]\n> Range (min … max): 89.8 ms … 97.2 ms 33 runs\n>\n> Benchmark 2: HEAD\n> Time (mean ± σ): 4.2 ms ± 0.6 ms [User: 2.2 ms, System: 1.8 ms]\n> Range (min … max): 3.1 ms … 8.1 ms 765 runs\n>\n> Summary\n> HEAD ran\n> 22.15 ± 3.19 times faster than HEAD~\n\nVery nice.\n\n> +\t\t/*\n> +\t\t * When the reference name is lexicographically bigger than the\n> +\t\t * current exclude pattern we know that it won't ever match any\n> +\t\t * of the following references, either. We thus advance to the\n> +\t\t * next pattern and re-check whether it matches.\n> +\t\t *\n> +\t\t * Otherwise, if it's smaller, then we do not have a match and\n> +\t\t * thus want to show the current reference.\n> +\t\t */\n> +\t\tcmp = strncmp(iter->ref.refname, pattern,\n> +\t\t\t iter->exclude_patterns_strlen);\n> +\t\tif (cmp > 0) {\n> +\t\t\titer->exclude_patterns_index++;\n> +\t\t\titer->exclude_patterns_strlen = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (cmp < 0)\n> +\t\t\treturn 0;\n\nPerhaps I am showing my ignorance for the reftable backend, but is it OK\nto call strncmp() against all patterns here?\n\nIn the packed-refs implementation which I worked on with Peff sometime\nlast year, we rejected exclude patterns that contain special glob\ncharacters in them for exactly this reason.\n\nThe implementation that I'm referring to has a helpful comment that\njogged my memory for what we were thinking at the time (see the\nfunction refs/packed-backend.c::populate_excluded_jump_list()).\n\nUgh, I just read the next hunk below, so ignore me here ;-).\n\n> +static char **filter_exclude_patterns(const char **exclude_patterns)\n> +{\n> +\tsize_t filtered_size = 0, filtered_alloc = 0;\n> +\tchar **filtered = NULL;\n> +\n> +\tif (!exclude_patterns)\n> +\t\treturn NULL;\n> +\n> +\tfor (size_t i = 0; ; i++) {\n> +\t\tconst char *exclude_pattern = exclude_patterns[i];\n> +\t\tint has_glob = 0;\n> +\n> +\t\tif (!exclude_pattern)\n> +\t\t\tbreak;\n> +\n> +\t\tfor (const char *p = exclude_pattern; *p; p++) {\n> +\t\t\thas_glob = is_glob_special(*p);\n> +\t\t\tif (has_glob)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\tif (has_glob)\n> +\t\t\tcontinue;\n> +\n> +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n> +\t\tfiltered[filtered_size++] = xstrdup(exclude_pattern);\n> +\t}\n> +\n> +\tif (filtered_size) {\n> +\t\tQSORT(filtered, filtered_size, qsort_strcmp);\n> +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n> +\t\tfiltered[filtered_size++] = NULL;\n> +\t}\n> +\n> +\treturn filtered;\n> +}\n\nOhhh, here's where we filter out un-excludeable patterns. Ignore me :-).\n\nOne question I had reading this is why we don't filter these out on the\nfly in the iterator itself instead of allocating a separate array that\nwe have to xstrdup() into and free later on.\n\nWe may be at the point of diminishing returns here, but I wonder if\nallocating this thing is more expensive than a few redundant strcmp()s\nand calls to is_glob_special(). I dunno.\n\nThanks,\nTaylor\n"},{"id":"502927","messageId":"ZulM4BU7zSn_NHMl@pks.im","threadId":"62084","inReplyTo":"ZulIB7k18+4CzwZb@nand.local","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T09:33:25Z","receivedAt":"2024-09-17T09:33:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 05:12:39AM -0400, Taylor Blau wrote:\n> On Mon, Sep 16, 2024 at 10:50:03AM +0200, Patrick Steinhardt wrote:\n> > diff --git a/refs.c b/refs.c\n> > index ceb72d4bd74..b3a367ea12c 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> > \treturn hide_refs->v;\n> > }\n> >\n> > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> > +\t\t\t\t\t const char *namespace,\n> > +\t\t\t\t\t struct strvec *out)\n> > +{\n> > +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n> > +\t\treturn exclude_patterns;\n> > +\n> > +\tfor (size_t i = 0; exclude_patterns[i]; i++)\n> > +\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n> > +\n> > +\treturn out->v;\n> > +}\n> > +\n> \n> Is it safe to concatenate each exclude pattern with the specified\n> namespace? If I'm reading this correctly, I think we silently do the\n> wrong thing for exclude patterns that start with '^'.\n> \n> I guess we reject such patterns in the hidden_refs_to_excludes()\n> function, but perhaps we wouldn't have to if this function stripped\n> those prefixes for us when the caller does or doesn't specify exclude\n> patterns with a '^'?\n\nYeah, as you mention, `hidden_refs_to_excludes()` drops excludes\ncompletely in case there's any pattern starting with '^' or '!'. So the\ncurrent assumption should be safe because we don't use excludes in this\nscenario at all.\n\nI also agree that we should eventually think about allowing '^'-scoped\nreferences. We can handle them correctly, but it requires a bit more\nthought wire that up. So I'd prefer to keep that out of this series, as\nit is basically a new feature.\n\nPatrick\n"},{"id":"502928","messageId":"ZulOK5GAWYbPlYeO@nand.local","threadId":"62084","inReplyTo":"ZulM4BU7zSn_NHMl@pks.im","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:38:51Z","receivedAt":"2024-09-17T09:38:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 17, 2024 at 11:33:25AM +0200, Patrick Steinhardt wrote:\n> On Tue, Sep 17, 2024 at 05:12:39AM -0400, Taylor Blau wrote:\n> > On Mon, Sep 16, 2024 at 10:50:03AM +0200, Patrick Steinhardt wrote:\n> > > diff --git a/refs.c b/refs.c\n> > > index ceb72d4bd74..b3a367ea12c 100644\n> > > --- a/refs.c\n> > > +++ b/refs.c\n> > > @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> > > \treturn hide_refs->v;\n> > > }\n> > >\n> > > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> > > +\t\t\t\t\t const char *namespace,\n> > > +\t\t\t\t\t struct strvec *out)\n> > > +{\n> > > +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n> > > +\t\treturn exclude_patterns;\n> > > +\n> > > +\tfor (size_t i = 0; exclude_patterns[i]; i++)\n> > > +\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n> > > +\n> > > +\treturn out->v;\n> > > +}\n> > > +\n> >\n> > Is it safe to concatenate each exclude pattern with the specified\n> > namespace? If I'm reading this correctly, I think we silently do the\n> > wrong thing for exclude patterns that start with '^'.\n> >\n> > I guess we reject such patterns in the hidden_refs_to_excludes()\n> > function, but perhaps we wouldn't have to if this function stripped\n> > those prefixes for us when the caller does or doesn't specify exclude\n> > patterns with a '^'?\n>\n> Yeah, as you mention, `hidden_refs_to_excludes()` drops excludes\n> completely in case there's any pattern starting with '^' or '!'. So the\n> current assumption should be safe because we don't use excludes in this\n\nRight... but can't exclude_patterns be arbitrary here, as it is a\nparameter to the function which is exported via the *.h header file?\n\nIOW, I don't think we can claim at all that we have passed the excluded\npatterns through hidden_refs_to_excludes() before calling\nget_namespaced_exclude_patterns().\n\n> I also agree that we should eventually think about allowing '^'-scoped\n> references. We can handle them correctly, but it requires a bit more\n> thought wire that up. So I'd prefer to keep that out of this series, as\n> it is basically a new feature.\n\nYeah, I agree.\n\nThanks,\nTaylor\n"},{"id":"502929","messageId":"ZulOLLdh1_jrYcN2@pks.im","threadId":"62084","inReplyTo":"ZulLWJWLgU7gL1MK@nand.local","subject":"Re: [PATCH v2 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T09:39:04Z","receivedAt":"2024-09-17T09:39:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 05:26:48AM -0400, Taylor Blau wrote:\n> On Mon, Sep 16, 2024 at 10:50:16AM +0200, Patrick Steinhardt wrote:\n> > +\t\t/*\n> > +\t\t * When the reference name is lexicographically bigger than the\n> > +\t\t * current exclude pattern we know that it won't ever match any\n> > +\t\t * of the following references, either. We thus advance to the\n> > +\t\t * next pattern and re-check whether it matches.\n> > +\t\t *\n> > +\t\t * Otherwise, if it's smaller, then we do not have a match and\n> > +\t\t * thus want to show the current reference.\n> > +\t\t */\n> > +\t\tcmp = strncmp(iter->ref.refname, pattern,\n> > +\t\t\t iter->exclude_patterns_strlen);\n> > +\t\tif (cmp > 0) {\n> > +\t\t\titer->exclude_patterns_index++;\n> > +\t\t\titer->exclude_patterns_strlen = 0;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\t\tif (cmp < 0)\n> > +\t\t\treturn 0;\n> \n> Perhaps I am showing my ignorance for the reftable backend, but is it OK\n> to call strncmp() against all patterns here?\n> \n> In the packed-refs implementation which I worked on with Peff sometime\n> last year, we rejected exclude patterns that contain special glob\n> characters in them for exactly this reason.\n> \n> The implementation that I'm referring to has a helpful comment that\n> jogged my memory for what we were thinking at the time (see the\n> function refs/packed-backend.c::populate_excluded_jump_list()).\n> \n> Ugh, I just read the next hunk below, so ignore me here ;-).\n\n;)\n\nI was also wondering whether we'd want to amend the generic parts of the\nrefs interface to filter out globs. But it is entirely feasible that a\nbackend can indeed filter out globs efficiently, even though none of the\ncurrent ones can. So it kind of makes sense to keep things as-is and let\nthe backends themselves decide what they can use.\n\n> > +static char **filter_exclude_patterns(const char **exclude_patterns)\n> > +{\n> > +\tsize_t filtered_size = 0, filtered_alloc = 0;\n> > +\tchar **filtered = NULL;\n> > +\n> > +\tif (!exclude_patterns)\n> > +\t\treturn NULL;\n> > +\n> > +\tfor (size_t i = 0; ; i++) {\n> > +\t\tconst char *exclude_pattern = exclude_patterns[i];\n> > +\t\tint has_glob = 0;\n> > +\n> > +\t\tif (!exclude_pattern)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tfor (const char *p = exclude_pattern; *p; p++) {\n> > +\t\t\thas_glob = is_glob_special(*p);\n> > +\t\t\tif (has_glob)\n> > +\t\t\t\tbreak;\n> > +\t\t}\n> > +\t\tif (has_glob)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n> > +\t\tfiltered[filtered_size++] = xstrdup(exclude_pattern);\n> > +\t}\n> > +\n> > +\tif (filtered_size) {\n> > +\t\tQSORT(filtered, filtered_size, qsort_strcmp);\n> > +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n> > +\t\tfiltered[filtered_size++] = NULL;\n> > +\t}\n> > +\n> > +\treturn filtered;\n> > +}\n> \n> Ohhh, here's where we filter out un-excludeable patterns. Ignore me :-).\n> \n> One question I had reading this is why we don't filter these out on the\n> fly in the iterator itself instead of allocating a separate array that\n> we have to xstrdup() into and free later on.\n> \n> We may be at the point of diminishing returns here, but I wonder if\n> allocating this thing is more expensive than a few redundant strcmp()s\n> and calls to is_glob_special(). I dunno.\n\nI think duplicating the array is the right thing to do anyway to not get\nweird lifetime issues with the exclude patterns. A caller may set up a\nref iterator that they may end up using for longer than they keep alive\nthe exlude patterns passed to the iterator. So by duplicating the array\nwe might end up wasting a bit of memory, but we avoid such lifetime\nproblems completely, which I think is a win to make the infrastructure\nless fragile.\n\nThanks for your review!\n\nPatrick\n"},{"id":"502930","messageId":"ZulPhat5pavpiuXT@pks.im","threadId":"62084","inReplyTo":"ZulOK5GAWYbPlYeO@nand.local","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T09:44:44Z","receivedAt":"2024-09-17T09:44:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 05:38:51AM -0400, Taylor Blau wrote:\n> On Tue, Sep 17, 2024 at 11:33:25AM +0200, Patrick Steinhardt wrote:\n> > On Tue, Sep 17, 2024 at 05:12:39AM -0400, Taylor Blau wrote:\n> > > On Mon, Sep 16, 2024 at 10:50:03AM +0200, Patrick Steinhardt wrote:\n> > > > diff --git a/refs.c b/refs.c\n> > > > index ceb72d4bd74..b3a367ea12c 100644\n> > > > --- a/refs.c\n> > > > +++ b/refs.c\n> > > > @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)\n> > > > \treturn hide_refs->v;\n> > > > }\n> > > >\n> > > > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,\n> > > > +\t\t\t\t\t const char *namespace,\n> > > > +\t\t\t\t\t struct strvec *out)\n> > > > +{\n> > > > +\tif (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)\n> > > > +\t\treturn exclude_patterns;\n> > > > +\n> > > > +\tfor (size_t i = 0; exclude_patterns[i]; i++)\n> > > > +\t\tstrvec_pushf(out, \"%s%s\", namespace, exclude_patterns[i]);\n> > > > +\n> > > > +\treturn out->v;\n> > > > +}\n> > > > +\n> > >\n> > > Is it safe to concatenate each exclude pattern with the specified\n> > > namespace? If I'm reading this correctly, I think we silently do the\n> > > wrong thing for exclude patterns that start with '^'.\n> > >\n> > > I guess we reject such patterns in the hidden_refs_to_excludes()\n> > > function, but perhaps we wouldn't have to if this function stripped\n> > > those prefixes for us when the caller does or doesn't specify exclude\n> > > patterns with a '^'?\n> >\n> > Yeah, as you mention, `hidden_refs_to_excludes()` drops excludes\n> > completely in case there's any pattern starting with '^' or '!'. So the\n> > current assumption should be safe because we don't use excludes in this\n> \n> Right... but can't exclude_patterns be arbitrary here, as it is a\n> parameter to the function which is exported via the *.h header file?\n> \n> IOW, I don't think we can claim at all that we have passed the excluded\n> patterns through hidden_refs_to_excludes() before calling\n> get_namespaced_exclude_patterns().\n\nI think the important thing to realize is that we're talking about two\ndifferent things:\n\n - We have exclude patterns. These are _patterns_ only and do not\n support '^' or '!'. These patterns are handled by the ref backend.\n\n - We have hidden refs, which are a mechanism of our transport layer.\n These _can_ support '^' or '!'.\n\nTo use hidden refs as exclude patterns they need to get translated to\nuse the exclude pattern syntax, which does not support every feature\nthat the hidden ref syntax does.\n\nSo every caller that uses exclude patterns must already make sure to\nfilter things accordingly, and they must make sure to use the correct\nsyntax for exclude patterns. And if they do, I think that the conversion\nto namespaced exclude patterns should be correct.\n\nOr am I missing something?\n\nPatrick\n"},{"id":"502932","messageId":"ZulRYg98ehjPLr2c@nand.local","threadId":"62084","inReplyTo":"ZulPhat5pavpiuXT@pks.im","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:52:34Z","receivedAt":"2024-09-17T09:52:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 17, 2024 at 11:44:44AM +0200, Patrick Steinhardt wrote:\n> I think the important thing to realize is that we're talking about two\n> different things:\n\nWow, I'm sorry -- I completely misunderstood what was going on in this\ndiff.\n\nI have to say, that is quite embarrassing having written this feature\nonly a little over a year ago. Sorry for the confusion, this patch LGTM.\n\nThanks,\nTaylor\n"},{"id":"502933","messageId":"ZulRrH2Pgrv/Cr/X@nand.local","threadId":"62084","inReplyTo":"ZulOLLdh1_jrYcN2@pks.im","subject":"Re: [PATCH v2 6/6] refs/reftable: wire up support for exclude patterns","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:53:48Z","receivedAt":"2024-09-17T09:53:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 17, 2024 at 11:39:04AM +0200, Patrick Steinhardt wrote:\n> > Ugh, I just read the next hunk below, so ignore me here ;-).\n>\n> ;)\n>\n> I was also wondering whether we'd want to amend the generic parts of the\n> refs interface to filter out globs. But it is entirely feasible that a\n> backend can indeed filter out globs efficiently, even though none of the\n> current ones can. So it kind of makes sense to keep things as-is and let\n> the backends themselves decide what they can use.\n\nYep, agreed.\n\n> > One question I had reading this is why we don't filter these out on the\n> > fly in the iterator itself instead of allocating a separate array that\n> > we have to xstrdup() into and free later on.\n> >\n> > We may be at the point of diminishing returns here, but I wonder if\n> > allocating this thing is more expensive than a few redundant strcmp()s\n> > and calls to is_glob_special(). I dunno.\n>\n> I think duplicating the array is the right thing to do anyway to not get\n> weird lifetime issues with the exclude patterns. A caller may set up a\n> ref iterator that they may end up using for longer than they keep alive\n> the exlude patterns passed to the iterator. So by duplicating the array\n> we might end up wasting a bit of memory, but we avoid such lifetime\n> problems completely, which I think is a win to make the infrastructure\n> less fragile.\n\nOK, I trust your judgement here over my own as not having nearly the\nfamiliarity with reftables as you do.\n\nThanks,\nTaylor\n"},{"id":"502934","messageId":"ZulR-G5nOpMsepLA@pks.im","threadId":"62084","inReplyTo":"ZulRYg98ehjPLr2c@nand.local","subject":"Re: [PATCH v2 1/6] refs: properly apply exclude patterns to namespaced refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T09:55:04Z","receivedAt":"2024-09-17T09:55:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 05:52:34AM -0400, Taylor Blau wrote:\n> On Tue, Sep 17, 2024 at 11:44:44AM +0200, Patrick Steinhardt wrote:\n> > I think the important thing to realize is that we're talking about two\n> > different things:\n> \n> Wow, I'm sorry -- I completely misunderstood what was going on in this\n> diff.\n> \n> I have to say, that is quite embarrassing having written this feature\n> only a little over a year ago. Sorry for the confusion, this patch LGTM.\n\nNo worries! If at all it points out that the patch message could've been\nclearer.\n\nPatrick\n"},{"id":"502958","messageId":"CAOLa=ZR2Q+8VKSink5FGJLhZVUZ6akZT565C1g2-fpczMkqwaw@mail.gmail.com","threadId":"62084","inReplyTo":"ZufWjY053IKObF8T@pks.im","subject":"Re: [PATCH 5/6] reftable/reader: make table iterator reseekable","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-17T16:44:44Z","receivedAt":"2024-09-17T16:44:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Sep 13, 2024 at 07:11:54AM -0500, karthik nayak wrote:\n>> Patrick Steinhardt writes:\n>> > diff --git a/t/unit-tests/t-reftable-reader.c b/t/unit-tests/t-reftable-reader.c\n>> > new file mode 100644\n>> > index 00000000000..7a46580b6f1\n>> > --- /dev/null\n>> > +++ b/t/unit-tests/t-reftable-reader.c\n>> > @@ -0,0 +1,96 @@\n>> > +#include \"test-lib.h\"\n>> > +#include \"lib-reftable.h\"\n>> > +#include \"reftable/blocksource.h\"\n>> > +#include \"reftable/reader.h\"\n>> > +\n>> > +static int t_reader_seek_once(void)\n>> > +{\n>> > +\tstruct reftable_ref_record records[] = {\n>> > +\t\t{\n>> > +\t\t\t.refname = (char *) \"refs/heads/main\",\n>> > +\t\t\t.value_type = REFTABLE_REF_VAL1,\n>> > +\t\t\t.value.val1 = { 42 },\n>> > +\t\t},\n>> > +\t};\n>> > +\tstruct reftable_block_source source = { 0 };\n>> > +\tstruct reftable_ref_record ref = { 0 };\n>> > +\tstruct reftable_iterator it = { 0 };\n>> > +\tstruct reftable_reader *reader;\n>> > +\tstruct strbuf buf = STRBUF_INIT;\n>> > +\tint ret;\n>> > +\n>> > +\tt_reftable_write_to_buf(&buf, records, ARRAY_SIZE(records), NULL, 0, NULL);\n>> > +\tblock_source_from_strbuf(&source, &buf);\n>> > +\n>> > +\tret = reftable_reader_new(&reader, &source, \"name\");\n>> > +\tcheck_int(ret, ==, 0);\n>> > +\n>> > +\treftable_reader_init_ref_iterator(reader, &it);\n>> > +\tret = reftable_iterator_seek_ref(&it, \"\");\n>> > +\tcheck_int(ret, ==, 0);\n>> > +\tret = reftable_iterator_next_ref(&it, &ref);\n>> > +\tcheck_int(ret, ==, 0);\n>> > +\n>> > +\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n>>\n>> s/20/GIT_SHA1_RAWSZ\n>\n> Indeed.\n>\n>> Also here and elsewhere, shouldn't we just do\n>> `check(reftable_ref_record_equal(...))` or even\n>> `!check(reftable_iterator_seek_ref(...))` ?\n>\n> I guess you mean `check(!reftable_iteraror_seek_ref())`, right?\n>\n\nYes.\n\n> In the case where we just expect a zero error code I can certainly adapt\n> the code to use `check(...)`. But the other cases shouldn't use\n> `check(!...)` because it is important that the returnd error code is\n> positive.\n>\n\nPerfect!\n\n> Patrick\n\nKarthik\n"},{"id":"502960","messageId":"CAOLa=ZS2dOOGauZa-yBtE7S_Ex8L8kNV=CAh4mQfZdhmjCUnbw@mail.gmail.com","threadId":"62084","inReplyTo":"ZufWkLwn0RQeCCpD@pks.im","subject":"Re: [PATCH 6/6] refs/reftable: wire up support for exclude patterns","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-17T17:31:27Z","receivedAt":"2024-09-17T17:31:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Sep 13, 2024 at 07:47:06AM -0500, karthik nayak wrote:\n>> Patrick Steinhardt writes:\n>> > +/*\n>> > + * Handle exclude patterns. Returns either `1`, which tells the caller that the\n>> > + * current reference shall not be shown. Or `0`, which indicates that it should\n>> > + * be shown.\n>> > + */\n>> > +static int should_exclude_current_ref(struct reftable_ref_iterator *iter)\n>> > +{\n>> > +\twhile (iter->exclude_patterns[iter->exclude_patterns_index]) {\n>> > +\t\tconst char *pattern = iter->exclude_patterns[iter->exclude_patterns_index];\n>> > +\t\tchar *ref_after_pattern;\n>> > +\t\tint cmp;\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Lazily cache the pattern length so that we don't have to\n>> > +\t\t * recompute it every time this function is called.\n>> > +\t\t */\n>> > +\t\tif (!iter->exclude_patterns_strlen)\n>> > +\t\t\titer->exclude_patterns_strlen = strlen(pattern);\n>> > +\n>> > +\t\t/*\n>> > +\t\t * When the reference name is lexicographically bigger than the\n>> > +\t\t * current exclude pattern we know that it won't ever match any\n>> > +\t\t * of the following references, either. We thus advance to the\n>> > +\t\t * next pattern and re-check whether it matches.\n>>\n>> So this means that the exclude patterns were lexicographically sorted.\n>> Otherwise this would work.\n>\n> Indeed. Good that you call out my assumption, as I in fact didn't verify\n> that it holds, and in fact it doesn't. It's not a correctness issue if\n> it doesn't hold, because it would simply mean that we don't skip over\n> some references where we really could. But it certainly is a perfomance\n> issue.\n>\n> Will fix and add a test for it.\n>\n>> > +\t\t * Note that the seeked-to reference may also be excluded. This\n>> > +\t\t * is not handled here though, but the caller is expected to\n>> > +\t\t * loop and re-verify the next reference for us.\n>> > +\t\t */\n>>\n>> The seeked-to reference here being the one with 0xff. We could get rid\n>> of this by doing something like this:\n>>\n>> int last_char_idx = iter->exclude_patterns_strlen - 1\n>> ref_after_pattern = xstrfmt(\"%s\", pattern);\n>> ref_after_pattern[last_char_idx] = ref_after_pattern[last_char_idx] + 1;\n>>\n>> instead no?\n>\n> Sorry, I don't quite follow what you mean with \"get rid of this\". What\n> exactly is \"this\"? Do you mean the re-looping?\n>\n> If so then the above doesn't fix it, no. We'd have to repeat a whole lot\n> of code here to also retrieve the next entry, store it into `iter->ref`,\n> check whether it is an actual ref starting with \"refs/\" and so on.\n> Looping once very much feels like the better thing to do.\n>\n\nI definitely responded to the wrong chunk of your comment. I meant to\nrespond to\n\n> * This is done by appending the highest possible character to\n> * the pattern. Consequently, all references that have the\n> * pattern as prefix and whose suffix starts with anything in\n> * the range [0x00, 0xfe] are skipped. And given that 0xff is a\n> * non-printable character that shouldn't ever be in a ref name,\n> * we'd not yield any such record, either.\n\nMy point being, we can even skip \"pattern + 0xff\" by just setting the\nseek to ref to be the next character in the pattern. But since \"pattern\n+ 0xff\" is anyways not an expected ref. They should behave the same.\n\n>> > @@ -580,9 +660,45 @@ static struct ref_iterator_vtable reftable_ref_iterator_vtable = {\n>> > \t.abort = reftable_ref_iterator_abort\n>> > };\n>> >\n>> > +static char **filter_exclude_patterns(const char **exclude_patterns)\n>> > +{\n>> > +\tsize_t filtered_size = 0, filtered_alloc = 0;\n>> > +\tchar **filtered = NULL;\n>> > +\n>> > +\tif (!exclude_patterns)\n>> > +\t\treturn NULL;\n>> > +\n>> > +\tfor (size_t i = 0; ; i++) {\n>> > +\t\tconst char *exclude_pattern = exclude_patterns[i];\n>> > +\t\tint has_glob = 0;\n>> > +\n>> > +\t\tif (!exclude_pattern)\n>> > +\t\t\tbreak;\n>> > +\n>> > +\t\tfor (const char *p = exclude_pattern; *p; p++) {\n>> > +\t\t\thas_glob = is_glob_special(*p);\n>> > +\t\t\tif (has_glob)\n>> > +\t\t\t\tbreak;\n>> > +\t\t}\n>>\n>> Why do we need to filter excludes here? Don't the callee's already do\n>> something like this?\n>\n> No, it doesn't. The code for exclude patterns is structured in such a\n> way that the responsibility is with the backend to decide what it can\n> and cannot filter. In theory there could be a backend that can exclude\n> refs based on globs efficiently, even though neither the \"files\" nor the\n> \"reftable\" backend can.\n\nThanks for the explanataion.\n"},{"id":"502961","messageId":"CAOLa=ZTeZPeAOhSx3JnDGeeRwD9=af071FJZj+gWGqE29d4Adg@mail.gmail.com","threadId":"62084","inReplyTo":"cover.1726476401.git.ps@pks.im","subject":"Re: [PATCH v2 0/6] refs/reftable: wire up exclude patterns","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-17T17:33:40Z","receivedAt":"2024-09-17T17:33:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\nHello,\n\n> Hi,\n>\n> this is the second version of my patch series that fixes preexisting\n> bugs with exclude patterns and wires them up for the reftable backend.\n>\n> Changes compared to v1:\n>\n> - Some typo fixes in commit messages.\n>\n> - Mention that the newly introduced function in patch 1 will be used\n> in patch 2, which is why it's declared in \"refs.h\".\n>\n> - Use `check()` instead of `check_int(ret, ==, 0)`.\n>\n> - Sort exclude patterns lexicographically. This fixes a bug where we\n> might not correctly skip over some refs when patterns are passed to\n> the reftable backend in non-lexicographic order. Add a test for\n> this.\n>\n> Thanks!\n>\n> Patrick\n>\n> Patrick Steinhardt (6):\n> refs: properly apply exclude patterns to namespaced refs\n> builtin/receive-pack: fix exclude patterns when announcing refs\n> Makefile: stop listing test library objects twice\n> t/unit-tests: introduce reftable library\n> reftable/reader: make table iterator reseekable\n> refs/reftable: wire up support for exclude patterns\n>\n> Makefile | 8 +-\n> builtin/receive-pack.c | 18 ++-\n> refs.c | 35 +++++-\n> refs.h | 9 ++\n> refs/reftable-backend.c | 133 ++++++++++++++++++++++-\n> reftable/reader.c | 1 +\n> t/t1419-exclude-refs.sh | 49 +++++++--\n> t/t5509-fetch-push-namespaces.sh | 9 ++\n> t/unit-tests/lib-reftable.c | 93 ++++++++++++++++\n> t/unit-tests/lib-reftable.h | 20 ++++\n> t/unit-tests/t-reftable-merged.c | 163 +++++++++++++++-------------\n> t/unit-tests/t-reftable-reader.c | 96 ++++++++++++++++\n> t/unit-tests/t-reftable-readwrite.c | 130 +++++++---------------\n> t/unit-tests/t-reftable-stack.c | 25 ++---\n> trace2.h | 1 +\n> trace2/tr2_ctr.c | 5 +\n> 16 files changed, 594 insertions(+), 201 deletions(-)\n> create mode 100644 t/unit-tests/lib-reftable.c\n> create mode 100644 t/unit-tests/lib-reftable.h\n> create mode 100644 t/unit-tests/t-reftable-reader.c\n>\n> Range-diff against v1:\n> 1: 8d347bc5599 ! 1: 7497166422e refs: properly apply exclude patterns to namespaced refs\n> @@ Commit message\n> to the non-stripped ones that still have the namespace prefix. In fact,\n> the \"transfer.hideRefs\" machinery does the former and applies to the\n> stripped reference by default, but rules can have \"^\" prefixed to switch\n> - this behaviour to iinstead match against the rull reference name.\n> + this behaviour to instead match against the full reference name.\n>\n> Namespaces are exclusively handled at the generic \"refs\" layer, the\n> respective backends have no clue that such a thing even exists. This\n> @@ Commit message\n> refs in the tests, and then we indeed surface the breakage.\n>\n> Fix this bug by prefixing exclude patterns with the namespace in the\n> - generic layer.\n> + generic layer. The newly introduced function will be used outside of\n> + \"refs.c\" in the next patch, so we add a declaration to \"refs.h\".\n>\n> Signed-off-by: Patrick Steinhardt \n>\n> 2: 0317a5a7ede = 2: 3dc6ae936c8 builtin/receive-pack: fix exclude patterns when announcing refs\n> 3: 503c44e6cab = 3: 4ba503520e6 Makefile: stop listing test library objects twice\n> 4: 3df4040dd3c = 4: 6747076420f t/unit-tests: introduce reftable library\n> 5: a281f936a2b ! 5: 3278cdf92fe reftable/reader: make table iterator reseekable\n> @@ t/unit-tests/t-reftable-reader.c (new)\n> +\tblock_source_from_strbuf(&source, &buf);\n> +\n> +\tret = reftable_reader_new(&reader, &source, \"name\");\n> -+\tcheck_int(ret, ==, 0);\n> ++\tcheck(!ret);\n> +\n> +\treftable_reader_init_ref_iterator(reader, &it);\n> +\tret = reftable_iterator_seek_ref(&it, \"\");\n> -+\tcheck_int(ret, ==, 0);\n> ++\tcheck(!ret);\n> +\tret = reftable_iterator_next_ref(&it, &ref);\n> -+\tcheck_int(ret, ==, 0);\n> ++\tcheck(!ret);\n> +\n> -+\tret = reftable_ref_record_equal(&ref, &records[0], 20);\n> ++\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n> +\tcheck_int(ret, ==, 1);\n> +\n> +\tret = reftable_iterator_next_ref(&it, &ref);\n> @@ t/unit-tests/t-reftable-reader.c (new)\n> +\tblock_source_from_strbuf(&source, &buf);\n> +\n> +\tret = reftable_reader_new(&reader, &source, \"name\");\n> -+\tcheck_int(ret, ==, 0);\n> ++\tcheck(!ret);\n> +\n> +\treftable_reader_init_ref_iterator(reader, &it);\n> +\n> +\tfor (size_t i = 0; i < 5; i++) {\n> +\t\tret = reftable_iterator_seek_ref(&it, \"\");\n> -+\t\tcheck_int(ret, ==, 0);\n> ++\t\tcheck(!ret);\n> +\t\tret = reftable_iterator_next_ref(&it, &ref);\n> -+\t\tcheck_int(ret, ==, 0);\n> ++\t\tcheck(!ret);\n> +\n> +\t\tret = reftable_ref_record_equal(&ref, &records[0], GIT_SHA1_RAWSZ);\n> +\t\tcheck_int(ret, ==, 1);\n> 6: f3922b81db6 ! 6: 050f4906393 refs/reftable: wire up support for exclude patterns\n> @@ refs/reftable-backend.c: static struct ref_iterator_vtable reftable_ref_iterator\n> \t.abort = reftable_ref_iterator_abort\n> };\n>\n> ++static int qsort_strcmp(const void *va, const void *vb)\n> ++{\n> ++\tconst char *a = *(const char **)va;\n> ++\tconst char *b = *(const char **)vb;\n> ++\treturn strcmp(a, b);\n> ++}\n> ++\n> +static char **filter_exclude_patterns(const char **exclude_patterns)\n> +{\n> +\tsize_t filtered_size = 0, filtered_alloc = 0;\n> @@ refs/reftable-backend.c: static struct ref_iterator_vtable reftable_ref_iterator\n> +\t}\n> +\n> +\tif (filtered_size) {\n> ++\t\tQSORT(filtered, filtered_size, qsort_strcmp);\n> +\t\tALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);\n> +\t\tfiltered[filtered_size++] = NULL;\n> +\t}\n> @@ t/t1419-exclude-refs.sh: test_expect_success 'several overlapping excluded regio\n> +\t\tassert_jumps 3 perf;;\n> +\t*)\n> +\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n> ++\tesac\n> ++'\n> ++\n> ++test_expect_success 'unordered excludes' '\n> ++\tfor_each_ref__exclude refs/heads \\\n> ++\t\trefs/heads/foo refs/heads/baz >actual 2>perf &&\n> ++\tfor_each_ref refs/heads/bar refs/heads/quux >expect &&\n> ++\n> ++\ttest_cmp expect actual &&\n> ++\tcase \"$GIT_DEFAULT_REF_FORMAT\" in\n> ++\tfiles)\n> ++\t\tassert_jumps 1 perf;;\n> ++\treftable)\n> ++\t\tassert_jumps 2 perf;;\n> ++\t*)\n> ++\t\tBUG \"unhandled ref format $GIT_DEFAULT_REF_FORMAT\";;\n> +\tesac\n> '\n>\n\nThe range diff here looks good based on my earlier review. Thanks\n"}]}