{"thread":{"id":"60541","subject":"[PATCH 1/8] reftable: wrap EXPECT macros in do/while","startedAt":"2023-11-21T07:04:14Z","lastAt":"2023-12-28T05:54:04Z","messageCount":58,"participants":["Patrick Steinhardt","Taylor Blau","Eric Sunshine","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"485024","messageId":"89a953135573d028ba7769953f50bf3f43e57d9c.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 1/8] reftable: wrap EXPECT macros in do/while","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:10Z","receivedAt":"2023-11-21T07:04:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `EXPECT` macros used by the reftable test framework are all using a\nsingle `if` statement with the actual condition. This results in weird\nsyntax when using them in if/else statements like the following:\n\n```\nif (foo)\n\tEXPECT(foo == 2)\nelse\n\tEXPECT(bar == 2)\n```\n\nNote that there need not be a trailing semicolon. Furthermore, it is not\nimmediately obvious whether the else now belongs to the `if (foo)` or\nwhether it belongs to the expanded `if (foo == 2)` from the macro.\n\nFix this by wrapping the macros in a do/while loop.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/test_framework.h | 58 +++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 26 deletions(-)\n\ndiff --git a/reftable/test_framework.h b/reftable/test_framework.h\nindex 774cb275bf..ee44f735ae 100644\n--- a/reftable/test_framework.h\n+++ b/reftable/test_framework.h\n@@ -12,32 +12,38 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"reftable-error.h\"\n \n-#define EXPECT_ERR(c) \\\n-\tif (c != 0) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n-\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT_STREQ(a, b) \\\n-\tif (strcmp(a, b)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n-\t\t\t__LINE__, #a, a, #b, b); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT(c) \\\n-\tif (!(c)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n-\t\t\t__LINE__, #c); \\\n-\t\tabort(); \\\n-\t}\n+#define EXPECT_ERR(c) \\\n+\tdo { \\\n+\t\tif (c != 0) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n+\t\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT_STREQ(a, b) \\\n+\tdo { \\\n+\t\tif (strcmp(a, b)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #a, a, #b, b); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT(c) \\\n+\tdo { \\\n+\t\tif (!(c)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #c); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n \n #define RUN_TEST(f) \\\n \tfprintf(stderr, \"running %s\\n\", #f); \\\n-- \n2.42.0\n\n"},{"id":"485025","messageId":"cover.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":null,"subject":"[PATCH 0/8] reftable: small set of fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:05Z","receivedAt":"2023-11-21T07:04:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhile working on the reftable backend I've hit several smaller issues in\nthe reftable library, which this patch series addresses.\n\nWe probably want to refactor t0032-reftable-unittest.sh to plug into the\nnew unit test architecture eventually, but for now I refrained from\ndoing so. I care more about getting things to a working state right now,\nbut I or somebody else from the Gitaly team will probably pick this\ntopic up later in this release cycle.\n\nOne issue I had was that this patch series starts to use more of the Git\ninfrastructure. Back when the library was introduced that there was some\ndiscussion around whether it should work standalone or not, but if I\nremember correctly the outcome was that it's okay to use internals like\ne.g. `strbuf`. And while things like `read_in_full()` and related are\ntrivial wrappers, this patch series start to hook into the tempfiles\ninterface which I really didn't want to reimplement.\n\nIt's a bit unfortunate that we don't yet have good test coverage as\nthere are no end-to-end tests, and most of the changes I did are not\neasily testable in unit tests. So until the reftable backend gets\nsubmitted you'll have to trust my reasoning as layed out in the commit\nmessages that the changes actually improve things.\n\nPatrick\n\nPatrick Steinhardt (8):\n reftable: wrap EXPECT macros in do/while\n reftable: handle interrupted reads\n reftable: handle interrupted writes\n reftable/stack: verify that `reftable_stack_add()` uses\n auto-compaction\n reftable/stack: perform auto-compaction with transactional interface\n reftable/stack: reuse buffers when reloading stack\n reftable/merged: reuse buffer to compute record keys\n reftable/stack: fix stale lock when dying\n\n reftable/blocksource.c | 2 +-\n reftable/merged.c | 20 ++++----\n reftable/stack.c | 71 ++++++++++----------------\n reftable/stack_test.c | 105 +++++++++++++++++++++++++++++++++++++-\n reftable/test_framework.h | 58 +++++++++++----------\n 5 files changed, 174 insertions(+), 82 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"485026","messageId":"b147296c5805c921fa403135f04c5bc44a8e59c2.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 2/8] reftable: handle interrupted reads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:14Z","receivedAt":"2023-11-21T07:04:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to pread(3P) and read(3P) where we don't properly handle\ninterrupts. Convert them to use `pread_in_full()` and `read_in_full()`,\nrespectively.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 2 +-\n reftable/stack.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 8331b34e82..a1ea304429 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -109,7 +109,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\n \tdest->data = reftable_malloc(size);\n-\tif (pread(b->fd, dest->data, size, off) != size)\n+\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n \t\treturn -1;\n \tdest->len = size;\n \treturn size;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ddbdf1b9c8..ed108a929b 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -92,7 +92,7 @@ static int fd_read_lines(int fd, char ***namesp)\n \t}\n \n \tbuf = reftable_malloc(size + 1);\n-\tif (read(fd, buf, size) != size) {\n+\tif (read_in_full(fd, buf, size) != size) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.42.0\n\n"},{"id":"485027","messageId":"3c14f67c441251d7467757706715ca6d9a4be7c0.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 3/8] reftable: handle interrupted writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:18Z","receivedAt":"2023-11-21T07:04:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to write(3P) where we don't properly handle interrupts.\nConvert them to use `write_in_full()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++---\n reftable/stack_test.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ed108a929b..f0cadad490 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -42,7 +42,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st,\n static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)\n {\n \tint *fdp = (int *)arg;\n-\treturn write(*fdp, data, sz);\n+\treturn write_in_full(*fdp, data, sz);\n }\n \n int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n@@ -554,7 +554,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n@@ -1024,7 +1024,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n \t}\n \n-\terr = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n+\terr = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex d0b717510f..0644c8ad2e 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -78,7 +78,7 @@ static void test_read_file(void)\n \tint i = 0;\n \n \tEXPECT(fd > 0);\n-\tn = write(fd, out, strlen(out));\n+\tn = write_in_full(fd, out, strlen(out));\n \tEXPECT(n == strlen(out));\n \terr = close(fd);\n \tEXPECT(err >= 0);\n-- \n2.42.0\n\n"},{"id":"485028","messageId":"c9b4ac7916d1ee2474ad37eaa21073adcd54fc5d.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 4/8] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:22Z","receivedAt":"2023-11-21T07:04:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While we have several tests that check whether we correctly perform\nauto-compaction when manually calling `reftable_stack_auto_compact()`,\nwe don't have any tests that verify whether `reftable_stack_add()` does\ncall it automatically. Add one.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack_test.c | 47 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 0644c8ad2e..c979d177c2 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -850,6 +850,52 @@ static void test_reftable_stack_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_add_performs_auto_compaction(void)\n+{\n+\tstruct reftable_write_options cfg = { 0 };\n+\tstruct reftable_stack *st = NULL;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err, i, n = 20;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\t\tchar name[100];\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n+\t\tref.refname = name;\n+\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_compaction_concurrent(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -960,6 +1006,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_add);\n \tRUN_TEST(test_reftable_stack_add_one);\n \tRUN_TEST(test_reftable_stack_auto_compaction);\n+\tRUN_TEST(test_reftable_stack_add_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent_clean);\n \tRUN_TEST(test_reftable_stack_hash_id);\n-- \n2.42.0\n\n"},{"id":"485029","messageId":"25522b042cdc5986972cc7b62e6b88be0569d3cb.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 5/8] reftable/stack: perform auto-compaction with transactional interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:26Z","receivedAt":"2023-11-21T07:04:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever updating references or reflog entries in the reftable stack, we\nneed to add a new table to the stack, thus growing the stack's length by\none. It can thus happen quite fast that the stack grows very long, which\nresults in performance issues when trying to read records. But besides\nperformance issues, this can also lead to exhaustion of file descriptors\nvery rapidly as every single table requires a separate descriptor when\nopening the stack.\n\nWhile git-pack-refs(1) fixes this issue for us by merging the tables, it\nruns too irregularly to keep the length of the stack within reasonable\nlimits. This is why the reftable stack has an auto-compaction mechanism:\n`reftable_stack_add()` will call `reftable_stack_auto_compact()` after\nits added the new table, which will auto-compact the stack as required.\n\nBut while this logic works alright for `reftable_stack_add()`, we do not\ndo the same in `reftable_addition_commit()`, which is the transactional\nequivalent to the former function that allows us to write multiple\nupdates to the stack atomically. Consequentially, we will easily run\ninto file descriptor exhaustion in code paths that use many separate\ntransactions like e.g. non-atomic fetches.\n\nFix this issue by calling `reftable_stack_auto_compact()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++++\n reftable/stack_test.c | 56 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f0cadad490..f5d18a842a 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -584,6 +584,12 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tadd->new_tables_len = 0;\n \n \terr = reftable_stack_reload(add->stack);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tif (!add->stack->disable_auto_compact)\n+\t\terr = reftable_stack_auto_compact(add->stack);\n+\n done:\n \treftable_addition_close(add);\n \treturn err;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex c979d177c2..4c2f794c49 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -289,6 +289,61 @@ static void test_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_write_options cfg = {0};\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_stack *st = NULL;\n+\tint i, n = 20, err;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\t\tchar name[100];\n+\n+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n+\t\tref.refname = name;\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\terr = reftable_stack_new_addition(&add, st);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_add(add, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_commit(add);\n+\t\tEXPECT_ERR(err);\n+\n+\t\treftable_addition_destroy(add);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_validate_refname(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -1014,6 +1069,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_log_normalize);\n \tRUN_TEST(test_reftable_stack_tombstone);\n \tRUN_TEST(test_reftable_stack_transaction_api);\n+\tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_validate_refname);\n-- \n2.42.0\n\n"},{"id":"485030","messageId":"54e8fd15d8ab3266b1159045b1029b2a38f447a7.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 6/8] reftable/stack: reuse buffers when reloading stack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:30Z","receivedAt":"2023-11-21T07:04:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_reload_once()` we iterate over all the tables added\nto the stack in order to figure out whether any of the tables needs to\nbe reloaded. We use a set of buffers in this context to compute the\npaths of these tables, but discard those buffers on every iteration.\nThis is quite wasteful given that we do not need to transfer ownership\nof the allocated buffer outside of the loop.\n\nRefactor the code to instead reuse the buffers to reduce the number of\nallocations we need to do.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f5d18a842a..2dd2373360 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -204,6 +204,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \t\treftable_calloc(sizeof(struct reftable_table) * names_len);\n \tint new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n+\tstruct strbuf table_path = STRBUF_INIT;\n \tint i;\n \n \twhile (*names) {\n@@ -223,13 +224,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \n \t\tif (!rd) {\n \t\t\tstruct reftable_block_source src = { NULL };\n-\t\t\tstruct strbuf table_path = STRBUF_INIT;\n \t\t\tstack_filename(&table_path, st, name);\n \n \t\t\terr = reftable_block_source_from_file(&src,\n \t\t\t\t\t\t\t table_path.buf);\n-\t\t\tstrbuf_release(&table_path);\n-\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \n@@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \tfor (i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n-\t\t\tstruct strbuf filename = STRBUF_INIT;\n-\t\t\tstack_filename(&filename, st, name);\n+\t\t\tstack_filename(&table_path, st, name);\n \n \t\t\treader_close(cur[i]);\n \t\t\treftable_reader_free(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n-\t\t\tunlink(filename.buf);\n-\n-\t\t\tstrbuf_release(&filename);\n+\t\t\tunlink(table_path.buf);\n \t\t}\n \t}\n \n@@ -288,6 +283,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \treftable_free(new_readers);\n \treftable_free(new_tables);\n \treftable_free(cur);\n+\tstrbuf_release(&table_path);\n \treturn err;\n }\n \n-- \n2.42.0\n\n"},{"id":"485031","messageId":"23c060d1e21573581ca6c5db50ca756b61078e3e.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 7/8] reftable/merged: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:35Z","receivedAt":"2023-11-21T07:04:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating over entries in the merged iterator's queue, we compute\nthe key of each of the entries and write it into a buffer. We do not\nreuse the buffer though and thus re-allocate it on every iteration,\nwhich is wasteful given that we never transfer ownership of the\nallocated bytes outside of the loop.\n\nRefactor the code to reuse the buffer. This also fixes a potential\nmemory leak when `merged_iter_advance_subiter()` returns an error.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/merged.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 5ded470c08..cd03f6da13 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -85,7 +85,7 @@ static int merged_iter_advance_subiter(struct merged_iter *mi, size_t idx)\n static int merged_iter_next_entry(struct merged_iter *mi,\n \t\t\t\t struct reftable_record *rec)\n {\n-\tstruct strbuf entry_key = STRBUF_INIT;\n+\tstruct strbuf entry_key = STRBUF_INIT, k = STRBUF_INIT;\n \tstruct pq_entry entry = { 0 };\n \tint err = 0;\n \n@@ -108,30 +108,28 @@ static int merged_iter_next_entry(struct merged_iter *mi,\n \treftable_record_key(&entry.rec, &entry_key);\n \twhile (!merged_iter_pqueue_is_empty(mi->pq)) {\n \t\tstruct pq_entry top = merged_iter_pqueue_top(mi->pq);\n-\t\tstruct strbuf k = STRBUF_INIT;\n-\t\tint err = 0, cmp = 0;\n+\t\tint cmp = 0;\n \n \t\treftable_record_key(&top.rec, &k);\n \n \t\tcmp = strbuf_cmp(&k, &entry_key);\n-\t\tstrbuf_release(&k);\n-\n-\t\tif (cmp > 0) {\n+\t\tif (cmp > 0)\n \t\t\tbreak;\n-\t\t}\n \n \t\tmerged_iter_pqueue_remove(&mi->pq);\n \t\terr = merged_iter_advance_subiter(mi, top.index);\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n+\t\tif (err < 0)\n+\t\t\tgoto done;\n \t\treftable_record_release(&top.rec);\n \t}\n \n \treftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));\n+\n+done:\n \treftable_record_release(&entry.rec);\n \tstrbuf_release(&entry_key);\n-\treturn 0;\n+\tstrbuf_release(&k);\n+\treturn err;\n }\n \n static int merged_iter_next(struct merged_iter *mi, struct reftable_record *rec)\n-- \n2.42.0\n\n"},{"id":"485032","messageId":"065c8803ac18633313af264f70c83717c4f6e10c.1700549493.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH 8/8] reftable/stack: fix stale lock when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-21T07:04:39Z","receivedAt":"2023-11-21T07:04:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a transaction via `reftable_stack_init_addition()`, we\ncreate a lockfile for the reftable stack itself which we'll write the\nnew list of tables to. But if we terminate abnormally e.g. via a call to\n`die()`, then we do not remove the lockfile. Subsequent executions of\nGit which try to modify references will thus fail with an out-of-date\nerror.\n\nFix this bug by registering the lock as a `struct tempfile`, which\nensures automatic cleanup for us.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 47 +++++++++++++++--------------------------------\n 1 file changed, 15 insertions(+), 32 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2dd2373360..2f1494aef2 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,6 +17,8 @@ license that can be found in the LICENSE file or at\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n \n+#include \"tempfile.h\"\n+\n static int stack_try_add(struct reftable_stack *st,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -440,8 +442,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n }\n \n struct reftable_addition {\n-\tint lock_file_fd;\n-\tstruct strbuf lock_file_name;\n+\tstruct tempfile *lock_file;\n \tstruct reftable_stack *stack;\n \n \tchar **new_tables;\n@@ -449,24 +450,19 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT \\\n-\t{ \\\n-\t\t.lock_file_name = STRBUF_INIT \\\n-\t}\n+#define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st)\n {\n+\tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err = 0;\n \tadd->stack = st;\n \n-\tstrbuf_reset(&add->lock_file_name);\n-\tstrbuf_addstr(&add->lock_file_name, st->list_file);\n-\tstrbuf_addstr(&add->lock_file_name, \".lock\");\n+\tstrbuf_addf(&lock_file_name, \"%s.lock\", st->list_file);\n \n-\tadd->lock_file_fd = open(add->lock_file_name.buf,\n-\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (add->lock_file_fd < 0) {\n+\tadd->lock_file = create_tempfile(lock_file_name.buf);\n+\tif (!add->lock_file) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n \t\t} else {\n@@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\tgoto done;\n \t}\n \tif (st->config.default_permissions) {\n-\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n+\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n@@ -495,6 +491,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (err) {\n \t\treftable_addition_close(add);\n \t}\n+\tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n \n@@ -512,15 +509,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n \tadd->new_tables = NULL;\n \tadd->new_tables_len = 0;\n \n-\tif (add->lock_file_fd > 0) {\n-\t\tclose(add->lock_file_fd);\n-\t\tadd->lock_file_fd = 0;\n-\t}\n-\tif (add->lock_file_name.len > 0) {\n-\t\tunlink(add->lock_file_name.buf);\n-\t\tstrbuf_release(&add->lock_file_name);\n-\t}\n-\n+\tdelete_tempfile(&add->lock_file);\n \tstrbuf_release(&nm);\n }\n \n@@ -536,8 +525,10 @@ void reftable_addition_destroy(struct reftable_addition *add)\n int reftable_addition_commit(struct reftable_addition *add)\n {\n \tstruct strbuf table_list = STRBUF_INIT;\n+\tint lock_file_fd = get_tempfile_fd(add->lock_file);\n \tint i = 0;\n \tint err = 0;\n+\n \tif (add->new_tables_len == 0)\n \t\tgoto done;\n \n@@ -550,28 +541,20 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = close(add->lock_file_fd);\n-\tadd->lock_file_fd = 0;\n-\tif (err < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = rename(add->lock_file_name.buf, add->stack->list_file);\n+\terr = rename_tempfile(&add->lock_file, add->stack->list_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n \t/* success, no more state to clean up. */\n-\tstrbuf_release(&add->lock_file_name);\n \tfor (i = 0; i < add->new_tables_len; i++) {\n \t\treftable_free(add->new_tables[i]);\n \t}\n-- \n2.42.0\n\n"},{"id":"485460","messageId":"0ebbb02d32e1f1f483c21157fe076c0890665f69.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 01/11] reftable: wrap EXPECT macros in do/while","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:52:57Z","receivedAt":"2023-12-08T14:53:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `EXPECT` macros used by the reftable test framework are all using a\nsingle `if` statement with the actual condition. This results in weird\nsyntax when using them in if/else statements like the following:\n\n```\nif (foo)\n\tEXPECT(foo == 2)\nelse\n\tEXPECT(bar == 2)\n```\n\nNote that there need not be a trailing semicolon. Furthermore, it is not\nimmediately obvious whether the else now belongs to the `if (foo)` or\nwhether it belongs to the expanded `if (foo == 2)` from the macro.\n\nFix this by wrapping the macros in a do/while loop.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/test_framework.h | 58 +++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 26 deletions(-)\n\ndiff --git a/reftable/test_framework.h b/reftable/test_framework.h\nindex 774cb275bf..ee44f735ae 100644\n--- a/reftable/test_framework.h\n+++ b/reftable/test_framework.h\n@@ -12,32 +12,38 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"reftable-error.h\"\n \n-#define EXPECT_ERR(c) \\\n-\tif (c != 0) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n-\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT_STREQ(a, b) \\\n-\tif (strcmp(a, b)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n-\t\t\t__LINE__, #a, a, #b, b); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT(c) \\\n-\tif (!(c)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n-\t\t\t__LINE__, #c); \\\n-\t\tabort(); \\\n-\t}\n+#define EXPECT_ERR(c) \\\n+\tdo { \\\n+\t\tif (c != 0) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n+\t\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT_STREQ(a, b) \\\n+\tdo { \\\n+\t\tif (strcmp(a, b)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #a, a, #b, b); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT(c) \\\n+\tdo { \\\n+\t\tif (!(c)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #c); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n \n #define RUN_TEST(f) \\\n \tfprintf(stderr, \"running %s\\n\", #f); \\\n-- \n2.43.0\n\n"},{"id":"485462","messageId":"cover.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH v2 00/11] reftable: small set of fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:52:53Z","receivedAt":"2023-12-08T14:53:01Z","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 addresses several\nsmallish issues in the reftable backend. Given that the first version\ndidn't receive any reviews yet I decided to squash in additional\nfindings into this series. This is both to reduce the number of follow\nup series, but also to hopefully push this topic onto the radar of folks\non the mailing list.\n\nChanges compared to v1:\n\n - Allocations were optimized further for `struct merged_iter` by\n making the buffers part of the structure itself so that they can\n be reused across iterations.\n\n - Allocations were optimized for `struct block_iter` in the same\n way.\n\n - Temporary stacks have a supposedly-random suffix so that concurrent\n writers don't conflict with each other. We used unseeded `rand()`\n calls for it though, so they weren't random after all. This is fixed\n by converting to use `git_rand()` instead.\n\nPatrick\n\nPatrick Steinhardt (11):\n reftable: wrap EXPECT macros in do/while\n reftable: handle interrupted reads\n reftable: handle interrupted writes\n reftable/stack: verify that `reftable_stack_add()` uses\n auto-compaction\n reftable/stack: perform auto-compaction with transactional interface\n reftable/stack: reuse buffers when reloading stack\n reftable/stack: fix stale lock when dying\n reftable/stack: fix use of unseeded randomness\n reftable/merged: reuse buffer to compute record keys\n reftable/block: introduce macro to initialize `struct block_iter`\n reftable/block: reuse buffer to compute record keys\n\n reftable/block.c | 23 ++++-----\n reftable/block.h | 6 +++\n reftable/block_test.c | 4 +-\n reftable/blocksource.c | 2 +-\n reftable/iter.h | 8 +--\n reftable/merged.c | 31 +++++------\n reftable/merged.h | 2 +\n reftable/reader.c | 7 ++-\n reftable/readwrite_test.c | 6 +--\n reftable/stack.c | 73 +++++++++++---------------\n reftable/stack_test.c | 105 +++++++++++++++++++++++++++++++++++++-\n reftable/test_framework.h | 58 +++++++++++----------\n 12 files changed, 211 insertions(+), 114 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"485463","messageId":"b404fdf066e802328bcbcefeb9da7c996738f840.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 02/11] reftable: handle interrupted reads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:02Z","receivedAt":"2023-12-08T14:53:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to pread(3P) and read(3P) where we don't properly handle\ninterrupts. Convert them to use `pread_in_full()` and `read_in_full()`,\nrespectively.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 2 +-\n reftable/stack.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 8331b34e82..a1ea304429 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -109,7 +109,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\n \tdest->data = reftable_malloc(size);\n-\tif (pread(b->fd, dest->data, size, off) != size)\n+\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n \t\treturn -1;\n \tdest->len = size;\n \treturn size;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ddbdf1b9c8..ed108a929b 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -92,7 +92,7 @@ static int fd_read_lines(int fd, char ***namesp)\n \t}\n \n \tbuf = reftable_malloc(size + 1);\n-\tif (read(fd, buf, size) != size) {\n+\tif (read_in_full(fd, buf, size) != size) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.43.0\n\n"},{"id":"485464","messageId":"8c1d78b12b5b8d7c4770e627790336c442aef665.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 03/11] reftable: handle interrupted writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:06Z","receivedAt":"2023-12-08T14:53:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to write(3P) where we don't properly handle interrupts.\nConvert them to use `write_in_full()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++---\n reftable/stack_test.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ed108a929b..f0cadad490 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -42,7 +42,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st,\n static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)\n {\n \tint *fdp = (int *)arg;\n-\treturn write(*fdp, data, sz);\n+\treturn write_in_full(*fdp, data, sz);\n }\n \n int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n@@ -554,7 +554,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n@@ -1024,7 +1024,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n \t}\n \n-\terr = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n+\terr = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex d0b717510f..0644c8ad2e 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -78,7 +78,7 @@ static void test_read_file(void)\n \tint i = 0;\n \n \tEXPECT(fd > 0);\n-\tn = write(fd, out, strlen(out));\n+\tn = write_in_full(fd, out, strlen(out));\n \tEXPECT(n == strlen(out));\n \terr = close(fd);\n \tEXPECT(err >= 0);\n-- \n2.43.0\n\n"},{"id":"485461","messageId":"8061b9d2fcb3e8c3d1fd641e705b9a8879e452f4.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:10Z","receivedAt":"2023-12-08T14:53:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While we have several tests that check whether we correctly perform\nauto-compaction when manually calling `reftable_stack_auto_compact()`,\nwe don't have any tests that verify whether `reftable_stack_add()` does\ncall it automatically. Add one.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack_test.c | 47 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 0644c8ad2e..c979d177c2 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -850,6 +850,52 @@ static void test_reftable_stack_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_add_performs_auto_compaction(void)\n+{\n+\tstruct reftable_write_options cfg = { 0 };\n+\tstruct reftable_stack *st = NULL;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err, i, n = 20;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\t\tchar name[100];\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n+\t\tref.refname = name;\n+\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_compaction_concurrent(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -960,6 +1006,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_add);\n \tRUN_TEST(test_reftable_stack_add_one);\n \tRUN_TEST(test_reftable_stack_auto_compaction);\n+\tRUN_TEST(test_reftable_stack_add_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent_clean);\n \tRUN_TEST(test_reftable_stack_hash_id);\n-- \n2.43.0\n\n"},{"id":"485465","messageId":"77b9ae8aa675dd96dd10f4a5369f1f994fa59939.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 05/11] reftable/stack: perform auto-compaction with transactional interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:14Z","receivedAt":"2023-12-08T14:53:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever updating references or reflog entries in the reftable stack, we\nneed to add a new table to the stack, thus growing the stack's length by\none. It can thus happen quite fast that the stack grows very long, which\nresults in performance issues when trying to read records. But besides\nperformance issues, this can also lead to exhaustion of file descriptors\nvery rapidly as every single table requires a separate descriptor when\nopening the stack.\n\nWhile git-pack-refs(1) fixes this issue for us by merging the tables, it\nruns too irregularly to keep the length of the stack within reasonable\nlimits. This is why the reftable stack has an auto-compaction mechanism:\n`reftable_stack_add()` will call `reftable_stack_auto_compact()` after\nits added the new table, which will auto-compact the stack as required.\n\nBut while this logic works alright for `reftable_stack_add()`, we do not\ndo the same in `reftable_addition_commit()`, which is the transactional\nequivalent to the former function that allows us to write multiple\nupdates to the stack atomically. Consequentially, we will easily run\ninto file descriptor exhaustion in code paths that use many separate\ntransactions like e.g. non-atomic fetches.\n\nFix this issue by calling `reftable_stack_auto_compact()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++++\n reftable/stack_test.c | 56 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f0cadad490..f5d18a842a 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -584,6 +584,12 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tadd->new_tables_len = 0;\n \n \terr = reftable_stack_reload(add->stack);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tif (!add->stack->disable_auto_compact)\n+\t\terr = reftable_stack_auto_compact(add->stack);\n+\n done:\n \treftable_addition_close(add);\n \treturn err;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex c979d177c2..4c2f794c49 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -289,6 +289,61 @@ static void test_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_write_options cfg = {0};\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_stack *st = NULL;\n+\tint i, n = 20, err;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\t\tchar name[100];\n+\n+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n+\t\tref.refname = name;\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\terr = reftable_stack_new_addition(&add, st);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_add(add, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_commit(add);\n+\t\tEXPECT_ERR(err);\n+\n+\t\treftable_addition_destroy(add);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_validate_refname(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -1014,6 +1069,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_log_normalize);\n \tRUN_TEST(test_reftable_stack_tombstone);\n \tRUN_TEST(test_reftable_stack_transaction_api);\n+\tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_validate_refname);\n-- \n2.43.0\n\n"},{"id":"485466","messageId":"f797feff8dec383f1db9ae403cd89b80d1743432.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 06/11] reftable/stack: reuse buffers when reloading stack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:18Z","receivedAt":"2023-12-08T14:53:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_reload_once()` we iterate over all the tables added\nto the stack in order to figure out whether any of the tables needs to\nbe reloaded. We use a set of buffers in this context to compute the\npaths of these tables, but discard those buffers on every iteration.\nThis is quite wasteful given that we do not need to transfer ownership\nof the allocated buffer outside of the loop.\n\nRefactor the code to instead reuse the buffers to reduce the number of\nallocations we need to do.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f5d18a842a..2dd2373360 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -204,6 +204,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \t\treftable_calloc(sizeof(struct reftable_table) * names_len);\n \tint new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n+\tstruct strbuf table_path = STRBUF_INIT;\n \tint i;\n \n \twhile (*names) {\n@@ -223,13 +224,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \n \t\tif (!rd) {\n \t\t\tstruct reftable_block_source src = { NULL };\n-\t\t\tstruct strbuf table_path = STRBUF_INIT;\n \t\t\tstack_filename(&table_path, st, name);\n \n \t\t\terr = reftable_block_source_from_file(&src,\n \t\t\t\t\t\t\t table_path.buf);\n-\t\t\tstrbuf_release(&table_path);\n-\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \n@@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \tfor (i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n-\t\t\tstruct strbuf filename = STRBUF_INIT;\n-\t\t\tstack_filename(&filename, st, name);\n+\t\t\tstack_filename(&table_path, st, name);\n \n \t\t\treader_close(cur[i]);\n \t\t\treftable_reader_free(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n-\t\t\tunlink(filename.buf);\n-\n-\t\t\tstrbuf_release(&filename);\n+\t\t\tunlink(table_path.buf);\n \t\t}\n \t}\n \n@@ -288,6 +283,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \treftable_free(new_readers);\n \treftable_free(new_tables);\n \treftable_free(cur);\n+\tstrbuf_release(&table_path);\n \treturn err;\n }\n \n-- \n2.43.0\n\n"},{"id":"485467","messageId":"e82a68aecd0a1179df3a59755864c71995e979d3.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 07/11] reftable/stack: fix stale lock when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:23Z","receivedAt":"2023-12-08T14:53:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a transaction via `reftable_stack_init_addition()`, we\ncreate a lockfile for the reftable stack itself which we'll write the\nnew list of tables to. But if we terminate abnormally e.g. via a call to\n`die()`, then we do not remove the lockfile. Subsequent executions of\nGit which try to modify references will thus fail with an out-of-date\nerror.\n\nFix this bug by registering the lock as a `struct tempfile`, which\nensures automatic cleanup for us.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 47 +++++++++++++++--------------------------------\n 1 file changed, 15 insertions(+), 32 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2dd2373360..2f1494aef2 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,6 +17,8 @@ license that can be found in the LICENSE file or at\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n \n+#include \"tempfile.h\"\n+\n static int stack_try_add(struct reftable_stack *st,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -440,8 +442,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n }\n \n struct reftable_addition {\n-\tint lock_file_fd;\n-\tstruct strbuf lock_file_name;\n+\tstruct tempfile *lock_file;\n \tstruct reftable_stack *stack;\n \n \tchar **new_tables;\n@@ -449,24 +450,19 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT \\\n-\t{ \\\n-\t\t.lock_file_name = STRBUF_INIT \\\n-\t}\n+#define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st)\n {\n+\tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err = 0;\n \tadd->stack = st;\n \n-\tstrbuf_reset(&add->lock_file_name);\n-\tstrbuf_addstr(&add->lock_file_name, st->list_file);\n-\tstrbuf_addstr(&add->lock_file_name, \".lock\");\n+\tstrbuf_addf(&lock_file_name, \"%s.lock\", st->list_file);\n \n-\tadd->lock_file_fd = open(add->lock_file_name.buf,\n-\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (add->lock_file_fd < 0) {\n+\tadd->lock_file = create_tempfile(lock_file_name.buf);\n+\tif (!add->lock_file) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n \t\t} else {\n@@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\tgoto done;\n \t}\n \tif (st->config.default_permissions) {\n-\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n+\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n@@ -495,6 +491,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (err) {\n \t\treftable_addition_close(add);\n \t}\n+\tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n \n@@ -512,15 +509,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n \tadd->new_tables = NULL;\n \tadd->new_tables_len = 0;\n \n-\tif (add->lock_file_fd > 0) {\n-\t\tclose(add->lock_file_fd);\n-\t\tadd->lock_file_fd = 0;\n-\t}\n-\tif (add->lock_file_name.len > 0) {\n-\t\tunlink(add->lock_file_name.buf);\n-\t\tstrbuf_release(&add->lock_file_name);\n-\t}\n-\n+\tdelete_tempfile(&add->lock_file);\n \tstrbuf_release(&nm);\n }\n \n@@ -536,8 +525,10 @@ void reftable_addition_destroy(struct reftable_addition *add)\n int reftable_addition_commit(struct reftable_addition *add)\n {\n \tstruct strbuf table_list = STRBUF_INIT;\n+\tint lock_file_fd = get_tempfile_fd(add->lock_file);\n \tint i = 0;\n \tint err = 0;\n+\n \tif (add->new_tables_len == 0)\n \t\tgoto done;\n \n@@ -550,28 +541,20 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = close(add->lock_file_fd);\n-\tadd->lock_file_fd = 0;\n-\tif (err < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = rename(add->lock_file_name.buf, add->stack->list_file);\n+\terr = rename_tempfile(&add->lock_file, add->stack->list_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n \t/* success, no more state to clean up. */\n-\tstrbuf_release(&add->lock_file_name);\n \tfor (i = 0; i < add->new_tables_len; i++) {\n \t\treftable_free(add->new_tables[i]);\n \t}\n-- \n2.43.0\n\n"},{"id":"485468","messageId":"bab4fb93df8d1a620eefeef99a49ea52c98dfc6e.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 08/11] reftable/stack: fix use of unseeded randomness","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:27Z","receivedAt":"2023-12-08T14:53:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing a new reftable stack, Git will first create the stack with\na random suffix so that concurrent updates will not try to write to the\nsame file. This random suffix is computed via a call to rand(3P). But we\nnever seed the function via srand(3P), which means that the suffix is in\nfact always the same.\n\nFix this bug by using `git_rand()` instead, which does not need to be\ninitialized. While this function is likely going to be slower depending\non the platform, this slowness should not matter in practice as we only\nuse it when writing a new reftable stack.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/readwrite_test.c | 6 +++---\n reftable/stack.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 469ab79a5a..278663f22d 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -141,8 +141,8 @@ static void test_log_buffer_size(void)\n \t*/\n \tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n \tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n-\t\thash1[i] = (uint8_t)(rand() % 256);\n-\t\thash2[i] = (uint8_t)(rand() % 256);\n+\t\thash1[i] = (uint8_t)(git_rand() % 256);\n+\t\thash2[i] = (uint8_t)(git_rand() % 256);\n \t}\n \tlog.value.update.old_hash = hash1;\n \tlog.value.update.new_hash = hash2;\n@@ -320,7 +320,7 @@ static void test_log_zlib_corruption(void)\n \t};\n \n \tfor (i = 0; i < sizeof(message) - 1; i++)\n-\t\tmessage[i] = (uint8_t)(rand() % 64 + ' ');\n+\t\tmessage[i] = (uint8_t)(git_rand() % 64 + ' ');\n \n \treftable_writer_set_limits(w, 1, 1);\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2f1494aef2..95963f67a2 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -434,7 +434,7 @@ int reftable_stack_add(struct reftable_stack *st,\n static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n {\n \tchar buf[100];\n-\tuint32_t rnd = (uint32_t)rand();\n+\tuint32_t rnd = (uint32_t)git_rand();\n \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n \t\t min, max, rnd);\n \tstrbuf_reset(dest);\n-- \n2.43.0\n\n"},{"id":"485469","messageId":"cbf77ec45a26c215884d26a4e249adeeaf634f3d.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 09/11] reftable/merged: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:31Z","receivedAt":"2023-12-08T14:53:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating over entries in the merged iterator's queue, we compute\nthe key of each of the entries and write it into a buffer. We do not\nreuse the buffer though and thus re-allocate it on every iteration,\nwhich is wasteful given that we never transfer ownership of the\nallocated bytes outside of the loop.\n\nRefactor the code to reuse the buffer. This also fixes a potential\nmemory leak when `merged_iter_advance_subiter()` returns an error.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/merged.c | 31 ++++++++++++++++---------------\n reftable/merged.h | 2 ++\n 2 files changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 5ded470c08..556bb5c556 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -52,6 +52,8 @@ static void merged_iter_close(void *p)\n \t\treftable_iterator_destroy(&mi->stack[i]);\n \t}\n \treftable_free(mi->stack);\n+\tstrbuf_release(&mi->key);\n+\tstrbuf_release(&mi->entry_key);\n }\n \n static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,\n@@ -85,7 +87,6 @@ static int merged_iter_advance_subiter(struct merged_iter *mi, size_t idx)\n static int merged_iter_next_entry(struct merged_iter *mi,\n \t\t\t\t struct reftable_record *rec)\n {\n-\tstruct strbuf entry_key = STRBUF_INIT;\n \tstruct pq_entry entry = { 0 };\n \tint err = 0;\n \n@@ -105,33 +106,31 @@ static int merged_iter_next_entry(struct merged_iter *mi,\n \t such a deployment, the loop below must be changed to collect all\n \t entries for the same key, and return new the newest one.\n \t*/\n-\treftable_record_key(&entry.rec, &entry_key);\n+\treftable_record_key(&entry.rec, &mi->entry_key);\n \twhile (!merged_iter_pqueue_is_empty(mi->pq)) {\n \t\tstruct pq_entry top = merged_iter_pqueue_top(mi->pq);\n-\t\tstruct strbuf k = STRBUF_INIT;\n-\t\tint err = 0, cmp = 0;\n+\t\tint cmp = 0;\n \n-\t\treftable_record_key(&top.rec, &k);\n+\t\treftable_record_key(&top.rec, &mi->key);\n \n-\t\tcmp = strbuf_cmp(&k, &entry_key);\n-\t\tstrbuf_release(&k);\n-\n-\t\tif (cmp > 0) {\n+\t\tcmp = strbuf_cmp(&mi->key, &mi->entry_key);\n+\t\tif (cmp > 0)\n \t\t\tbreak;\n-\t\t}\n \n \t\tmerged_iter_pqueue_remove(&mi->pq);\n \t\terr = merged_iter_advance_subiter(mi, top.index);\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n+\t\tif (err < 0)\n+\t\t\tgoto done;\n \t\treftable_record_release(&top.rec);\n \t}\n \n \treftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));\n+\n+done:\n \treftable_record_release(&entry.rec);\n-\tstrbuf_release(&entry_key);\n-\treturn 0;\n+\tstrbuf_release(&mi->entry_key);\n+\tstrbuf_release(&mi->key);\n+\treturn err;\n }\n \n static int merged_iter_next(struct merged_iter *mi, struct reftable_record *rec)\n@@ -248,6 +247,8 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,\n \t\t.typ = reftable_record_type(rec),\n \t\t.hash_id = mt->hash_id,\n \t\t.suppress_deletions = mt->suppress_deletions,\n+\t\t.key = STRBUF_INIT,\n+\t\t.entry_key = STRBUF_INIT,\n \t};\n \tint n = 0;\n \tint err = 0;\ndiff --git a/reftable/merged.h b/reftable/merged.h\nindex 7d9f95d27e..d5b39dfe7f 100644\n--- a/reftable/merged.h\n+++ b/reftable/merged.h\n@@ -31,6 +31,8 @@ struct merged_iter {\n \tuint8_t typ;\n \tint suppress_deletions;\n \tstruct merged_iter_pqueue pq;\n+\tstruct strbuf key;\n+\tstruct strbuf entry_key;\n };\n \n void merged_table_release(struct reftable_merged_table *mt);\n-- \n2.43.0\n\n"},{"id":"485470","messageId":"c9a1405a9a1311d21a20c041b105681411660591.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 10/11] reftable/block: introduce macro to initialize `struct block_iter`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:35Z","receivedAt":"2023-12-08T14:53:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are a bunch of locations where we initialize members of `struct\nblock_iter`, which makes it harder than necessary to expand this struct\nto have additional members. Unify the logic via a new `BLOCK_ITER_INIT`\nmacro that initializes all members.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 4 +---\n reftable/block.h | 4 ++++\n reftable/block_test.c | 4 ++--\n reftable/iter.h | 8 ++++----\n reftable/reader.c | 7 +++----\n 5 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 34d4d07369..8c6a8c77fc 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -389,9 +389,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \tstruct reftable_record rec = reftable_new_record(block_reader_type(br));\n \tstruct strbuf key = STRBUF_INIT;\n \tint err = 0;\n-\tstruct block_iter next = {\n-\t\t.last_key = STRBUF_INIT,\n-\t};\n+\tstruct block_iter next = BLOCK_ITER_INIT;\n \n \tint i = binsearch(br->restart_count, &restart_key_less, &args);\n \tif (args.error) {\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 87c77539b5..51699af233 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -86,6 +86,10 @@ struct block_iter {\n \tstruct strbuf last_key;\n };\n \n+#define BLOCK_ITER_INIT { \\\n+\t.last_key = STRBUF_INIT, \\\n+}\n+\n /* initializes a block reader. */\n int block_reader_init(struct block_reader *br, struct reftable_block *bl,\n \t\t uint32_t header_off, uint32_t table_block_size,\ndiff --git a/reftable/block_test.c b/reftable/block_test.c\nindex cb88af4a56..c00bbc8aed 100644\n--- a/reftable/block_test.c\n+++ b/reftable/block_test.c\n@@ -32,7 +32,7 @@ static void test_block_read_write(void)\n \tint i = 0;\n \tint n;\n \tstruct block_reader br = { 0 };\n-\tstruct block_iter it = { .last_key = STRBUF_INIT };\n+\tstruct block_iter it = BLOCK_ITER_INIT;\n \tint j = 0;\n \tstruct strbuf want = STRBUF_INIT;\n \n@@ -87,7 +87,7 @@ static void test_block_read_write(void)\n \tblock_iter_close(&it);\n \n \tfor (i = 0; i < N; i++) {\n-\t\tstruct block_iter it = { .last_key = STRBUF_INIT };\n+\t\tstruct block_iter it = BLOCK_ITER_INIT;\n \t\tstrbuf_reset(&want);\n \t\tstrbuf_addstr(&want, names[i]);\n \ndiff --git a/reftable/iter.h b/reftable/iter.h\nindex 09eb0cbfa5..47d67d84df 100644\n--- a/reftable/iter.h\n+++ b/reftable/iter.h\n@@ -53,10 +53,10 @@ struct indexed_table_ref_iter {\n \tint is_finished;\n };\n \n-#define INDEXED_TABLE_REF_ITER_INIT \\\n-\t{ \\\n-\t\t.cur = { .last_key = STRBUF_INIT }, .oid = STRBUF_INIT, \\\n-\t}\n+#define INDEXED_TABLE_REF_ITER_INIT { \\\n+\t.cur = BLOCK_ITER_INIT, \\\n+\t.oid = STRBUF_INIT, \\\n+}\n \n void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it,\n \t\t\t\t\t struct indexed_table_ref_iter *itr);\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex b4db23ce18..9de64f50b4 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -224,10 +224,9 @@ struct table_iter {\n \tstruct block_iter bi;\n \tint is_finished;\n };\n-#define TABLE_ITER_INIT \\\n-\t{ \\\n-\t\t.bi = {.last_key = STRBUF_INIT } \\\n-\t}\n+#define TABLE_ITER_INIT { \\\n+\t.bi = BLOCK_ITER_INIT \\\n+}\n \n static void table_iter_copy_from(struct table_iter *dest,\n \t\t\t\t struct table_iter *src)\n-- \n2.43.0\n\n"},{"id":"485471","messageId":"02b11f3a80608ba8748a0d0e2294f432e02464e5.1702047081.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"[PATCH v2 11/11] reftable/block: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T14:53:40Z","receivedAt":"2023-12-08T14:53:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating over entries in the block iterator we compute the key of\neach of the entries and write it into a buffer. We do not reuse the\nbuffer though and thus re-allocate it on every iteration, which is\nwasteful.\n\nRefactor the code to reuse the buffer.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 19 ++++++++-----------\n reftable/block.h | 2 ++\n 2 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 8c6a8c77fc..1df3d8a0f0 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -323,30 +323,28 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)\n \t\t.len = it->br->block_len - it->next_off,\n \t};\n \tstruct string_view start = in;\n-\tstruct strbuf key = STRBUF_INIT;\n \tuint8_t extra = 0;\n \tint n = 0;\n \n \tif (it->next_off >= it->br->block_len)\n \t\treturn 1;\n \n-\tn = reftable_decode_key(&key, &extra, it->last_key, in);\n+\tn = reftable_decode_key(&it->key, &extra, it->last_key, in);\n \tif (n < 0)\n \t\treturn -1;\n \n-\tif (!key.len)\n+\tif (!it->key.len)\n \t\treturn REFTABLE_FORMAT_ERROR;\n \n \tstring_view_consume(&in, n);\n-\tn = reftable_record_decode(rec, key, extra, in, it->br->hash_size);\n+\tn = reftable_record_decode(rec, it->key, extra, in, it->br->hash_size);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&in, n);\n \n \tstrbuf_reset(&it->last_key);\n-\tstrbuf_addbuf(&it->last_key, &key);\n+\tstrbuf_addbuf(&it->last_key, &it->key);\n \tit->next_off += start.len - in.len;\n-\tstrbuf_release(&key);\n \treturn 0;\n }\n \n@@ -377,6 +375,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)\n void block_iter_close(struct block_iter *it)\n {\n \tstrbuf_release(&it->last_key);\n+\tstrbuf_release(&it->key);\n }\n \n int block_reader_seek(struct block_reader *br, struct block_iter *it,\n@@ -387,7 +386,6 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t\t.r = br,\n \t};\n \tstruct reftable_record rec = reftable_new_record(block_reader_type(br));\n-\tstruct strbuf key = STRBUF_INIT;\n \tint err = 0;\n \tstruct block_iter next = BLOCK_ITER_INIT;\n \n@@ -414,8 +412,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t\tif (err < 0)\n \t\t\tgoto done;\n \n-\t\treftable_record_key(&rec, &key);\n-\t\tif (err > 0 || strbuf_cmp(&key, want) >= 0) {\n+\t\treftable_record_key(&rec, &it->key);\n+\t\tif (err > 0 || strbuf_cmp(&it->key, want) >= 0) {\n \t\t\terr = 0;\n \t\t\tgoto done;\n \t\t}\n@@ -424,8 +422,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t}\n \n done:\n-\tstrbuf_release(&key);\n-\tstrbuf_release(&next.last_key);\n+\tblock_iter_close(&next);\n \treftable_record_release(&rec);\n \n \treturn err;\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 51699af233..17481e6331 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -84,10 +84,12 @@ struct block_iter {\n \n \t/* key for last entry we read. */\n \tstruct strbuf last_key;\n+\tstruct strbuf key;\n };\n \n #define BLOCK_ITER_INIT { \\\n \t.last_key = STRBUF_INIT, \\\n+\t.key = STRBUF_INIT, \\\n }\n \n /* initializes a block reader. */\n-- \n2.43.0\n\n"},{"id":"485483","messageId":"ZXOK2o0jTgLmxPWZ@nand.local","threadId":"60541","inReplyTo":"b404fdf066e802328bcbcefeb9da7c996738f840.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 02/11] reftable: handle interrupted reads","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T21:30:02Z","receivedAt":"2023-12-08T21:30:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:53:02PM +0100, Patrick Steinhardt wrote:\n> There are calls to pread(3P) and read(3P) where we don't properly handle\n> interrupts. Convert them to use `pread_in_full()` and `read_in_full()`,\n\nJust checking... do you mean \"interrupt\" in the kernel sense? Or are you\nreferring to the possibility of short reads/writes (in later patches)?\n\nThanks,\nTaylor\n"},{"id":"485484","messageId":"ZXOML2pcqVnVo0oX@nand.local","threadId":"60541","inReplyTo":"8061b9d2fcb3e8c3d1fd641e705b9a8879e452f4.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T21:35:43Z","receivedAt":"2023-12-08T21:35:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote:\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index 0644c8ad2e..c979d177c2 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -850,6 +850,52 @@ static void test_reftable_stack_auto_compaction(void)\n> \tclear_dir(dir);\n> }\n>\n> +static void test_reftable_stack_add_performs_auto_compaction(void)\n> +{\n> +\tstruct reftable_write_options cfg = { 0 };\n> +\tstruct reftable_stack *st = NULL;\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tint err, i, n = 20;\n> +\n> +\terr = reftable_new_stack(&st, dir, cfg);\n> +\tEXPECT_ERR(err);\n> +\n> +\tfor (i = 0; i <= n; i++) {\n> +\t\tstruct reftable_ref_record ref = {\n> +\t\t\t.update_index = reftable_stack_next_update_index(st),\n> +\t\t\t.value_type = REFTABLE_REF_SYMREF,\n> +\t\t\t.value.symref = \"master\",\n> +\t\t};\n> +\t\tchar name[100];\n> +\n> +\t\t/*\n> +\t\t * Disable auto-compaction for all but the last runs. Like this\n> +\t\t * we can ensure that we indeed honor this setting and have\n> +\t\t * better control over when exactly auto compaction runs.\n> +\t\t */\n> +\t\tst->disable_auto_compact = i != n;\n> +\n> +\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n> +\t\tref.refname = name;\n\nIs there a reason that we have to use snprintf() here and not a strbuf?\n\nI would have expected to see something like:\n\n struct strbuf buf = STRBUF_INIT;\n /* ... */\n strbuf_addf(&buf, \"branch%04d\", i);\n ref.refname = strbuf_detach(&buf, NULL);\n\nI guess it doesn't matter too much, but I think if we can avoid using\nsnprintf(), it's worth doing. If we must use snprintf() here, we should\nprobably use Git's xsnprintf() instead.\n\n> +\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n> +\t\tEXPECT_ERR(err);\n> +\n> +\t\t/*\n> +\t\t * The stack length should grow continuously for all runs where\n> +\t\t * auto compaction is disabled. When enabled, we should merge\n> +\t\t * all tables in the stack.\n> +\t\t */\n> +\t\tif (i != n)\n> +\t\t\tEXPECT(st->merged->stack_len == i + 1);\n> +\t\telse\n> +\t\t\tEXPECT(st->merged->stack_len == 1);\n\nYou could shorten this to\n\n EXPECT(st->merged->stack_len == (i == n ? 1 : i + 1);\n\nBut I like the version that you wrote here better, because it clearly\nindicates when we should and should not perform compaction.\n\nThanks,\nTaylor\n"},{"id":"485485","messageId":"ZXOVVWGyUtrGWYMB@nand.local","threadId":"60541","inReplyTo":"77b9ae8aa675dd96dd10f4a5369f1f994fa59939.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 05/11] reftable/stack: perform auto-compaction with transactional interface","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:14:45Z","receivedAt":"2023-12-08T22:14:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:53:14PM +0100, Patrick Steinhardt wrote:\n> [...] It can thus happen quite fast that the stack grows very long, which\n> results in performance issues when trying to read records.\n\nThis sentence was a little confusing to read at first glance. Perhaps\ninstead:\n\n The stack can grow to become quite long rather quickly, leading to\n performance issues when trying to read records.\n\n> But besides\n> performance issues, this can also lead to exhaustion of file descriptors\n> very rapidly as every single table requires a separate descriptor when\n> opening the stack.\n\nWell explained, thanks. Everything else here looks good to me.\n\nThanks,\nTaylor\n"},{"id":"485486","messageId":"ZXOV8TCqaH0xXRnS@nand.local","threadId":"60541","inReplyTo":"f797feff8dec383f1db9ae403cd89b80d1743432.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 06/11] reftable/stack: reuse buffers when reloading stack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:17:21Z","receivedAt":"2023-12-08T22:17:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:53:18PM +0100, Patrick Steinhardt wrote:\n> In `reftable_stack_reload_once()` we iterate over all the tables added\n> to the stack in order to figure out whether any of the tables needs to\n> be reloaded. We use a set of buffers in this context to compute the\n> paths of these tables, but discard those buffers on every iteration.\n> This is quite wasteful given that we do not need to transfer ownership\n> of the allocated buffer outside of the loop.\n>\n> Refactor the code to instead reuse the buffers to reduce the number of\n> allocations we need to do.\n\n> @@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n> \tfor (i = 0; i < cur_len; i++) {\n> \t\tif (cur[i]) {\n> \t\t\tconst char *name = reader_name(cur[i]);\n> -\t\t\tstruct strbuf filename = STRBUF_INIT;\n> -\t\t\tstack_filename(&filename, st, name);\n> +\t\t\tstack_filename(&table_path, st, name);\n\nThis initially caught me by surprise, but on closer inspection I agree\nthat this is OK, since stack_filename() calls strbuf_reset() before\nadjusting the buffer contents.\n\n(As a side-note, I do find the side-effect of stack_filename() to be a\nlittle surprising, but that's not the fault of this series and not worth\nchanging here.)\n\nThanks,\nTaylor\n"},{"id":"485487","messageId":"ZXOXpQstse8CdI7J@nand.local","threadId":"60541","inReplyTo":"e82a68aecd0a1179df3a59755864c71995e979d3.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 07/11] reftable/stack: fix stale lock when dying","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:24:37Z","receivedAt":"2023-12-08T22:24:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:53:23PM +0100, Patrick Steinhardt wrote:\n> When starting a transaction via `reftable_stack_init_addition()`, we\n> create a lockfile for the reftable stack itself which we'll write the\n> new list of tables to. But if we terminate abnormally e.g. via a call to\n> `die()`, then we do not remove the lockfile. Subsequent executions of\n> Git which try to modify references will thus fail with an out-of-date\n> error.\n>\n> Fix this bug by registering the lock as a `struct tempfile`, which\n> ensures automatic cleanup for us.\n\nMakes sense.\n\n> @@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> \t\tgoto done;\n> \t}\n> \tif (st->config.default_permissions) {\n> -\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n> +\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n\nHmm. Would we want to use add->lock_file->filename.buf here instead? I\ndon't think that it matters (other than that the lockfile's pathname is\nabsolute). But it arguably makes it clearer to readers that we're\ntouching calling chmod() on the lockfile here, and hardens us against\nthe contents of the temporary strbuf changing.\n\nThanks,\nTaylor\n"},{"id":"485488","messageId":"ZXOYMhlAdIP32l1O@nand.local","threadId":"60541","inReplyTo":"cover.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 00/11] reftable: small set of fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:26:58Z","receivedAt":"2023-12-08T22:27:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 08, 2023 at 03:52:53PM +0100, Patrick Steinhardt wrote:\n> reftable/block.c | 23 ++++-----\n> reftable/block.h | 6 +++\n> reftable/block_test.c | 4 +-\n> reftable/blocksource.c | 2 +-\n> reftable/iter.h | 8 +--\n> reftable/merged.c | 31 +++++------\n> reftable/merged.h | 2 +\n> reftable/reader.c | 7 ++-\n> reftable/readwrite_test.c | 6 +--\n> reftable/stack.c | 73 +++++++++++---------------\n> reftable/stack_test.c | 105 +++++++++++++++++++++++++++++++++++++-\n> reftable/test_framework.h | 58 +++++++++++----------\n> 12 files changed, 211 insertions(+), 114 deletions(-)\n\nThis all looks good to me. I gave this a pretty careful read and added a\ncouple of minor suggestions, but nothing that would indicate the need\nfor a re-roll.\n\nThanks for working on this!\n\nThanks,\nTaylor\n"},{"id":"485498","messageId":"CAPig+cRGZvyhSs9=3-tkBKRZDjDUsb-VDs+dzOaZof__qyBjbA@mail.gmail.com","threadId":"60541","inReplyTo":"ZXOML2pcqVnVo0oX@nand.local","subject":"Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-08T23:46:33Z","receivedAt":"2023-12-08T23:46:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau wrote:\n> On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote:\n> > +static void test_reftable_stack_add_performs_auto_compaction(void)\n> > +{\n> > + char name[100];\n> > + snprintf(name, sizeof(name), \"branch%04d\", i);\n> > + ref.refname = name;\n>\n> Is there a reason that we have to use snprintf() here and not a strbuf?\n>\n> I would have expected to see something like:\n>\n> struct strbuf buf = STRBUF_INIT;\n> /* ... */\n> strbuf_addf(&buf, \"branch%04d\", i);\n> ref.refname = strbuf_detach(&buf, NULL);\n\nIf I'm reading the code correctly, this use of strbuf would leak each\ntime through the loop.\n\n> I guess it doesn't matter too much, but I think if we can avoid using\n> snprintf(), it's worth doing. If we must use snprintf() here, we should\n> probably use Git's xsnprintf() instead.\n\nxstrfmt() from strbuf.h would be even simpler if the intention is to\nallocate a new string which will be freed later.\n\nIn this case, though, assuming I understand the intent, I think the\nmore common and safe idiom in this codebase is something like this:\n\n struct strbuf name = STRBUF_INIT;\n strbuf_addstr(&name, \"branch\");\n size_t len = name.len;\n for (...) {\n strbuf_setlen(&name, len);\n strbuf_addf(&name, \"%04d\", i);\n ref.refname = name.buf;\n ...\n }\n strbuf_release(&name);\n"},{"id":"485515","messageId":"5b2a64ca9fa1d290b5b7838ad15c0894b65ae777.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 01/11] reftable: wrap EXPECT macros in do/while","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:29Z","receivedAt":"2023-12-11T09:07:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `EXPECT` macros used by the reftable test framework are all using a\nsingle `if` statement with the actual condition. This results in weird\nsyntax when using them in if/else statements like the following:\n\n```\nif (foo)\n\tEXPECT(foo == 2)\nelse\n\tEXPECT(bar == 2)\n```\n\nNote that there need not be a trailing semicolon. Furthermore, it is not\nimmediately obvious whether the else now belongs to the `if (foo)` or\nwhether it belongs to the expanded `if (foo == 2)` from the macro.\n\nFix this by wrapping the macros in a do/while loop.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/test_framework.h | 58 +++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 26 deletions(-)\n\ndiff --git a/reftable/test_framework.h b/reftable/test_framework.h\nindex 774cb275bf..ee44f735ae 100644\n--- a/reftable/test_framework.h\n+++ b/reftable/test_framework.h\n@@ -12,32 +12,38 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"reftable-error.h\"\n \n-#define EXPECT_ERR(c) \\\n-\tif (c != 0) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n-\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT_STREQ(a, b) \\\n-\tif (strcmp(a, b)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n-\t\t\t__LINE__, #a, a, #b, b); \\\n-\t\tabort(); \\\n-\t}\n-\n-#define EXPECT(c) \\\n-\tif (!(c)) { \\\n-\t\tfflush(stderr); \\\n-\t\tfflush(stdout); \\\n-\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n-\t\t\t__LINE__, #c); \\\n-\t\tabort(); \\\n-\t}\n+#define EXPECT_ERR(c) \\\n+\tdo { \\\n+\t\tif (c != 0) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: error == %d (%s), want 0\\n\", \\\n+\t\t\t\t__FILE__, __LINE__, c, reftable_error_str(c)); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT_STREQ(a, b) \\\n+\tdo { \\\n+\t\tif (strcmp(a, b)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s:%d: %s (%s) != %s (%s)\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #a, a, #b, b); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n+\n+#define EXPECT(c) \\\n+\tdo { \\\n+\t\tif (!(c)) { \\\n+\t\t\tfflush(stderr); \\\n+\t\t\tfflush(stdout); \\\n+\t\t\tfprintf(stderr, \"%s: %d: failed assertion %s\\n\", __FILE__, \\\n+\t\t\t\t__LINE__, #c); \\\n+\t\t\tabort(); \\\n+\t\t} \\\n+\t} while (0)\n \n #define RUN_TEST(f) \\\n \tfprintf(stderr, \"running %s\\n\", #f); \\\n-- \n2.43.0\n\n"},{"id":"485516","messageId":"cover.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"[PATCH v3 00/11] reftable: small set of fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:25Z","receivedAt":"2023-12-11T09:07:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series that addresses several\nsmallish issues in the reftable backend.\n\nThere's only a small set of changes compared to v2:\n\n - Patch 4: convert to use a `struct strbuf` instead of `snprintf()`.\n\n - Patch 5: improve commit message.\n\n - Patch 6: note that `stack_filename()` resets the `struct strbuf` in\n the commit message.\n\n - Patch 7: use the `struct filelock`'s lock path instead of the\n temporary buffer.\n\nThanks for your suggestions, Taylor and Eric!\n\nPatrick\n\nPatrick Steinhardt (11):\n reftable: wrap EXPECT macros in do/while\n reftable: handle interrupted reads\n reftable: handle interrupted writes\n reftable/stack: verify that `reftable_stack_add()` uses\n auto-compaction\n reftable/stack: perform auto-compaction with transactional interface\n reftable/stack: reuse buffers when reloading stack\n reftable/stack: fix stale lock when dying\n reftable/stack: fix use of unseeded randomness\n reftable/merged: reuse buffer to compute record keys\n reftable/block: introduce macro to initialize `struct block_iter`\n reftable/block: reuse buffer to compute record keys\n\n reftable/block.c | 23 ++++----\n reftable/block.h | 6 +++\n reftable/block_test.c | 4 +-\n reftable/blocksource.c | 2 +-\n reftable/iter.h | 8 +--\n reftable/merged.c | 31 +++++------\n reftable/merged.h | 2 +\n reftable/reader.c | 7 ++-\n reftable/readwrite_test.c | 6 +--\n reftable/stack.c | 73 +++++++++++---------------\n reftable/stack_test.c | 107 +++++++++++++++++++++++++++++++++++++-\n reftable/test_framework.h | 58 ++++++++++++---------\n 12 files changed, 213 insertions(+), 114 deletions(-)\n\nRange-diff against v2:\n 1: 0ebbb02d32 = 1: 5b2a64ca9f reftable: wrap EXPECT macros in do/while\n 2: b404fdf066 = 2: 3e8e63ece5 reftable: handle interrupted reads\n 3: 8c1d78b12b = 3: 1700d00d1c reftable: handle interrupted writes\n 4: 8061b9d2fc ! 4: 5e27d0a556 reftable/stack: verify that `reftable_stack_add()` uses auto-compaction\n @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)\n +{\n +\tstruct reftable_write_options cfg = { 0 };\n +\tstruct reftable_stack *st = NULL;\n ++\tstruct strbuf refname = STRBUF_INIT;\n +\tchar *dir = get_tmp_dir(__LINE__);\n +\tint err, i, n = 20;\n +\n @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)\n +\t\t\t.value_type = REFTABLE_REF_SYMREF,\n +\t\t\t.value.symref = \"master\",\n +\t\t};\n -+\t\tchar name[100];\n +\n +\t\t/*\n +\t\t * Disable auto-compaction for all but the last runs. Like this\n @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)\n +\t\t */\n +\t\tst->disable_auto_compact = i != n;\n +\n -+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n -+\t\tref.refname = name;\n ++\t\tstrbuf_reset(&refname);\n ++\t\tstrbuf_addf(&refname, \"branch-%04d\", i);\n ++\t\tref.refname = refname.buf;\n +\n +\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n +\t\tEXPECT_ERR(err);\n @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)\n +\t}\n +\n +\treftable_stack_destroy(st);\n ++\tstrbuf_release(&refname);\n +\tclear_dir(dir);\n +}\n +\n 5: 77b9ae8aa6 ! 5: dd180eba40 reftable/stack: perform auto-compaction with transactional interface\n @@ Commit message\n \n Whenever updating references or reflog entries in the reftable stack, we\n need to add a new table to the stack, thus growing the stack's length by\n - one. It can thus happen quite fast that the stack grows very long, which\n - results in performance issues when trying to read records. But besides\n - performance issues, this can also lead to exhaustion of file descriptors\n - very rapidly as every single table requires a separate descriptor when\n + one. The stack can grow to become quite long rather quickly, leading to\n + performance issues when trying to read records. But besides performance\n + issues, this can also lead to exhaustion of file descriptors very\n + rapidly as every single table requires a separate descriptor when\n opening the stack.\n \n While git-pack-refs(1) fixes this issue for us by merging the tables, it\n 6: f797feff8d ! 6: 6ed9ba60db reftable/stack: reuse buffers when reloading stack\n @@ Commit message\n of the allocated buffer outside of the loop.\n \n Refactor the code to instead reuse the buffers to reduce the number of\n - allocations we need to do.\n + allocations we need to do. Note that we do not have to manually reset\n + the buffer because `stack_filename()` does this for us already.\n \n Signed-off-by: Patrick Steinhardt \n \n 7: e82a68aecd ! 7: fbd9efa56d reftable/stack: fix stale lock when dying\n @@ reftable/stack.c: static int reftable_stack_init_addition(struct reftable_additi\n \t}\n \tif (st->config.default_permissions) {\n -\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n -+\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n ++\t\tif (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n 8: bab4fb93df = 8: 5598460b81 reftable/stack: fix use of unseeded randomness\n 9: cbf77ec45a = 9: 79e0382603 reftable/merged: reuse buffer to compute record keys\n10: c9a1405a9a = 10: 8574ad7635 reftable/block: introduce macro to initialize `struct block_iter`\n11: 02b11f3a80 = 11: eeb6c35823 reftable/block: reuse buffer to compute record keys\n\nbase-commit: 564d0252ca632e0264ed670534a51d18a689ef5d\n-- \n2.43.0\n\n"},{"id":"485517","messageId":"3e8e63ece52c507296b012be6632dbed568ff97d.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 02/11] reftable: handle interrupted reads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:34Z","receivedAt":"2023-12-11T09:07:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to pread(3P) and read(3P) where we don't properly handle\ninterrupts. Convert them to use `pread_in_full()` and `read_in_full()`,\nrespectively.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 2 +-\n reftable/stack.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 8331b34e82..a1ea304429 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -109,7 +109,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\n \tdest->data = reftable_malloc(size);\n-\tif (pread(b->fd, dest->data, size, off) != size)\n+\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n \t\treturn -1;\n \tdest->len = size;\n \treturn size;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ddbdf1b9c8..ed108a929b 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -92,7 +92,7 @@ static int fd_read_lines(int fd, char ***namesp)\n \t}\n \n \tbuf = reftable_malloc(size + 1);\n-\tif (read(fd, buf, size) != size) {\n+\tif (read_in_full(fd, buf, size) != size) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.43.0\n\n"},{"id":"485518","messageId":"1700d00d1ca017730d9188caf4eff9c02720131a.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 03/11] reftable: handle interrupted writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:38Z","receivedAt":"2023-12-11T09:07:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are calls to write(3P) where we don't properly handle interrupts.\nConvert them to use `write_in_full()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++---\n reftable/stack_test.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ed108a929b..f0cadad490 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -42,7 +42,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st,\n static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)\n {\n \tint *fdp = (int *)arg;\n-\treturn write(*fdp, data, sz);\n+\treturn write_in_full(*fdp, data, sz);\n }\n \n int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n@@ -554,7 +554,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n@@ -1024,7 +1024,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n \t}\n \n-\terr = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n+\terr = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex d0b717510f..0644c8ad2e 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -78,7 +78,7 @@ static void test_read_file(void)\n \tint i = 0;\n \n \tEXPECT(fd > 0);\n-\tn = write(fd, out, strlen(out));\n+\tn = write_in_full(fd, out, strlen(out));\n \tEXPECT(n == strlen(out));\n \terr = close(fd);\n \tEXPECT(err >= 0);\n-- \n2.43.0\n\n"},{"id":"485519","messageId":"5e27d0a5566d90969734e92984cfafe6048924f4.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:42Z","receivedAt":"2023-12-11T09:07:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While we have several tests that check whether we correctly perform\nauto-compaction when manually calling `reftable_stack_auto_compact()`,\nwe don't have any tests that verify whether `reftable_stack_add()` does\ncall it automatically. Add one.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 0644c8ad2e..52b4dc3b14 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_add_performs_auto_compaction(void)\n+{\n+\tstruct reftable_write_options cfg = { 0 };\n+\tstruct reftable_stack *st = NULL;\n+\tstruct strbuf refname = STRBUF_INIT;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err, i, n = 20;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\tstrbuf_reset(&refname);\n+\t\tstrbuf_addf(&refname, \"branch-%04d\", i);\n+\t\tref.refname = refname.buf;\n+\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tstrbuf_release(&refname);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_compaction_concurrent(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -960,6 +1008,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_add);\n \tRUN_TEST(test_reftable_stack_add_one);\n \tRUN_TEST(test_reftable_stack_auto_compaction);\n+\tRUN_TEST(test_reftable_stack_add_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent);\n \tRUN_TEST(test_reftable_stack_compaction_concurrent_clean);\n \tRUN_TEST(test_reftable_stack_hash_id);\n-- \n2.43.0\n\n"},{"id":"485520","messageId":"dd180eba40d41e8b0ddf8b9422720c0883cc04b8.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 05/11] reftable/stack: perform auto-compaction with transactional interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:46Z","receivedAt":"2023-12-11T09:07:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever updating references or reflog entries in the reftable stack, we\nneed to add a new table to the stack, thus growing the stack's length by\none. The stack can grow to become quite long rather quickly, leading to\nperformance issues when trying to read records. But besides performance\nissues, this can also lead to exhaustion of file descriptors very\nrapidly as every single table requires a separate descriptor when\nopening the stack.\n\nWhile git-pack-refs(1) fixes this issue for us by merging the tables, it\nruns too irregularly to keep the length of the stack within reasonable\nlimits. This is why the reftable stack has an auto-compaction mechanism:\n`reftable_stack_add()` will call `reftable_stack_auto_compact()` after\nits added the new table, which will auto-compact the stack as required.\n\nBut while this logic works alright for `reftable_stack_add()`, we do not\ndo the same in `reftable_addition_commit()`, which is the transactional\nequivalent to the former function that allows us to write multiple\nupdates to the stack atomically. Consequentially, we will easily run\ninto file descriptor exhaustion in code paths that use many separate\ntransactions like e.g. non-atomic fetches.\n\nFix this issue by calling `reftable_stack_auto_compact()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 6 +++++\n reftable/stack_test.c | 56 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f0cadad490..f5d18a842a 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -584,6 +584,12 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tadd->new_tables_len = 0;\n \n \terr = reftable_stack_reload(add->stack);\n+\tif (err)\n+\t\tgoto done;\n+\n+\tif (!add->stack->disable_auto_compact)\n+\t\terr = reftable_stack_auto_compact(add->stack);\n+\n done:\n \treftable_addition_close(add);\n \treturn err;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 52b4dc3b14..14a3fc11ee 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -289,6 +289,61 @@ static void test_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_write_options cfg = {0};\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_stack *st = NULL;\n+\tint i, n = 20, err;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\tfor (i = 0; i <= n; i++) {\n+\t\tstruct reftable_ref_record ref = {\n+\t\t\t.update_index = reftable_stack_next_update_index(st),\n+\t\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t\t.value.symref = \"master\",\n+\t\t};\n+\t\tchar name[100];\n+\n+\t\tsnprintf(name, sizeof(name), \"branch%04d\", i);\n+\t\tref.refname = name;\n+\n+\t\t/*\n+\t\t * Disable auto-compaction for all but the last runs. Like this\n+\t\t * we can ensure that we indeed honor this setting and have\n+\t\t * better control over when exactly auto compaction runs.\n+\t\t */\n+\t\tst->disable_auto_compact = i != n;\n+\n+\t\terr = reftable_stack_new_addition(&add, st);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_add(add, &write_test_ref, &ref);\n+\t\tEXPECT_ERR(err);\n+\n+\t\terr = reftable_addition_commit(add);\n+\t\tEXPECT_ERR(err);\n+\n+\t\treftable_addition_destroy(add);\n+\n+\t\t/*\n+\t\t * The stack length should grow continuously for all runs where\n+\t\t * auto compaction is disabled. When enabled, we should merge\n+\t\t * all tables in the stack.\n+\t\t */\n+\t\tif (i != n)\n+\t\t\tEXPECT(st->merged->stack_len == i + 1);\n+\t\telse\n+\t\t\tEXPECT(st->merged->stack_len == 1);\n+\t}\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_validate_refname(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -1016,6 +1071,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_log_normalize);\n \tRUN_TEST(test_reftable_stack_tombstone);\n \tRUN_TEST(test_reftable_stack_transaction_api);\n+\tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_validate_refname);\n-- \n2.43.0\n\n"},{"id":"485521","messageId":"6ed9ba60db081a5210d325dabcbf232771754277.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 06/11] reftable/stack: reuse buffers when reloading stack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:50Z","receivedAt":"2023-12-11T09:07:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_reload_once()` we iterate over all the tables added\nto the stack in order to figure out whether any of the tables needs to\nbe reloaded. We use a set of buffers in this context to compute the\npaths of these tables, but discard those buffers on every iteration.\nThis is quite wasteful given that we do not need to transfer ownership\nof the allocated buffer outside of the loop.\n\nRefactor the code to instead reuse the buffers to reduce the number of\nallocations we need to do. Note that we do not have to manually reset\nthe buffer because `stack_filename()` does this for us already.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f5d18a842a..2dd2373360 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -204,6 +204,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \t\treftable_calloc(sizeof(struct reftable_table) * names_len);\n \tint new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n+\tstruct strbuf table_path = STRBUF_INIT;\n \tint i;\n \n \twhile (*names) {\n@@ -223,13 +224,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \n \t\tif (!rd) {\n \t\t\tstruct reftable_block_source src = { NULL };\n-\t\t\tstruct strbuf table_path = STRBUF_INIT;\n \t\t\tstack_filename(&table_path, st, name);\n \n \t\t\terr = reftable_block_source_from_file(&src,\n \t\t\t\t\t\t\t table_path.buf);\n-\t\t\tstrbuf_release(&table_path);\n-\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \n@@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \tfor (i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n-\t\t\tstruct strbuf filename = STRBUF_INIT;\n-\t\t\tstack_filename(&filename, st, name);\n+\t\t\tstack_filename(&table_path, st, name);\n \n \t\t\treader_close(cur[i]);\n \t\t\treftable_reader_free(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n-\t\t\tunlink(filename.buf);\n-\n-\t\t\tstrbuf_release(&filename);\n+\t\t\tunlink(table_path.buf);\n \t\t}\n \t}\n \n@@ -288,6 +283,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \treftable_free(new_readers);\n \treftable_free(new_tables);\n \treftable_free(cur);\n+\tstrbuf_release(&table_path);\n \treturn err;\n }\n \n-- \n2.43.0\n\n"},{"id":"485522","messageId":"fbd9efa56d96a8d1bf9d3f1f4070b85d2be5ad12.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 07/11] reftable/stack: fix stale lock when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:54Z","receivedAt":"2023-12-11T09:07:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a transaction via `reftable_stack_init_addition()`, we\ncreate a lockfile for the reftable stack itself which we'll write the\nnew list of tables to. But if we terminate abnormally e.g. via a call to\n`die()`, then we do not remove the lockfile. Subsequent executions of\nGit which try to modify references will thus fail with an out-of-date\nerror.\n\nFix this bug by registering the lock as a `struct tempfile`, which\nensures automatic cleanup for us.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 47 +++++++++++++++--------------------------------\n 1 file changed, 15 insertions(+), 32 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2dd2373360..0c235724e2 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,6 +17,8 @@ license that can be found in the LICENSE file or at\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n \n+#include \"tempfile.h\"\n+\n static int stack_try_add(struct reftable_stack *st,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -440,8 +442,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n }\n \n struct reftable_addition {\n-\tint lock_file_fd;\n-\tstruct strbuf lock_file_name;\n+\tstruct tempfile *lock_file;\n \tstruct reftable_stack *stack;\n \n \tchar **new_tables;\n@@ -449,24 +450,19 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT \\\n-\t{ \\\n-\t\t.lock_file_name = STRBUF_INIT \\\n-\t}\n+#define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st)\n {\n+\tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err = 0;\n \tadd->stack = st;\n \n-\tstrbuf_reset(&add->lock_file_name);\n-\tstrbuf_addstr(&add->lock_file_name, st->list_file);\n-\tstrbuf_addstr(&add->lock_file_name, \".lock\");\n+\tstrbuf_addf(&lock_file_name, \"%s.lock\", st->list_file);\n \n-\tadd->lock_file_fd = open(add->lock_file_name.buf,\n-\t\t\t\t O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (add->lock_file_fd < 0) {\n+\tadd->lock_file = create_tempfile(lock_file_name.buf);\n+\tif (!add->lock_file) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n \t\t} else {\n@@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\tgoto done;\n \t}\n \tif (st->config.default_permissions) {\n-\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n+\t\tif (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n@@ -495,6 +491,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (err) {\n \t\treftable_addition_close(add);\n \t}\n+\tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n \n@@ -512,15 +509,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n \tadd->new_tables = NULL;\n \tadd->new_tables_len = 0;\n \n-\tif (add->lock_file_fd > 0) {\n-\t\tclose(add->lock_file_fd);\n-\t\tadd->lock_file_fd = 0;\n-\t}\n-\tif (add->lock_file_name.len > 0) {\n-\t\tunlink(add->lock_file_name.buf);\n-\t\tstrbuf_release(&add->lock_file_name);\n-\t}\n-\n+\tdelete_tempfile(&add->lock_file);\n \tstrbuf_release(&nm);\n }\n \n@@ -536,8 +525,10 @@ void reftable_addition_destroy(struct reftable_addition *add)\n int reftable_addition_commit(struct reftable_addition *add)\n {\n \tstruct strbuf table_list = STRBUF_INIT;\n+\tint lock_file_fd = get_tempfile_fd(add->lock_file);\n \tint i = 0;\n \tint err = 0;\n+\n \tif (add->new_tables_len == 0)\n \t\tgoto done;\n \n@@ -550,28 +541,20 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tstrbuf_addstr(&table_list, \"\\n\");\n \t}\n \n-\terr = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);\n+\terr = write_in_full(lock_file_fd, table_list.buf, table_list.len);\n \tstrbuf_release(&table_list);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = close(add->lock_file_fd);\n-\tadd->lock_file_fd = 0;\n-\tif (err < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = rename(add->lock_file_name.buf, add->stack->list_file);\n+\terr = rename_tempfile(&add->lock_file, add->stack->list_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n \t/* success, no more state to clean up. */\n-\tstrbuf_release(&add->lock_file_name);\n \tfor (i = 0; i < add->new_tables_len; i++) {\n \t\treftable_free(add->new_tables[i]);\n \t}\n-- \n2.43.0\n\n"},{"id":"485523","messageId":"5598460b8112e20d5f8a3889a482732d9475e6a7.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 08/11] reftable/stack: fix use of unseeded randomness","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:07:59Z","receivedAt":"2023-12-11T09:08:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing a new reftable stack, Git will first create the stack with\na random suffix so that concurrent updates will not try to write to the\nsame file. This random suffix is computed via a call to rand(3P). But we\nnever seed the function via srand(3P), which means that the suffix is in\nfact always the same.\n\nFix this bug by using `git_rand()` instead, which does not need to be\ninitialized. While this function is likely going to be slower depending\non the platform, this slowness should not matter in practice as we only\nuse it when writing a new reftable stack.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/readwrite_test.c | 6 +++---\n reftable/stack.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 469ab79a5a..278663f22d 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -141,8 +141,8 @@ static void test_log_buffer_size(void)\n \t*/\n \tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n \tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n-\t\thash1[i] = (uint8_t)(rand() % 256);\n-\t\thash2[i] = (uint8_t)(rand() % 256);\n+\t\thash1[i] = (uint8_t)(git_rand() % 256);\n+\t\thash2[i] = (uint8_t)(git_rand() % 256);\n \t}\n \tlog.value.update.old_hash = hash1;\n \tlog.value.update.new_hash = hash2;\n@@ -320,7 +320,7 @@ static void test_log_zlib_corruption(void)\n \t};\n \n \tfor (i = 0; i < sizeof(message) - 1; i++)\n-\t\tmessage[i] = (uint8_t)(rand() % 64 + ' ');\n+\t\tmessage[i] = (uint8_t)(git_rand() % 64 + ' ');\n \n \treftable_writer_set_limits(w, 1, 1);\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 0c235724e2..16bab82063 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -434,7 +434,7 @@ int reftable_stack_add(struct reftable_stack *st,\n static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)\n {\n \tchar buf[100];\n-\tuint32_t rnd = (uint32_t)rand();\n+\tuint32_t rnd = (uint32_t)git_rand();\n \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n \t\t min, max, rnd);\n \tstrbuf_reset(dest);\n-- \n2.43.0\n\n"},{"id":"485524","messageId":"79e03826039e9b91e456e4c7d2c5a2367a082736.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 09/11] reftable/merged: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:03Z","receivedAt":"2023-12-11T09:08:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating over entries in the merged iterator's queue, we compute\nthe key of each of the entries and write it into a buffer. We do not\nreuse the buffer though and thus re-allocate it on every iteration,\nwhich is wasteful given that we never transfer ownership of the\nallocated bytes outside of the loop.\n\nRefactor the code to reuse the buffer. This also fixes a potential\nmemory leak when `merged_iter_advance_subiter()` returns an error.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/merged.c | 31 ++++++++++++++++---------------\n reftable/merged.h | 2 ++\n 2 files changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 5ded470c08..556bb5c556 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -52,6 +52,8 @@ static void merged_iter_close(void *p)\n \t\treftable_iterator_destroy(&mi->stack[i]);\n \t}\n \treftable_free(mi->stack);\n+\tstrbuf_release(&mi->key);\n+\tstrbuf_release(&mi->entry_key);\n }\n \n static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,\n@@ -85,7 +87,6 @@ static int merged_iter_advance_subiter(struct merged_iter *mi, size_t idx)\n static int merged_iter_next_entry(struct merged_iter *mi,\n \t\t\t\t struct reftable_record *rec)\n {\n-\tstruct strbuf entry_key = STRBUF_INIT;\n \tstruct pq_entry entry = { 0 };\n \tint err = 0;\n \n@@ -105,33 +106,31 @@ static int merged_iter_next_entry(struct merged_iter *mi,\n \t such a deployment, the loop below must be changed to collect all\n \t entries for the same key, and return new the newest one.\n \t*/\n-\treftable_record_key(&entry.rec, &entry_key);\n+\treftable_record_key(&entry.rec, &mi->entry_key);\n \twhile (!merged_iter_pqueue_is_empty(mi->pq)) {\n \t\tstruct pq_entry top = merged_iter_pqueue_top(mi->pq);\n-\t\tstruct strbuf k = STRBUF_INIT;\n-\t\tint err = 0, cmp = 0;\n+\t\tint cmp = 0;\n \n-\t\treftable_record_key(&top.rec, &k);\n+\t\treftable_record_key(&top.rec, &mi->key);\n \n-\t\tcmp = strbuf_cmp(&k, &entry_key);\n-\t\tstrbuf_release(&k);\n-\n-\t\tif (cmp > 0) {\n+\t\tcmp = strbuf_cmp(&mi->key, &mi->entry_key);\n+\t\tif (cmp > 0)\n \t\t\tbreak;\n-\t\t}\n \n \t\tmerged_iter_pqueue_remove(&mi->pq);\n \t\terr = merged_iter_advance_subiter(mi, top.index);\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n+\t\tif (err < 0)\n+\t\t\tgoto done;\n \t\treftable_record_release(&top.rec);\n \t}\n \n \treftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));\n+\n+done:\n \treftable_record_release(&entry.rec);\n-\tstrbuf_release(&entry_key);\n-\treturn 0;\n+\tstrbuf_release(&mi->entry_key);\n+\tstrbuf_release(&mi->key);\n+\treturn err;\n }\n \n static int merged_iter_next(struct merged_iter *mi, struct reftable_record *rec)\n@@ -248,6 +247,8 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,\n \t\t.typ = reftable_record_type(rec),\n \t\t.hash_id = mt->hash_id,\n \t\t.suppress_deletions = mt->suppress_deletions,\n+\t\t.key = STRBUF_INIT,\n+\t\t.entry_key = STRBUF_INIT,\n \t};\n \tint n = 0;\n \tint err = 0;\ndiff --git a/reftable/merged.h b/reftable/merged.h\nindex 7d9f95d27e..d5b39dfe7f 100644\n--- a/reftable/merged.h\n+++ b/reftable/merged.h\n@@ -31,6 +31,8 @@ struct merged_iter {\n \tuint8_t typ;\n \tint suppress_deletions;\n \tstruct merged_iter_pqueue pq;\n+\tstruct strbuf key;\n+\tstruct strbuf entry_key;\n };\n \n void merged_table_release(struct reftable_merged_table *mt);\n-- \n2.43.0\n\n"},{"id":"485525","messageId":"8574ad7635614fcc64c23ac3bb464f460768c7f3.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 10/11] reftable/block: introduce macro to initialize `struct block_iter`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:07Z","receivedAt":"2023-12-11T09:08:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are a bunch of locations where we initialize members of `struct\nblock_iter`, which makes it harder than necessary to expand this struct\nto have additional members. Unify the logic via a new `BLOCK_ITER_INIT`\nmacro that initializes all members.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 4 +---\n reftable/block.h | 4 ++++\n reftable/block_test.c | 4 ++--\n reftable/iter.h | 8 ++++----\n reftable/reader.c | 7 +++----\n 5 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 34d4d07369..8c6a8c77fc 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -389,9 +389,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \tstruct reftable_record rec = reftable_new_record(block_reader_type(br));\n \tstruct strbuf key = STRBUF_INIT;\n \tint err = 0;\n-\tstruct block_iter next = {\n-\t\t.last_key = STRBUF_INIT,\n-\t};\n+\tstruct block_iter next = BLOCK_ITER_INIT;\n \n \tint i = binsearch(br->restart_count, &restart_key_less, &args);\n \tif (args.error) {\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 87c77539b5..51699af233 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -86,6 +86,10 @@ struct block_iter {\n \tstruct strbuf last_key;\n };\n \n+#define BLOCK_ITER_INIT { \\\n+\t.last_key = STRBUF_INIT, \\\n+}\n+\n /* initializes a block reader. */\n int block_reader_init(struct block_reader *br, struct reftable_block *bl,\n \t\t uint32_t header_off, uint32_t table_block_size,\ndiff --git a/reftable/block_test.c b/reftable/block_test.c\nindex cb88af4a56..c00bbc8aed 100644\n--- a/reftable/block_test.c\n+++ b/reftable/block_test.c\n@@ -32,7 +32,7 @@ static void test_block_read_write(void)\n \tint i = 0;\n \tint n;\n \tstruct block_reader br = { 0 };\n-\tstruct block_iter it = { .last_key = STRBUF_INIT };\n+\tstruct block_iter it = BLOCK_ITER_INIT;\n \tint j = 0;\n \tstruct strbuf want = STRBUF_INIT;\n \n@@ -87,7 +87,7 @@ static void test_block_read_write(void)\n \tblock_iter_close(&it);\n \n \tfor (i = 0; i < N; i++) {\n-\t\tstruct block_iter it = { .last_key = STRBUF_INIT };\n+\t\tstruct block_iter it = BLOCK_ITER_INIT;\n \t\tstrbuf_reset(&want);\n \t\tstrbuf_addstr(&want, names[i]);\n \ndiff --git a/reftable/iter.h b/reftable/iter.h\nindex 09eb0cbfa5..47d67d84df 100644\n--- a/reftable/iter.h\n+++ b/reftable/iter.h\n@@ -53,10 +53,10 @@ struct indexed_table_ref_iter {\n \tint is_finished;\n };\n \n-#define INDEXED_TABLE_REF_ITER_INIT \\\n-\t{ \\\n-\t\t.cur = { .last_key = STRBUF_INIT }, .oid = STRBUF_INIT, \\\n-\t}\n+#define INDEXED_TABLE_REF_ITER_INIT { \\\n+\t.cur = BLOCK_ITER_INIT, \\\n+\t.oid = STRBUF_INIT, \\\n+}\n \n void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it,\n \t\t\t\t\t struct indexed_table_ref_iter *itr);\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex b4db23ce18..9de64f50b4 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -224,10 +224,9 @@ struct table_iter {\n \tstruct block_iter bi;\n \tint is_finished;\n };\n-#define TABLE_ITER_INIT \\\n-\t{ \\\n-\t\t.bi = {.last_key = STRBUF_INIT } \\\n-\t}\n+#define TABLE_ITER_INIT { \\\n+\t.bi = BLOCK_ITER_INIT \\\n+}\n \n static void table_iter_copy_from(struct table_iter *dest,\n \t\t\t\t struct table_iter *src)\n-- \n2.43.0\n\n"},{"id":"485526","messageId":"eeb6c358231a2cc0647ca44ea3161a32a06157b9.1702285387.git.ps@pks.im","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"[PATCH v3 11/11] reftable/block: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:12Z","receivedAt":"2023-12-11T09:08:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating over entries in the block iterator we compute the key of\neach of the entries and write it into a buffer. We do not reuse the\nbuffer though and thus re-allocate it on every iteration, which is\nwasteful.\n\nRefactor the code to reuse the buffer.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 19 ++++++++-----------\n reftable/block.h | 2 ++\n 2 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 8c6a8c77fc..1df3d8a0f0 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -323,30 +323,28 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)\n \t\t.len = it->br->block_len - it->next_off,\n \t};\n \tstruct string_view start = in;\n-\tstruct strbuf key = STRBUF_INIT;\n \tuint8_t extra = 0;\n \tint n = 0;\n \n \tif (it->next_off >= it->br->block_len)\n \t\treturn 1;\n \n-\tn = reftable_decode_key(&key, &extra, it->last_key, in);\n+\tn = reftable_decode_key(&it->key, &extra, it->last_key, in);\n \tif (n < 0)\n \t\treturn -1;\n \n-\tif (!key.len)\n+\tif (!it->key.len)\n \t\treturn REFTABLE_FORMAT_ERROR;\n \n \tstring_view_consume(&in, n);\n-\tn = reftable_record_decode(rec, key, extra, in, it->br->hash_size);\n+\tn = reftable_record_decode(rec, it->key, extra, in, it->br->hash_size);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&in, n);\n \n \tstrbuf_reset(&it->last_key);\n-\tstrbuf_addbuf(&it->last_key, &key);\n+\tstrbuf_addbuf(&it->last_key, &it->key);\n \tit->next_off += start.len - in.len;\n-\tstrbuf_release(&key);\n \treturn 0;\n }\n \n@@ -377,6 +375,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)\n void block_iter_close(struct block_iter *it)\n {\n \tstrbuf_release(&it->last_key);\n+\tstrbuf_release(&it->key);\n }\n \n int block_reader_seek(struct block_reader *br, struct block_iter *it,\n@@ -387,7 +386,6 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t\t.r = br,\n \t};\n \tstruct reftable_record rec = reftable_new_record(block_reader_type(br));\n-\tstruct strbuf key = STRBUF_INIT;\n \tint err = 0;\n \tstruct block_iter next = BLOCK_ITER_INIT;\n \n@@ -414,8 +412,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t\tif (err < 0)\n \t\t\tgoto done;\n \n-\t\treftable_record_key(&rec, &key);\n-\t\tif (err > 0 || strbuf_cmp(&key, want) >= 0) {\n+\t\treftable_record_key(&rec, &it->key);\n+\t\tif (err > 0 || strbuf_cmp(&it->key, want) >= 0) {\n \t\t\terr = 0;\n \t\t\tgoto done;\n \t\t}\n@@ -424,8 +422,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \t}\n \n done:\n-\tstrbuf_release(&key);\n-\tstrbuf_release(&next.last_key);\n+\tblock_iter_close(&next);\n \treftable_record_release(&rec);\n \n \treturn err;\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 51699af233..17481e6331 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -84,10 +84,12 @@ struct block_iter {\n \n \t/* key for last entry we read. */\n \tstruct strbuf last_key;\n+\tstruct strbuf key;\n };\n \n #define BLOCK_ITER_INIT { \\\n \t.last_key = STRBUF_INIT, \\\n+\t.key = STRBUF_INIT, \\\n }\n \n /* initializes a block reader. */\n-- \n2.43.0\n\n"},{"id":"485527","messageId":"ZXbRir8Nnc3mMIoF@tanuki","threadId":"60541","inReplyTo":"ZXOK2o0jTgLmxPWZ@nand.local","subject":"Re: [PATCH v2 02/11] reftable: handle interrupted reads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:26Z","receivedAt":"2023-12-11T09:08:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 04:30:02PM -0500, Taylor Blau wrote:\n> On Fri, Dec 08, 2023 at 03:53:02PM +0100, Patrick Steinhardt wrote:\n> > There are calls to pread(3P) and read(3P) where we don't properly handle\n> > interrupts. Convert them to use `pread_in_full()` and `read_in_full()`,\n> \n> Just checking... do you mean \"interrupt\" in the kernel sense? Or are you\n> referring to the possibility of short reads/writes (in later patches)?\n\nBoth. The callsites I'm converting are explicitly checking that they get\nthe exact number of requested bytes. That means that we'll have to loop\nboth around EINTR/EAGAIN, but also around short reads.\n\nPatrick\n"},{"id":"485528","messageId":"ZXbRkOiD80zT7tC5@tanuki","threadId":"60541","inReplyTo":"CAPig+cRGZvyhSs9=3-tkBKRZDjDUsb-VDs+dzOaZof__qyBjbA@mail.gmail.com","subject":"Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:32Z","receivedAt":"2023-12-11T09:08:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote:\n> On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau wrote:\n> > On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote:\n> > > +static void test_reftable_stack_add_performs_auto_compaction(void)\n> > > +{\n> > > + char name[100];\n> > > + snprintf(name, sizeof(name), \"branch%04d\", i);\n> > > + ref.refname = name;\n> >\n> > Is there a reason that we have to use snprintf() here and not a strbuf?\n> >\n> > I would have expected to see something like:\n> >\n> > struct strbuf buf = STRBUF_INIT;\n> > /* ... */\n> > strbuf_addf(&buf, \"branch%04d\", i);\n> > ref.refname = strbuf_detach(&buf, NULL);\n> \n> If I'm reading the code correctly, this use of strbuf would leak each\n> time through the loop.\n> \n> > I guess it doesn't matter too much, but I think if we can avoid using\n> > snprintf(), it's worth doing. If we must use snprintf() here, we should\n> > probably use Git's xsnprintf() instead.\n> \n> xstrfmt() from strbuf.h would be even simpler if the intention is to\n> allocate a new string which will be freed later.\n> \n> In this case, though, assuming I understand the intent, I think the\n> more common and safe idiom in this codebase is something like this:\n> \n> struct strbuf name = STRBUF_INIT;\n> strbuf_addstr(&name, \"branch\");\n> size_t len = name.len;\n> for (...) {\n> strbuf_setlen(&name, len);\n> strbuf_addf(&name, \"%04d\", i);\n> ref.refname = name.buf;\n> ...\n> }\n> strbuf_release(&name);\n\nYeah, I'll convert this to use a `struct strbuf` instead. But instead of\ntracking the length I'll just use a `strbuf_reset()` followed by\n`strbuf_addf(\"branch-%04d\")`. It's simpler to read and we don't need to\nsqueeze every last drop of performance out of this loop anyway.\n\nPatrick\n"},{"id":"485529","messageId":"ZXbRlhcx9f22hILb@tanuki","threadId":"60541","inReplyTo":"ZXOVVWGyUtrGWYMB@nand.local","subject":"Re: [PATCH v2 05/11] reftable/stack: perform auto-compaction with transactional interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:38Z","receivedAt":"2023-12-11T09:08:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 05:14:45PM -0500, Taylor Blau wrote:\n> On Fri, Dec 08, 2023 at 03:53:14PM +0100, Patrick Steinhardt wrote:\n> > [...] It can thus happen quite fast that the stack grows very long, which\n> > results in performance issues when trying to read records.\n> \n> This sentence was a little confusing to read at first glance. Perhaps\n> instead:\n> \n> The stack can grow to become quite long rather quickly, leading to\n> performance issues when trying to read records.\n\nThanks, this reads better indeed.\n\nPatrick\n"},{"id":"485530","messageId":"ZXbRmwj1vZ2dA3s9@tanuki","threadId":"60541","inReplyTo":"ZXOV8TCqaH0xXRnS@nand.local","subject":"Re: [PATCH v2 06/11] reftable/stack: reuse buffers when reloading stack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:43Z","receivedAt":"2023-12-11T09:08:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 05:17:21PM -0500, Taylor Blau wrote:\n> On Fri, Dec 08, 2023 at 03:53:18PM +0100, Patrick Steinhardt wrote:\n> > In `reftable_stack_reload_once()` we iterate over all the tables added\n> > to the stack in order to figure out whether any of the tables needs to\n> > be reloaded. We use a set of buffers in this context to compute the\n> > paths of these tables, but discard those buffers on every iteration.\n> > This is quite wasteful given that we do not need to transfer ownership\n> > of the allocated buffer outside of the loop.\n> >\n> > Refactor the code to instead reuse the buffers to reduce the number of\n> > allocations we need to do.\n> \n> > @@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n> > \tfor (i = 0; i < cur_len; i++) {\n> > \t\tif (cur[i]) {\n> > \t\t\tconst char *name = reader_name(cur[i]);\n> > -\t\t\tstruct strbuf filename = STRBUF_INIT;\n> > -\t\t\tstack_filename(&filename, st, name);\n> > +\t\t\tstack_filename(&table_path, st, name);\n> \n> This initially caught me by surprise, but on closer inspection I agree\n> that this is OK, since stack_filename() calls strbuf_reset() before\n> adjusting the buffer contents.\n> \n> (As a side-note, I do find the side-effect of stack_filename() to be a\n> little surprising, but that's not the fault of this series and not worth\n> changing here.)\n\nAgreed, I also found this to be a bit confusing at first. I'll amend the\ncommit message with \"Note that we do not have to manually reset the\nbuffer because `stack_filename()` does this for us already.\" to help\nfuture readers.\n\nPatrick\n"},{"id":"485531","messageId":"ZXbRoDh7j18olBDx@tanuki","threadId":"60541","inReplyTo":"ZXOXpQstse8CdI7J@nand.local","subject":"Re: [PATCH v2 07/11] reftable/stack: fix stale lock when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T09:08:48Z","receivedAt":"2023-12-11T09:08:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 05:24:37PM -0500, Taylor Blau wrote:\n> On Fri, Dec 08, 2023 at 03:53:23PM +0100, Patrick Steinhardt wrote:\n> > When starting a transaction via `reftable_stack_init_addition()`, we\n> > create a lockfile for the reftable stack itself which we'll write the\n> > new list of tables to. But if we terminate abnormally e.g. via a call to\n> > `die()`, then we do not remove the lockfile. Subsequent executions of\n> > Git which try to modify references will thus fail with an out-of-date\n> > error.\n> >\n> > Fix this bug by registering the lock as a `struct tempfile`, which\n> > ensures automatic cleanup for us.\n> \n> Makes sense.\n> \n> > @@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> > \t\tgoto done;\n> > \t}\n> > \tif (st->config.default_permissions) {\n> > -\t\tif (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {\n> > +\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n> \n> Hmm. Would we want to use add->lock_file->filename.buf here instead? I\n> don't think that it matters (other than that the lockfile's pathname is\n> absolute). But it arguably makes it clearer to readers that we're\n> touching calling chmod() on the lockfile here, and hardens us against\n> the contents of the temporary strbuf changing.\n\nIt doesn't make much of a difference, but I can see how this might make\nthings a bit clearer to the reader. Will change.\n\nPatrick\n"},{"id":"485532","messageId":"CAPig+cTmAqDqu4Hiz+JO2GfV3+CqgVTXwFKWSTJXXAJ8Kg-xbw@mail.gmail.com","threadId":"60541","inReplyTo":"ZXbRkOiD80zT7tC5@tanuki","subject":"Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-11T09:36:38Z","receivedAt":"2023-12-11T09:36:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 11, 2023 at 4:08 AM Patrick Steinhardt wrote:\n> On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote:\n> > In this case, though, assuming I understand the intent, I think the\n> > more common and safe idiom in this codebase is something like this:\n> >\n> > struct strbuf name = STRBUF_INIT;\n> > strbuf_addstr(&name, \"branch\");\n> > size_t len = name.len;\n> > for (...) {\n> > strbuf_setlen(&name, len);\n> > strbuf_addf(&name, \"%04d\", i);\n> > ref.refname = name.buf;\n> > ...\n> > }\n> > strbuf_release(&name);\n>\n> Yeah, I'll convert this to use a `struct strbuf` instead. But instead of\n> tracking the length I'll just use a `strbuf_reset()` followed by\n> `strbuf_addf(\"branch-%04d\")`. It's simpler to read and we don't need to\n> squeeze every last drop of performance out of this loop anyway.\n\nSounds perfectly reasonable to me, and I agree with the reasoning,\nespecially the lower cognitive load with strbuf_reset(). Thanks.\n"},{"id":"485543","messageId":"ZXdt17pN68tsmH1H@nand.local","threadId":"60541","inReplyTo":"5e27d0a5566d90969734e92984cfafe6048924f4.1702285387.git.ps@pks.im","subject":"Re: [PATCH v3 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-11T20:15:19Z","receivedAt":"2023-12-11T20:15:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 11, 2023 at 10:07:42AM +0100, Patrick Steinhardt wrote:\n> While we have several tests that check whether we correctly perform\n> auto-compaction when manually calling `reftable_stack_auto_compact()`,\n> we don't have any tests that verify whether `reftable_stack_add()` does\n> call it automatically. Add one.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 49 insertions(+)\n>\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index 0644c8ad2e..52b4dc3b14 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void)\n> \tclear_dir(dir);\n> }\n>\n> +static void test_reftable_stack_add_performs_auto_compaction(void)\n> +{\n> +\tstruct reftable_write_options cfg = { 0 };\n> +\tstruct reftable_stack *st = NULL;\n> +\tstruct strbuf refname = STRBUF_INIT;\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tint err, i, n = 20;\n> +\n> +\terr = reftable_new_stack(&st, dir, cfg);\n> +\tEXPECT_ERR(err);\n> +\n> +\tfor (i = 0; i <= n; i++) {\n> +\t\tstruct reftable_ref_record ref = {\n> +\t\t\t.update_index = reftable_stack_next_update_index(st),\n> +\t\t\t.value_type = REFTABLE_REF_SYMREF,\n> +\t\t\t.value.symref = \"master\",\n> +\t\t};\n> +\n> +\t\t/*\n> +\t\t * Disable auto-compaction for all but the last runs. Like this\n> +\t\t * we can ensure that we indeed honor this setting and have\n> +\t\t * better control over when exactly auto compaction runs.\n> +\t\t */\n> +\t\tst->disable_auto_compact = i != n;\n> +\n> +\t\tstrbuf_reset(&refname);\n> +\t\tstrbuf_addf(&refname, \"branch-%04d\", i);\n> +\t\tref.refname = refname.buf;\n\nDoes the reftable backend take ownership of the \"refname\" field? If so,\nthen I think we'd want to use strbuf_detach() here to avoid a\ndouble-free() when you call strbuf_release() below.\n\nThanks,\nTaylor\n"},{"id":"485544","messageId":"ZXduGvCJIa25eldZ@nand.local","threadId":"60541","inReplyTo":"cover.1702285387.git.ps@pks.im","subject":"Re: [PATCH v3 00/11] reftable: small set of fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-11T20:16:26Z","receivedAt":"2023-12-11T20:16:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 11, 2023 at 10:07:25AM +0100, Patrick Steinhardt wrote:\n> reftable/block.c | 23 ++++----\n> reftable/block.h | 6 +++\n> reftable/block_test.c | 4 +-\n> reftable/blocksource.c | 2 +-\n> reftable/iter.h | 8 +--\n> reftable/merged.c | 31 +++++------\n> reftable/merged.h | 2 +\n> reftable/reader.c | 7 ++-\n> reftable/readwrite_test.c | 6 +--\n> reftable/stack.c | 73 +++++++++++---------------\n> reftable/stack_test.c | 107 +++++++++++++++++++++++++++++++++++++-\n> reftable/test_framework.h | 58 ++++++++++++---------\n> 12 files changed, 213 insertions(+), 114 deletions(-)\n>\n> Range-diff against v2:\n\nI had one small question on the new version of the fourth patch, but\notherwise this version LGTM.\n\nThanks,\nTaylor\n"},{"id":"485556","messageId":"ZXfXJaDTdEx8tohk@tanuki","threadId":"60541","inReplyTo":"ZXdt17pN68tsmH1H@nand.local","subject":"Re: [PATCH v3 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T03:44:37Z","receivedAt":"2023-12-12T03:44:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 11, 2023 at 03:15:19PM -0500, Taylor Blau wrote:\n> On Mon, Dec 11, 2023 at 10:07:42AM +0100, Patrick Steinhardt wrote:\n> > While we have several tests that check whether we correctly perform\n> > auto-compaction when manually calling `reftable_stack_auto_compact()`,\n> > we don't have any tests that verify whether `reftable_stack_add()` does\n> > call it automatically. Add one.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 49 insertions(+)\n> >\n> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> > index 0644c8ad2e..52b4dc3b14 100644\n> > --- a/reftable/stack_test.c\n> > +++ b/reftable/stack_test.c\n> > @@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void)\n> > \tclear_dir(dir);\n> > }\n> >\n> > +static void test_reftable_stack_add_performs_auto_compaction(void)\n> > +{\n> > +\tstruct reftable_write_options cfg = { 0 };\n> > +\tstruct reftable_stack *st = NULL;\n> > +\tstruct strbuf refname = STRBUF_INIT;\n> > +\tchar *dir = get_tmp_dir(__LINE__);\n> > +\tint err, i, n = 20;\n> > +\n> > +\terr = reftable_new_stack(&st, dir, cfg);\n> > +\tEXPECT_ERR(err);\n> > +\n> > +\tfor (i = 0; i <= n; i++) {\n> > +\t\tstruct reftable_ref_record ref = {\n> > +\t\t\t.update_index = reftable_stack_next_update_index(st),\n> > +\t\t\t.value_type = REFTABLE_REF_SYMREF,\n> > +\t\t\t.value.symref = \"master\",\n> > +\t\t};\n> > +\n> > +\t\t/*\n> > +\t\t * Disable auto-compaction for all but the last runs. Like this\n> > +\t\t * we can ensure that we indeed honor this setting and have\n> > +\t\t * better control over when exactly auto compaction runs.\n> > +\t\t */\n> > +\t\tst->disable_auto_compact = i != n;\n> > +\n> > +\t\tstrbuf_reset(&refname);\n> > +\t\tstrbuf_addf(&refname, \"branch-%04d\", i);\n> > +\t\tref.refname = refname.buf;\n> \n> Does the reftable backend take ownership of the \"refname\" field? If so,\n> then I think we'd want to use strbuf_detach() here to avoid a\n> double-free() when you call strbuf_release() below.\n\nNo it doesn't. `reftable_stack_add()` will lock the stack and commit the\nnew table immediately, so there's no need to transfer ownership of\nmemory here.\n\nPatrick\n"},{"id":"485557","messageId":"ZXfXQrNVReK9-RLP@tanuki","threadId":"60541","inReplyTo":"ZXduGvCJIa25eldZ@nand.local","subject":"Re: [PATCH v3 00/11] reftable: small set of fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T03:45:06Z","receivedAt":"2023-12-12T03:45:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 11, 2023 at 03:16:26PM -0500, Taylor Blau wrote:\n> On Mon, Dec 11, 2023 at 10:07:25AM +0100, Patrick Steinhardt wrote:\n> > reftable/block.c | 23 ++++----\n> > reftable/block.h | 6 +++\n> > reftable/block_test.c | 4 +-\n> > reftable/blocksource.c | 2 +-\n> > reftable/iter.h | 8 +--\n> > reftable/merged.c | 31 +++++------\n> > reftable/merged.h | 2 +\n> > reftable/reader.c | 7 ++-\n> > reftable/readwrite_test.c | 6 +--\n> > reftable/stack.c | 73 +++++++++++---------------\n> > reftable/stack_test.c | 107 +++++++++++++++++++++++++++++++++++++-\n> > reftable/test_framework.h | 58 ++++++++++++---------\n> > 12 files changed, 213 insertions(+), 114 deletions(-)\n> >\n> > Range-diff against v2:\n> \n> I had one small question on the new version of the fourth patch, but\n> otherwise this version LGTM.\n\nThanks for your review!\n\nPatrick\n"},{"id":"485916","messageId":"CAOw_e7Yfdt_Wqm-9XDJknaN-iH=haP0R4K-S4c_E3EFDzvG5aA@mail.gmail.com","threadId":"60541","inReplyTo":"25522b042cdc5986972cc7b62e6b88be0569d3cb.1700549493.git.ps@pks.im","subject":"Re: [PATCH 5/8] reftable/stack: perform auto-compaction with transactional interface","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T10:29:52Z","receivedAt":"2023-12-21T10:30:04Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Tue, Nov 21, 2023 at 8:07 AM Patrick Steinhardt wrote:\n> Whenever updating references or reflog entries in the reftable stack, we\n> need to add a new table to the stack, thus growing the stack's length by\n..\n\nbug is correctly identified, but can't the fix remove the\n\n if (!st->disable_auto_compact)\n return reftable_stack_auto_compact(st);\n\nfragment from reftable_stack_add? reftable_addition_commit eventually\ncalls reftable_addition_commit.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"485917","messageId":"CAOw_e7bQ+jxO2zhj32mDksq9uBKQfNt=wMNP5K6Oy1DqievCdg@mail.gmail.com","threadId":"60541","inReplyTo":"02b11f3a80608ba8748a0d0e2294f432e02464e5.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 11/11] reftable/block: reuse buffer to compute record keys","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T10:43:27Z","receivedAt":"2023-12-21T10:43:38Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Fri, Dec 8, 2023 at 3:53 PM Patrick Steinhardt wrote:\n\n> @@ -84,10 +84,12 @@ struct block_iter {\n>\n> /* key for last entry we read. */\n> struct strbuf last_key;\n> + struct strbuf key;\n> };\n\nit's slightly more efficient, but the new field has no essential\nmeaning. If I encountered this code with the change you make here, I\nwould probably refactor it in the opposite direction to increase code\nclarity.\n\nI suspect that the gains are too small to be measurable, but if you\nare after small efficiency gains, you can have\nreftable_record_decode() consume the key to avoid copying overhead in\nrecord.c.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"485918","messageId":"ZYQXP6xPiZbM2aOf@framework","threadId":"60541","inReplyTo":"CAOw_e7Yfdt_Wqm-9XDJknaN-iH=haP0R4K-S4c_E3EFDzvG5aA@mail.gmail.com","subject":"Re: [PATCH 5/8] reftable/stack: perform auto-compaction with transactional interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-21T10:45:19Z","receivedAt":"2023-12-21T10:45:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 21, 2023 at 11:29:52AM +0100, Han-Wen Nienhuys wrote:\n> On Tue, Nov 21, 2023 at 8:07 AM Patrick Steinhardt wrote:\n> > Whenever updating references or reflog entries in the reftable stack, we\n> > need to add a new table to the stack, thus growing the stack's length by\n> ..\n> \n> bug is correctly identified, but can't the fix remove the\n> \n> if (!st->disable_auto_compact)\n> return reftable_stack_auto_compact(st);\n> \n> fragment from reftable_stack_add? reftable_addition_commit eventually\n> calls reftable_addition_commit.\n\n\"calls reftable_stack_add\" you probably mean here. But yes, you're\nright. As this patch series has already been merged to `next` I can roll\nthis cleanup into my second patch series that addresses issues with the\nreftable library [1]. Does that work for you?\n\n[1]: http://public-inbox.org/git/xmqqr0jgsn9g.fsf@gitster.g/T/#t\n\nPatrick\n"},{"id":"485920","messageId":"CAOw_e7Yps5T32cAZrKO2meebEsdo4h6AWoFF3A4PCqF9AHuNxQ@mail.gmail.com","threadId":"60541","inReplyTo":"23c060d1e21573581ca6c5db50ca756b61078e3e.1700549493.git.ps@pks.im","subject":"Re: [PATCH 7/8] reftable/merged: reuse buffer to compute record keys","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T10:48:50Z","receivedAt":"2023-12-21T10:49:01Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Tue, Nov 21, 2023 at 8:07 AM Patrick Steinhardt wrote:\n>\n> When iterating over entries in the merged iterator's queue, we compute\n> the key of each of the entries and write it into a buffer. We do not\n> reuse the buffer though and thus re-allocate it on every iteration,\n> which is wasteful given that we never transfer ownership of the\n> allocated bytes outside of the loop.\n>\n\nFrom a brief read, change looks good.\n\nIn the C code, each key has to pass through the pqueue which is\n(assuming auto-compaction) has log2(#tables) entries. The JGit code\nassumes that the base table will be very large and the rest small.\nThis means that most keys come from the base table, and has some\nspecial casing so those keys don't have to pass through the pqueue. If\nyou worry about efficiency, this might be something to look into.\n\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"485921","messageId":"CAOw_e7Zh025FdRmHdxr4M43cZkKmO8W0W+h=Gb6+DJ-O2V-_3g@mail.gmail.com","threadId":"60541","inReplyTo":"bab4fb93df8d1a620eefeef99a49ea52c98dfc6e.1702047081.git.ps@pks.im","subject":"Re: [PATCH v2 08/11] reftable/stack: fix use of unseeded randomness","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T10:49:41Z","receivedAt":"2023-12-21T10:49:52Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Fri, Dec 8, 2023 at 3:53 PM Patrick Steinhardt wrote:\n>\n> When writing a new reftable stack, Git will first create the stack with\n> a random suffix so that concurrent updates will not try to write to the\n\nI had noticed this already. Thanks for fixing!\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"485923","messageId":"CAOw_e7bvud5M-1+kCi3gRuso3DqC19ujjKx079ORHKWiwC=Zzg@mail.gmail.com","threadId":"60541","inReplyTo":"ZXbRmwj1vZ2dA3s9@tanuki","subject":"Re: [PATCH v2 06/11] reftable/stack: reuse buffers when reloading stack","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T10:58:32Z","receivedAt":"2023-12-21T10:58:44Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Mon, Dec 11, 2023 at 10:08 AM Patrick Steinhardt wrote:\n\n> > This initially caught me by surprise, but on closer inspection I agree\n> > that this is OK, since stack_filename() calls strbuf_reset() before\n> > adjusting the buffer contents.\n> >\n> > (As a side-note, I do find the side-effect of stack_filename() to be a\n> > little surprising, but that's not the fault of this series and not worth\n> > changing here.)\n>\n> Agreed, I also found this to be a bit confusing at first. I'll amend the\n> commit message with \"Note that we do not have to manually reset the\n> buffer because `stack_filename()` does this for us already.\" to help\n> future readers.\n\nIn C++ it is expected that assignment operators clear the destination\nbefore executing the assignment, so it depends on your expectations.\nIf this is confusing, maybe another name is in order?\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"485924","messageId":"CAOw_e7ZJ1HRw1VkB0r65NWrd9DGrKpJ+FiAGUUpPQpXwZvDW-A@mail.gmail.com","threadId":"60541","inReplyTo":"cover.1700549493.git.ps@pks.im","subject":"Re: [PATCH 0/8] reftable: small set of fixes","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2023-12-21T11:08:02Z","receivedAt":"2023-12-21T11:08:14Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Tue, Nov 21, 2023 at 8:07 AM Patrick Steinhardt wrote:\n> It's a bit unfortunate that we don't yet have good test coverage as\n> there are no end-to-end tests, and most of the changes I did are not\n> easily testable in unit tests. So until the reftable backend gets\n..\n> reftable/stack: perform auto-compaction with transactional interface\n\nyou can test autocompaction more precisely using the stats\nin reftable_compaction_stats. See\nhttps://github.com/hanwen/reftable/blob/master/stack_test.go#L126\nfor an example.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"486078","messageId":"ZY0Ndkv2v7TXVskc@tanuki","threadId":"60541","inReplyTo":"CAOw_e7bQ+jxO2zhj32mDksq9uBKQfNt=wMNP5K6Oy1DqievCdg@mail.gmail.com","subject":"Re: [PATCH v2 11/11] reftable/block: reuse buffer to compute record keys","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-28T05:53:58Z","receivedAt":"2023-12-28T05:54:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 21, 2023 at 11:43:27AM +0100, Han-Wen Nienhuys wrote:\n> On Fri, Dec 8, 2023 at 3:53 PM Patrick Steinhardt wrote:\n> \n> > @@ -84,10 +84,12 @@ struct block_iter {\n> >\n> > /* key for last entry we read. */\n> > struct strbuf last_key;\n> > + struct strbuf key;\n> > };\n> \n> it's slightly more efficient, but the new field has no essential\n> meaning. If I encountered this code with the change you make here, I\n> would probably refactor it in the opposite direction to increase code\n> clarity.\n> \n> I suspect that the gains are too small to be measurable, but if you\n> are after small efficiency gains, you can have\n> reftable_record_decode() consume the key to avoid copying overhead in\n> record.c.\n\nFair enough. I've done a better job for these kinds of refactorings for\nthe reftable library in my second patch series [1] by including some\nbenchmarks via Valgrind. This should result in less handwavy and\nactually measurable/significant performance improvements.\n\nPatrick\n\n[1]: https://public-inbox.org/git/xmqqr0jgsn9g.fsf@gitster.g/T/#t\n"}]}