{"thread":{"id":"63891","subject":"[PATCH 0/5] reftable: a couple of improvements for libgit2","startedAt":"2025-08-01T14:47:58Z","lastAt":"2025-08-13T14:38:18Z","messageCount":52,"participants":["Patrick Steinhardt","Eric Sunshine","Junio C Hamano","Justin Tobler","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"523209","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":null,"subject":"[PATCH 0/5] reftable: a couple of improvements for libgit2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:46Z","receivedAt":"2025-08-01T14:47:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series contains a couple of improvements I required for\nlibgit2. With those changes libgit2 is now able to run its full test\nsuite with reftable-enabled repositories. I still need to invest a bit\nof work to make it memory-leak free and compile on Windows, but overall\nI think that support for reftables is almost ready.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (5):\n reftable/writer: fix type used for number of records\n reftable/writer: drop Git-specific `QSORT()` macro\n reftable/stack: fix compiler warning due to missing braces\n reftable/stack: reorder code to avoid forward declarations\n reftable/stack: allow passing flags to `reftable_stack_add()`\n\n refs/reftable-backend.c | 8 +-\n reftable/reftable-stack.h | 9 +-\n reftable/reftable-writer.h | 4 +-\n reftable/stack.c | 366 +++++++++++++++++++---------------------\n reftable/writer.c | 23 +--\n t/unit-tests/t-reftable-stack.c | 50 +++---\n 6 files changed, 227 insertions(+), 233 deletions(-)\n\n\n---\nbase-commit: e813a0200a7121b97fec535f0d0b460b0a33356c\nchange-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603\n\n"},{"id":"523210","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-1-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH 1/5] reftable/writer: fix type used for number of records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:47Z","receivedAt":"2025-08-01T14:47:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`\naccept an array of records that should be added to the new table.\nCallers of this function are expected to also pass the number of such\nrecords to the function to tell it how many such records it is supposed\nto write.\n\nBut while all callers pass in a `size_t`, which is a sensible choice,\nthe function in fact accepts an `int` as argument, which is less so. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-writer.h | 4 ++--\n reftable/writer.c | 17 +++++++++--------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 0fbeff17f4..1e7003cd69 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n the records before adding them, reordering the records array passed in.\n */\n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n);\n+\t\t\t struct reftable_ref_record *refs, size_t n);\n \n /*\n adds reftable_log_records. Log records are keyed by (refname, decreasing\n@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,\n the records before adding them, reordering records array passed in.\n */\n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n);\n+\t\t\t struct reftable_log_record *logs, size_t n);\n \n /* reftable_writer_close finalizes the reftable. The writer is retained so\n * statistics can be inspected. */\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 3b4ebdd6dc..5bad130c7e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n }\n \n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n)\n+\t\t\t struct reftable_ref_record *refs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(refs, n, reftable_ref_record_compare_name);\n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n-\t}\n+\n \treturn err;\n }\n \n@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,\n }\n \n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n)\n+\t\t\t struct reftable_log_record *logs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(logs, n, reftable_log_record_compare_key);\n \n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n-\t}\n+\n \treturn err;\n }\n \n\n-- \n2.50.1\n\n"},{"id":"523211","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-2-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH 2/5] reftable/writer: drop Git-specific `QSORT()` macro","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:48Z","receivedAt":"2025-08-01T14:48:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer accidentally uses the Git-specific `QSORT()` macro.\nThis macro removes the need for the caller to provide the element size,\nbut other than that it's mostly equivalent to `qsort()`.\n\nReplace the macro accordingly to make the library usable outside of Git.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 5bad130c7e..0133b64975 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(refs, n, reftable_ref_record_compare_name);\n+\tif (n)\n+\t\tqsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(logs, n, reftable_log_record_compare_key);\n+\tif (n)\n+\t\tqsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n\n-- \n2.50.1\n\n"},{"id":"523212","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-3-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:49Z","receivedAt":"2025-08-01T14:48:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While perfectly legal, older compiler toolchains complain when\nzero-initializing structs that contain nested structs with `{0}`:\n\n /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]\n struct reftable_addition empty = REFTABLE_ADDITION_INIT;\n ^~~~~~~~~~~~~~~~~~~~~~\n /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'\n #define REFTABLE_ADDITION_INIT {0}\n ^\n\nSilence this warning by using `{{0}}` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 4caf96aa1d..3480ad21c3 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -704,7 +704,7 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT {0}\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-- \n2.50.1\n\n"},{"id":"523213","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-4-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH 4/5] reftable/stack: reorder code to avoid forward declarations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:50Z","receivedAt":"2025-08-01T14:48:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a couple of forward declarations in the stack-related code of\nthe reftable library. These declarations aren't really required, but are\nsimply caused by unfortunate ordering.\n\nReorder the code and remove the forward declarations.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 176 insertions(+), 188 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 3480ad21c3..d6e4ea93a3 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,18 +17,6 @@\n #include \"table.h\"\n #include \"writer.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-\t\t\t void *arg);\n-static int stack_write_compact(struct reftable_stack *st,\n-\t\t\t struct reftable_writer *wr,\n-\t\t\t size_t first, size_t last,\n-\t\t\t struct reftable_log_expiry_config *config);\n-static void reftable_addition_close(struct reftable_addition *add);\n-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n-\t\t\t\t\t int reuse_open);\n-\n static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,\n \t\t\t const char *name)\n {\n@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)\n \treturn stack_fsync(writer->opts, writer->fd);\n }\n \n-int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n-\t\t const struct reftable_write_options *_opts)\n-{\n-\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct reftable_stack *p;\n-\tint err;\n-\n-\tp = reftable_calloc(1, sizeof(*p));\n-\tif (!p) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\tif (_opts)\n-\t\topts = *_opts;\n-\tif (opts.hash_id == 0)\n-\t\topts.hash_id = REFTABLE_HASH_SHA1;\n-\n-\t*dest = NULL;\n-\n-\treftable_buf_reset(&list_file_name);\n-\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n-\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n-\t\tgoto out;\n-\n-\tp->list_file = reftable_buf_detach(&list_file_name);\n-\tp->list_fd = -1;\n-\tp->opts = opts;\n-\tp->reftable_dir = reftable_strdup(dir);\n-\tif (!p->reftable_dir) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\terr = reftable_stack_reload_maybe_reuse(p, 1);\n-\tif (err < 0)\n-\t\tgoto out;\n-\n-\t*dest = p;\n-\terr = 0;\n-\n-out:\n-\tif (err < 0)\n-\t\treftable_stack_destroy(p);\n-\treturn err;\n-}\n-\n static int fd_read_lines(int fd, char ***namesp)\n {\n \tchar *buf = NULL;\n@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \treturn err;\n }\n \n+int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n+\t\t const struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *p;\n+\tint err;\n+\n+\tp = reftable_calloc(1, sizeof(*p));\n+\tif (!p) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\tif (opts.hash_id == 0)\n+\t\topts.hash_id = REFTABLE_HASH_SHA1;\n+\n+\t*dest = NULL;\n+\n+\treftable_buf_reset(&list_file_name);\n+\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n+\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n+\t\tgoto out;\n+\n+\tp->list_file = reftable_buf_detach(&list_file_name);\n+\tp->list_fd = -1;\n+\tp->opts = opts;\n+\tp->reftable_dir = reftable_strdup(dir);\n+\tif (!p->reftable_dir) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\terr = reftable_stack_reload_maybe_reuse(p, 1);\n+\tif (err < 0)\n+\t\tgoto out;\n+\n+\t*dest = p;\n+\terr = 0;\n+\n+out:\n+\tif (err < 0)\n+\t\treftable_stack_destroy(p);\n+\treturn err;\n+}\n+\n /* -1 = error\n 0 = up to date\n 1 = changed. */\n@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)\n \treturn err;\n }\n \n-int reftable_stack_add(struct reftable_stack *st,\n-\t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n-{\n-\tint err = stack_try_add(st, write, arg);\n-\tif (err < 0) {\n-\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n-\t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_OUTDATED_ERROR.\n-\t\t\t*/\n-\t\t\treftable_stack_reload(st);\n-\t\t}\n-\t\treturn err;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n-{\n-\tchar buf[100];\n-\tuint32_t rnd = reftable_rand();\n-\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n-\t\t min, max, rnd);\n-\treftable_buf_reset(dest);\n-\treturn reftable_buf_addstr(dest, buf);\n-}\n-\n struct reftable_addition {\n \tstruct reftable_flock tables_list_lock;\n \tstruct reftable_stack *stack;\n@@ -706,6 +666,26 @@ struct reftable_addition {\n \n #define REFTABLE_ADDITION_INIT {{0}}\n \n+static void reftable_addition_close(struct reftable_addition *add)\n+{\n+\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < add->new_tables_len; i++) {\n+\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n+\t\t\tunlink(nm.buf);\n+\t\treftable_free(add->new_tables[i]);\n+\t\tadd->new_tables[i] = NULL;\n+\t}\n+\treftable_free(add->new_tables);\n+\tadd->new_tables = NULL;\n+\tadd->new_tables_len = 0;\n+\tadd->new_tables_cap = 0;\n+\n+\tflock_release(&add->tables_list_lock);\n+\treftable_buf_release(&nm);\n+}\n+\n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st,\n \t\t\t\t\tunsigned int flags)\n@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \treturn err;\n }\n \n-static void reftable_addition_close(struct reftable_addition *add)\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+\t\t\t void *arg)\n {\n-\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n-\tsize_t i;\n+\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tfor (i = 0; i < add->new_tables_len; i++) {\n-\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n-\t\t\tunlink(nm.buf);\n-\t\treftable_free(add->new_tables[i]);\n-\t\tadd->new_tables[i] = NULL;\n+\terr = reftable_addition_add(&add, write_table, arg);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_addition_commit(&add);\n+done:\n+\treftable_addition_close(&add);\n+\treturn err;\n+}\n+\n+int reftable_stack_add(struct reftable_stack *st,\n+\t\t int (*write)(struct reftable_writer *wr, void *arg),\n+\t\t void *arg)\n+{\n+\tint err = stack_try_add(st, write, arg);\n+\tif (err < 0) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n+\t\t\t/* Ignore error return, we want to propagate\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n+\t\t\t*/\n+\t\t\treftable_stack_reload(st);\n+\t\t}\n+\t\treturn err;\n \t}\n-\treftable_free(add->new_tables);\n-\tadd->new_tables = NULL;\n-\tadd->new_tables_len = 0;\n-\tadd->new_tables_cap = 0;\n \n-\tflock_release(&add->tables_list_lock);\n-\treftable_buf_release(&nm);\n+\treturn 0;\n+}\n+\n+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n+{\n+\tchar buf[100];\n+\tuint32_t rnd = reftable_rand();\n+\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n+\t\t min, max, rnd);\n+\treftable_buf_reset(dest);\n+\treturn reftable_buf_addstr(dest, buf);\n }\n \n void reftable_addition_destroy(struct reftable_addition *add)\n@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \treturn err;\n }\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-\t\t\t void *arg)\n-{\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_add(&add, write_table, arg);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_commit(&add);\n-done:\n-\treftable_addition_close(&add);\n-\treturn err;\n-}\n-\n int reftable_addition_add(struct reftable_addition *add,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \treturn 1;\n }\n \n-static int stack_compact_locked(struct reftable_stack *st,\n-\t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct reftable_log_expiry_config *config,\n-\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n-{\n-\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n-\tstruct reftable_writer *wr = NULL;\n-\tstruct fd_writer writer= {\n-\t\t.opts = &st->opts,\n-\t};\n-\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n-\tint err = 0;\n-\n-\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n-\t\t\t reftable_table_max_update_index(st->tables[last]));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_filename(&tab_file_path, st, next_name.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\tif (st->opts.default_permissions &&\n-\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\twriter.fd = tab_file.fd;\n-\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n-\t\t\t\t &writer, &st->opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_write_compact(st, wr, first, last, config);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_writer_close(wr);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_close(&tab_file);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\t*tab_file_out = tab_file;\n-\ttab_file = REFTABLE_TMPFILE_INIT;\n-\n-done:\n-\ttmpfile_delete(&tab_file);\n-\treftable_writer_free(wr);\n-\treftable_buf_release(&next_name);\n-\treftable_buf_release(&tab_file_path);\n-\treturn err;\n-}\n-\n static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,\n \treturn err;\n }\n \n+static int stack_compact_locked(struct reftable_stack *st,\n+\t\t\t\tsize_t first, size_t last,\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n+{\n+\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *wr = NULL;\n+\tstruct fd_writer writer= {\n+\t\t.opts = &st->opts,\n+\t};\n+\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n+\tint err = 0;\n+\n+\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n+\t\t\t reftable_table_max_update_index(st->tables[last]));\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_filename(&tab_file_path, st, next_name.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\tif (st->opts.default_permissions &&\n+\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\twriter.fd = tab_file.fd;\n+\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n+\t\t\t\t &writer, &st->opts);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_write_compact(st, wr, first, last, config);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_writer_close(wr);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_close(&tab_file);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*tab_file_out = tab_file;\n+\ttab_file = REFTABLE_TMPFILE_INIT;\n+\n+done:\n+\ttmpfile_delete(&tab_file);\n+\treftable_writer_free(wr);\n+\treftable_buf_release(&next_name);\n+\treftable_buf_release(&tab_file_path);\n+\treturn err;\n+}\n+\n enum stack_compact_range_flags {\n \t/*\n \t * Perform a best-effort compaction. That is, even if we cannot lock\n\n-- \n2.50.1\n\n"},{"id":"523214","messageId":"20250801-pks-reftable-fixes-for-libgit2-v1-5-f446e1c33cb9@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH 5/5] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T14:47:51Z","receivedAt":"2025-08-01T14:48:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_add()` function is a simple wrapper to lock the\nstack, add records to it via a callback and then commit the\nresult. One problem with it though is that it doesn't accept any flags\nfor creating the addition. This makes it impossible to automatically\nreload the stack in case it was modified before we managed to lock the\nstack.\n\nAdd a `flags` field to plug this gap and pass it through accordingly.\nFor now this new flag won't be used by us, but it will be used by\nlibgit2.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 8 +++----\n reftable/reftable-stack.h | 9 +++++---\n reftable/stack.c | 8 +++----\n t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------\n 4 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4c3817f4ec..3f0deab338 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n \n done:\n \treturn ret;\n@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 910ec6ef3a..d70fcb705d 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);\n * transaction. Releases the lock if held. */\n void reftable_addition_destroy(struct reftable_addition *add);\n \n-/* add a new table to the stack. The write_table function must call\n- * reftable_writer_set_limits, add refs and return an error value. */\n+/*\n+ * Add a new table to the stack. The write_table function must call\n+ * reftable_writer_set_limits, add refs and return an error value.\n+ * The flags are passed through to `reftable_stack_new_addition()`.\n+ */\n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *write_arg),\n-\t\t void *write_arg);\n+\t\t void *write_arg, unsigned flags);\n \n struct reftable_iterator;\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex d6e4ea93a3..f77d7f58e8 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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-\t\t\t void *arg)\n+\t\t\t void *arg, unsigned flags)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tint err = reftable_stack_init_addition(&add, st, flags);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -756,9 +756,9 @@ static int stack_try_add(struct reftable_stack *st,\n \n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n+\t\t void *arg, unsigned flags)\n {\n-\tint err = stack_try_add(st, write, arg);\n+\tint err = stack_try_add(st, write, arg, flags);\n \tif (err < 0) {\n \t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 2f49c97519..ce10247903 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tref.refname = buf;\n \t\tt_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)\n \terr = reftable_new_stack(&st2, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st1, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st1, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck(!err);\n \treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 1);\n \tcheck_int(st->stats.attempts, ==, 0);\n@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \twrite_file_buf(table_path.buf, \"\", 0);\n \n \tref.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 2);\n \tcheck_int(st->stats.attempts, ==, 1);\n@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \tfor (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {\n-\t\terr = reftable_stack_add(st, write_error, &i);\n+\t\terr = reftable_stack_add(st, write_error, &i, 0);\n \t\tcheck_int(err, ==, i);\n \t}\n \n@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\terr = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\terr = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \n-\t\terr = reftable_stack_add(st, write_test_log, &arg);\n+\t\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)\n \tcheck(!err);\n \n \tinput.value.update.message = (char *) \"one\\ntwo\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tinput.value.update.message = (char *) \"one\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_log(st, input.refname, &dest);\n@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)\n \n \tinput.value.update.message = (char *) \"two\\n\";\n \targ.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \terr = reftable_stack_read_log(st, input.refname, &dest);\n \tcheck(!err);\n@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \t/* can't read it with the wrong hash ID. */\n@@ -927,7 +927,7 @@ static void t_reflog_expire(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -978,7 +978,7 @@ static void t_empty_add(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_nothing, NULL);\n+\terr = reftable_stack_add(st, write_nothing, NULL, 0);\n \tcheck(!err);\n \n \terr = reftable_new_stack(&st2, dir, &opts);\n@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)\n \t\t};\n \t\tsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_stack_auto_compact(st);\n@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)\n \t\t};\n \t\txsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\tcheck(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));\n@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)\n \t\tsnprintf(buf, sizeof(buf), \"branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf;\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\t/*\n\n-- \n2.50.1\n\n"},{"id":"523234","messageId":"CAPig+cSeYUenjTm54higv6ANCOw0RJsF1hevS_p71=sH3c+E+Q@mail.gmail.com","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-3-f446e1c33cb9@pks.im","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-01T19:19:52Z","receivedAt":"2025-08-01T19:20:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 1, 2025 at 10:48 AM Patrick Steinhardt wrote:\n> While perfectly legal, older compiler toolchains complain when\n> zero-initializing structs that contain nested structs with `{0}`:\n> [...]\n> Silence this warning by using `{{0}}` instead.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> -#define REFTABLE_ADDITION_INIT {0}\n> +#define REFTABLE_ADDITION_INIT {{0}}\n\nFor what it's worth, the last time this sort of issue came up[*], I\nbelieve the ultimate response was that the project ought not pander to\nthis particular instance of buggy compiler and that, instead, an\nindividual developer should, if bothered by the warning, use the\nappropriate compiler option to suppress the warning. Whether or not\nthat attitude should apply also to the reftable code which is meant to\nbe shareable, is a separate question, but at least that previous\ndiscussion thread provides some background regarding how different\ndevelopers feel or felt about the issue.\n\n[*]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n"},{"id":"523372","messageId":"aJBNHzE3RQBZZTcU@pks.im","threadId":"63891","inReplyTo":"CAPig+cSeYUenjTm54higv6ANCOw0RJsF1hevS_p71=sH3c+E+Q@mail.gmail.com","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T06:03:20Z","receivedAt":"2025-08-04T06:03:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 01, 2025 at 03:19:52PM -0400, Eric Sunshine wrote:\n> On Fri, Aug 1, 2025 at 10:48 AM Patrick Steinhardt wrote:\n> > While perfectly legal, older compiler toolchains complain when\n> > zero-initializing structs that contain nested structs with `{0}`:\n> > [...]\n> > Silence this warning by using `{{0}}` instead.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > -#define REFTABLE_ADDITION_INIT {0}\n> > +#define REFTABLE_ADDITION_INIT {{0}}\n> \n> For what it's worth, the last time this sort of issue came up[*], I\n> believe the ultimate response was that the project ought not pander to\n> this particular instance of buggy compiler and that, instead, an\n> individual developer should, if bothered by the warning, use the\n> appropriate compiler option to suppress the warning. Whether or not\n> that attitude should apply also to the reftable code which is meant to\n> be shareable, is a separate question, but at least that previous\n> discussion thread provides some background regarding how different\n> developers feel or felt about the issue.\n> \n> [*]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n\nYeah, in general I'm also of the opinion that we shouldn't bother. But\nin libgit2 we have pipelines that use such older compilers, and we don't\nwant to drop those for now. So I think we should treat the reftable\nlibrary specially, doubly so as this is the only instance that causes\nproblems.\n\nThanks for digging up that old thread!\n\nPatrick\n"},{"id":"523411","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH v2 0/6] reftable: a couple of improvements for libgit2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:21Z","receivedAt":"2025-08-04T09:40:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series contains a couple of improvements I required for\nlibgit2. With those changes libgit2 is now able to run its full test\nsuite with reftable-enabled repositories. I still need to invest a bit\nof work to make it memory-leak free and compile on Windows, but overall\nI think that support for reftables is almost ready.\n\nChanges in v2:\n - Another commit that fixes handling of outdated stacks when doing\n compaction. This issue is hit in libgit2, which has a test that\n performs writes with a couple dozen concurrent threads.\n - Add a link to past discussions around `{0}` vs `{{0}}` as provided\n by Eric.\n - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (6):\n reftable/writer: fix type used for number of records\n reftable/writer: drop Git-specific `QSORT()` macro\n reftable/stack: fix compiler warning due to missing braces\n reftable/stack: reorder code to avoid forward declarations\n reftable/stack: allow passing flags to `reftable_stack_add()`\n reftable/stack: handle outdated stacks when compacting\n\n refs/reftable-backend.c | 8 +-\n reftable/reftable-stack.h | 9 +-\n reftable/reftable-writer.h | 4 +-\n reftable/stack.c | 398 ++++++++++++++++++++--------------------\n reftable/writer.c | 23 ++-\n t/unit-tests/t-reftable-stack.c | 50 ++---\n 6 files changed, 253 insertions(+), 239 deletions(-)\n\nRange-diff versus v1:\n\n1: e8aacccd51 = 1: c8bba1f88b reftable/writer: fix type used for number of records\n2: b1a331bc78 = 2: 6c20e9a874 reftable/writer: drop Git-specific `QSORT()` macro\n3: 1b3556685d ! 3: c2ed4eab6a reftable/stack: fix compiler warning due to missing braces\n @@ Commit message\n \n Silence this warning by using `{{0}}` instead.\n \n + Note that we had the discussion around whether or not we want to handle\n + such errors in the past already [1], where we basically decided that we\n + do not care about such old-and-buggy compilers. But the reftable library\n + is a special case because it is used by projects other than Git, and\n + libgit2 for example hits the above issue in its pipeline. As there is\n + only a single problematic instance of this issue we do the pragmatic\n + thing and simply make the compiler happy.\n +\n + [1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n +\n Signed-off-by: Patrick Steinhardt \n \n ## reftable/stack.c ##\n4: c1fb437781 = 4: 0f1bb446ce reftable/stack: reorder code to avoid forward declarations\n5: 33dcf7ca70 = 5: 5aaff713dd reftable/stack: allow passing flags to `reftable_stack_add()`\n-: ---------- > 6: ccfa5b59a0 reftable/stack: handle outdated stacks when compacting\n\n---\nbase-commit: e813a0200a7121b97fec535f0d0b460b0a33356c\nchange-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603\n\n"},{"id":"523412","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-1-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 1/6] reftable/writer: fix type used for number of records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:22Z","receivedAt":"2025-08-04T09:40:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`\naccept an array of records that should be added to the new table.\nCallers of this function are expected to also pass the number of such\nrecords to the function to tell it how many such records it is supposed\nto write.\n\nBut while all callers pass in a `size_t`, which is a sensible choice,\nthe function in fact accepts an `int` as argument, which is less so. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-writer.h | 4 ++--\n reftable/writer.c | 17 +++++++++--------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 0fbeff17f4..1e7003cd69 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n the records before adding them, reordering the records array passed in.\n */\n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n);\n+\t\t\t struct reftable_ref_record *refs, size_t n);\n \n /*\n adds reftable_log_records. Log records are keyed by (refname, decreasing\n@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,\n the records before adding them, reordering records array passed in.\n */\n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n);\n+\t\t\t struct reftable_log_record *logs, size_t n);\n \n /* reftable_writer_close finalizes the reftable. The writer is retained so\n * statistics can be inspected. */\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 3b4ebdd6dc..5bad130c7e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n }\n \n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n)\n+\t\t\t struct reftable_ref_record *refs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(refs, n, reftable_ref_record_compare_name);\n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n-\t}\n+\n \treturn err;\n }\n \n@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,\n }\n \n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n)\n+\t\t\t struct reftable_log_record *logs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(logs, n, reftable_log_record_compare_key);\n \n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n-\t}\n+\n \treturn err;\n }\n \n\n-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523413","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-2-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 2/6] reftable/writer: drop Git-specific `QSORT()` macro","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:23Z","receivedAt":"2025-08-04T09:40:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer accidentally uses the Git-specific `QSORT()` macro.\nThis macro removes the need for the caller to provide the element size,\nbut other than that it's mostly equivalent to `qsort()`.\n\nReplace the macro accordingly to make the library usable outside of Git.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 5bad130c7e..0133b64975 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(refs, n, reftable_ref_record_compare_name);\n+\tif (n)\n+\t\tqsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(logs, n, reftable_log_record_compare_key);\n+\tif (n)\n+\t\tqsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n\n-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523414","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-3-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 3/6] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:24Z","receivedAt":"2025-08-04T09:40:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While perfectly legal, older compiler toolchains complain when\nzero-initializing structs that contain nested structs with `{0}`:\n\n /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]\n struct reftable_addition empty = REFTABLE_ADDITION_INIT;\n ^~~~~~~~~~~~~~~~~~~~~~\n /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'\n #define REFTABLE_ADDITION_INIT {0}\n ^\n\nSilence this warning by using `{{0}}` instead.\n\nNote that we had the discussion around whether or not we want to handle\nsuch errors in the past already [1], where we basically decided that we\ndo not care about such old-and-buggy compilers. But the reftable library\nis a special case because it is used by projects other than Git, and\nlibgit2 for example hits the above issue in its pipeline. As there is\nonly a single problematic instance of this issue we do the pragmatic\nthing and simply make the compiler happy.\n\n[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 4caf96aa1d..3480ad21c3 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -704,7 +704,7 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT {0}\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-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523415","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-4-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 4/6] reftable/stack: reorder code to avoid forward declarations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:25Z","receivedAt":"2025-08-04T09:40:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a couple of forward declarations in the stack-related code of\nthe reftable library. These declarations aren't really required, but are\nsimply caused by unfortunate ordering.\n\nReorder the code and remove the forward declarations.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 176 insertions(+), 188 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 3480ad21c3..d6e4ea93a3 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,18 +17,6 @@\n #include \"table.h\"\n #include \"writer.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-\t\t\t void *arg);\n-static int stack_write_compact(struct reftable_stack *st,\n-\t\t\t struct reftable_writer *wr,\n-\t\t\t size_t first, size_t last,\n-\t\t\t struct reftable_log_expiry_config *config);\n-static void reftable_addition_close(struct reftable_addition *add);\n-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n-\t\t\t\t\t int reuse_open);\n-\n static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,\n \t\t\t const char *name)\n {\n@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)\n \treturn stack_fsync(writer->opts, writer->fd);\n }\n \n-int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n-\t\t const struct reftable_write_options *_opts)\n-{\n-\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct reftable_stack *p;\n-\tint err;\n-\n-\tp = reftable_calloc(1, sizeof(*p));\n-\tif (!p) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\tif (_opts)\n-\t\topts = *_opts;\n-\tif (opts.hash_id == 0)\n-\t\topts.hash_id = REFTABLE_HASH_SHA1;\n-\n-\t*dest = NULL;\n-\n-\treftable_buf_reset(&list_file_name);\n-\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n-\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n-\t\tgoto out;\n-\n-\tp->list_file = reftable_buf_detach(&list_file_name);\n-\tp->list_fd = -1;\n-\tp->opts = opts;\n-\tp->reftable_dir = reftable_strdup(dir);\n-\tif (!p->reftable_dir) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\terr = reftable_stack_reload_maybe_reuse(p, 1);\n-\tif (err < 0)\n-\t\tgoto out;\n-\n-\t*dest = p;\n-\terr = 0;\n-\n-out:\n-\tif (err < 0)\n-\t\treftable_stack_destroy(p);\n-\treturn err;\n-}\n-\n static int fd_read_lines(int fd, char ***namesp)\n {\n \tchar *buf = NULL;\n@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \treturn err;\n }\n \n+int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n+\t\t const struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *p;\n+\tint err;\n+\n+\tp = reftable_calloc(1, sizeof(*p));\n+\tif (!p) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\tif (opts.hash_id == 0)\n+\t\topts.hash_id = REFTABLE_HASH_SHA1;\n+\n+\t*dest = NULL;\n+\n+\treftable_buf_reset(&list_file_name);\n+\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n+\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n+\t\tgoto out;\n+\n+\tp->list_file = reftable_buf_detach(&list_file_name);\n+\tp->list_fd = -1;\n+\tp->opts = opts;\n+\tp->reftable_dir = reftable_strdup(dir);\n+\tif (!p->reftable_dir) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\terr = reftable_stack_reload_maybe_reuse(p, 1);\n+\tif (err < 0)\n+\t\tgoto out;\n+\n+\t*dest = p;\n+\terr = 0;\n+\n+out:\n+\tif (err < 0)\n+\t\treftable_stack_destroy(p);\n+\treturn err;\n+}\n+\n /* -1 = error\n 0 = up to date\n 1 = changed. */\n@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)\n \treturn err;\n }\n \n-int reftable_stack_add(struct reftable_stack *st,\n-\t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n-{\n-\tint err = stack_try_add(st, write, arg);\n-\tif (err < 0) {\n-\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n-\t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_OUTDATED_ERROR.\n-\t\t\t*/\n-\t\t\treftable_stack_reload(st);\n-\t\t}\n-\t\treturn err;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n-{\n-\tchar buf[100];\n-\tuint32_t rnd = reftable_rand();\n-\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n-\t\t min, max, rnd);\n-\treftable_buf_reset(dest);\n-\treturn reftable_buf_addstr(dest, buf);\n-}\n-\n struct reftable_addition {\n \tstruct reftable_flock tables_list_lock;\n \tstruct reftable_stack *stack;\n@@ -706,6 +666,26 @@ struct reftable_addition {\n \n #define REFTABLE_ADDITION_INIT {{0}}\n \n+static void reftable_addition_close(struct reftable_addition *add)\n+{\n+\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < add->new_tables_len; i++) {\n+\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n+\t\t\tunlink(nm.buf);\n+\t\treftable_free(add->new_tables[i]);\n+\t\tadd->new_tables[i] = NULL;\n+\t}\n+\treftable_free(add->new_tables);\n+\tadd->new_tables = NULL;\n+\tadd->new_tables_len = 0;\n+\tadd->new_tables_cap = 0;\n+\n+\tflock_release(&add->tables_list_lock);\n+\treftable_buf_release(&nm);\n+}\n+\n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st,\n \t\t\t\t\tunsigned int flags)\n@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \treturn err;\n }\n \n-static void reftable_addition_close(struct reftable_addition *add)\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+\t\t\t void *arg)\n {\n-\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n-\tsize_t i;\n+\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tfor (i = 0; i < add->new_tables_len; i++) {\n-\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n-\t\t\tunlink(nm.buf);\n-\t\treftable_free(add->new_tables[i]);\n-\t\tadd->new_tables[i] = NULL;\n+\terr = reftable_addition_add(&add, write_table, arg);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_addition_commit(&add);\n+done:\n+\treftable_addition_close(&add);\n+\treturn err;\n+}\n+\n+int reftable_stack_add(struct reftable_stack *st,\n+\t\t int (*write)(struct reftable_writer *wr, void *arg),\n+\t\t void *arg)\n+{\n+\tint err = stack_try_add(st, write, arg);\n+\tif (err < 0) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n+\t\t\t/* Ignore error return, we want to propagate\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n+\t\t\t*/\n+\t\t\treftable_stack_reload(st);\n+\t\t}\n+\t\treturn err;\n \t}\n-\treftable_free(add->new_tables);\n-\tadd->new_tables = NULL;\n-\tadd->new_tables_len = 0;\n-\tadd->new_tables_cap = 0;\n \n-\tflock_release(&add->tables_list_lock);\n-\treftable_buf_release(&nm);\n+\treturn 0;\n+}\n+\n+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n+{\n+\tchar buf[100];\n+\tuint32_t rnd = reftable_rand();\n+\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n+\t\t min, max, rnd);\n+\treftable_buf_reset(dest);\n+\treturn reftable_buf_addstr(dest, buf);\n }\n \n void reftable_addition_destroy(struct reftable_addition *add)\n@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \treturn err;\n }\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-\t\t\t void *arg)\n-{\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_add(&add, write_table, arg);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_commit(&add);\n-done:\n-\treftable_addition_close(&add);\n-\treturn err;\n-}\n-\n int reftable_addition_add(struct reftable_addition *add,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \treturn 1;\n }\n \n-static int stack_compact_locked(struct reftable_stack *st,\n-\t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct reftable_log_expiry_config *config,\n-\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n-{\n-\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n-\tstruct reftable_writer *wr = NULL;\n-\tstruct fd_writer writer= {\n-\t\t.opts = &st->opts,\n-\t};\n-\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n-\tint err = 0;\n-\n-\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n-\t\t\t reftable_table_max_update_index(st->tables[last]));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_filename(&tab_file_path, st, next_name.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\tif (st->opts.default_permissions &&\n-\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\twriter.fd = tab_file.fd;\n-\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n-\t\t\t\t &writer, &st->opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_write_compact(st, wr, first, last, config);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_writer_close(wr);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_close(&tab_file);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\t*tab_file_out = tab_file;\n-\ttab_file = REFTABLE_TMPFILE_INIT;\n-\n-done:\n-\ttmpfile_delete(&tab_file);\n-\treftable_writer_free(wr);\n-\treftable_buf_release(&next_name);\n-\treftable_buf_release(&tab_file_path);\n-\treturn err;\n-}\n-\n static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,\n \treturn err;\n }\n \n+static int stack_compact_locked(struct reftable_stack *st,\n+\t\t\t\tsize_t first, size_t last,\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n+{\n+\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *wr = NULL;\n+\tstruct fd_writer writer= {\n+\t\t.opts = &st->opts,\n+\t};\n+\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n+\tint err = 0;\n+\n+\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n+\t\t\t reftable_table_max_update_index(st->tables[last]));\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_filename(&tab_file_path, st, next_name.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\tif (st->opts.default_permissions &&\n+\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\twriter.fd = tab_file.fd;\n+\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n+\t\t\t\t &writer, &st->opts);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_write_compact(st, wr, first, last, config);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_writer_close(wr);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_close(&tab_file);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*tab_file_out = tab_file;\n+\ttab_file = REFTABLE_TMPFILE_INIT;\n+\n+done:\n+\ttmpfile_delete(&tab_file);\n+\treftable_writer_free(wr);\n+\treftable_buf_release(&next_name);\n+\treftable_buf_release(&tab_file_path);\n+\treturn err;\n+}\n+\n enum stack_compact_range_flags {\n \t/*\n \t * Perform a best-effort compaction. That is, even if we cannot lock\n\n-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523416","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-5-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:26Z","receivedAt":"2025-08-04T09:40:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_add()` function is a simple wrapper to lock the\nstack, add records to it via a callback and then commit the\nresult. One problem with it though is that it doesn't accept any flags\nfor creating the addition. This makes it impossible to automatically\nreload the stack in case it was modified before we managed to lock the\nstack.\n\nAdd a `flags` field to plug this gap and pass it through accordingly.\nFor now this new flag won't be used by us, but it will be used by\nlibgit2.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 8 +++----\n reftable/reftable-stack.h | 9 +++++---\n reftable/stack.c | 8 +++----\n t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------\n 4 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4c3817f4ec1..3f0deab338c 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n \n done:\n \treturn ret;\n@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 910ec6ef3a2..d70fcb705dc 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);\n * transaction. Releases the lock if held. */\n void reftable_addition_destroy(struct reftable_addition *add);\n \n-/* add a new table to the stack. The write_table function must call\n- * reftable_writer_set_limits, add refs and return an error value. */\n+/*\n+ * Add a new table to the stack. The write_table function must call\n+ * reftable_writer_set_limits, add refs and return an error value.\n+ * The flags are passed through to `reftable_stack_new_addition()`.\n+ */\n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *write_arg),\n-\t\t void *write_arg);\n+\t\t void *write_arg, unsigned flags);\n \n struct reftable_iterator;\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex d6e4ea93a37..f77d7f58e8e 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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-\t\t\t void *arg)\n+\t\t\t void *arg, unsigned flags)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tint err = reftable_stack_init_addition(&add, st, flags);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -756,9 +756,9 @@ static int stack_try_add(struct reftable_stack *st,\n \n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n+\t\t void *arg, unsigned flags)\n {\n-\tint err = stack_try_add(st, write, arg);\n+\tint err = stack_try_add(st, write, arg, flags);\n \tif (err < 0) {\n \t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 2f49c975194..ce10247903c 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tref.refname = buf;\n \t\tt_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)\n \terr = reftable_new_stack(&st2, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st1, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st1, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck(!err);\n \treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 1);\n \tcheck_int(st->stats.attempts, ==, 0);\n@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \twrite_file_buf(table_path.buf, \"\", 0);\n \n \tref.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 2);\n \tcheck_int(st->stats.attempts, ==, 1);\n@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \tfor (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {\n-\t\terr = reftable_stack_add(st, write_error, &i);\n+\t\terr = reftable_stack_add(st, write_error, &i, 0);\n \t\tcheck_int(err, ==, i);\n \t}\n \n@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\terr = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\terr = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \n-\t\terr = reftable_stack_add(st, write_test_log, &arg);\n+\t\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)\n \tcheck(!err);\n \n \tinput.value.update.message = (char *) \"one\\ntwo\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tinput.value.update.message = (char *) \"one\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_log(st, input.refname, &dest);\n@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)\n \n \tinput.value.update.message = (char *) \"two\\n\";\n \targ.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \terr = reftable_stack_read_log(st, input.refname, &dest);\n \tcheck(!err);\n@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \t/* can't read it with the wrong hash ID. */\n@@ -927,7 +927,7 @@ static void t_reflog_expire(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -978,7 +978,7 @@ static void t_empty_add(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_nothing, NULL);\n+\terr = reftable_stack_add(st, write_nothing, NULL, 0);\n \tcheck(!err);\n \n \terr = reftable_new_stack(&st2, dir, &opts);\n@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)\n \t\t};\n \t\tsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_stack_auto_compact(st);\n@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)\n \t\t};\n \t\txsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\tcheck(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));\n@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)\n \t\tsnprintf(buf, sizeof(buf), \"branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf;\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\t/*\n\n-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523417","messageId":"20250804-pks-reftable-fixes-for-libgit2-v2-6-fef06209a984@pks.im","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im","subject":"[PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T09:40:27Z","receivedAt":"2025-08-04T09:40:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we compact the reftable stack we first acquire the lock for the\n\"tables.list\" file and then reload the stack to check that it is still\nup-to-date. This is done by calling `stack_uptodate()`, which knows to\nreturn zero in case the stack is up-to-date, a positive value if it is\nnot and a negative error code on unexpected conditions.\n\nWe don't do proper error checking though, but instead we only check\nwhether the returned error code is non-zero. If so, we simply bubble it\nup the calling stack, which means that callers may see an unexpected\npositive value.\n\nFix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.\nHandle this situation in `reftable_addition_commit()`, where we perform\na best-effort auto-compaction.\n\nAll other callsites of `stack_uptodate()` know to handle a positive\nreturn value and thus don't need to be fixed.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 32 ++++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f77d7f58e8..effa2fc8cb 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \treturn err;\n }\n \n-/* -1 = error\n- 0 = up to date\n- 1 = changed. */\n+/*\n+ * Check whether the given stack is up-to-date with what we have in memory.\n+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n+ * otherwise.\n+ */\n static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n@@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\t * control. It is possible that a concurrent writer is already\n \t\t * trying to compact parts of the stack, which would lead to a\n \t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n-\t\t * already. This is a benign error though, so we ignore it.\n+\t\t * already. Similarly, the stack may have been rewritten by a\n+\t\t * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.\n+\t\t * Both of these errors are benign, so we simply ignore them.\n \t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n-\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n+\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR &&\n+\t\t err != REFTABLE_OUTDATED_ERROR)\n \t\t\tgoto done;\n \t\terr = 0;\n \t}\n@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * Check whether the stack is up-to-date. We unfortunately cannot\n+\t * handle the situation gracefully in case it's _not_ up-to-date\n+\t * because the range of tables that the user has requested us to\n+\t * compact may have been changed. So instead we abort.\n+\t *\n+\t * We could in theory improve the situation by having the caller not\n+\t * pass in a range, but instead the list of tables to compact. If so,\n+\t * we could check that relevant tables still exist. But for now it's\n+\t * good enough to just abort.\n+\t */\n \terr = stack_uptodate(st);\n-\tif (err)\n+\tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0) {\n+\t\terr = REFTABLE_OUTDATED_ERROR;\n+\t\tgoto done;\n+\t}\n \n \t/*\n \t * Lock all tables in the user-provided range. This is the slice of our\n\n-- \n2.50.1.723.g3e08bea96f.dirty\n\n"},{"id":"523475","messageId":"xmqqwm7i527l.fsf@gitster.g","threadId":"63891","inReplyTo":"aJBNHzE3RQBZZTcU@pks.im","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-04T19:14:22Z","receivedAt":"2025-08-04T19:14:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Yeah, in general I'm also of the opinion that we shouldn't bother. But\n> in libgit2 we have pipelines that use such older compilers, and we don't\n> want to drop those for now. So I think we should treat the reftable\n> library specially, doubly so as this is the only instance that causes\n> problems.\n\nHmph. Shouldn't there be some kind of \"shim\" layer where these\nthings are defined per project convention and/or toolchain being\nused? So when building for git proper, you'd use {0} just as\neverybody else do, but for others your include file supplied by that\nproject would use something else (like {{0}} in this case)? That\nkind of approach would be a better solution than open coding QSORT()\nin the longer term, for example.\n\n\n"},{"id":"523505","messageId":"aJGNaQwnd6_A0Ppw@pks.im","threadId":"63891","inReplyTo":"xmqqwm7i527l.fsf@gitster.g","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-05T04:49:45Z","receivedAt":"2025-08-05T04:49:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Yeah, in general I'm also of the opinion that we shouldn't bother. But\n> > in libgit2 we have pipelines that use such older compilers, and we don't\n> > want to drop those for now. So I think we should treat the reftable\n> > library specially, doubly so as this is the only instance that causes\n> > problems.\n> \n> Hmph. Shouldn't there be some kind of \"shim\" layer where these\n> things are defined per project convention and/or toolchain being\n> used? So when building for git proper, you'd use {0} just as\n> everybody else do, but for others your include file supplied by that\n> project would use something else (like {{0}} in this case)? That\n> kind of approach would be a better solution than open coding QSORT()\n> in the longer term, for example.\n\nWe do have a shim layer, but I don't think it makes sense to use it for\nevery small piece. The intent of that layer is to paper over platform\ndifferences that we cannot easily hide away in a generic fashion. So\nthings like mmap, random numbers, handling includes or registering\nlockfiles via atexit(3p).\n\nBut I don't think it makes sense to use the shim layer for things like\n`{0}` vs `{{0}}` or `QSORT()`. It doesn't really have any downside to\nchange to `{{0}}`, and the one invocation of `QSORT()` is trivial to\nconvert to qsort(3p) and unlikely to cause any more maintenance in the\nlong run than the invocation of `QSORT()` would.\n\nOn the other hand, it does have a downside if we moved all of this into\nthe shim layer, because now every user of the reftable library will have\nto reinvent the wheel. The other downside is that the more we move into\nthe shim layer, the more the different implementations of the reftable\nbackend may diverge in behaviour. Reversely, the more code is the exact\nsame across implementations the more we can benefit from the tests that\nthose other implementations use and the more solid the reftable library\nbecomes.\n\nPatrick\n"},{"id":"523964","messageId":"gisipxu5fdge3762fddx4qyiccwdtg34a42uiv4lpevuwgmsbz@lcj7fcifogvk","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-4-fef06209a984@pks.im","subject":"Re: [PATCH v2 4/6] reftable/stack: reorder code to avoid forward declarations","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-11T19:10:16Z","receivedAt":"2025-08-11T19:10:19Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/04 11:40AM, Patrick Steinhardt wrote:\n> We have a couple of forward declarations in the stack-related code of\n> the reftable library. These declarations aren't really required, but are\n> simply caused by unfortunate ordering.\n> \n> Reorder the code and remove the forward declarations.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------\n> 1 file changed, 176 insertions(+), 188 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 3480ad21c3..d6e4ea93a3 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -17,18 +17,6 @@\n> #include \"table.h\"\n> #include \"writer.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> -\t\t\t void *arg);\n> -static int stack_write_compact(struct reftable_stack *st,\n> -\t\t\t struct reftable_writer *wr,\n> -\t\t\t size_t first, size_t last,\n> -\t\t\t struct reftable_log_expiry_config *config);\n> -static void reftable_addition_close(struct reftable_addition *add);\n> -static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> -\t\t\t\t\t int reuse_open);\n> -\n\nNice cleanup :)\n"},{"id":"523966","messageId":"6gcuu35ey34nrxtsgpelfykvgxo7vqqhgsq3obnbducmplaymv@blgaxqyp6low","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-5-fef06209a984@pks.im","subject":"Re: [PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-11T19:34:49Z","receivedAt":"2025-08-11T19:34:51Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/04 11:40AM, Patrick Steinhardt wrote:\n> The `reftable_stack_add()` function is a simple wrapper to lock the\n> stack, add records to it via a callback and then commit the\n> result. One problem with it though is that it doesn't accept any flags\n> for creating the addition. This makes it impossible to automatically\n> reload the stack in case it was modified before we managed to lock the\n> stack.\n> \n> Add a `flags` field to plug this gap and pass it through accordingly.\n> For now this new flag won't be used by us, but it will be used by\n> libgit2.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/reftable-backend.c | 8 +++----\n> reftable/reftable-stack.h | 9 +++++---\n> reftable/stack.c | 8 +++----\n> t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------\n> 4 files changed, 39 insertions(+), 36 deletions(-)\n> \n[snip]\n> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n> index 910ec6ef3a2..d70fcb705dc 100644\n> --- a/reftable/reftable-stack.h\n> +++ b/reftable/reftable-stack.h\n> @@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);\n> * transaction. Releases the lock if held. */\n> void reftable_addition_destroy(struct reftable_addition *add);\n> \n> -/* add a new table to the stack. The write_table function must call\n> - * reftable_writer_set_limits, add refs and return an error value. */\n> +/*\n> + * Add a new table to the stack. The write_table function must call\n> + * reftable_writer_set_limits, add refs and return an error value.\n> + * The flags are passed through to `reftable_stack_new_addition()`.\n> + */\n> int reftable_stack_add(struct reftable_stack *st,\n> \t\t int (*write_table)(struct reftable_writer *wr,\n> \t\t\t\t\t void *write_arg),\n> -\t\t void *write_arg);\n> +\t\t void *write_arg, unsigned flags);\n> \n> struct reftable_iterator;\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index d6e4ea93a37..f77d7f58e8e 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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> -\t\t\t void *arg)\n> +\t\t\t void *arg, unsigned flags)\n> {\n> \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n> -\tint err = reftable_stack_init_addition(&add, st, 0);\n> +\tint err = reftable_stack_init_addition(&add, st, flags);\n\nOk, so now if the `REFTABLE_STACK_NEW_ADDITION_RELOAD` flag is provided,\nreftable_stack_init_addition() will attempt to reload the stack if it is\noutdated before locking the stack. I assume Git itself hasn't needed\nthis because it just uses reftable_stack_new_addition() directly when\nneccessary.\n\nThis patch looks good.\n\n-Justin\n"},{"id":"523967","messageId":"3vkwdsqcas22um2djaic43ntapedwz4wqnrrnhvhngqdul2vmj@2rwfqngzyuup","threadId":"63891","inReplyTo":"20250804-pks-reftable-fixes-for-libgit2-v2-6-fef06209a984@pks.im","subject":"Re: [PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-11T20:04:05Z","receivedAt":"2025-08-11T20:04:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/04 11:40AM, Patrick Steinhardt wrote:\n> When we compact the reftable stack we first acquire the lock for the\n> \"tables.list\" file and then reload the stack to check that it is still\n> up-to-date. This is done by calling `stack_uptodate()`, which knows to\n> return zero in case the stack is up-to-date, a positive value if it is\n> not and a negative error code on unexpected conditions.\n\nSo `stack_uptodate()` returns a negative value for error cases and a\npositive value if the stack is out of date. `REFTABLE_OUTDATED_ERROR` is\nreally also an error, but it is special cased to differentiate it from\nthe others.\n\n> We don't do proper error checking though, but instead we only check\n> whether the returned error code is non-zero. If so, we simply bubble it\n> up the calling stack, which means that callers may see an unexpected\n> positive value.\n> \n> Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.\n> Handle this situation in `reftable_addition_commit()`, where we perform\n> a best-effort auto-compaction.\n> \n> All other callsites of `stack_uptodate()` know to handle a positive\n> return value and thus don't need to be fixed.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack.c | 32 ++++++++++++++++++++++++++------\n> 1 file changed, 26 insertions(+), 6 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index f77d7f58e8..effa2fc8cb 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n> \treturn err;\n> }\n> \n> -/* -1 = error\n> - 0 = up to date\n> - 1 = changed. */\n> +/*\n> + * Check whether the given stack is up-to-date with what we have in memory.\n> + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n> + * otherwise.\n> + */\n> static int stack_uptodate(struct reftable_stack *st)\n> {\n> \tchar **names = NULL;\n> @@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)\n> \t\t * control. It is possible that a concurrent writer is already\n> \t\t * trying to compact parts of the stack, which would lead to a\n> \t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n> -\t\t * already. This is a benign error though, so we ignore it.\n> +\t\t * already. Similarly, the stack may have been rewritten by a\n> +\t\t * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.\n> +\t\t * Both of these errors are benign, so we simply ignore them.\n> \t\t */\n> \t\terr = reftable_stack_auto_compact(add->stack);\n> -\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n> +\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR &&\n> +\t\t err != REFTABLE_OUTDATED_ERROR)\n> \t\t\tgoto done;\n> \t\terr = 0;\n> \t}\n> @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t\tgoto done;\n> \t}\n> \n> +\t/*\n> +\t * Check whether the stack is up-to-date. We unfortunately cannot\n> +\t * handle the situation gracefully in case it's _not_ up-to-date\n> +\t * because the range of tables that the user has requested us to\n> +\t * compact may have been changed. So instead we abort.\n> +\t *\n> +\t * We could in theory improve the situation by having the caller not\n> +\t * pass in a range, but instead the list of tables to compact. If so,\n> +\t * we could check that relevant tables still exist. But for now it's\n> +\t * good enough to just abort.\n> +\t */\n> \terr = stack_uptodate(st);\n> -\tif (err)\n> +\tif (err < 0)\n> \t\tgoto done;\n> +\tif (err > 0) {\n> +\t\terr = REFTABLE_OUTDATED_ERROR;\n> +\t\tgoto done;\n> +\t}\n\nI was thinking that maybe `stack_uptodate()` could maybe handle\nreturning the `REFTABLE_OUTDATED_ERROR` directly so we could avoid\nhaving to map the error here. This could require callers to check for\n`err == REFTABLE_OUTDATED_ERROR` instead of `err > 0`. Probably not a\nbig deal either way though.\n\nOtherwise this looks good to me :)\n\n-Justin\n\n> \n> \t/*\n> \t * Lock all tables in the user-provided range. This is the slice of our\n> \n> -- \n> 2.50.1.723.g3e08bea96f.dirty\n> \n> \n"},{"id":"524010","messageId":"qruwf2zjl2uvf33mp4ajklvgx7wq7ctghu53rxzbgndfojudvh@ylr4otznu2og","threadId":"63891","inReplyTo":"aJGNaQwnd6_A0Ppw@pks.im","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2025-08-12T04:18:09Z","receivedAt":"2025-08-12T04:18:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Aug 05, 2025 at 06:49:45AM -0800, Patrick Steinhardt wrote:\n> On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > \n> > > Yeah, in general I'm also of the opinion that we shouldn't bother. But\n> > > in libgit2 we have pipelines that use such older compilers, and we don't\n> > > want to drop those for now. So I think we should treat the reftable\n> > > library specially, doubly so as this is the only instance that causes\n> > > problems.\n> > \n> > Hmph. Shouldn't there be some kind of \"shim\" layer where these\n> > things are defined per project convention and/or toolchain being\n> > used? So when building for git proper, you'd use {0} just as\n> > everybody else do, but for others your include file supplied by that\n> > project would use something else (like {{0}} in this case)? That\n> > kind of approach would be a better solution than open coding QSORT()\n> > in the longer term, for example.\n> \n> We do have a shim layer, but I don't think it makes sense to use it for\n> every small piece. The intent of that layer is to paper over platform\n> differences that we cannot easily hide away in a generic fashion. So\n> things like mmap, random numbers, handling includes or registering\n> lockfiles via atexit(3p).\n> \n> But I don't think it makes sense to use the shim layer for things like\n> `{0}` vs `{{0}}`\n\nI think the suggestion for using a shim layer solution is relevant, because\nadditionally to the compatibility issues of the zero initializer, you also\nneed to take into consideration that the proposed solution will still trigger\nwarnings when compiled as C++ (where {0} should be instead {}).\n\nWhy not do instead something like?\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 4caf96aa1d..80ce8a7083 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -704,8 +704,6 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \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 \t\t\t\t\tunsigned int flags)\n@@ -859,7 +857,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \t\t\t\tunsigned int flags)\n {\n \tint err = 0;\n-\tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n+\tstatic const struct reftable_addition empty;\n \n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \tif (!*dest)\n@@ -879,8 +877,11 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t\t\t void *arg),\n \t\t\t void *arg)\n {\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tstruct reftable_addition add;\n+\tint err;\n+\n+\tmemset(&add, 0, sizeof(add));\n+\terr = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \nCarlo\n"},{"id":"524022","messageId":"aJsJE3UzKIVeg3di@pks.im","threadId":"63891","inReplyTo":"qruwf2zjl2uvf33mp4ajklvgx7wq7ctghu53rxzbgndfojudvh@ylr4otznu2og","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:27:47Z","receivedAt":"2025-08-12T09:27:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 11, 2025 at 09:18:09PM -0700, Carlo Arenas wrote:\n> On Tue, Aug 05, 2025 at 06:49:45AM -0800, Patrick Steinhardt wrote:\n> > On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:\n> > > Patrick Steinhardt writes:\n> > > \n> > > > Yeah, in general I'm also of the opinion that we shouldn't bother. But\n> > > > in libgit2 we have pipelines that use such older compilers, and we don't\n> > > > want to drop those for now. So I think we should treat the reftable\n> > > > library specially, doubly so as this is the only instance that causes\n> > > > problems.\n> > > \n> > > Hmph. Shouldn't there be some kind of \"shim\" layer where these\n> > > things are defined per project convention and/or toolchain being\n> > > used? So when building for git proper, you'd use {0} just as\n> > > everybody else do, but for others your include file supplied by that\n> > > project would use something else (like {{0}} in this case)? That\n> > > kind of approach would be a better solution than open coding QSORT()\n> > > in the longer term, for example.\n> > \n> > We do have a shim layer, but I don't think it makes sense to use it for\n> > every small piece. The intent of that layer is to paper over platform\n> > differences that we cannot easily hide away in a generic fashion. So\n> > things like mmap, random numbers, handling includes or registering\n> > lockfiles via atexit(3p).\n> > \n> > But I don't think it makes sense to use the shim layer for things like\n> > `{0}` vs `{{0}}`\n> \n> I think the suggestion for using a shim layer solution is relevant, because\n> additionally to the compatibility issues of the zero initializer, you also\n> need to take into consideration that the proposed solution will still trigger\n> warnings when compiled as C++ (where {0} should be instead {}).\n\nDo we even support compiling Git as C++?\n\n> Why not do instead something like?\n\nBut this is fair indeed -- the definition is internal anyway and not\nexposed to outside callers, so we can just as well use `memset()`.\n\nPatrick\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 4caf96aa1d..80ce8a7083 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -704,8 +704,6 @@ struct reftable_addition {\n> \tuint64_t next_update_index;\n> };\n> \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> \t\t\t\t\tunsigned int flags)\n> @@ -859,7 +857,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n> \t\t\t\tunsigned int flags)\n> {\n> \tint err = 0;\n> -\tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n> +\tstatic const struct reftable_addition empty;\n> \n> \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n> \tif (!*dest)\n> @@ -879,8 +877,11 @@ static int stack_try_add(struct reftable_stack *st,\n> \t\t\t\t\t void *arg),\n> \t\t\t void *arg)\n> {\n> -\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n> -\tint err = reftable_stack_init_addition(&add, st, 0);\n> +\tstruct reftable_addition add;\n> +\tint err;\n> +\n> +\tmemset(&add, 0, sizeof(add));\n> +\terr = reftable_stack_init_addition(&add, st, 0);\n> \tif (err < 0)\n> \t\tgoto done;\n> \n> Carlo\n"},{"id":"524023","messageId":"aJsJGgw-XdeYZ_xZ@pks.im","threadId":"63891","inReplyTo":"6gcuu35ey34nrxtsgpelfykvgxo7vqqhgsq3obnbducmplaymv@blgaxqyp6low","subject":"Re: [PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:27:54Z","receivedAt":"2025-08-12T09:27:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 11, 2025 at 02:34:49PM -0500, Justin Tobler wrote:\n> On 25/08/04 11:40AM, Patrick Steinhardt wrote:\n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index d6e4ea93a37..f77d7f58e8e 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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> > -\t\t\t void *arg)\n> > +\t\t\t void *arg, unsigned flags)\n> > {\n> > \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n> > -\tint err = reftable_stack_init_addition(&add, st, 0);\n> > +\tint err = reftable_stack_init_addition(&add, st, flags);\n> \n> Ok, so now if the `REFTABLE_STACK_NEW_ADDITION_RELOAD` flag is provided,\n> reftable_stack_init_addition() will attempt to reload the stack if it is\n> outdated before locking the stack. I assume Git itself hasn't needed\n> this because it just uses reftable_stack_new_addition() directly when\n> neccessary.\n\nNot in all code paths, and arguably Git should pass the flag in those\ncode paths that don't yet. We already perform all verification under the\nlock anyway, so this would be the right thing to do.\n\nIn this patch series I wanted to focus on improvements to the library\nfor now, not to the backend. But honestly, there isn't really a good\nreason to not already fix this while we're working on this.\n\nPatrick\n"},{"id":"524024","messageId":"aJsJIjzOnfNiD6km@pks.im","threadId":"63891","inReplyTo":"3vkwdsqcas22um2djaic43ntapedwz4wqnrrnhvhngqdul2vmj@2rwfqngzyuup","subject":"Re: [PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:28:02Z","receivedAt":"2025-08-12T09:28:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 11, 2025 at 03:04:05PM -0500, Justin Tobler wrote:\n> On 25/08/04 11:40AM, Patrick Steinhardt wrote:\n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index f77d7f58e8..effa2fc8cb 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,\n> > \t\tgoto done;\n> > \t}\n> > \n> > +\t/*\n> > +\t * Check whether the stack is up-to-date. We unfortunately cannot\n> > +\t * handle the situation gracefully in case it's _not_ up-to-date\n> > +\t * because the range of tables that the user has requested us to\n> > +\t * compact may have been changed. So instead we abort.\n> > +\t *\n> > +\t * We could in theory improve the situation by having the caller not\n> > +\t * pass in a range, but instead the list of tables to compact. If so,\n> > +\t * we could check that relevant tables still exist. But for now it's\n> > +\t * good enough to just abort.\n> > +\t */\n> > \terr = stack_uptodate(st);\n> > -\tif (err)\n> > +\tif (err < 0)\n> > \t\tgoto done;\n> > +\tif (err > 0) {\n> > +\t\terr = REFTABLE_OUTDATED_ERROR;\n> > +\t\tgoto done;\n> > +\t}\n> \n> I was thinking that maybe `stack_uptodate()` could maybe handle\n> returning the `REFTABLE_OUTDATED_ERROR` directly so we could avoid\n> having to map the error here. This could require callers to check for\n> `err == REFTABLE_OUTDATED_ERROR` instead of `err > 0`. Probably not a\n> big deal either way though.\n\nWe probably could, but I don't see a strong-enough reason to do so now.\n\nPatrick\n"},{"id":"524025","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH v3 0/8] reftable: a couple of improvements for libgit2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:14Z","receivedAt":"2025-08-12T09:54:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series contains a couple of improvements I required for\nlibgit2. With those changes libgit2 is now able to run its full test\nsuite with reftable-enabled repositories. I still need to invest a bit\nof work to make it memory-leak free and compile on Windows, but overall\nI think that support for reftables is almost ready.\n\nChanges in v2:\n - Another commit that fixes handling of outdated stacks when doing\n compaction. This issue is hit in libgit2, which has a test that\n performs writes with a couple dozen concurrent threads.\n - Add a link to past discussions around `{0}` vs `{{0}}` as provided\n by Eric.\n - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im\n\nChanges in v3:\n - Adapt `reftable_stack_init_addition()` to `memset()` the addition\n instead of using `{{0}}`.\n - Add another commit on top that makes the `flock` interface more\n robust by not relying on `errno`.\n - Add another commit to fix races with concurrent writers caused by\n the backend not passing `REFTABLE_STACK_NEW_ADDITION_RELOAD`.\n - Link to v2: https://lore.kernel.org/r/20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (8):\n reftable/writer: fix type used for number of records\n reftable/writer: drop Git-specific `QSORT()` macro\n reftable/stack: reorder code to avoid forward declarations\n reftable/stack: fix compiler warning due to missing braces\n reftable/stack: allow passing flags to `reftable_stack_add()`\n reftable/stack: handle outdated stacks when compacting\n reftable: don't second-guess errors from flock interface\n refs/reftable: always reload stacks when creating lock\n\n refs/reftable-backend.c | 23 ++-\n reftable/reftable-stack.h | 9 +-\n reftable/reftable-writer.h | 4 +-\n reftable/stack.c | 439 +++++++++++++++++++---------------------\n reftable/system.c | 2 +-\n reftable/system.h | 4 +-\n reftable/writer.c | 23 ++-\n t/unit-tests/t-reftable-stack.c | 50 ++---\n 8 files changed, 275 insertions(+), 279 deletions(-)\n\nRange-diff versus v2:\n\n1: ce08ba1217 = 1: 1613715dc9 reftable/writer: fix type used for number of records\n2: 80a87ff19a = 2: 4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro\n3: e230c97347 < -: ---------- reftable/stack: fix compiler warning due to missing braces\n4: dd413b76a2 ! 3: 3977a1f497 reftable/stack: reorder code to avoid forward declarations\n @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)\n \tstruct reftable_stack *stack;\n @@ reftable/stack.c: struct reftable_addition {\n \n - #define REFTABLE_ADDITION_INIT {{0}}\n + #define REFTABLE_ADDITION_INIT {0}\n \n +static void reftable_addition_close(struct reftable_addition *add)\n +{\n-: ---------- > 4: e8424dd2ee reftable/stack: fix compiler warning due to missing braces\n5: 494bbd6c97 ! 5: 4f0f5cd8e1 reftable/stack: allow passing flags to `reftable_stack_add()`\n @@ reftable/stack.c: static int reftable_stack_init_addition(struct reftable_additi\n -\t\t\t void *arg)\n +\t\t\t void *arg, unsigned flags)\n {\n - \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n --\tint err = reftable_stack_init_addition(&add, st, 0);\n -+\tint err = reftable_stack_init_addition(&add, st, flags);\n + \tstruct reftable_addition add;\n + \tint err;\n + \n +-\terr = reftable_stack_init_addition(&add, st, 0);\n ++\terr = reftable_stack_init_addition(&add, st, flags);\n \tif (err < 0)\n \t\tgoto done;\n \n6: 46411bf2c2 = 6: e6e06c8d34 reftable/stack: handle outdated stacks when compacting\n-: ---------- > 7: 86d57dad6e reftable: don't second-guess errors from flock interface\n-: ---------- > 8: b7bb745ffb refs/reftable: always reload stacks when creating lock\n\n---\nbase-commit: e813a0200a7121b97fec535f0d0b460b0a33356c\nchange-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603\n\n"},{"id":"524026","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-1-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 1/8] reftable/writer: fix type used for number of records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:15Z","receivedAt":"2025-08-12T09:54:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`\naccept an array of records that should be added to the new table.\nCallers of this function are expected to also pass the number of such\nrecords to the function to tell it how many such records it is supposed\nto write.\n\nBut while all callers pass in a `size_t`, which is a sensible choice,\nthe function in fact accepts an `int` as argument, which is less so. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-writer.h | 4 ++--\n reftable/writer.c | 17 +++++++++--------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 0fbeff17f4..1e7003cd69 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n the records before adding them, reordering the records array passed in.\n */\n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n);\n+\t\t\t struct reftable_ref_record *refs, size_t n);\n \n /*\n adds reftable_log_records. Log records are keyed by (refname, decreasing\n@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,\n the records before adding them, reordering records array passed in.\n */\n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n);\n+\t\t\t struct reftable_log_record *logs, size_t n);\n \n /* reftable_writer_close finalizes the reftable. The writer is retained so\n * statistics can be inspected. */\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 3b4ebdd6dc..5bad130c7e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n }\n \n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n)\n+\t\t\t struct reftable_ref_record *refs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(refs, n, reftable_ref_record_compare_name);\n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n-\t}\n+\n \treturn err;\n }\n \n@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,\n }\n \n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n)\n+\t\t\t struct reftable_log_record *logs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(logs, n, reftable_log_record_compare_key);\n \n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n-\t}\n+\n \treturn err;\n }\n \n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524027","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-2-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 2/8] reftable/writer: drop Git-specific `QSORT()` macro","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:16Z","receivedAt":"2025-08-12T09:54:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer accidentally uses the Git-specific `QSORT()` macro.\nThis macro removes the need for the caller to provide the element size,\nbut other than that it's mostly equivalent to `qsort()`.\n\nReplace the macro accordingly to make the library usable outside of Git.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 5bad130c7e..0133b64975 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(refs, n, reftable_ref_record_compare_name);\n+\tif (n)\n+\t\tqsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(logs, n, reftable_log_record_compare_key);\n+\tif (n)\n+\t\tqsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524028","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-3-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 3/8] reftable/stack: reorder code to avoid forward declarations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:17Z","receivedAt":"2025-08-12T09:54:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a couple of forward declarations in the stack-related code of\nthe reftable library. These declarations aren't really required, but are\nsimply caused by unfortunate ordering.\n\nReorder the code and remove the forward declarations.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 176 insertions(+), 188 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 4caf96aa1d..ed80710572 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,18 +17,6 @@\n #include \"table.h\"\n #include \"writer.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-\t\t\t void *arg);\n-static int stack_write_compact(struct reftable_stack *st,\n-\t\t\t struct reftable_writer *wr,\n-\t\t\t size_t first, size_t last,\n-\t\t\t struct reftable_log_expiry_config *config);\n-static void reftable_addition_close(struct reftable_addition *add);\n-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n-\t\t\t\t\t int reuse_open);\n-\n static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,\n \t\t\t const char *name)\n {\n@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)\n \treturn stack_fsync(writer->opts, writer->fd);\n }\n \n-int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n-\t\t const struct reftable_write_options *_opts)\n-{\n-\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct reftable_stack *p;\n-\tint err;\n-\n-\tp = reftable_calloc(1, sizeof(*p));\n-\tif (!p) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\tif (_opts)\n-\t\topts = *_opts;\n-\tif (opts.hash_id == 0)\n-\t\topts.hash_id = REFTABLE_HASH_SHA1;\n-\n-\t*dest = NULL;\n-\n-\treftable_buf_reset(&list_file_name);\n-\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n-\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n-\t\tgoto out;\n-\n-\tp->list_file = reftable_buf_detach(&list_file_name);\n-\tp->list_fd = -1;\n-\tp->opts = opts;\n-\tp->reftable_dir = reftable_strdup(dir);\n-\tif (!p->reftable_dir) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\terr = reftable_stack_reload_maybe_reuse(p, 1);\n-\tif (err < 0)\n-\t\tgoto out;\n-\n-\t*dest = p;\n-\terr = 0;\n-\n-out:\n-\tif (err < 0)\n-\t\treftable_stack_destroy(p);\n-\treturn err;\n-}\n-\n static int fd_read_lines(int fd, char ***namesp)\n {\n \tchar *buf = NULL;\n@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \treturn err;\n }\n \n+int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n+\t\t const struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *p;\n+\tint err;\n+\n+\tp = reftable_calloc(1, sizeof(*p));\n+\tif (!p) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\tif (opts.hash_id == 0)\n+\t\topts.hash_id = REFTABLE_HASH_SHA1;\n+\n+\t*dest = NULL;\n+\n+\treftable_buf_reset(&list_file_name);\n+\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n+\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n+\t\tgoto out;\n+\n+\tp->list_file = reftable_buf_detach(&list_file_name);\n+\tp->list_fd = -1;\n+\tp->opts = opts;\n+\tp->reftable_dir = reftable_strdup(dir);\n+\tif (!p->reftable_dir) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\terr = reftable_stack_reload_maybe_reuse(p, 1);\n+\tif (err < 0)\n+\t\tgoto out;\n+\n+\t*dest = p;\n+\terr = 0;\n+\n+out:\n+\tif (err < 0)\n+\t\treftable_stack_destroy(p);\n+\treturn err;\n+}\n+\n /* -1 = error\n 0 = up to date\n 1 = changed. */\n@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)\n \treturn err;\n }\n \n-int reftable_stack_add(struct reftable_stack *st,\n-\t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n-{\n-\tint err = stack_try_add(st, write, arg);\n-\tif (err < 0) {\n-\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n-\t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_OUTDATED_ERROR.\n-\t\t\t*/\n-\t\t\treftable_stack_reload(st);\n-\t\t}\n-\t\treturn err;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n-{\n-\tchar buf[100];\n-\tuint32_t rnd = reftable_rand();\n-\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n-\t\t min, max, rnd);\n-\treftable_buf_reset(dest);\n-\treturn reftable_buf_addstr(dest, buf);\n-}\n-\n struct reftable_addition {\n \tstruct reftable_flock tables_list_lock;\n \tstruct reftable_stack *stack;\n@@ -706,6 +666,26 @@ struct reftable_addition {\n \n #define REFTABLE_ADDITION_INIT {0}\n \n+static void reftable_addition_close(struct reftable_addition *add)\n+{\n+\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < add->new_tables_len; i++) {\n+\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n+\t\t\tunlink(nm.buf);\n+\t\treftable_free(add->new_tables[i]);\n+\t\tadd->new_tables[i] = NULL;\n+\t}\n+\treftable_free(add->new_tables);\n+\tadd->new_tables = NULL;\n+\tadd->new_tables_len = 0;\n+\tadd->new_tables_cap = 0;\n+\n+\tflock_release(&add->tables_list_lock);\n+\treftable_buf_release(&nm);\n+}\n+\n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st,\n \t\t\t\t\tunsigned int flags)\n@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \treturn err;\n }\n \n-static void reftable_addition_close(struct reftable_addition *add)\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+\t\t\t void *arg)\n {\n-\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n-\tsize_t i;\n+\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tfor (i = 0; i < add->new_tables_len; i++) {\n-\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n-\t\t\tunlink(nm.buf);\n-\t\treftable_free(add->new_tables[i]);\n-\t\tadd->new_tables[i] = NULL;\n+\terr = reftable_addition_add(&add, write_table, arg);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_addition_commit(&add);\n+done:\n+\treftable_addition_close(&add);\n+\treturn err;\n+}\n+\n+int reftable_stack_add(struct reftable_stack *st,\n+\t\t int (*write)(struct reftable_writer *wr, void *arg),\n+\t\t void *arg)\n+{\n+\tint err = stack_try_add(st, write, arg);\n+\tif (err < 0) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n+\t\t\t/* Ignore error return, we want to propagate\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n+\t\t\t*/\n+\t\t\treftable_stack_reload(st);\n+\t\t}\n+\t\treturn err;\n \t}\n-\treftable_free(add->new_tables);\n-\tadd->new_tables = NULL;\n-\tadd->new_tables_len = 0;\n-\tadd->new_tables_cap = 0;\n \n-\tflock_release(&add->tables_list_lock);\n-\treftable_buf_release(&nm);\n+\treturn 0;\n+}\n+\n+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n+{\n+\tchar buf[100];\n+\tuint32_t rnd = reftable_rand();\n+\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n+\t\t min, max, rnd);\n+\treftable_buf_reset(dest);\n+\treturn reftable_buf_addstr(dest, buf);\n }\n \n void reftable_addition_destroy(struct reftable_addition *add)\n@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \treturn err;\n }\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-\t\t\t void *arg)\n-{\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_add(&add, write_table, arg);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_commit(&add);\n-done:\n-\treftable_addition_close(&add);\n-\treturn err;\n-}\n-\n int reftable_addition_add(struct reftable_addition *add,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \treturn 1;\n }\n \n-static int stack_compact_locked(struct reftable_stack *st,\n-\t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct reftable_log_expiry_config *config,\n-\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n-{\n-\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n-\tstruct reftable_writer *wr = NULL;\n-\tstruct fd_writer writer= {\n-\t\t.opts = &st->opts,\n-\t};\n-\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n-\tint err = 0;\n-\n-\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n-\t\t\t reftable_table_max_update_index(st->tables[last]));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_filename(&tab_file_path, st, next_name.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\tif (st->opts.default_permissions &&\n-\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\twriter.fd = tab_file.fd;\n-\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n-\t\t\t\t &writer, &st->opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_write_compact(st, wr, first, last, config);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_writer_close(wr);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_close(&tab_file);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\t*tab_file_out = tab_file;\n-\ttab_file = REFTABLE_TMPFILE_INIT;\n-\n-done:\n-\ttmpfile_delete(&tab_file);\n-\treftable_writer_free(wr);\n-\treftable_buf_release(&next_name);\n-\treftable_buf_release(&tab_file_path);\n-\treturn err;\n-}\n-\n static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,\n \treturn err;\n }\n \n+static int stack_compact_locked(struct reftable_stack *st,\n+\t\t\t\tsize_t first, size_t last,\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n+{\n+\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *wr = NULL;\n+\tstruct fd_writer writer= {\n+\t\t.opts = &st->opts,\n+\t};\n+\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n+\tint err = 0;\n+\n+\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n+\t\t\t reftable_table_max_update_index(st->tables[last]));\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_filename(&tab_file_path, st, next_name.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\tif (st->opts.default_permissions &&\n+\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\twriter.fd = tab_file.fd;\n+\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n+\t\t\t\t &writer, &st->opts);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_write_compact(st, wr, first, last, config);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_writer_close(wr);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_close(&tab_file);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*tab_file_out = tab_file;\n+\ttab_file = REFTABLE_TMPFILE_INIT;\n+\n+done:\n+\ttmpfile_delete(&tab_file);\n+\treftable_writer_free(wr);\n+\treftable_buf_release(&next_name);\n+\treftable_buf_release(&tab_file_path);\n+\treturn err;\n+}\n+\n enum stack_compact_range_flags {\n \t/*\n \t * Perform a best-effort compaction. That is, even if we cannot lock\n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524029","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-4-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 4/8] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:18Z","receivedAt":"2025-08-12T09:54:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While perfectly legal, older compiler toolchains complain when\nzero-initializing structs that contain nested structs with `{0}`:\n\n /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]\n struct reftable_addition empty = REFTABLE_ADDITION_INIT;\n ^~~~~~~~~~~~~~~~~~~~~~\n /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'\n #define REFTABLE_ADDITION_INIT {0}\n ^\n\nWe had the discussion around whether or not we want to handle such bogus\ncompiler errors in the past already [1]. Back then we basically decided\nthat we do not care about such old-and-buggy compilers, so while we\ncould fix the issue by using `{{0}}` instead this is not the preferred\nway to handle this in the Git codebase.\n\nWe have an easier fix though: we can just drop the macro altogether and\nhandle initialization of the struct in `reftable_stack_addition_init()`.\nCallers are expected to call this function already, so this change even\nsimplifies the calling convention.\n\n[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n\nSuggested-by: Carlo Arenas \nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ed80710572..9db90cf4ed 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -664,8 +664,6 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT {0}\n-\n static void reftable_addition_close(struct reftable_addition *add)\n {\n \tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n@@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tstruct reftable_buf lock_file_name = REFTABLE_BUF_INIT;\n \tint err;\n \n+\tmemset(add, 0, sizeof(*add));\n \tadd->stack = st;\n \n \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n@@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t\t\t void *arg),\n \t\t\t void *arg)\n {\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tstruct reftable_addition add;\n+\tint err;\n+\n+\terr = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \t\t\t\tstruct reftable_stack *st,\n \t\t\t\tunsigned int flags)\n {\n-\tint err = 0;\n-\tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n+\tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \tif (!*dest)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \n-\t**dest = empty;\n \terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n \t}\n+\n \treturn err;\n }\n \n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524030","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-5-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:19Z","receivedAt":"2025-08-12T09:54:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_add()` function is a simple wrapper to lock the\nstack, add records to it via a callback and then commit the\nresult. One problem with it though is that it doesn't accept any flags\nfor creating the addition. This makes it impossible to automatically\nreload the stack in case it was modified before we managed to lock the\nstack.\n\nAdd a `flags` field to plug this gap and pass it through accordingly.\nFor now this new flag won't be used by us, but it will be used by\nlibgit2.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 8 +++----\n reftable/reftable-stack.h | 9 +++++---\n reftable/stack.c | 8 +++----\n t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------\n 4 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4c3817f4ec1..3f0deab338c 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n \n done:\n \treturn ret;\n@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 910ec6ef3a2..d70fcb705dc 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);\n * transaction. Releases the lock if held. */\n void reftable_addition_destroy(struct reftable_addition *add);\n \n-/* add a new table to the stack. The write_table function must call\n- * reftable_writer_set_limits, add refs and return an error value. */\n+/*\n+ * Add a new table to the stack. The write_table function must call\n+ * reftable_writer_set_limits, add refs and return an error value.\n+ * The flags are passed through to `reftable_stack_new_addition()`.\n+ */\n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *write_arg),\n-\t\t void *write_arg);\n+\t\t void *write_arg, unsigned flags);\n \n struct reftable_iterator;\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 9db90cf4ed0..1ce4d90cb82 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -736,12 +736,12 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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-\t\t\t void *arg)\n+\t\t\t void *arg, unsigned flags)\n {\n \tstruct reftable_addition add;\n \tint err;\n \n-\terr = reftable_stack_init_addition(&add, st, 0);\n+\terr = reftable_stack_init_addition(&add, st, flags);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -757,9 +757,9 @@ static int stack_try_add(struct reftable_stack *st,\n \n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n+\t\t void *arg, unsigned flags)\n {\n-\tint err = stack_try_add(st, write, arg);\n+\tint err = stack_try_add(st, write, arg, flags);\n \tif (err < 0) {\n \t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 2f49c975194..ce10247903c 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tref.refname = buf;\n \t\tt_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)\n \terr = reftable_new_stack(&st2, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st1, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st1, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck(!err);\n \treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 1);\n \tcheck_int(st->stats.attempts, ==, 0);\n@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \twrite_file_buf(table_path.buf, \"\", 0);\n \n \tref.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 2);\n \tcheck_int(st->stats.attempts, ==, 1);\n@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \tfor (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {\n-\t\terr = reftable_stack_add(st, write_error, &i);\n+\t\terr = reftable_stack_add(st, write_error, &i, 0);\n \t\tcheck_int(err, ==, i);\n \t}\n \n@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\terr = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\terr = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \n-\t\terr = reftable_stack_add(st, write_test_log, &arg);\n+\t\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)\n \tcheck(!err);\n \n \tinput.value.update.message = (char *) \"one\\ntwo\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tinput.value.update.message = (char *) \"one\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_log(st, input.refname, &dest);\n@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)\n \n \tinput.value.update.message = (char *) \"two\\n\";\n \targ.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \terr = reftable_stack_read_log(st, input.refname, &dest);\n \tcheck(!err);\n@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \t/* can't read it with the wrong hash ID. */\n@@ -927,7 +927,7 @@ static void t_reflog_expire(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -978,7 +978,7 @@ static void t_empty_add(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_nothing, NULL);\n+\terr = reftable_stack_add(st, write_nothing, NULL, 0);\n \tcheck(!err);\n \n \terr = reftable_new_stack(&st2, dir, &opts);\n@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)\n \t\t};\n \t\tsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_stack_auto_compact(st);\n@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)\n \t\t};\n \t\txsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\tcheck(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));\n@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)\n \t\tsnprintf(buf, sizeof(buf), \"branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf;\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\t/*\n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524031","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-6-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 6/8] reftable/stack: handle outdated stacks when compacting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:20Z","receivedAt":"2025-08-12T09:54:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we compact the reftable stack we first acquire the lock for the\n\"tables.list\" file and then reload the stack to check that it is still\nup-to-date. This is done by calling `stack_uptodate()`, which knows to\nreturn zero in case the stack is up-to-date, a positive value if it is\nnot and a negative error code on unexpected conditions.\n\nWe don't do proper error checking though, but instead we only check\nwhether the returned error code is non-zero. If so, we simply bubble it\nup the calling stack, which means that callers may see an unexpected\npositive value.\n\nFix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.\nHandle this situation in `reftable_addition_commit()`, where we perform\na best-effort auto-compaction.\n\nAll other callsites of `stack_uptodate()` know to handle a positive\nreturn value and thus don't need to be fixed.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 32 ++++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ce4d90cb8..af0f94d882 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \treturn err;\n }\n \n-/* -1 = error\n- 0 = up to date\n- 1 = changed. */\n+/*\n+ * Check whether the given stack is up-to-date with what we have in memory.\n+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n+ * otherwise.\n+ */\n static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n@@ -850,10 +852,13 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\t * control. It is possible that a concurrent writer is already\n \t\t * trying to compact parts of the stack, which would lead to a\n \t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n-\t\t * already. This is a benign error though, so we ignore it.\n+\t\t * already. Similarly, the stack may have been rewritten by a\n+\t\t * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.\n+\t\t * Both of these errors are benign, so we simply ignore them.\n \t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n-\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n+\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR &&\n+\t\t err != REFTABLE_OUTDATED_ERROR)\n \t\t\tgoto done;\n \t\terr = 0;\n \t}\n@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * Check whether the stack is up-to-date. We unfortunately cannot\n+\t * handle the situation gracefully in case it's _not_ up-to-date\n+\t * because the range of tables that the user has requested us to\n+\t * compact may have been changed. So instead we abort.\n+\t *\n+\t * We could in theory improve the situation by having the caller not\n+\t * pass in a range, but instead the list of tables to compact. If so,\n+\t * we could check that relevant tables still exist. But for now it's\n+\t * good enough to just abort.\n+\t */\n \terr = stack_uptodate(st);\n-\tif (err)\n+\tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0) {\n+\t\terr = REFTABLE_OUTDATED_ERROR;\n+\t\tgoto done;\n+\t}\n \n \t/*\n \t * Lock all tables in the user-provided range. This is the slice of our\n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524032","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-7-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 7/8] reftable: don't second-guess errors from flock interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:21Z","receivedAt":"2025-08-12T09:54:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `flock` interface is implemented as part of \"reftable/system.c\" and\nthus needs to be implemented by the integrator between the reftable\nlibrary and its parent code base. As such, we cannot rely on any\nspecific implementation thereof.\n\nRegardless of that, users of the `flock` subsystem rely on `errno` being\nset to specific values. This is fragile and not documented anywhere and\ndoesn't really make for a good interface.\n\nRefactor the code so that the implementations themselves are expected to\nreturn reftable-specific error codes. Our implementation of the `flock`\nsubsystem already knows to do this for all error paths except one.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 37 ++++++++-----------------------------\n reftable/system.c | 2 +-\n reftable/system.h | 4 +++-\n 3 files changed, 12 insertions(+), 31 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex af0f94d882..f91ce50bcd 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n \t\t\t st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST) {\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\t} else {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n+\n \tif (st->opts.default_permissions) {\n \t\tif (chmod(add->tables_list_lock.path,\n \t\t\t st->opts.default_permissions) < 0) {\n@@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * which are part of the user-specified range.\n \t */\n \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST)\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\telse\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n \n \t/*\n \t * Check whether the stack is up-to-date. We unfortunately cannot\n@@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t * tables, otherwise there would be nothing to compact.\n \t\t\t * In that case, we return a lock error to our caller.\n \t\t\t */\n-\t\t\tif (errno == EEXIST && last - (i - 1) >= 2 &&\n+\t\t\tif (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&\n \t\t\t flags & STACK_COMPACT_RANGE_BEST_EFFORT) {\n \t\t\t\terr = 0;\n \t\t\t\t/*\n@@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t */\n \t\t\t\tfirst = (i - 1) + 1;\n \t\t\t\tbreak;\n-\t\t\t} else if (errno == EEXIST) {\n-\t\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\t\t\tgoto done;\n-\t\t\t} else {\n-\t\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\t\tgoto done;\n \t\t\t}\n+\n+\t\t\tgoto done;\n \t\t}\n \n \t\t/*\n@@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t * of tables.\n \t\t */\n \t\terr = flock_close(&table_locks[nlocks++]);\n-\t\tif (err < 0) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\t\tif (err < 0)\n \t\t\tgoto done;\n-\t\t}\n \t}\n \n \t/*\n@@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * the new table.\n \t */\n \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST)\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\telse\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n \n \tif (st->opts.default_permissions) {\n \t\tif (chmod(tables_list_lock.path,\ndiff --git a/reftable/system.c b/reftable/system.c\nindex 1ee268b125..725a25844e 100644\n--- a/reftable/system.c\n+++ b/reftable/system.c\n@@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,\n \t\treftable_free(lockfile);\n \t\tif (errno == EEXIST)\n \t\t\treturn REFTABLE_LOCK_ERROR;\n-\t\treturn -1;\n+\t\treturn REFTABLE_IO_ERROR;\n \t}\n \n \tl->fd = get_lock_file_fd(lockfile);\ndiff --git a/reftable/system.h b/reftable/system.h\nindex beb9d2431f..c54ed4cad6 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -81,7 +81,9 @@ struct reftable_flock {\n * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative\n * we block indefinitely.\n *\n- * Retrun 0 on success, a reftable error code on error.\n+ * Retrun 0 on success, a reftable error code on error. Specifically,\n+ * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already\n+ * locked.\n */\n int flock_acquire(struct reftable_flock *l, const char *target_path,\n \t\t long timeout_ms);\n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524033","messageId":"20250812-pks-reftable-fixes-for-libgit2-v3-8-cf3b2267867e@pks.im","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"[PATCH v3 8/8] refs/reftable: always reload stacks when creating lock","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-12T09:54:22Z","receivedAt":"2025-08-12T09:54:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new addition via either `reftable_stack_new_addition()`\nor its convenince wrapper `reftable_stack_add()` we:\n\n 1. Create the \"tables.list.lock\" file.\n\n 2. Verify that the current version of the \"tables.list\" file is\n up-to-date.\n\n 3. Write the new table records if so.\n\nBy default, the second step would cause us to bail out if we see that\nthere has been a concurrent write to the stack that made our in-memory\ncopy of the stack out-of-date. This is a safety mechanism to not write\nrecords to the stack based on outdated information.\n\nThe downside though is that concurrent writes may now cause us to bail\nout, which is not a good user experience. In addition, this isn't even\nnecessary for us, as Git knows to perform all checks for the old state\nof references under the lock. (Well, in all except one case: when we\nexpire the reflog we first create the log iterator before we create the\nlock, but this ordering is fixed as part of this commit.)\n\nConsequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`\nflag. The effect of this flag is that we reload the stack after having\nacquired the lock in case the stack is out-of-date. This plugs the race\nwith concurrent writers, but we continue performing the verifications of\nthe expected old state to catch actual conflicts in the references we\nare about to write.\n\nAdapt the remaining callsites that don't yet pass this flag to do so.\nWhile at it, drop a needless manual reload.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 3f0deab338..66d25411f1 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \tif (!arg) {\n \t\tstruct reftable_addition *addition;\n \n-\t\tret = reftable_stack_reload(be->stack);\n-\t\tif (ret)\n-\t\t\treturn ret;\n-\n \t\tret = reftable_stack_new_addition(&addition, be->stack,\n \t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n@@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \treturn ret;\n@@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\n@@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_init_log_iterator(be->stack, &it);\n+\tret = reftable_stack_new_addition(&add, be->stack,\n+\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_iterator_seek_log(&it, refname);\n+\tret = reftable_stack_init_log_iterator(be->stack, &it);\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, be->stack, 0);\n+\tret = reftable_iterator_seek_log(&it, refname);\n \tif (ret < 0)\n \t\tgoto done;\n \n\n-- \n2.51.0.rc1.163.g2494970778.dirty\n\n"},{"id":"524048","messageId":"xmqqwm78wqqp.fsf@gitster.g","threadId":"63891","inReplyTo":"aJsJE3UzKIVeg3di@pks.im","subject":"Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-12T14:37:18Z","receivedAt":"2025-08-12T14:37:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I think the suggestion for using a shim layer solution is relevant, because\n>> additionally to the compatibility issues of the zero initializer, you also\n>> need to take into consideration that the proposed solution will still trigger\n>> warnings when compiled as C++ (where {0} should be instead {}).\n>\n> Do we even support compiling Git as C++?\n\nThere was an earlier effort to rename variables like \"new\", but I\nthink the motivation was more like some folks wanted to try building\nwith C++ compilers. I do not know what the outcome of it was.\n\nBut people like to disect and include pieces of useful and/or\npopular software in their programs within the limitation of how the\noriginal is licensed. Especially a part well libified like reftable\nmay be a good candidate.\n"},{"id":"524066","messageId":"puevci6dx6qf235usn7sxi52hlt2n5dcbe4gb2r3l4yekvr7mq@qp7l4nwkbnvg","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-4-cf3b2267867e@pks.im","subject":"Re: [PATCH v3 4/8] reftable/stack: fix compiler warning due to missing braces","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-12T16:51:43Z","receivedAt":"2025-08-12T16:51:45Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/12 11:54AM, Patrick Steinhardt wrote:\n> While perfectly legal, older compiler toolchains complain when\n> zero-initializing structs that contain nested structs with `{0}`:\n> \n> /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]\n> struct reftable_addition empty = REFTABLE_ADDITION_INIT;\n> ^~~~~~~~~~~~~~~~~~~~~~\n> /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'\n> #define REFTABLE_ADDITION_INIT {0}\n> ^\n> \n> We had the discussion around whether or not we want to handle such bogus\n> compiler errors in the past already [1]. Back then we basically decided\n> that we do not care about such old-and-buggy compilers, so while we\n> could fix the issue by using `{{0}}` instead this is not the preferred\n> way to handle this in the Git codebase.\n> \n> We have an easier fix though: we can just drop the macro altogether and\n> handle initialization of the struct in `reftable_stack_addition_init()`.\n> Callers are expected to call this function already, so this change even\n> simplifies the calling convention.\n> \n> [1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n> \n> Suggested-by: Carlo Arenas \n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack.c | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index ed80710572..9db90cf4ed 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -664,8 +664,6 @@ struct reftable_addition {\n> \tuint64_t next_update_index;\n> };\n> \n> -#define REFTABLE_ADDITION_INIT {0}\n\nIt looks like there are only two places where this macro gets used.\nBeing that `reftable_stack_init_addition()` is always expected to be\ncalled, deferring initialization of the structure to that point seems\nsensible.\n\n> -\n> static void reftable_addition_close(struct reftable_addition *add)\n> {\n> \tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n> @@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> \tstruct reftable_buf lock_file_name = REFTABLE_BUF_INIT;\n> \tint err;\n> \n> +\tmemset(add, 0, sizeof(*add));\n\nLooks good.\n\n> \tadd->stack = st;\n> \n> \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n> @@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,\n> \t\t\t\t\t void *arg),\n> \t\t\t void *arg)\n> {\n> -\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n> -\tint err = reftable_stack_init_addition(&add, st, 0);\n> +\tstruct reftable_addition add;\n> +\tint err;\n> +\n> +\terr = reftable_stack_init_addition(&add, st, 0);\n> \tif (err < 0)\n> \t\tgoto done;\n> \n> @@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n> \t\t\t\tstruct reftable_stack *st,\n> \t\t\t\tunsigned int flags)\n> {\n> -\tint err = 0;\n> -\tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n> +\tint err;\n> \n> \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n> \tif (!*dest)\n> \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> \n> -\t**dest = empty;\n\nNow resetting the `reftable_addition` is handled by\n`reftable_stack_init_addition` automatically, which is nicer IMO.\n\n> \terr = reftable_stack_init_addition(*dest, st, flags);\n> \tif (err) {\n> \t\treftable_free(*dest);\n> \t\t*dest = NULL;\n> \t}\n> +\n> \treturn err;\n> }\n> \n> \n> -- \n> 2.51.0.rc1.163.g2494970778.dirty\n> \n"},{"id":"524067","messageId":"dlqh2lcmz4kh7pvwwwfj2cewaqqrdnmixfwxey7u6xxqjy35yx@apdcts2rtyc4","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-5-cf3b2267867e@pks.im","subject":"Re: [PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-12T16:59:03Z","receivedAt":"2025-08-12T16:59:04Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/12 11:54AM, Patrick Steinhardt wrote:\n> The `reftable_stack_add()` function is a simple wrapper to lock the\n> stack, add records to it via a callback and then commit the\n> result. One problem with it though is that it doesn't accept any flags\n> for creating the addition. This makes it impossible to automatically\n> reload the stack in case it was modified before we managed to lock the\n> stack.\n> \n> Add a `flags` field to plug this gap and pass it through accordingly.\n> For now this new flag won't be used by us, but it will be used by\n> libgit2.\n\nIt looks like we will use the new `flags` field for\n`reftable-stack-add()` later in the series though.\n\n> \n> Signed-off-by: Patrick Steinhardt \n"},{"id":"524072","messageId":"ckadsyx65an4seplaytey5fd3mdfwc3pnbtlpkslulod76l3s4@56mra6ydeg2p","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-7-cf3b2267867e@pks.im","subject":"Re: [PATCH v3 7/8] reftable: don't second-guess errors from flock interface","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-12T17:05:25Z","receivedAt":"2025-08-12T17:05:27Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/12 11:54AM, Patrick Steinhardt wrote:\n> The `flock` interface is implemented as part of \"reftable/system.c\" and\n> thus needs to be implemented by the integrator between the reftable\n> library and its parent code base. As such, we cannot rely on any\n> specific implementation thereof.\n> \n> Regardless of that, users of the `flock` subsystem rely on `errno` being\n> set to specific values. This is fragile and not documented anywhere and\n> doesn't really make for a good interface.\n> \n> Refactor the code so that the implementations themselves are expected to\n> return reftable-specific error codes. Our implementation of the `flock`\n> subsystem already knows to do this for all error paths except one.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack.c | 37 ++++++++-----------------------------\n> reftable/system.c | 2 +-\n> reftable/system.h | 4 +++-\n> 3 files changed, 12 insertions(+), 31 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index af0f94d882..f91ce50bcd 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> \n> \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n> \t\t\t st->opts.lock_timeout_ms);\n> -\tif (err < 0) {\n> -\t\tif (errno == EEXIST) {\n> -\t\t\terr = REFTABLE_LOCK_ERROR;\n> -\t\t} else {\n> -\t\t\terr = REFTABLE_IO_ERROR;\n> -\t\t}\n> +\tif (err < 0)\n> \t\tgoto done;\n> -\t}\n> +\n> \tif (st->opts.default_permissions) {\n> \t\tif (chmod(add->tables_list_lock.path,\n> \t\t\t st->opts.default_permissions) < 0) {\n> @@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t * which are part of the user-specified range.\n> \t */\n> \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n> -\tif (err < 0) {\n> -\t\tif (errno == EEXIST)\n> -\t\t\terr = REFTABLE_LOCK_ERROR;\n> -\t\telse\n> -\t\t\terr = REFTABLE_IO_ERROR;\n> +\tif (err < 0)\n> \t\tgoto done;\n> -\t}\n> \n> \t/*\n> \t * Check whether the stack is up-to-date. We unfortunately cannot\n> @@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t\t\t * tables, otherwise there would be nothing to compact.\n> \t\t\t * In that case, we return a lock error to our caller.\n> \t\t\t */\n> -\t\t\tif (errno == EEXIST && last - (i - 1) >= 2 &&\n> +\t\t\tif (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&\n> \t\t\t flags & STACK_COMPACT_RANGE_BEST_EFFORT) {\n> \t\t\t\terr = 0;\n> \t\t\t\t/*\n> @@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t\t\t\t */\n> \t\t\t\tfirst = (i - 1) + 1;\n> \t\t\t\tbreak;\n> -\t\t\t} else if (errno == EEXIST) {\n> -\t\t\t\terr = REFTABLE_LOCK_ERROR;\n> -\t\t\t\tgoto done;\n> -\t\t\t} else {\n> -\t\t\t\terr = REFTABLE_IO_ERROR;\n> -\t\t\t\tgoto done;\n> \t\t\t}\n> +\n> +\t\t\tgoto done;\n> \t\t}\n> \n> \t\t/*\n> @@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t\t * of tables.\n> \t\t */\n> \t\terr = flock_close(&table_locks[nlocks++]);\n> -\t\tif (err < 0) {\n> -\t\t\terr = REFTABLE_IO_ERROR;\n> +\t\tif (err < 0)\n> \t\t\tgoto done;\n> -\t\t}\n> \t}\n> \n> \t/*\n> @@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t * the new table.\n> \t */\n> \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n> -\tif (err < 0) {\n> -\t\tif (errno == EEXIST)\n> -\t\t\terr = REFTABLE_LOCK_ERROR;\n> -\t\telse\n> -\t\t\terr = REFTABLE_IO_ERROR;\n> +\tif (err < 0)\n\nNow we no longer rely on errno to determine the correct err to return.\nNice.\n\n> \t\tgoto done;\n> -\t}\n> \n> \tif (st->opts.default_permissions) {\n> \t\tif (chmod(tables_list_lock.path,\n> diff --git a/reftable/system.c b/reftable/system.c\n> index 1ee268b125..725a25844e 100644\n> --- a/reftable/system.c\n> +++ b/reftable/system.c\n> @@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,\n> \t\treftable_free(lockfile);\n> \t\tif (errno == EEXIST)\n> \t\t\treturn REFTABLE_LOCK_ERROR;\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_IO_ERROR;\n> \t}\n> \n> \tl->fd = get_lock_file_fd(lockfile);\n> diff --git a/reftable/system.h b/reftable/system.h\n> index beb9d2431f..c54ed4cad6 100644\n> --- a/reftable/system.h\n> +++ b/reftable/system.h\n> @@ -81,7 +81,9 @@ struct reftable_flock {\n> * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative\n> * we block indefinitely.\n> *\n> - * Retrun 0 on success, a reftable error code on error.\n> + * Retrun 0 on success, a reftable error code on error. Specifically,\n\nNot a new typo, but we could fix it:\n\ns/Retrun/Return/\n\n> + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already\n> + * locked.\n> */\n> int flock_acquire(struct reftable_flock *l, const char *target_path,\n> \t\t long timeout_ms);\n> \n> -- \n> 2.51.0.rc1.163.g2494970778.dirty\n> \n"},{"id":"524074","messageId":"hxbcsuqwb2clzft4vuyxhxwzvft2lfhtgv3de6yx6aqftyqc76@57gtrzgik54z","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-8-cf3b2267867e@pks.im","subject":"Re: [PATCH v3 8/8] refs/reftable: always reload stacks when creating lock","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-12T17:12:48Z","receivedAt":"2025-08-12T17:12:50Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/12 11:54AM, Patrick Steinhardt wrote:\n> When creating a new addition via either `reftable_stack_new_addition()`\n> or its convenince wrapper `reftable_stack_add()` we:\n> \n> 1. Create the \"tables.list.lock\" file.\n> \n> 2. Verify that the current version of the \"tables.list\" file is\n> up-to-date.\n> \n> 3. Write the new table records if so.\n> \n> By default, the second step would cause us to bail out if we see that\n> there has been a concurrent write to the stack that made our in-memory\n> copy of the stack out-of-date. This is a safety mechanism to not write\n> records to the stack based on outdated information.\n> \n> The downside though is that concurrent writes may now cause us to bail\n> out, which is not a good user experience. In addition, this isn't even\n> necessary for us, as Git knows to perform all checks for the old state\n> of references under the lock. (Well, in all except one case: when we\n> expire the reflog we first create the log iterator before we create the\n> lock, but this ordering is fixed as part of this commit.)\n> \n> Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`\n> flag. The effect of this flag is that we reload the stack after having\n> acquired the lock in case the stack is out-of-date. This plugs the race\n> with concurrent writers, but we continue performing the verifications of\n> the expected old state to catch actual conflicts in the references we\n> are about to write.\n> \n> Adapt the remaining callsites that don't yet pass this flag to do so.\n> While at it, drop a needless manual reload.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/reftable-backend.c | 23 ++++++++++++-----------\n> 1 file changed, 12 insertions(+), 11 deletions(-)\n> \n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 3f0deab338..66d25411f1 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n> \tif (!arg) {\n> \t\tstruct reftable_addition *addition;\n> \n> -\t\tret = reftable_stack_reload(be->stack);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n\nHere we don't need to reload because `reftable_stack_new_addition()`\nimmediately following already does this for us.\n\n> -\n> \t\tret = reftable_stack_new_addition(&addition, be->stack,\n> \t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> \t\tif (ret) {\n> @@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n> \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n> \tif (ret)\n> \t\tgoto done;\n> -\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n> +\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n> +\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> \n> done:\n> \tassert(ret != REFTABLE_API_ERROR);\n> @@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n> \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n> \tif (ret)\n> \t\tgoto done;\n> -\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n> +\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n> +\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> \n> done:\n> \tassert(ret != REFTABLE_API_ERROR);\n> @@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n> \t\tgoto done;\n> \targ.stack = be->stack;\n> \n> -\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n> +\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,\n> +\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> \n> done:\n> \treturn ret;\n> @@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n> \t\treturn ret;\n> \targ.stack = be->stack;\n> \n> -\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n> +\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,\n> +\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> \n> \tassert(ret != REFTABLE_API_ERROR);\n> \treturn ret;\n> @@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n> \tif (ret < 0)\n> \t\tgoto done;\n> \n> -\tret = reftable_stack_init_log_iterator(be->stack, &it);\n> +\tret = reftable_stack_new_addition(&add, be->stack,\n> +\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n\nHere we change the order so that we now acquire the lock first.\n\nThis patch looks good to me :)\n\n-Justin\n\n> \tif (ret < 0)\n> \t\tgoto done;\n> \n> -\tret = reftable_iterator_seek_log(&it, refname);\n> +\tret = reftable_stack_init_log_iterator(be->stack, &it);\n> \tif (ret < 0)\n> \t\tgoto done;\n> \n> -\tret = reftable_stack_new_addition(&add, be->stack, 0);\n> +\tret = reftable_iterator_seek_log(&it, refname);\n> \tif (ret < 0)\n> \t\tgoto done;\n> \n> \n> -- \n> 2.51.0.rc1.163.g2494970778.dirty\n> \n"},{"id":"524082","messageId":"CAPUEsph_rWfbxobf0fWuGcWF=XNa4911FAXvB7agbfyUiZDr6Q@mail.gmail.com","threadId":"63891","inReplyTo":"20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im","subject":"Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2025-08-12T19:00:53Z","receivedAt":"2025-08-12T19:01:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt wrote:\n\n> Range-diff versus v2:\n>\n> 1: ce08ba1217 = 1: 1613715dc9 reftable/writer: fix type used for number of records\n> 2: 80a87ff19a = 2: 4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro\n> 3: e230c97347 < -: ---------- reftable/stack: fix compiler warning due to missing braces\n> 4: dd413b76a2 ! 3: 3977a1f497 reftable/stack: reorder code to avoid forward declarations\n> @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)\n> struct reftable_stack *stack;\n> @@ reftable/stack.c: struct reftable_addition {\n>\n> - #define REFTABLE_ADDITION_INIT {{0}}\n> + #define REFTABLE_ADDITION_INIT {0}\n\nThis define shouldn't be needed anymore AFAIK\n\nCarlo\n"},{"id":"524101","messageId":"aJwsq-dLyQ9I7fDo@pks.im","threadId":"63891","inReplyTo":"CAPUEsph_rWfbxobf0fWuGcWF=XNa4911FAXvB7agbfyUiZDr6Q@mail.gmail.com","subject":"Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:11:55Z","receivedAt":"2025-08-13T06:12:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 12, 2025 at 12:00:53PM -0700, Carlo Arenas wrote:\n> On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt wrote:\n> \n> > Range-diff versus v2:\n> >\n> > 1: ce08ba1217 = 1: 1613715dc9 reftable/writer: fix type used for number of records\n> > 2: 80a87ff19a = 2: 4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro\n> > 3: e230c97347 < -: ---------- reftable/stack: fix compiler warning due to missing braces\n> > 4: dd413b76a2 ! 3: 3977a1f497 reftable/stack: reorder code to avoid forward declarations\n> > @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)\n> > struct reftable_stack *stack;\n> > @@ reftable/stack.c: struct reftable_addition {\n> >\n> > - #define REFTABLE_ADDITION_INIT {{0}}\n> > + #define REFTABLE_ADDITION_INIT {0}\n> \n> This define shouldn't be needed anymore AFAIK\n\nIt doesn't exist anymore after this patch series, as it gets removed in\nthe fourth patch. The above change in the range-diff is merely a result\nof me swapping the order of patch 3 and 4.\n\nPatrick\n"},{"id":"524102","messageId":"aJwsuC8ZUgcfO1zP@pks.im","threadId":"63891","inReplyTo":"dlqh2lcmz4kh7pvwwwfj2cewaqqrdnmixfwxey7u6xxqjy35yx@apdcts2rtyc4","subject":"Re: [PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:12:08Z","receivedAt":"2025-08-13T06:12:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 12, 2025 at 11:59:03AM -0500, Justin Tobler wrote:\n> On 25/08/12 11:54AM, Patrick Steinhardt wrote:\n> > The `reftable_stack_add()` function is a simple wrapper to lock the\n> > stack, add records to it via a callback and then commit the\n> > result. One problem with it though is that it doesn't accept any flags\n> > for creating the addition. This makes it impossible to automatically\n> > reload the stack in case it was modified before we managed to lock the\n> > stack.\n> > \n> > Add a `flags` field to plug this gap and pass it through accordingly.\n> > For now this new flag won't be used by us, but it will be used by\n> > libgit2.\n> \n> It looks like we will use the new `flags` field for\n> `reftable-stack-add()` later in the series though.\n\nGood point, this is definitely stale now. Will adjust.\n\nPatrick\n"},{"id":"524103","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im","subject":"[PATCH v4 0/8] reftable: a couple of improvements for libgit2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:24Z","receivedAt":"2025-08-13T06:25:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series contains a couple of improvements I required for\nlibgit2. With those changes libgit2 is now able to run its full test\nsuite with reftable-enabled repositories. I still need to invest a bit\nof work to make it memory-leak free and compile on Windows, but overall\nI think that support for reftables is almost ready.\n\nChanges in v2:\n - Another commit that fixes handling of outdated stacks when doing\n compaction. This issue is hit in libgit2, which has a test that\n performs writes with a couple dozen concurrent threads.\n - Add a link to past discussions around `{0}` vs `{{0}}` as provided\n by Eric.\n - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im\n\nChanges in v3:\n - Adapt `reftable_stack_init_addition()` to `memset()` the addition\n instead of using `{{0}}`.\n - Add another commit on top that makes the `flock` interface more\n robust by not relying on `errno`.\n - Add another commit to fix races with concurrent writers caused by\n the backend not passing `REFTABLE_STACK_NEW_ADDITION_RELOAD`.\n - Link to v2: https://lore.kernel.org/r/20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im\n\nChanges in v4:\n - Adjust stale commit message.\n - Fix a typo while at it.\n - Link to v3: https://lore.kernel.org/r/20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (8):\n reftable/writer: fix type used for number of records\n reftable/writer: drop Git-specific `QSORT()` macro\n reftable/stack: reorder code to avoid forward declarations\n reftable/stack: fix compiler warning due to missing braces\n reftable/stack: allow passing flags to `reftable_stack_add()`\n reftable/stack: handle outdated stacks when compacting\n reftable: don't second-guess errors from flock interface\n refs/reftable: always reload stacks when creating lock\n\n refs/reftable-backend.c | 23 ++-\n reftable/reftable-stack.h | 9 +-\n reftable/reftable-writer.h | 4 +-\n reftable/stack.c | 439 +++++++++++++++++++---------------------\n reftable/system.c | 2 +-\n reftable/system.h | 4 +-\n reftable/writer.c | 23 ++-\n t/unit-tests/t-reftable-stack.c | 50 ++---\n 8 files changed, 275 insertions(+), 279 deletions(-)\n\nRange-diff versus v3:\n\n1: afb757e6c1 = 1: 631293a69e reftable/writer: fix type used for number of records\n2: 75dd5f6cb6 = 2: a583b9f51c reftable/writer: drop Git-specific `QSORT()` macro\n3: 6badd0ae26 = 3: 7153526268 reftable/stack: reorder code to avoid forward declarations\n4: 9f5aa7f808 = 4: 7372cbe037 reftable/stack: fix compiler warning due to missing braces\n5: d4fea00eb2 ! 5: 57810b984e reftable/stack: allow passing flags to `reftable_stack_add()`\n @@ Commit message\n stack.\n \n Add a `flags` field to plug this gap and pass it through accordingly.\n - For now this new flag won't be used by us, but it will be used by\n - libgit2.\n + This flag will be used in a subsequent patch and by libgit2.\n \n Signed-off-by: Patrick Steinhardt \n \n6: d2cdd89280 = 6: 211d3f646e reftable/stack: handle outdated stacks when compacting\n7: 635dd20dcd ! 7: f8d541af00 reftable: don't second-guess errors from flock interface\n @@ reftable/system.h: struct reftable_flock {\n * we block indefinitely.\n *\n - * Retrun 0 on success, a reftable error code on error.\n -+ * Retrun 0 on success, a reftable error code on error. Specifically,\n ++ * Return 0 on success, a reftable error code on error. Specifically,\n + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already\n + * locked.\n */\n8: 4396793747 = 8: 912d00a4a8 refs/reftable: always reload stacks when creating lock\n\n---\nbase-commit: e813a0200a7121b97fec535f0d0b460b0a33356c\nchange-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603\n\n"},{"id":"524104","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-1-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 1/8] reftable/writer: fix type used for number of records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:25Z","receivedAt":"2025-08-13T06:25:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`\naccept an array of records that should be added to the new table.\nCallers of this function are expected to also pass the number of such\nrecords to the function to tell it how many such records it is supposed\nto write.\n\nBut while all callers pass in a `size_t`, which is a sensible choice,\nthe function in fact accepts an `int` as argument, which is less so. Fix\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-writer.h | 4 ++--\n reftable/writer.c | 17 +++++++++--------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 0fbeff17f4..1e7003cd69 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n the records before adding them, reordering the records array passed in.\n */\n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n);\n+\t\t\t struct reftable_ref_record *refs, size_t n);\n \n /*\n adds reftable_log_records. Log records are keyed by (refname, decreasing\n@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,\n the records before adding them, reordering records array passed in.\n */\n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n);\n+\t\t\t struct reftable_log_record *logs, size_t n);\n \n /* reftable_writer_close finalizes the reftable. The writer is retained so\n * statistics can be inspected. */\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 3b4ebdd6dc..5bad130c7e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n }\n \n int reftable_writer_add_refs(struct reftable_writer *w,\n-\t\t\t struct reftable_ref_record *refs, int n)\n+\t\t\t struct reftable_ref_record *refs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(refs, n, reftable_ref_record_compare_name);\n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n-\t}\n+\n \treturn err;\n }\n \n@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,\n }\n \n int reftable_writer_add_logs(struct reftable_writer *w,\n-\t\t\t struct reftable_log_record *logs, int n)\n+\t\t\t struct reftable_log_record *logs, size_t n)\n {\n \tint err = 0;\n-\tint i = 0;\n+\n \tQSORT(logs, n, reftable_log_record_compare_key);\n \n-\tfor (i = 0; err == 0 && i < n; i++) {\n+\tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n-\t}\n+\n \treturn err;\n }\n \n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524105","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-2-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 2/8] reftable/writer: drop Git-specific `QSORT()` macro","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:26Z","receivedAt":"2025-08-13T06:25:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer accidentally uses the Git-specific `QSORT()` macro.\nThis macro removes the need for the caller to provide the element size,\nbut other than that it's mostly equivalent to `qsort()`.\n\nReplace the macro accordingly to make the library usable outside of Git.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 5bad130c7e..0133b64975 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(refs, n, reftable_ref_record_compare_name);\n+\tif (n)\n+\t\tqsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_ref(w, &refs[i]);\n@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,\n {\n \tint err = 0;\n \n-\tQSORT(logs, n, reftable_log_record_compare_key);\n+\tif (n)\n+\t\tqsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);\n \n \tfor (size_t i = 0; err == 0 && i < n; i++)\n \t\terr = reftable_writer_add_log(w, &logs[i]);\n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524106","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-3-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 3/8] reftable/stack: reorder code to avoid forward declarations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:27Z","receivedAt":"2025-08-13T06:25:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a couple of forward declarations in the stack-related code of\nthe reftable library. These declarations aren't really required, but are\nsimply caused by unfortunate ordering.\n\nReorder the code and remove the forward declarations.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 176 insertions(+), 188 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 4caf96aa1d..ed80710572 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -17,18 +17,6 @@\n #include \"table.h\"\n #include \"writer.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-\t\t\t void *arg);\n-static int stack_write_compact(struct reftable_stack *st,\n-\t\t\t struct reftable_writer *wr,\n-\t\t\t size_t first, size_t last,\n-\t\t\t struct reftable_log_expiry_config *config);\n-static void reftable_addition_close(struct reftable_addition *add);\n-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n-\t\t\t\t\t int reuse_open);\n-\n static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,\n \t\t\t const char *name)\n {\n@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)\n \treturn stack_fsync(writer->opts, writer->fd);\n }\n \n-int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n-\t\t const struct reftable_write_options *_opts)\n-{\n-\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct reftable_stack *p;\n-\tint err;\n-\n-\tp = reftable_calloc(1, sizeof(*p));\n-\tif (!p) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\tif (_opts)\n-\t\topts = *_opts;\n-\tif (opts.hash_id == 0)\n-\t\topts.hash_id = REFTABLE_HASH_SHA1;\n-\n-\t*dest = NULL;\n-\n-\treftable_buf_reset(&list_file_name);\n-\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n-\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n-\t\tgoto out;\n-\n-\tp->list_file = reftable_buf_detach(&list_file_name);\n-\tp->list_fd = -1;\n-\tp->opts = opts;\n-\tp->reftable_dir = reftable_strdup(dir);\n-\tif (!p->reftable_dir) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n-\t}\n-\n-\terr = reftable_stack_reload_maybe_reuse(p, 1);\n-\tif (err < 0)\n-\t\tgoto out;\n-\n-\t*dest = p;\n-\terr = 0;\n-\n-out:\n-\tif (err < 0)\n-\t\treftable_stack_destroy(p);\n-\treturn err;\n-}\n-\n static int fd_read_lines(int fd, char ***namesp)\n {\n \tchar *buf = NULL;\n@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \treturn err;\n }\n \n+int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n+\t\t const struct reftable_write_options *_opts)\n+{\n+\tstruct reftable_buf list_file_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *p;\n+\tint err;\n+\n+\tp = reftable_calloc(1, sizeof(*p));\n+\tif (!p) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\tif (_opts)\n+\t\topts = *_opts;\n+\tif (opts.hash_id == 0)\n+\t\topts.hash_id = REFTABLE_HASH_SHA1;\n+\n+\t*dest = NULL;\n+\n+\treftable_buf_reset(&list_file_name);\n+\tif ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||\n+\t (err = reftable_buf_addstr(&list_file_name, \"/tables.list\")) < 0)\n+\t\tgoto out;\n+\n+\tp->list_file = reftable_buf_detach(&list_file_name);\n+\tp->list_fd = -1;\n+\tp->opts = opts;\n+\tp->reftable_dir = reftable_strdup(dir);\n+\tif (!p->reftable_dir) {\n+\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\tgoto out;\n+\t}\n+\n+\terr = reftable_stack_reload_maybe_reuse(p, 1);\n+\tif (err < 0)\n+\t\tgoto out;\n+\n+\t*dest = p;\n+\terr = 0;\n+\n+out:\n+\tif (err < 0)\n+\t\treftable_stack_destroy(p);\n+\treturn err;\n+}\n+\n /* -1 = error\n 0 = up to date\n 1 = changed. */\n@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)\n \treturn err;\n }\n \n-int reftable_stack_add(struct reftable_stack *st,\n-\t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n-{\n-\tint err = stack_try_add(st, write, arg);\n-\tif (err < 0) {\n-\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n-\t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_OUTDATED_ERROR.\n-\t\t\t*/\n-\t\t\treftable_stack_reload(st);\n-\t\t}\n-\t\treturn err;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n-{\n-\tchar buf[100];\n-\tuint32_t rnd = reftable_rand();\n-\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n-\t\t min, max, rnd);\n-\treftable_buf_reset(dest);\n-\treturn reftable_buf_addstr(dest, buf);\n-}\n-\n struct reftable_addition {\n \tstruct reftable_flock tables_list_lock;\n \tstruct reftable_stack *stack;\n@@ -706,6 +666,26 @@ struct reftable_addition {\n \n #define REFTABLE_ADDITION_INIT {0}\n \n+static void reftable_addition_close(struct reftable_addition *add)\n+{\n+\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < add->new_tables_len; i++) {\n+\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n+\t\t\tunlink(nm.buf);\n+\t\treftable_free(add->new_tables[i]);\n+\t\tadd->new_tables[i] = NULL;\n+\t}\n+\treftable_free(add->new_tables);\n+\tadd->new_tables = NULL;\n+\tadd->new_tables_len = 0;\n+\tadd->new_tables_cap = 0;\n+\n+\tflock_release(&add->tables_list_lock);\n+\treftable_buf_release(&nm);\n+}\n+\n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st,\n \t\t\t\t\tunsigned int flags)\n@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \treturn err;\n }\n \n-static void reftable_addition_close(struct reftable_addition *add)\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+\t\t\t void *arg)\n {\n-\tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n-\tsize_t i;\n+\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tfor (i = 0; i < add->new_tables_len; i++) {\n-\t\tif (!stack_filename(&nm, add->stack, add->new_tables[i]))\n-\t\t\tunlink(nm.buf);\n-\t\treftable_free(add->new_tables[i]);\n-\t\tadd->new_tables[i] = NULL;\n+\terr = reftable_addition_add(&add, write_table, arg);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_addition_commit(&add);\n+done:\n+\treftable_addition_close(&add);\n+\treturn err;\n+}\n+\n+int reftable_stack_add(struct reftable_stack *st,\n+\t\t int (*write)(struct reftable_writer *wr, void *arg),\n+\t\t void *arg)\n+{\n+\tint err = stack_try_add(st, write, arg);\n+\tif (err < 0) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n+\t\t\t/* Ignore error return, we want to propagate\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n+\t\t\t*/\n+\t\t\treftable_stack_reload(st);\n+\t\t}\n+\t\treturn err;\n \t}\n-\treftable_free(add->new_tables);\n-\tadd->new_tables = NULL;\n-\tadd->new_tables_len = 0;\n-\tadd->new_tables_cap = 0;\n \n-\tflock_release(&add->tables_list_lock);\n-\treftable_buf_release(&nm);\n+\treturn 0;\n+}\n+\n+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n+{\n+\tchar buf[100];\n+\tuint32_t rnd = reftable_rand();\n+\tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n+\t\t min, max, rnd);\n+\treftable_buf_reset(dest);\n+\treturn reftable_buf_addstr(dest, buf);\n }\n \n void reftable_addition_destroy(struct reftable_addition *add)\n@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \treturn err;\n }\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-\t\t\t void *arg)\n-{\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_add(&add, write_table, arg);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_addition_commit(&add);\n-done:\n-\treftable_addition_close(&add);\n-\treturn err;\n-}\n-\n int reftable_addition_add(struct reftable_addition *add,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *arg),\n@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \treturn 1;\n }\n \n-static int stack_compact_locked(struct reftable_stack *st,\n-\t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct reftable_log_expiry_config *config,\n-\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n-{\n-\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n-\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n-\tstruct reftable_writer *wr = NULL;\n-\tstruct fd_writer writer= {\n-\t\t.opts = &st->opts,\n-\t};\n-\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n-\tint err = 0;\n-\n-\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n-\t\t\t reftable_table_max_update_index(st->tables[last]));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_filename(&tab_file_path, st, next_name.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\tif (st->opts.default_permissions &&\n-\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\twriter.fd = tab_file.fd;\n-\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n-\t\t\t\t &writer, &st->opts);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = stack_write_compact(st, wr, first, last, config);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_writer_close(wr);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = tmpfile_close(&tab_file);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\t*tab_file_out = tab_file;\n-\ttab_file = REFTABLE_TMPFILE_INIT;\n-\n-done:\n-\ttmpfile_delete(&tab_file);\n-\treftable_writer_free(wr);\n-\treftable_buf_release(&next_name);\n-\treftable_buf_release(&tab_file_path);\n-\treturn err;\n-}\n-\n static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,\n \treturn err;\n }\n \n+static int stack_compact_locked(struct reftable_stack *st,\n+\t\t\t\tsize_t first, size_t last,\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct reftable_tmpfile *tab_file_out)\n+{\n+\tstruct reftable_buf next_name = REFTABLE_BUF_INIT;\n+\tstruct reftable_buf tab_file_path = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *wr = NULL;\n+\tstruct fd_writer writer= {\n+\t\t.opts = &st->opts,\n+\t};\n+\tstruct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;\n+\tint err = 0;\n+\n+\terr = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),\n+\t\t\t reftable_table_max_update_index(st->tables[last]));\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_filename(&tab_file_path, st, next_name.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_buf_addstr(&tab_file_path, \".temp.XXXXXX\");\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_from_pattern(&tab_file, tab_file_path.buf);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\tif (st->opts.default_permissions &&\n+\t chmod(tab_file.path, st->opts.default_permissions) < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\twriter.fd = tab_file.fd;\n+\terr = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,\n+\t\t\t\t &writer, &st->opts);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = stack_write_compact(st, wr, first, last, config);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = reftable_writer_close(wr);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\terr = tmpfile_close(&tab_file);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*tab_file_out = tab_file;\n+\ttab_file = REFTABLE_TMPFILE_INIT;\n+\n+done:\n+\ttmpfile_delete(&tab_file);\n+\treftable_writer_free(wr);\n+\treftable_buf_release(&next_name);\n+\treftable_buf_release(&tab_file_path);\n+\treturn err;\n+}\n+\n enum stack_compact_range_flags {\n \t/*\n \t * Perform a best-effort compaction. That is, even if we cannot lock\n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524107","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-4-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 4/8] reftable/stack: fix compiler warning due to missing braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:28Z","receivedAt":"2025-08-13T06:25:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While perfectly legal, older compiler toolchains complain when\nzero-initializing structs that contain nested structs with `{0}`:\n\n /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]\n struct reftable_addition empty = REFTABLE_ADDITION_INIT;\n ^~~~~~~~~~~~~~~~~~~~~~\n /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'\n #define REFTABLE_ADDITION_INIT {0}\n ^\n\nWe had the discussion around whether or not we want to handle such bogus\ncompiler errors in the past already [1]. Back then we basically decided\nthat we do not care about such old-and-buggy compilers, so while we\ncould fix the issue by using `{{0}}` instead this is not the preferred\nway to handle this in the Git codebase.\n\nWe have an easier fix though: we can just drop the macro altogether and\nhandle initialization of the struct in `reftable_stack_addition_init()`.\nCallers are expected to call this function already, so this change even\nsimplifies the calling convention.\n\n[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/\n\nSuggested-by: Carlo Arenas \nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ed80710572..9db90cf4ed 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -664,8 +664,6 @@ struct reftable_addition {\n \tuint64_t next_update_index;\n };\n \n-#define REFTABLE_ADDITION_INIT {0}\n-\n static void reftable_addition_close(struct reftable_addition *add)\n {\n \tstruct reftable_buf nm = REFTABLE_BUF_INIT;\n@@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tstruct reftable_buf lock_file_name = REFTABLE_BUF_INIT;\n \tint err;\n \n+\tmemset(add, 0, sizeof(*add));\n \tadd->stack = st;\n \n \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n@@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t\t\t void *arg),\n \t\t\t void *arg)\n {\n-\tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st, 0);\n+\tstruct reftable_addition add;\n+\tint err;\n+\n+\terr = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \t\t\t\tstruct reftable_stack *st,\n \t\t\t\tunsigned int flags)\n {\n-\tint err = 0;\n-\tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n+\tint err;\n \n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \tif (!*dest)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \n-\t**dest = empty;\n \terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n \t}\n+\n \treturn err;\n }\n \n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524108","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-5-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:29Z","receivedAt":"2025-08-13T06:25:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_add()` function is a simple wrapper to lock the\nstack, add records to it via a callback and then commit the\nresult. One problem with it though is that it doesn't accept any flags\nfor creating the addition. This makes it impossible to automatically\nreload the stack in case it was modified before we managed to lock the\nstack.\n\nAdd a `flags` field to plug this gap and pass it through accordingly.\nThis flag will be used in a subsequent patch and by libgit2.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 8 +++----\n reftable/reftable-stack.h | 9 +++++---\n reftable/stack.c | 8 +++----\n t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------\n 4 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4c3817f4ec1..3f0deab338c 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n \n done:\n \treturn ret;\n@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 910ec6ef3a2..d70fcb705dc 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);\n * transaction. Releases the lock if held. */\n void reftable_addition_destroy(struct reftable_addition *add);\n \n-/* add a new table to the stack. The write_table function must call\n- * reftable_writer_set_limits, add refs and return an error value. */\n+/*\n+ * Add a new table to the stack. The write_table function must call\n+ * reftable_writer_set_limits, add refs and return an error value.\n+ * The flags are passed through to `reftable_stack_new_addition()`.\n+ */\n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t void *write_arg),\n-\t\t void *write_arg);\n+\t\t void *write_arg, unsigned flags);\n \n struct reftable_iterator;\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 9db90cf4ed0..1ce4d90cb82 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -736,12 +736,12 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\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-\t\t\t void *arg)\n+\t\t\t void *arg, unsigned flags)\n {\n \tstruct reftable_addition add;\n \tint err;\n \n-\terr = reftable_stack_init_addition(&add, st, 0);\n+\terr = reftable_stack_init_addition(&add, st, flags);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -757,9 +757,9 @@ static int stack_try_add(struct reftable_stack *st,\n \n int reftable_stack_add(struct reftable_stack *st,\n \t\t int (*write)(struct reftable_writer *wr, void *arg),\n-\t\t void *arg)\n+\t\t void *arg, unsigned flags)\n {\n-\tint err = stack_try_add(st, write, arg);\n+\tint err = stack_try_add(st, write, arg, flags);\n \tif (err < 0) {\n \t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 2f49c975194..ce10247903c 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tref.refname = buf;\n \t\tt_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)\n \terr = reftable_new_stack(&st2, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st1, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st1, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st2, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st2, write_test_ref, &ref2, 0);\n \tcheck(!err);\n \treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 1);\n \tcheck_int(st->stats.attempts, ==, 0);\n@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)\n \twrite_file_buf(table_path.buf, \"\", 0);\n \n \tref.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \tcheck_int(st->merged->tables_len, ==, 2);\n \tcheck_int(st->stats.attempts, ==, 1);\n@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref1);\n+\terr = reftable_stack_add(st, write_test_ref, &ref1, 0);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref2);\n+\terr = reftable_stack_add(st, write_test_ref, &ref2, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \tfor (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {\n-\t\terr = reftable_stack_add(st, write_error, &i);\n+\t\terr = reftable_stack_add(st, write_error, &i, 0);\n \t\tcheck_int(err, ==, i);\n \t}\n \n@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\terr = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\terr = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \n-\t\terr = reftable_stack_add(st, write_test_log, &arg);\n+\t\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)\n \tcheck(!err);\n \n \tinput.value.update.message = (char *) \"one\\ntwo\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck_int(err, ==, REFTABLE_API_ERROR);\n \n \tinput.value.update.message = (char *) \"one\";\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \n \terr = reftable_stack_read_log(st, input.refname, &dest);\n@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)\n \n \tinput.value.update.message = (char *) \"two\\n\";\n \targ.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_log, &arg);\n+\terr = reftable_stack_add(st, write_test_log, &arg, 0);\n \tcheck(!err);\n \terr = reftable_stack_read_log(st, input.refname, &dest);\n \tcheck(!err);\n@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i]);\n+\t\tint err = reftable_stack_add(st, write_test_ref, &refs[i], 0);\n \t\tcheck(!err);\n \t}\n \n@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \tcheck(!err);\n \n \t/* can't read it with the wrong hash ID. */\n@@ -927,7 +927,7 @@ static void t_reflog_expire(void)\n \t\t\t.log = &logs[i],\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n-\t\tint err = reftable_stack_add(st, write_test_log, &arg);\n+\t\tint err = reftable_stack_add(st, write_test_log, &arg, 0);\n \t\tcheck(!err);\n \t}\n \n@@ -978,7 +978,7 @@ static void t_empty_add(void)\n \terr = reftable_new_stack(&st, dir, &opts);\n \tcheck(!err);\n \n-\terr = reftable_stack_add(st, write_nothing, NULL);\n+\terr = reftable_stack_add(st, write_nothing, NULL, 0);\n \tcheck(!err);\n \n \terr = reftable_new_stack(&st2, dir, &opts);\n@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)\n \t\t};\n \t\tsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_stack_auto_compact(st);\n@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)\n \t\t};\n \t\txsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\tcheck(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));\n@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)\n \t\tsnprintf(buf, sizeof(buf), \"branch-%04\"PRIuMAX, (uintmax_t)i);\n \t\tref.refname = buf;\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, 0);\n \t\tcheck(!err);\n \n \t\t/*\n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524109","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-6-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 6/8] reftable/stack: handle outdated stacks when compacting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:30Z","receivedAt":"2025-08-13T06:25:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we compact the reftable stack we first acquire the lock for the\n\"tables.list\" file and then reload the stack to check that it is still\nup-to-date. This is done by calling `stack_uptodate()`, which knows to\nreturn zero in case the stack is up-to-date, a positive value if it is\nnot and a negative error code on unexpected conditions.\n\nWe don't do proper error checking though, but instead we only check\nwhether the returned error code is non-zero. If so, we simply bubble it\nup the calling stack, which means that callers may see an unexpected\npositive value.\n\nFix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.\nHandle this situation in `reftable_addition_commit()`, where we perform\na best-effort auto-compaction.\n\nAll other callsites of `stack_uptodate()` know to handle a positive\nreturn value and thus don't need to be fixed.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 32 ++++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ce4d90cb8..af0f94d882 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \treturn err;\n }\n \n-/* -1 = error\n- 0 = up to date\n- 1 = changed. */\n+/*\n+ * Check whether the given stack is up-to-date with what we have in memory.\n+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n+ * otherwise.\n+ */\n static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n@@ -850,10 +852,13 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\t * control. It is possible that a concurrent writer is already\n \t\t * trying to compact parts of the stack, which would lead to a\n \t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n-\t\t * already. This is a benign error though, so we ignore it.\n+\t\t * already. Similarly, the stack may have been rewritten by a\n+\t\t * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.\n+\t\t * Both of these errors are benign, so we simply ignore them.\n \t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n-\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n+\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR &&\n+\t\t err != REFTABLE_OUTDATED_ERROR)\n \t\t\tgoto done;\n \t\terr = 0;\n \t}\n@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * Check whether the stack is up-to-date. We unfortunately cannot\n+\t * handle the situation gracefully in case it's _not_ up-to-date\n+\t * because the range of tables that the user has requested us to\n+\t * compact may have been changed. So instead we abort.\n+\t *\n+\t * We could in theory improve the situation by having the caller not\n+\t * pass in a range, but instead the list of tables to compact. If so,\n+\t * we could check that relevant tables still exist. But for now it's\n+\t * good enough to just abort.\n+\t */\n \terr = stack_uptodate(st);\n-\tif (err)\n+\tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0) {\n+\t\terr = REFTABLE_OUTDATED_ERROR;\n+\t\tgoto done;\n+\t}\n \n \t/*\n \t * Lock all tables in the user-provided range. This is the slice of our\n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524110","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-7-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 7/8] reftable: don't second-guess errors from flock interface","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:31Z","receivedAt":"2025-08-13T06:25:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `flock` interface is implemented as part of \"reftable/system.c\" and\nthus needs to be implemented by the integrator between the reftable\nlibrary and its parent code base. As such, we cannot rely on any\nspecific implementation thereof.\n\nRegardless of that, users of the `flock` subsystem rely on `errno` being\nset to specific values. This is fragile and not documented anywhere and\ndoesn't really make for a good interface.\n\nRefactor the code so that the implementations themselves are expected to\nreturn reftable-specific error codes. Our implementation of the `flock`\nsubsystem already knows to do this for all error paths except one.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 37 ++++++++-----------------------------\n reftable/system.c | 2 +-\n reftable/system.h | 4 +++-\n 3 files changed, 12 insertions(+), 31 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex af0f94d882..f91ce50bcd 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \terr = flock_acquire(&add->tables_list_lock, st->list_file,\n \t\t\t st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST) {\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\t} else {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n+\n \tif (st->opts.default_permissions) {\n \t\tif (chmod(add->tables_list_lock.path,\n \t\t\t st->opts.default_permissions) < 0) {\n@@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * which are part of the user-specified range.\n \t */\n \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST)\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\telse\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n \n \t/*\n \t * Check whether the stack is up-to-date. We unfortunately cannot\n@@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t * tables, otherwise there would be nothing to compact.\n \t\t\t * In that case, we return a lock error to our caller.\n \t\t\t */\n-\t\t\tif (errno == EEXIST && last - (i - 1) >= 2 &&\n+\t\t\tif (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&\n \t\t\t flags & STACK_COMPACT_RANGE_BEST_EFFORT) {\n \t\t\t\terr = 0;\n \t\t\t\t/*\n@@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t */\n \t\t\t\tfirst = (i - 1) + 1;\n \t\t\t\tbreak;\n-\t\t\t} else if (errno == EEXIST) {\n-\t\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\t\t\tgoto done;\n-\t\t\t} else {\n-\t\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\t\tgoto done;\n \t\t\t}\n+\n+\t\t\tgoto done;\n \t\t}\n \n \t\t/*\n@@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t * of tables.\n \t\t */\n \t\terr = flock_close(&table_locks[nlocks++]);\n-\t\tif (err < 0) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\t\tif (err < 0)\n \t\t\tgoto done;\n-\t\t}\n \t}\n \n \t/*\n@@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * the new table.\n \t */\n \terr = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);\n-\tif (err < 0) {\n-\t\tif (errno == EEXIST)\n-\t\t\terr = REFTABLE_LOCK_ERROR;\n-\t\telse\n-\t\t\terr = REFTABLE_IO_ERROR;\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n \n \tif (st->opts.default_permissions) {\n \t\tif (chmod(tables_list_lock.path,\ndiff --git a/reftable/system.c b/reftable/system.c\nindex 1ee268b125..725a25844e 100644\n--- a/reftable/system.c\n+++ b/reftable/system.c\n@@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,\n \t\treftable_free(lockfile);\n \t\tif (errno == EEXIST)\n \t\t\treturn REFTABLE_LOCK_ERROR;\n-\t\treturn -1;\n+\t\treturn REFTABLE_IO_ERROR;\n \t}\n \n \tl->fd = get_lock_file_fd(lockfile);\ndiff --git a/reftable/system.h b/reftable/system.h\nindex beb9d2431f..0c623617f0 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -81,7 +81,9 @@ struct reftable_flock {\n * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative\n * we block indefinitely.\n *\n- * Retrun 0 on success, a reftable error code on error.\n+ * Return 0 on success, a reftable error code on error. Specifically,\n+ * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already\n+ * locked.\n */\n int flock_acquire(struct reftable_flock *l, const char *target_path,\n \t\t long timeout_ms);\n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524111","messageId":"20250813-pks-reftable-fixes-for-libgit2-v4-8-42b5544c8e2a@pks.im","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"[PATCH v4 8/8] refs/reftable: always reload stacks when creating lock","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-13T06:25:32Z","receivedAt":"2025-08-13T06:26:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new addition via either `reftable_stack_new_addition()`\nor its convenince wrapper `reftable_stack_add()` we:\n\n 1. Create the \"tables.list.lock\" file.\n\n 2. Verify that the current version of the \"tables.list\" file is\n up-to-date.\n\n 3. Write the new table records if so.\n\nBy default, the second step would cause us to bail out if we see that\nthere has been a concurrent write to the stack that made our in-memory\ncopy of the stack out-of-date. This is a safety mechanism to not write\nrecords to the stack based on outdated information.\n\nThe downside though is that concurrent writes may now cause us to bail\nout, which is not a good user experience. In addition, this isn't even\nnecessary for us, as Git knows to perform all checks for the old state\nof references under the lock. (Well, in all except one case: when we\nexpire the reflog we first create the log iterator before we create the\nlock, but this ordering is fixed as part of this commit.)\n\nConsequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`\nflag. The effect of this flag is that we reload the stack after having\nacquired the lock in case the stack is out-of-date. This plugs the race\nwith concurrent writers, but we continue performing the verifications of\nthe expected old state to catch actual conflicts in the references we\nare about to write.\n\nAdapt the remaining callsites that don't yet pass this flag to do so.\nWhile at it, drop a needless manual reload.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 3f0deab338..66d25411f1 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \tif (!arg) {\n \t\tstruct reftable_addition *addition;\n \n-\t\tret = reftable_stack_reload(be->stack);\n-\t\tif (ret)\n-\t\t\treturn ret;\n-\n \t\tret = reftable_stack_new_addition(&addition, be->stack,\n \t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n@@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);\n \tif (ret)\n \t\tgoto done;\n-\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);\n+\tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \t\tgoto done;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);\n+\tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n done:\n \treturn ret;\n@@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \t\treturn ret;\n \targ.stack = be->stack;\n \n-\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);\n+\tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,\n+\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\n@@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_init_log_iterator(be->stack, &it);\n+\tret = reftable_stack_new_addition(&add, be->stack,\n+\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_iterator_seek_log(&it, refname);\n+\tret = reftable_stack_init_log_iterator(be->stack, &it);\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, be->stack, 0);\n+\tret = reftable_iterator_seek_log(&it, refname);\n \tif (ret < 0)\n \t\tgoto done;\n \n\n-- \n2.51.0.rc1.215.g0f929dcec7.dirty\n\n"},{"id":"524125","messageId":"xmqqldnnpafs.fsf@gitster.g","threadId":"63891","inReplyTo":"aJwsq-dLyQ9I7fDo@pks.im","subject":"Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-13T14:23:35Z","receivedAt":"2025-08-13T14:23:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Aug 12, 2025 at 12:00:53PM -0700, Carlo Arenas wrote:\n>> On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt wrote:\n>> \n>> > Range-diff versus v2:\n>> >\n>> > 1: ce08ba1217 = 1: 1613715dc9 reftable/writer: fix type used for number of records\n>> > 2: 80a87ff19a = 2: 4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro\n>> > 3: e230c97347 < -: ---------- reftable/stack: fix compiler warning due to missing braces\n>> > 4: dd413b76a2 ! 3: 3977a1f497 reftable/stack: reorder code to avoid forward declarations\n>> > @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)\n>> > struct reftable_stack *stack;\n>> > @@ reftable/stack.c: struct reftable_addition {\n>> >\n>> > - #define REFTABLE_ADDITION_INIT {{0}}\n>> > + #define REFTABLE_ADDITION_INIT {0}\n>> \n>> This define shouldn't be needed anymore AFAIK\n>\n> It doesn't exist anymore after this patch series, as it gets removed in\n> the fourth patch. The above change in the range-diff is merely a result\n> of me swapping the order of patch 3 and 4.\n\nInteresting.\n\nWhat is being shown here is that the reordering patch in the\nprevious round had {{0}} here (due to the old \"fix compiler\" step\nmaking that change) while the new iteration has {0} instead. These\nare both context lines that we are seeing here. As the \"fix\ncompiler warning\" step finds no counterpart, old #3 is not shown\nabove this part, and new #4 that removes this #define does not\nappear in the output, either.\n\nA diff-of-diff is a bit confusing to read when something like this\nhappens.\n"},{"id":"524127","messageId":"tea72qawkdmsqoualurcjbdaonjfwjjk2a3fehcm7nmt2rdmbk@xyhkwpapmwcw","threadId":"63891","inReplyTo":"20250813-pks-reftable-fixes-for-libgit2-v4-0-42b5544c8e2a@pks.im","subject":"Re: [PATCH v4 0/8] reftable: a couple of improvements for libgit2","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-13T14:38:16Z","receivedAt":"2025-08-13T14:38:18Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/13 08:25AM, Patrick Steinhardt wrote:\n> Changes in v4:\n> - Adjust stale commit message.\n> - Fix a typo while at it.\n> - Link to v3: https://lore.kernel.org/r/20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im\n\nAfter reviewing the range-diff, this version looks good to me.\n\n-Justin\n"}]}