From 9077923c8ea83f7023c86c517f080396bf96dbb3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:15 +0200 Subject: [PATCH 1/8] reftable/writer: fix type used for number of records Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()` accept an array of records that should be added to the new table. Callers of this function are expected to also pass the number of such records to the function to tell it how many such records it is supposed to write. But while all callers pass in a `size_t`, which is a sensible choice, the function in fact accepts an `int` as argument, which is less so. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/reftable-writer.h | 4 ++-- reftable/writer.c | 17 +++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h index 0fbeff17f4..1e7003cd69 100644 --- a/reftable/reftable-writer.h +++ b/reftable/reftable-writer.h @@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w, the records before adding them, reordering the records array passed in. */ int reftable_writer_add_refs(struct reftable_writer *w, - struct reftable_ref_record *refs, int n); + struct reftable_ref_record *refs, size_t n); /* adds reftable_log_records. Log records are keyed by (refname, decreasing @@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w, the records before adding them, reordering records array passed in. */ int reftable_writer_add_logs(struct reftable_writer *w, - struct reftable_log_record *logs, int n); + struct reftable_log_record *logs, size_t n); /* reftable_writer_close finalizes the reftable. The writer is retained so * statistics can be inspected. */ diff --git a/reftable/writer.c b/reftable/writer.c index 3b4ebdd6dc..5bad130c7e 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -395,14 +395,15 @@ out: } int reftable_writer_add_refs(struct reftable_writer *w, - struct reftable_ref_record *refs, int n) + struct reftable_ref_record *refs, size_t n) { int err = 0; - int i = 0; + QSORT(refs, n, reftable_ref_record_compare_name); - for (i = 0; err == 0 && i < n; i++) { + + for (size_t i = 0; err == 0 && i < n; i++) err = reftable_writer_add_ref(w, &refs[i]); - } + return err; } @@ -486,15 +487,15 @@ done: } int reftable_writer_add_logs(struct reftable_writer *w, - struct reftable_log_record *logs, int n) + struct reftable_log_record *logs, size_t n) { int err = 0; - int i = 0; + QSORT(logs, n, reftable_log_record_compare_key); - for (i = 0; err == 0 && i < n; i++) { + for (size_t i = 0; err == 0 && i < n; i++) err = reftable_writer_add_log(w, &logs[i]); - } + return err; } From d4a2159a78432d787c3f198a58c718b4b4e3d9bb Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:16 +0200 Subject: [PATCH 2/8] reftable/writer: drop Git-specific `QSORT()` macro The reftable writer accidentally uses the Git-specific `QSORT()` macro. This macro removes the need for the caller to provide the element size, but other than that it's mostly equivalent to `qsort()`. Replace the macro accordingly to make the library usable outside of Git. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/writer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/reftable/writer.c b/reftable/writer.c index 5bad130c7e..0133b64975 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w, { int err = 0; - QSORT(refs, n, reftable_ref_record_compare_name); + if (n) + qsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name); for (size_t i = 0; err == 0 && i < n; i++) err = reftable_writer_add_ref(w, &refs[i]); @@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w, { int err = 0; - QSORT(logs, n, reftable_log_record_compare_key); + if (n) + qsort(logs, n, sizeof(*logs), reftable_log_record_compare_key); for (size_t i = 0; err == 0 && i < n; i++) err = reftable_writer_add_log(w, &logs[i]); From 5ed5f5dc01636ac8590a499bb1d63b26789c73aa Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:17 +0200 Subject: [PATCH 3/8] reftable/stack: reorder code to avoid forward declarations We have a couple of forward declarations in the stack-related code of the reftable library. These declarations aren't really required, but are simply caused by unfortunate ordering. Reorder the code and remove the forward declarations. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 364 +++++++++++++++++++++++------------------------ 1 file changed, 176 insertions(+), 188 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index 4caf96aa1d..ed80710572 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -17,18 +17,6 @@ #include "table.h" #include "writer.h" -static int stack_try_add(struct reftable_stack *st, - int (*write_table)(struct reftable_writer *wr, - void *arg), - void *arg); -static int stack_write_compact(struct reftable_stack *st, - struct reftable_writer *wr, - size_t first, size_t last, - struct reftable_log_expiry_config *config); -static void reftable_addition_close(struct reftable_addition *add); -static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st, - int reuse_open); - static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st, const char *name) { @@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg) return stack_fsync(writer->opts, writer->fd); } -int reftable_new_stack(struct reftable_stack **dest, const char *dir, - const struct reftable_write_options *_opts) -{ - struct reftable_buf list_file_name = REFTABLE_BUF_INIT; - struct reftable_write_options opts = { 0 }; - struct reftable_stack *p; - int err; - - p = reftable_calloc(1, sizeof(*p)); - if (!p) { - err = REFTABLE_OUT_OF_MEMORY_ERROR; - goto out; - } - - if (_opts) - opts = *_opts; - if (opts.hash_id == 0) - opts.hash_id = REFTABLE_HASH_SHA1; - - *dest = NULL; - - reftable_buf_reset(&list_file_name); - if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 || - (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0) - goto out; - - p->list_file = reftable_buf_detach(&list_file_name); - p->list_fd = -1; - p->opts = opts; - p->reftable_dir = reftable_strdup(dir); - if (!p->reftable_dir) { - err = REFTABLE_OUT_OF_MEMORY_ERROR; - goto out; - } - - err = reftable_stack_reload_maybe_reuse(p, 1); - if (err < 0) - goto out; - - *dest = p; - err = 0; - -out: - if (err < 0) - reftable_stack_destroy(p); - return err; -} - static int fd_read_lines(int fd, char ***namesp) { char *buf = NULL; @@ -591,6 +531,54 @@ out: return err; } +int reftable_new_stack(struct reftable_stack **dest, const char *dir, + const struct reftable_write_options *_opts) +{ + struct reftable_buf list_file_name = REFTABLE_BUF_INIT; + struct reftable_write_options opts = { 0 }; + struct reftable_stack *p; + int err; + + p = reftable_calloc(1, sizeof(*p)); + if (!p) { + err = REFTABLE_OUT_OF_MEMORY_ERROR; + goto out; + } + + if (_opts) + opts = *_opts; + if (opts.hash_id == 0) + opts.hash_id = REFTABLE_HASH_SHA1; + + *dest = NULL; + + reftable_buf_reset(&list_file_name); + if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 || + (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0) + goto out; + + p->list_file = reftable_buf_detach(&list_file_name); + p->list_fd = -1; + p->opts = opts; + p->reftable_dir = reftable_strdup(dir); + if (!p->reftable_dir) { + err = REFTABLE_OUT_OF_MEMORY_ERROR; + goto out; + } + + err = reftable_stack_reload_maybe_reuse(p, 1); + if (err < 0) + goto out; + + *dest = p; + err = 0; + +out: + if (err < 0) + reftable_stack_destroy(p); + return err; +} + /* -1 = error 0 = up to date 1 = changed. */ @@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st) return err; } -int reftable_stack_add(struct reftable_stack *st, - int (*write)(struct reftable_writer *wr, void *arg), - void *arg) -{ - int err = stack_try_add(st, write, arg); - if (err < 0) { - if (err == REFTABLE_OUTDATED_ERROR) { - /* Ignore error return, we want to propagate - REFTABLE_OUTDATED_ERROR. - */ - reftable_stack_reload(st); - } - return err; - } - - return 0; -} - -static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max) -{ - char buf[100]; - uint32_t rnd = reftable_rand(); - snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x", - min, max, rnd); - reftable_buf_reset(dest); - return reftable_buf_addstr(dest, buf); -} - struct reftable_addition { struct reftable_flock tables_list_lock; struct reftable_stack *stack; @@ -706,6 +666,26 @@ struct reftable_addition { #define REFTABLE_ADDITION_INIT {0} +static void reftable_addition_close(struct reftable_addition *add) +{ + struct reftable_buf nm = REFTABLE_BUF_INIT; + size_t i; + + for (i = 0; i < add->new_tables_len; i++) { + if (!stack_filename(&nm, add->stack, add->new_tables[i])) + unlink(nm.buf); + reftable_free(add->new_tables[i]); + add->new_tables[i] = NULL; + } + reftable_free(add->new_tables); + add->new_tables = NULL; + add->new_tables_len = 0; + add->new_tables_cap = 0; + + flock_release(&add->tables_list_lock); + reftable_buf_release(&nm); +} + static int reftable_stack_init_addition(struct reftable_addition *add, struct reftable_stack *st, unsigned int flags) @@ -754,24 +734,52 @@ done: return err; } -static void reftable_addition_close(struct reftable_addition *add) +static int stack_try_add(struct reftable_stack *st, + int (*write_table)(struct reftable_writer *wr, + void *arg), + void *arg) { - struct reftable_buf nm = REFTABLE_BUF_INIT; - size_t i; + struct reftable_addition add = REFTABLE_ADDITION_INIT; + int err = reftable_stack_init_addition(&add, st, 0); + if (err < 0) + goto done; - for (i = 0; i < add->new_tables_len; i++) { - if (!stack_filename(&nm, add->stack, add->new_tables[i])) - unlink(nm.buf); - reftable_free(add->new_tables[i]); - add->new_tables[i] = NULL; + err = reftable_addition_add(&add, write_table, arg); + if (err < 0) + goto done; + + err = reftable_addition_commit(&add); +done: + reftable_addition_close(&add); + return err; +} + +int reftable_stack_add(struct reftable_stack *st, + int (*write)(struct reftable_writer *wr, void *arg), + void *arg) +{ + int err = stack_try_add(st, write, arg); + if (err < 0) { + if (err == REFTABLE_OUTDATED_ERROR) { + /* Ignore error return, we want to propagate + REFTABLE_OUTDATED_ERROR. + */ + reftable_stack_reload(st); + } + return err; } - reftable_free(add->new_tables); - add->new_tables = NULL; - add->new_tables_len = 0; - add->new_tables_cap = 0; - flock_release(&add->tables_list_lock); - reftable_buf_release(&nm); + return 0; +} + +static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max) +{ + char buf[100]; + uint32_t rnd = reftable_rand(); + snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x", + min, max, rnd); + reftable_buf_reset(dest); + return reftable_buf_addstr(dest, buf); } void reftable_addition_destroy(struct reftable_addition *add) @@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest, return err; } -static int stack_try_add(struct reftable_stack *st, - int (*write_table)(struct reftable_writer *wr, - void *arg), - void *arg) -{ - struct reftable_addition add = REFTABLE_ADDITION_INIT; - int err = reftable_stack_init_addition(&add, st, 0); - if (err < 0) - goto done; - - err = reftable_addition_add(&add, write_table, arg); - if (err < 0) - goto done; - - err = reftable_addition_commit(&add); -done: - reftable_addition_close(&add); - return err; -} - int reftable_addition_add(struct reftable_addition *add, int (*write_table)(struct reftable_writer *wr, void *arg), @@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st) return 1; } -static int stack_compact_locked(struct reftable_stack *st, - size_t first, size_t last, - struct reftable_log_expiry_config *config, - struct reftable_tmpfile *tab_file_out) -{ - struct reftable_buf next_name = REFTABLE_BUF_INIT; - struct reftable_buf tab_file_path = REFTABLE_BUF_INIT; - struct reftable_writer *wr = NULL; - struct fd_writer writer= { - .opts = &st->opts, - }; - struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT; - int err = 0; - - err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]), - reftable_table_max_update_index(st->tables[last])); - if (err < 0) - goto done; - - err = stack_filename(&tab_file_path, st, next_name.buf); - if (err < 0) - goto done; - - err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX"); - if (err < 0) - goto done; - - err = tmpfile_from_pattern(&tab_file, tab_file_path.buf); - if (err < 0) - goto done; - - if (st->opts.default_permissions && - chmod(tab_file.path, st->opts.default_permissions) < 0) { - err = REFTABLE_IO_ERROR; - goto done; - } - - writer.fd = tab_file.fd; - err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush, - &writer, &st->opts); - if (err < 0) - goto done; - - err = stack_write_compact(st, wr, first, last, config); - if (err < 0) - goto done; - - err = reftable_writer_close(wr); - if (err < 0) - goto done; - - err = tmpfile_close(&tab_file); - if (err < 0) - goto done; - - *tab_file_out = tab_file; - tab_file = REFTABLE_TMPFILE_INIT; - -done: - tmpfile_delete(&tab_file); - reftable_writer_free(wr); - reftable_buf_release(&next_name); - reftable_buf_release(&tab_file_path); - return err; -} - static int stack_write_compact(struct reftable_stack *st, struct reftable_writer *wr, size_t first, size_t last, @@ -1172,6 +1094,72 @@ done: return err; } +static int stack_compact_locked(struct reftable_stack *st, + size_t first, size_t last, + struct reftable_log_expiry_config *config, + struct reftable_tmpfile *tab_file_out) +{ + struct reftable_buf next_name = REFTABLE_BUF_INIT; + struct reftable_buf tab_file_path = REFTABLE_BUF_INIT; + struct reftable_writer *wr = NULL; + struct fd_writer writer= { + .opts = &st->opts, + }; + struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT; + int err = 0; + + err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]), + reftable_table_max_update_index(st->tables[last])); + if (err < 0) + goto done; + + err = stack_filename(&tab_file_path, st, next_name.buf); + if (err < 0) + goto done; + + err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX"); + if (err < 0) + goto done; + + err = tmpfile_from_pattern(&tab_file, tab_file_path.buf); + if (err < 0) + goto done; + + if (st->opts.default_permissions && + chmod(tab_file.path, st->opts.default_permissions) < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + + writer.fd = tab_file.fd; + err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush, + &writer, &st->opts); + if (err < 0) + goto done; + + err = stack_write_compact(st, wr, first, last, config); + if (err < 0) + goto done; + + err = reftable_writer_close(wr); + if (err < 0) + goto done; + + err = tmpfile_close(&tab_file); + if (err < 0) + goto done; + + *tab_file_out = tab_file; + tab_file = REFTABLE_TMPFILE_INIT; + +done: + tmpfile_delete(&tab_file); + reftable_writer_free(wr); + reftable_buf_release(&next_name); + reftable_buf_release(&tab_file_path); + return err; +} + enum stack_compact_range_flags { /* * Perform a best-effort compaction. That is, even if we cannot lock From 6fb1d819b7c7796e7cfaae44f056d73436469efc Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:18 +0200 Subject: [PATCH 4/8] reftable/stack: fix compiler warning due to missing braces While perfectly legal, older compiler toolchains complain when zero-initializing structs that contain nested structs with `{0}`: /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] struct reftable_addition empty = REFTABLE_ADDITION_INIT; ^~~~~~~~~~~~~~~~~~~~~~ /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT' #define REFTABLE_ADDITION_INIT {0} ^ We had the discussion around whether or not we want to handle such bogus compiler errors in the past already [1]. Back then we basically decided that we do not care about such old-and-buggy compilers, so while we could fix the issue by using `{{0}}` instead this is not the preferred way to handle this in the Git codebase. We have an easier fix though: we can just drop the macro altogether and handle initialization of the struct in `reftable_stack_addition_init()`. Callers are expected to call this function already, so this change even simplifies the calling convention. [1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/ Suggested-by: Carlo Arenas Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index ed80710572..9db90cf4ed 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -664,8 +664,6 @@ struct reftable_addition { uint64_t next_update_index; }; -#define REFTABLE_ADDITION_INIT {0} - static void reftable_addition_close(struct reftable_addition *add) { struct reftable_buf nm = REFTABLE_BUF_INIT; @@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, struct reftable_buf lock_file_name = REFTABLE_BUF_INIT; int err; + memset(add, 0, sizeof(*add)); add->stack = st; err = flock_acquire(&add->tables_list_lock, st->list_file, @@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st, void *arg), void *arg) { - struct reftable_addition add = REFTABLE_ADDITION_INIT; - int err = reftable_stack_init_addition(&add, st, 0); + struct reftable_addition add; + int err; + + err = reftable_stack_init_addition(&add, st, 0); if (err < 0) goto done; @@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest, struct reftable_stack *st, unsigned int flags) { - int err = 0; - struct reftable_addition empty = REFTABLE_ADDITION_INIT; + int err; REFTABLE_CALLOC_ARRAY(*dest, 1); if (!*dest) return REFTABLE_OUT_OF_MEMORY_ERROR; - **dest = empty; err = reftable_stack_init_addition(*dest, st, flags); if (err) { reftable_free(*dest); *dest = NULL; } + return err; } From 178c5885007b83dd10cac1e09b72ef8d9fe2ac29 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:19 +0200 Subject: [PATCH 5/8] reftable/stack: allow passing flags to `reftable_stack_add()` The `reftable_stack_add()` function is a simple wrapper to lock the stack, add records to it via a callback and then commit the result. One problem with it though is that it doesn't accept any flags for creating the addition. This makes it impossible to automatically reload the stack in case it was modified before we managed to lock the stack. Add a `flags` field to plug this gap and pass it through accordingly. For now this new flag won't be used by us, but it will be used by libgit2. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/reftable-backend.c | 8 +++--- reftable/reftable-stack.h | 9 ++++-- reftable/stack.c | 8 +++--- t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++----------------- 4 files changed, 39 insertions(+), 36 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4c3817f4ec..3f0deab338 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store, ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1); if (ret) goto done; - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg); + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0); done: assert(ret != REFTABLE_API_ERROR); @@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store, ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1); if (ret) goto done; - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg); + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0); done: assert(ret != REFTABLE_API_ERROR); @@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store, goto done; arg.stack = be->stack; - ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg); + ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0); done: return ret; @@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store, return ret; arg.stack = be->stack; - ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg); + ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0); assert(ret != REFTABLE_API_ERROR); return ret; diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h index 910ec6ef3a..d70fcb705d 100644 --- a/reftable/reftable-stack.h +++ b/reftable/reftable-stack.h @@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add); * transaction. Releases the lock if held. */ void reftable_addition_destroy(struct reftable_addition *add); -/* add a new table to the stack. The write_table function must call - * reftable_writer_set_limits, add refs and return an error value. */ +/* + * Add a new table to the stack. The write_table function must call + * reftable_writer_set_limits, add refs and return an error value. + * The flags are passed through to `reftable_stack_new_addition()`. + */ int reftable_stack_add(struct reftable_stack *st, int (*write_table)(struct reftable_writer *wr, void *write_arg), - void *write_arg); + void *write_arg, unsigned flags); struct reftable_iterator; diff --git a/reftable/stack.c b/reftable/stack.c index 9db90cf4ed..1ce4d90cb8 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -736,12 +736,12 @@ done: static int stack_try_add(struct reftable_stack *st, int (*write_table)(struct reftable_writer *wr, void *arg), - void *arg) + void *arg, unsigned flags) { struct reftable_addition add; int err; - err = reftable_stack_init_addition(&add, st, 0); + err = reftable_stack_init_addition(&add, st, flags); if (err < 0) goto done; @@ -757,9 +757,9 @@ done: int reftable_stack_add(struct reftable_stack *st, int (*write)(struct reftable_writer *wr, void *arg), - void *arg) + void *arg, unsigned flags) { - int err = stack_try_add(st, write, arg); + int err = stack_try_add(st, write, arg, flags); if (err < 0) { if (err == REFTABLE_OUTDATED_ERROR) { /* Ignore error return, we want to propagate diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c index 2f49c97519..ce10247903 100644 --- a/t/unit-tests/t-reftable-stack.c +++ b/t/unit-tests/t-reftable-stack.c @@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st, ref.refname = buf; t_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1); - err = reftable_stack_add(st, &write_test_ref, &ref); + err = reftable_stack_add(st, &write_test_ref, &ref, 0); check(!err); } @@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void) err = reftable_new_stack(&st, dir, &opts); check(!err); - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); err = reftable_stack_read_ref(st, ref.refname, &dest); @@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void) err = reftable_new_stack(&st2, dir, &opts); check(!err); - err = reftable_stack_add(st1, write_test_ref, &ref1); + err = reftable_stack_add(st1, write_test_ref, &ref1, 0); check(!err); - err = reftable_stack_add(st2, write_test_ref, &ref2); + err = reftable_stack_add(st2, write_test_ref, &ref2, 0); check_int(err, ==, REFTABLE_OUTDATED_ERROR); err = reftable_stack_reload(st2); check(!err); - err = reftable_stack_add(st2, write_test_ref, &ref2); + err = reftable_stack_add(st2, write_test_ref, &ref2, 0); check(!err); reftable_stack_destroy(st1); reftable_stack_destroy(st2); @@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void) err = reftable_new_stack(&st, dir, &opts); check(!err); - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); check_int(st->merged->tables_len, ==, 1); check_int(st->stats.attempts, ==, 0); @@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void) write_file_buf(table_path.buf, "", 0); ref.update_index = 2; - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); check_int(st->merged->tables_len, ==, 2); check_int(st->stats.attempts, ==, 1); @@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void) err = reftable_new_stack(&st, dir, &opts); check(!err); - err = reftable_stack_add(st, write_test_ref, &ref1); + err = reftable_stack_add(st, write_test_ref, &ref1, 0); check(!err); - err = reftable_stack_add(st, write_test_ref, &ref2); + err = reftable_stack_add(st, write_test_ref, &ref2, 0); check_int(err, ==, REFTABLE_API_ERROR); reftable_stack_destroy(st); clear_dir(dir); @@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void) err = reftable_new_stack(&st, dir, &opts); check(!err); for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) { - err = reftable_stack_add(st, write_error, &i); + err = reftable_stack_add(st, write_error, &i, 0); check_int(err, ==, i); } @@ -546,7 +546,7 @@ static void t_reftable_stack_add(void) } for (i = 0; i < N; i++) { - int err = reftable_stack_add(st, write_test_ref, &refs[i]); + int err = reftable_stack_add(st, write_test_ref, &refs[i], 0); check(!err); } @@ -555,7 +555,7 @@ static void t_reftable_stack_add(void) .log = &logs[i], .update_index = reftable_stack_next_update_index(st), }; - int err = reftable_stack_add(st, write_test_log, &arg); + int err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); } @@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void) } for (i = 0; i < N; i++) { - err = reftable_stack_add(st, write_test_ref, &refs[i]); + err = reftable_stack_add(st, write_test_ref, &refs[i], 0); check(!err); } @@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void) .update_index = reftable_stack_next_update_index(st), }; - err = reftable_stack_add(st, write_test_log, &arg); + err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); } @@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void) check(!err); input.value.update.message = (char *) "one\ntwo"; - err = reftable_stack_add(st, write_test_log, &arg); + err = reftable_stack_add(st, write_test_log, &arg, 0); check_int(err, ==, REFTABLE_API_ERROR); input.value.update.message = (char *) "one"; - err = reftable_stack_add(st, write_test_log, &arg); + err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); err = reftable_stack_read_log(st, input.refname, &dest); @@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void) input.value.update.message = (char *) "two\n"; arg.update_index = 2; - err = reftable_stack_add(st, write_test_log, &arg); + err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); err = reftable_stack_read_log(st, input.refname, &dest); check(!err); @@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void) } } for (i = 0; i < N; i++) { - int err = reftable_stack_add(st, write_test_ref, &refs[i]); + int err = reftable_stack_add(st, write_test_ref, &refs[i], 0); check(!err); } @@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void) .log = &logs[i], .update_index = reftable_stack_next_update_index(st), }; - int err = reftable_stack_add(st, write_test_log, &arg); + int err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); } @@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void) err = reftable_new_stack(&st, dir, &opts); check(!err); - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); /* can't read it with the wrong hash ID. */ @@ -927,7 +927,7 @@ static void t_reflog_expire(void) .log = &logs[i], .update_index = reftable_stack_next_update_index(st), }; - int err = reftable_stack_add(st, write_test_log, &arg); + int err = reftable_stack_add(st, write_test_log, &arg, 0); check(!err); } @@ -978,7 +978,7 @@ static void t_empty_add(void) err = reftable_new_stack(&st, dir, &opts); check(!err); - err = reftable_stack_add(st, write_nothing, NULL); + err = reftable_stack_add(st, write_nothing, NULL, 0); check(!err); err = reftable_new_stack(&st2, dir, &opts); @@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void) }; snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i); - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); err = reftable_stack_auto_compact(st); @@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void) }; xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i); - err = reftable_stack_add(st, &write_test_ref, &ref); + err = reftable_stack_add(st, &write_test_ref, &ref, 0); check(!err); check(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5)); @@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void) snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i); ref.refname = buf; - err = reftable_stack_add(st, write_test_ref, &ref); + err = reftable_stack_add(st, write_test_ref, &ref, 0); check(!err); /* From 54d25de3ea93d42457bfdec43949683544d0031b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:20 +0200 Subject: [PATCH 6/8] reftable/stack: handle outdated stacks when compacting When we compact the reftable stack we first acquire the lock for the "tables.list" file and then reload the stack to check that it is still up-to-date. This is done by calling `stack_uptodate()`, which knows to return zero in case the stack is up-to-date, a positive value if it is not and a negative error code on unexpected conditions. We don't do proper error checking though, but instead we only check whether the returned error code is non-zero. If so, we simply bubble it up the calling stack, which means that callers may see an unexpected positive value. Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead. Handle this situation in `reftable_addition_commit()`, where we perform a best-effort auto-compaction. All other callsites of `stack_uptodate()` know to handle a positive return value and thus don't need to be fixed. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index 1ce4d90cb8..af0f94d882 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -579,9 +579,11 @@ out: return err; } -/* -1 = error - 0 = up to date - 1 = changed. */ +/* + * Check whether the given stack is up-to-date with what we have in memory. + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code + * otherwise. + */ static int stack_uptodate(struct reftable_stack *st) { char **names = NULL; @@ -850,10 +852,13 @@ int reftable_addition_commit(struct reftable_addition *add) * control. It is possible that a concurrent writer is already * trying to compact parts of the stack, which would lead to a * `REFTABLE_LOCK_ERROR` because parts of the stack are locked - * already. This is a benign error though, so we ignore it. + * already. Similarly, the stack may have been rewritten by a + * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`. + * Both of these errors are benign, so we simply ignore them. */ err = reftable_stack_auto_compact(add->stack); - if (err < 0 && err != REFTABLE_LOCK_ERROR) + if (err < 0 && err != REFTABLE_LOCK_ERROR && + err != REFTABLE_OUTDATED_ERROR) goto done; err = 0; } @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st, goto done; } + /* + * Check whether the stack is up-to-date. We unfortunately cannot + * handle the situation gracefully in case it's _not_ up-to-date + * because the range of tables that the user has requested us to + * compact may have been changed. So instead we abort. + * + * We could in theory improve the situation by having the caller not + * pass in a range, but instead the list of tables to compact. If so, + * we could check that relevant tables still exist. But for now it's + * good enough to just abort. + */ err = stack_uptodate(st); - if (err) + if (err < 0) goto done; + if (err > 0) { + err = REFTABLE_OUTDATED_ERROR; + goto done; + } /* * Lock all tables in the user-provided range. This is the slice of our From 8fd7a0ebe100ac3ed757408bbafe478e205804f4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:21 +0200 Subject: [PATCH 7/8] reftable: don't second-guess errors from flock interface The `flock` interface is implemented as part of "reftable/system.c" and thus needs to be implemented by the integrator between the reftable library and its parent code base. As such, we cannot rely on any specific implementation thereof. Regardless of that, users of the `flock` subsystem rely on `errno` being set to specific values. This is fragile and not documented anywhere and doesn't really make for a good interface. Refactor the code so that the implementations themselves are expected to return reftable-specific error codes. Our implementation of the `flock` subsystem already knows to do this for all error paths except one. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 37 ++++++++----------------------------- reftable/system.c | 2 +- reftable/system.h | 4 +++- 3 files changed, 12 insertions(+), 31 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index af0f94d882..f91ce50bcd 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add, err = flock_acquire(&add->tables_list_lock, st->list_file, st->opts.lock_timeout_ms); - if (err < 0) { - if (errno == EEXIST) { - err = REFTABLE_LOCK_ERROR; - } else { - err = REFTABLE_IO_ERROR; - } + if (err < 0) goto done; - } + if (st->opts.default_permissions) { if (chmod(add->tables_list_lock.path, st->opts.default_permissions) < 0) { @@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st, * which are part of the user-specified range. */ err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); - if (err < 0) { - if (errno == EEXIST) - err = REFTABLE_LOCK_ERROR; - else - err = REFTABLE_IO_ERROR; + if (err < 0) goto done; - } /* * Check whether the stack is up-to-date. We unfortunately cannot @@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st, * tables, otherwise there would be nothing to compact. * In that case, we return a lock error to our caller. */ - if (errno == EEXIST && last - (i - 1) >= 2 && + if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 && flags & STACK_COMPACT_RANGE_BEST_EFFORT) { err = 0; /* @@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st, */ first = (i - 1) + 1; break; - } else if (errno == EEXIST) { - err = REFTABLE_LOCK_ERROR; - goto done; - } else { - err = REFTABLE_IO_ERROR; - goto done; } + + goto done; } /* @@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st, * of tables. */ err = flock_close(&table_locks[nlocks++]); - if (err < 0) { - err = REFTABLE_IO_ERROR; + if (err < 0) goto done; - } } /* @@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st, * the new table. */ err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); - if (err < 0) { - if (errno == EEXIST) - err = REFTABLE_LOCK_ERROR; - else - err = REFTABLE_IO_ERROR; + if (err < 0) goto done; - } if (st->opts.default_permissions) { if (chmod(tables_list_lock.path, diff --git a/reftable/system.c b/reftable/system.c index 1ee268b125..725a25844e 100644 --- a/reftable/system.c +++ b/reftable/system.c @@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path, reftable_free(lockfile); if (errno == EEXIST) return REFTABLE_LOCK_ERROR; - return -1; + return REFTABLE_IO_ERROR; } l->fd = get_lock_file_fd(lockfile); diff --git a/reftable/system.h b/reftable/system.h index beb9d2431f..c54ed4cad6 100644 --- a/reftable/system.h +++ b/reftable/system.h @@ -81,7 +81,9 @@ struct reftable_flock { * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative * we block indefinitely. * - * Retrun 0 on success, a reftable error code on error. + * Retrun 0 on success, a reftable error code on error. Specifically, + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already + * locked. */ int flock_acquire(struct reftable_flock *l, const char *target_path, long timeout_ms); From 16684b6fae43b45309f0a75d7e0cc207954e98c8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:22 +0200 Subject: [PATCH 8/8] refs/reftable: always reload stacks when creating lock When creating a new addition via either `reftable_stack_new_addition()` or its convenince wrapper `reftable_stack_add()` we: 1. Create the "tables.list.lock" file. 2. Verify that the current version of the "tables.list" file is up-to-date. 3. Write the new table records if so. By default, the second step would cause us to bail out if we see that there has been a concurrent write to the stack that made our in-memory copy of the stack out-of-date. This is a safety mechanism to not write records to the stack based on outdated information. The downside though is that concurrent writes may now cause us to bail out, which is not a good user experience. In addition, this isn't even necessary for us, as Git knows to perform all checks for the old state of references under the lock. (Well, in all except one case: when we expire the reflog we first create the log iterator before we create the lock, but this ordering is fixed as part of this commit.) Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD` flag. The effect of this flag is that we reload the stack after having acquired the lock in case the stack is out-of-date. This plugs the race with concurrent writers, but we continue performing the verifications of the expected old state to catch actual conflicts in the references we are about to write. Adapt the remaining callsites that don't yet pass this flag to do so. While at it, drop a needless manual reload. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/reftable-backend.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 3f0deab338..66d25411f1 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, if (!arg) { struct reftable_addition *addition; - ret = reftable_stack_reload(be->stack); - if (ret) - return ret; - ret = reftable_stack_new_addition(&addition, be->stack, REFTABLE_STACK_NEW_ADDITION_RELOAD); if (ret) { @@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store, ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1); if (ret) goto done; - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0); + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, + REFTABLE_STACK_NEW_ADDITION_RELOAD); done: assert(ret != REFTABLE_API_ERROR); @@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store, ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1); if (ret) goto done; - ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0); + ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, + REFTABLE_STACK_NEW_ADDITION_RELOAD); done: assert(ret != REFTABLE_API_ERROR); @@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store, goto done; arg.stack = be->stack; - ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0); + ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, + REFTABLE_STACK_NEW_ADDITION_RELOAD); done: return ret; @@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store, return ret; arg.stack = be->stack; - ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0); + ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, + REFTABLE_STACK_NEW_ADDITION_RELOAD); assert(ret != REFTABLE_API_ERROR); return ret; @@ -2552,6 +2552,11 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (ret < 0) goto done; + ret = reftable_stack_new_addition(&add, be->stack, + REFTABLE_STACK_NEW_ADDITION_RELOAD); + if (ret < 0) + goto done; + ret = reftable_stack_init_log_iterator(be->stack, &it); if (ret < 0) goto done; @@ -2560,10 +2565,6 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (ret < 0) goto done; - ret = reftable_stack_new_addition(&add, be->stack, 0); - if (ret < 0) - goto done; - ret = reftable_backend_read_ref(be, refname, &oid, &referent, &type); if (ret < 0) goto done;