From 9077923c8ea83f7023c86c517f080396bf96dbb3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 12 Aug 2025 11:54:15 +0200 Subject: [PATCH 01/21] 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 0fbeff17f462ed..1e7003cd698879 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 3b4ebdd6dced34..5bad130c7ed09d 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w, } 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 @@ int reftable_writer_add_log(struct reftable_writer *w, } 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 02/21] 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 5bad130c7ed09d..0133b649759bcf 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 03/21] 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 4caf96aa1d6961..ed807105725973 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 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st, 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 @@ static int reftable_stack_init_addition(struct reftable_addition *add, 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 @@ static int stack_write_compact(struct reftable_stack *st, 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 04/21] 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 ed807105725973..9db90cf4ed0f8d 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 05/21] 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 4c3817f4ec1a88..3f0deab338c288 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 910ec6ef3a2f57..d70fcb705dcffe 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 9db90cf4ed0f8d..1ce4d90cb8214d 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -736,12 +736,12 @@ static int reftable_stack_init_addition(struct reftable_addition *add, 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 @@ static int stack_try_add(struct reftable_stack *st, 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 2f49c9751948f1..ce10247903c5f3 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 06/21] 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 1ce4d90cb8214d..af0f94d8826443 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir, 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 07/21] 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 af0f94d8826443..f91ce50bcdd4ee 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 1ee268b125ddb6..725a25844ea179 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 beb9d2431f7037..c54ed4cad61f73 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 08/21] 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 | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 3f0deab338c288..66d25411f1e9e3 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,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (ret < 0) goto done; - ret = reftable_stack_init_log_iterator(be->stack, &it); + ret = reftable_stack_new_addition(&add, be->stack, + REFTABLE_STACK_NEW_ADDITION_RELOAD); if (ret < 0) goto done; - ret = reftable_iterator_seek_log(&it, refname); + ret = reftable_stack_init_log_iterator(be->stack, &it); if (ret < 0) goto done; - ret = reftable_stack_new_addition(&add, be->stack, 0); + ret = reftable_iterator_seek_log(&it, refname); if (ret < 0) goto done; From e715f776820f22d0951e02947dd0c4f889e83df8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Aug 2025 16:59:29 -0400 Subject: [PATCH 09/21] describe: pass oid struct by const pointer We pass a "struct object_id" to describe_blob() by value. This isn't wrong, as an oid is composed only of copy-able values. But it's unusual; typically we pass structs by const pointer, including object_ids. Let's do so. It similarly makes sense for us to hold that pointer in the callback data (rather than yet another copy of the oid). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index d7dd8139dec4b6..383d3e6b9a87c8 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -490,7 +490,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) struct process_commit_data { struct object_id current_commit; - struct object_id looking_for; + const struct object_id *looking_for; struct strbuf *dst; struct rev_info *revs; }; @@ -505,7 +505,7 @@ static void process_object(struct object *obj, const char *path, void *data) { struct process_commit_data *pcd = data; - if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) { + if (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) { reset_revision_walk(); describe_commit(&pcd->current_commit, pcd->dst); strbuf_addf(pcd->dst, ":%s", path); @@ -514,7 +514,7 @@ static void process_object(struct object *obj, const char *path, void *data) } } -static void describe_blob(struct object_id oid, struct strbuf *dst) +static void describe_blob(const struct object_id *oid, struct strbuf *dst) { struct rev_info revs; struct strvec args = STRVEC_INIT; @@ -554,7 +554,7 @@ static void describe(const char *arg, int last_one) describe_commit(&oid, &sb); else if (odb_read_object_info(the_repository->objects, &oid, NULL) == OBJ_BLOB) - describe_blob(oid, &sb); + describe_blob(&oid, &sb); else die(_("%s is neither a commit nor blob"), arg); From db2664b6f7c88910b1ab21bcdbac87be098df8a2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Aug 2025 17:01:25 -0400 Subject: [PATCH 10/21] describe: error if blob not found If describe_blob() does not find the blob in question, it returns an empty strbuf, and we print an empty line. This differs from describe_commit(), which always either returns an answer or calls die() itself. As the blob function was bolted onto the command afterwards, I think its behavior is not intentional, and it is just a bug that it does not report an error. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 3 +++ t/t6120-describe.sh | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/builtin/describe.c b/builtin/describe.c index 383d3e6b9a87c8..06e413d9375864 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -535,6 +535,9 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst) reset_revision_walk(); release_revisions(&revs); strvec_clear(&args); + + if (!dst->len) + die(_("blob '%s' not reachable from HEAD"), oid_to_hex(oid)); } static void describe(const char *arg, int last_one) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 256ccaefb75bb6..470631d17d1c5c 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -409,6 +409,12 @@ test_expect_success 'describe tag object' ' test_grep "fatal: test-blob-1 is neither a commit nor blob" actual ' +test_expect_success 'describe an unreachable blob' ' + blob=$(echo not-found-anywhere | git hash-object -w --stdin) && + test_must_fail git describe $blob 2>actual && + test_grep "blob .$blob. not reachable from HEAD" actual +' + test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' ' i=1 && while test $i -lt 8000 From c6478715a52b8f757e898e1d9f8f8d1732fafb24 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Aug 2025 17:01:54 -0400 Subject: [PATCH 11/21] describe: catch unborn branch in describe_blob() When describing a blob, we search for it by traversing from HEAD. We do this by feeding the name HEAD to setup_revisions(). But if we are on an unborn branch, this will fail with a confusing message: $ git describe $blob fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' It is OK for this to be an error (we cannot find $blob in an empty traversal, so we'd eventually complain about that). But the error message could be more helpful. Let's resolve HEAD ourselves and pass the resolved object id to setup_revisions(). If resolving fails, then we can print a more useful message. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 8 +++++++- t/t6120-describe.sh | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/builtin/describe.c b/builtin/describe.c index 06e413d9375864..f7bea3c8c5ebab 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -518,10 +518,16 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst) { struct rev_info revs; struct strvec args = STRVEC_INIT; + struct object_id head_oid; struct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs}; + if (repo_get_oid(the_repository, "HEAD", &head_oid)) + die(_("cannot search for blob '%s' on an unborn branch"), + oid_to_hex(oid)); + strvec_pushl(&args, "internal: The first arg is not parsed", - "--objects", "--in-commit-order", "--reverse", "HEAD", + "--objects", "--in-commit-order", "--reverse", + oid_to_hex(&head_oid), NULL); repo_init_revisions(the_repository, &revs, NULL); diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 470631d17d1c5c..feec57bcbc577a 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -415,6 +415,14 @@ test_expect_success 'describe an unreachable blob' ' test_grep "blob .$blob. not reachable from HEAD" actual ' +test_expect_success 'describe blob on an unborn branch' ' + oldbranch=$(git symbolic-ref HEAD) && + test_when_finished "git symbolic-ref HEAD $oldbranch" && + git symbolic-ref HEAD refs/heads/does-not-exist && + test_must_fail git describe test-blob 2>actual && + test_grep "cannot search .* on an unborn branch" actual +' + test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' ' i=1 && while test $i -lt 8000 From c4cf8caadd407d8b1eafec38b3dfc1f77f61cc19 Mon Sep 17 00:00:00 2001 From: Adam Dinwoodie Date: Tue, 19 Aug 2025 08:43:29 +0100 Subject: [PATCH 12/21] t/t1517: mark tests that fail with GIT_TEST_INSTALLED The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests outside a repository, 2025-08-08) to automatically loop over all "main" Git commands will, when run against an installed build using GIT_TEST_INSTALLED rather than the build in the build directory, include some extra git-gui commands that are installed by `make install`, or credential helpers that might be installed manually from the contrib directories. These fail the test, so record them as such. Signed-off-by: Adam Dinwoodie Signed-off-by: Junio C Hamano --- t/t1517-outside-repo.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh index 3dc602872a0037..5f3b9f400d994c 100755 --- a/t/t1517-outside-repo.sh +++ b/t/t1517-outside-repo.sh @@ -111,8 +111,11 @@ for cmd in $(git --list-cmds=main) do cmd=${cmd%.*} # strip .sh, .perl, etc. case "$cmd" in - archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \ + archimport | citool | credential-netrc | credential-libsecret | \ + credential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \ + daemon | \ difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \ + gui | gui--askpass | \ http-backend | http-fetch | http-push | init-db | \ merge-octopus | merge-one-file | merge-resolve | mergetool | \ mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \ From 217e4a23d76fe95a0f6ab0f6159de2460db6fcd9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Aug 2025 15:24:55 -0400 Subject: [PATCH 13/21] t5510: make confusing config cleanup more explicit Several tests set a config variable in a sub-repo we chdir into via a subshell, like this: ( cd "$D" && cd two && git config foo.bar baz ) But they also clean up the variable with a when_finished directive outside of the subshell, like this: test_when_finished "git config unset foo.bar" At first glance, this shouldn't work! The cleanup clause cannot be run from the subshell (since environment changes there are lost by the time the test snippet finishes). But since the cleanup command runs outside the subshell, our working directory will not have been switched into "two". But it does work. Why? The answer is that an earlier test does a "cd two" that moves the whole test's working directory out of $TRASH_DIRECTORY and into "two". So the subshell is a bit of a red herring; we are already in the right directory! That's why we need the "cd $D" at the top of the shell, to put us back to a known spot. Let's make this cleanup code more explicitly specify where we expect the config command to run. That makes the script more robust against running a subset of the tests, and ultimately will make it easier to refactor the script to avoid these top-level chdirs. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5510-fetch.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ebc696546bc332..64fea9f4a51446 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -119,7 +119,7 @@ test_expect_success "fetch test remote HEAD change" ' test "z$head" = "z$branch"' test_expect_success "fetch test followRemoteHEAD never" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -134,7 +134,7 @@ test_expect_success "fetch test followRemoteHEAD never" ' ' test_expect_success "fetch test followRemoteHEAD warn no change" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -154,7 +154,7 @@ test_expect_success "fetch test followRemoteHEAD warn no change" ' ' test_expect_success "fetch test followRemoteHEAD warn create" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -170,7 +170,7 @@ test_expect_success "fetch test followRemoteHEAD warn create" ' ' test_expect_success "fetch test followRemoteHEAD warn detached" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -187,7 +187,7 @@ test_expect_success "fetch test followRemoteHEAD warn detached" ' ' test_expect_success "fetch test followRemoteHEAD warn quiet" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -205,7 +205,7 @@ test_expect_success "fetch test followRemoteHEAD warn quiet" ' ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -223,7 +223,7 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is sa ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -243,7 +243,7 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is di ' test_expect_success "fetch test followRemoteHEAD always" ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && @@ -260,7 +260,7 @@ test_expect_success "fetch test followRemoteHEAD always" ' ' test_expect_success 'followRemoteHEAD does not kick in with refspecs' ' - test_when_finished "git config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && ( cd "$D" && cd two && From 1de2903c0f065b4c14326a741a57cc7e7b63610f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Aug 2025 15:26:06 -0400 Subject: [PATCH 14/21] t5510: stop changing top-level working directory Several tests in t5510 do a bare "cd subrepo", not in a subshell. This changes the working directory for subsequent tests. As a result, almost every test has to start with "cd $D" to go back to the top-level. Our usual style is to do per-test environment changes like this in a subshell, so that tests can assume they are starting at the top-level $TRASH_DIRECTORY. Let's switch to that style, which lets us drop all of that extra path-handling. Most cases can switch to using a subshell, but in a few spots we can simplify by doing "git init foo && git -C foo ...". We do have to make sure that we weren't intentionally touching the environment in any code which was moved into a subshell (e.g., with a test_when_finished), but that isn't the case for any of these tests. All of the references to the $D variable can go away, replaced generally with $PWD or $TRASH_DIRECTORY (if we use it inside a chdir'd subshell). Note in one test, "fetch --prune prints the remotes url", we make sure to use $(pwd) to get the Windows-style path on that platform (for the other tests, the exact form doesn't matter). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5510-fetch.sh | 356 +++++++++++++++++++++-------------------------- 1 file changed, 161 insertions(+), 195 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 64fea9f4a51446..93e309e2130b17 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -14,8 +14,6 @@ then test_done fi -D=$(pwd) - test_expect_success setup ' echo >file original && git add file && @@ -51,46 +49,50 @@ test_expect_success "clone and setup child repos" ' ' test_expect_success "fetch test" ' - cd "$D" && echo >file updated by origin && git commit -a -m "updated by origin" && - cd two && - git fetch && - git rev-parse --verify refs/heads/one && - mine=$(git rev-parse refs/heads/one) && - his=$(cd ../one && git rev-parse refs/heads/main) && - test "z$mine" = "z$his" + ( + cd two && + git fetch && + git rev-parse --verify refs/heads/one && + mine=$(git rev-parse refs/heads/one) && + his=$(cd ../one && git rev-parse refs/heads/main) && + test "z$mine" = "z$his" + ) ' test_expect_success "fetch test for-merge" ' - cd "$D" && - cd three && - git fetch && - git rev-parse --verify refs/heads/two && - git rev-parse --verify refs/heads/one && - main_in_two=$(cd ../two && git rev-parse main) && - one_in_two=$(cd ../two && git rev-parse one) && - { - echo "$one_in_two " && - echo "$main_in_two not-for-merge" - } >expected && - cut -f -2 .git/FETCH_HEAD >actual && - test_cmp expected actual' + ( + cd three && + git fetch && + git rev-parse --verify refs/heads/two && + git rev-parse --verify refs/heads/one && + main_in_two=$(cd ../two && git rev-parse main) && + one_in_two=$(cd ../two && git rev-parse one) && + { + echo "$one_in_two " && + echo "$main_in_two not-for-merge" + } >expected && + cut -f -2 .git/FETCH_HEAD >actual && + test_cmp expected actual + ) +' test_expect_success "fetch test remote HEAD" ' - cd "$D" && - cd two && - git fetch && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/main) && - test "z$head" = "z$branch"' + ( + cd two && + git fetch && + git rev-parse --verify refs/remotes/origin/HEAD && + git rev-parse --verify refs/remotes/origin/main && + head=$(git rev-parse refs/remotes/origin/HEAD) && + branch=$(git rev-parse refs/remotes/origin/main) && + test "z$head" = "z$branch" + ) +' test_expect_success "fetch test remote HEAD in bare repository" ' test_when_finished rm -rf barerepo && ( - cd "$D" && git init --bare barerepo && cd barerepo && git remote add upstream ../two && @@ -105,23 +107,24 @@ test_expect_success "fetch test remote HEAD in bare repository" ' test_expect_success "fetch test remote HEAD change" ' - cd "$D" && - cd two && - git switch -c other && - git push -u origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git fetch && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/other) && - test "z$head" = "z$branch"' + ( + cd two && + git switch -c other && + git push -u origin other && + git rev-parse --verify refs/remotes/origin/HEAD && + git rev-parse --verify refs/remotes/origin/main && + git rev-parse --verify refs/remotes/origin/other && + git remote set-head origin other && + git fetch && + head=$(git rev-parse refs/remotes/origin/HEAD) && + branch=$(git rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" + ) +' test_expect_success "fetch test followRemoteHEAD never" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git update-ref --no-deref -d refs/remotes/origin/HEAD && git config set remote.origin.followRemoteHEAD "never" && @@ -134,9 +137,8 @@ test_expect_success "fetch test followRemoteHEAD never" ' ' test_expect_success "fetch test followRemoteHEAD warn no change" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git rev-parse --verify refs/remotes/origin/other && git remote set-head origin other && @@ -154,9 +156,8 @@ test_expect_success "fetch test followRemoteHEAD warn no change" ' ' test_expect_success "fetch test followRemoteHEAD warn create" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git update-ref --no-deref -d refs/remotes/origin/HEAD && git config set remote.origin.followRemoteHEAD "warn" && @@ -170,9 +171,8 @@ test_expect_success "fetch test followRemoteHEAD warn create" ' ' test_expect_success "fetch test followRemoteHEAD warn detached" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git update-ref --no-deref -d refs/remotes/origin/HEAD && git update-ref refs/remotes/origin/HEAD HEAD && @@ -187,9 +187,8 @@ test_expect_success "fetch test followRemoteHEAD warn detached" ' ' test_expect_success "fetch test followRemoteHEAD warn quiet" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git rev-parse --verify refs/remotes/origin/other && git remote set-head origin other && @@ -205,9 +204,8 @@ test_expect_success "fetch test followRemoteHEAD warn quiet" ' ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git rev-parse --verify refs/remotes/origin/other && git remote set-head origin other && @@ -223,9 +221,8 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is sa ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git rev-parse --verify refs/remotes/origin/other && git remote set-head origin other && @@ -243,9 +240,8 @@ test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is di ' test_expect_success "fetch test followRemoteHEAD always" ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git rev-parse --verify refs/remotes/origin/other && git remote set-head origin other && @@ -260,9 +256,8 @@ test_expect_success "fetch test followRemoteHEAD always" ' ' test_expect_success 'followRemoteHEAD does not kick in with refspecs' ' - test_when_finished "git -C \"$D/two\" config unset remote.origin.followRemoteHEAD" && + test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && ( - cd "$D" && cd two && git remote set-head origin other && git config set remote.origin.followRemoteHEAD always && @@ -274,93 +269,100 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' ' ' test_expect_success 'fetch --prune on its own works as expected' ' - cd "$D" && git clone . prune && - cd prune && - git update-ref refs/remotes/origin/extrabranch main && + ( + cd prune && + git update-ref refs/remotes/origin/extrabranch main && - git fetch --prune origin && - test_must_fail git rev-parse origin/extrabranch + git fetch --prune origin && + test_must_fail git rev-parse origin/extrabranch + ) ' test_expect_success 'fetch --prune with a branch name keeps branches' ' - cd "$D" && git clone . prune-branch && - cd prune-branch && - git update-ref refs/remotes/origin/extrabranch main && + ( + cd prune-branch && + git update-ref refs/remotes/origin/extrabranch main && - git fetch --prune origin main && - git rev-parse origin/extrabranch + git fetch --prune origin main && + git rev-parse origin/extrabranch + ) ' test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' - cd "$D" && git clone . prune-namespace && - cd prune-namespace && + ( + cd prune-namespace && - git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* && - git rev-parse origin/main + git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* && + git rev-parse origin/main + ) ' test_expect_success 'fetch --prune handles overlapping refspecs' ' - cd "$D" && git update-ref refs/pull/42/head main && git clone . prune-overlapping && - cd prune-overlapping && - git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && + ( + cd prune-overlapping && + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && - git fetch --prune origin && - git rev-parse origin/main && - git rev-parse origin/pr/42 && + git fetch --prune origin && + git rev-parse origin/main && + git rev-parse origin/pr/42 && - git config --unset-all remote.origin.fetch && - git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && - git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && + git config --unset-all remote.origin.fetch && + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && - git fetch --prune origin && - git rev-parse origin/main && - git rev-parse origin/pr/42 + git fetch --prune origin && + git rev-parse origin/main && + git rev-parse origin/pr/42 + ) ' test_expect_success 'fetch --prune --tags prunes branches but not tags' ' - cd "$D" && git clone . prune-tags && - cd prune-tags && - git tag sometag main && - # Create what looks like a remote-tracking branch from an earlier - # fetch that has since been deleted from the remote: - git update-ref refs/remotes/origin/fake-remote main && - - git fetch --prune --tags origin && - git rev-parse origin/main && - test_must_fail git rev-parse origin/fake-remote && - git rev-parse sometag + ( + cd prune-tags && + git tag sometag main && + # Create what looks like a remote-tracking branch from an earlier + # fetch that has since been deleted from the remote: + git update-ref refs/remotes/origin/fake-remote main && + + git fetch --prune --tags origin && + git rev-parse origin/main && + test_must_fail git rev-parse origin/fake-remote && + git rev-parse sometag + ) ' test_expect_success 'fetch --prune --tags with branch does not prune other things' ' - cd "$D" && git clone . prune-tags-branch && - cd prune-tags-branch && - git tag sometag main && - git update-ref refs/remotes/origin/extrabranch main && + ( + cd prune-tags-branch && + git tag sometag main && + git update-ref refs/remotes/origin/extrabranch main && - git fetch --prune --tags origin main && - git rev-parse origin/extrabranch && - git rev-parse sometag + git fetch --prune --tags origin main && + git rev-parse origin/extrabranch && + git rev-parse sometag + ) ' test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' ' - cd "$D" && git clone . prune-tags-refspec && - cd prune-tags-refspec && - git tag sometag main && - git update-ref refs/remotes/origin/foo/otherbranch main && - git update-ref refs/remotes/origin/extrabranch main && - - git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* && - test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch && - git rev-parse origin/extrabranch && - git rev-parse sometag + ( + cd prune-tags-refspec && + git tag sometag main && + git update-ref refs/remotes/origin/foo/otherbranch main && + git update-ref refs/remotes/origin/extrabranch main && + + git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* && + test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch && + git rev-parse origin/extrabranch && + git rev-parse sometag + ) ' test_expect_success 'fetch --tags gets tags even without a configured remote' ' @@ -381,21 +383,21 @@ test_expect_success 'fetch --tags gets tags even without a configured remote' ' ' test_expect_success REFFILES 'fetch --prune fails to delete branches' ' - cd "$D" && git clone . prune-fail && - cd prune-fail && - git update-ref refs/remotes/origin/extrabranch main && - git pack-refs --all && - : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds && - >.git/packed-refs.new && + ( + cd prune-fail && + git update-ref refs/remotes/origin/extrabranch main && + git pack-refs --all && + : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds && + >.git/packed-refs.new && - test_must_fail git fetch --prune origin + test_must_fail git fetch --prune origin + ) ' test_expect_success 'fetch --atomic works with a single branch' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && git branch atomic-branch && oid=$(git rev-parse atomic-branch) && @@ -408,9 +410,8 @@ test_expect_success 'fetch --atomic works with a single branch' ' ' test_expect_success 'fetch --atomic works with multiple branches' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && git branch atomic-branch-1 && git branch atomic-branch-2 && @@ -423,9 +424,8 @@ test_expect_success 'fetch --atomic works with multiple branches' ' ' test_expect_success 'fetch --atomic works with mixed branches and tags' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && git branch atomic-mixed-branch && git tag atomic-mixed-tag && @@ -437,9 +437,8 @@ test_expect_success 'fetch --atomic works with mixed branches and tags' ' ' test_expect_success 'fetch --atomic prunes references' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git branch atomic-prune-delete && git clone . atomic && git branch --delete atomic-prune-delete && @@ -453,9 +452,8 @@ test_expect_success 'fetch --atomic prunes references' ' ' test_expect_success 'fetch --atomic aborts with non-fast-forward update' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git branch atomic-non-ff && git clone . atomic && git rev-parse HEAD >actual && @@ -472,9 +470,8 @@ test_expect_success 'fetch --atomic aborts with non-fast-forward update' ' ' test_expect_success 'fetch --atomic executes a single reference transaction only' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && git branch atomic-hooks-1 && git branch atomic-hooks-2 && @@ -499,9 +496,8 @@ test_expect_success 'fetch --atomic executes a single reference transaction only ' test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && git branch atomic-hooks-abort-1 && git branch atomic-hooks-abort-2 && @@ -536,9 +532,8 @@ test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' ' test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' ' - test_when_finished "rm -rf \"$D\"/atomic" && + test_when_finished "rm -rf atomic" && - cd "$D" && git clone . atomic && oid=$(git rev-parse HEAD) && @@ -574,8 +569,7 @@ test_expect_success REFFILES 'fetch --atomic fails transaction if reference lock ' test_expect_success '--refmap="" ignores configured refspec' ' - cd "$TRASH_DIRECTORY" && - git clone "$D" remote-refs && + git clone . remote-refs && git -C remote-refs rev-parse remotes/origin/main >old && git -C remote-refs update-ref refs/remotes/origin/main main~1 && git -C remote-refs rev-parse remotes/origin/main >new && @@ -599,34 +593,26 @@ test_expect_success '--refmap="" and --prune' ' test_expect_success 'fetch tags when there is no tags' ' - cd "$D" && - - mkdir notags && - cd notags && - git init && - - git fetch -t .. + git init notags && + git -C notags fetch -t .. ' test_expect_success 'fetch following tags' ' - cd "$D" && git tag -a -m "annotated" anno HEAD && git tag light HEAD && - mkdir four && - cd four && - git init && - - git fetch .. :track && - git show-ref --verify refs/tags/anno && - git show-ref --verify refs/tags/light - + git init four && + ( + cd four && + git fetch .. :track && + git show-ref --verify refs/tags/anno && + git show-ref --verify refs/tags/light + ) ' test_expect_success 'fetch uses remote ref names to describe new refs' ' - cd "$D" && git init descriptive && ( cd descriptive && @@ -654,30 +640,20 @@ test_expect_success 'fetch uses remote ref names to describe new refs' ' test_expect_success 'fetch must not resolve short tag name' ' - cd "$D" && - - mkdir five && - cd five && - git init && - - test_must_fail git fetch .. anno:five + git init five && + test_must_fail git -C five fetch .. anno:five ' test_expect_success 'fetch can now resolve short remote name' ' - cd "$D" && git update-ref refs/remotes/six/HEAD HEAD && - mkdir six && - cd six && - git init && - - git fetch .. six:six + git init six && + git -C six fetch .. six:six ' test_expect_success 'create bundle 1' ' - cd "$D" && echo >file updated again by origin && git commit -a -m "tip" && git bundle create --version=3 bundle1 main^..main @@ -691,35 +667,36 @@ test_expect_success 'header of bundle looks right' ' OID refs/heads/main EOF - sed -e "s/$OID_REGEX/OID/g" -e "5q" "$D"/bundle1 >actual && + sed -e "s/$OID_REGEX/OID/g" -e "5q" bundle1 >actual && test_cmp expect actual ' test_expect_success 'create bundle 2' ' - cd "$D" && git bundle create bundle2 main~2..main ' test_expect_success 'unbundle 1' ' - cd "$D/bundle" && - git checkout -b some-branch && - test_must_fail git fetch "$D/bundle1" main:main + ( + cd bundle && + git checkout -b some-branch && + test_must_fail git fetch bundle1 main:main + ) ' test_expect_success 'bundle 1 has only 3 files ' ' - cd "$D" && test_bundle_object_count bundle1 3 ' test_expect_success 'unbundle 2' ' - cd "$D/bundle" && - git fetch ../bundle2 main:main && - test "tip" = "$(git log -1 --pretty=oneline main | cut -d" " -f2)" + ( + cd bundle && + git fetch ../bundle2 main:main && + test "tip" = "$(git log -1 --pretty=oneline main | cut -d" " -f2)" + ) ' test_expect_success 'bundle does not prerequisite objects' ' - cd "$D" && touch file2 && git add file2 && git commit -m add.file2 file2 && @@ -729,7 +706,6 @@ test_expect_success 'bundle does not prerequisite objects' ' test_expect_success 'bundle should be able to create a full history' ' - cd "$D" && git tag -a -m "1.0" v1.0 main && git bundle create bundle4 v1.0 @@ -783,7 +759,6 @@ test_expect_success 'quoting of a strangely named repo' ' test_expect_success 'bundle should record HEAD correctly' ' - cd "$D" && git bundle create bundle5 HEAD main && git bundle list-heads bundle5 >actual && for h in HEAD refs/heads/main @@ -803,7 +778,6 @@ test_expect_success 'mark initial state of origin/main' ' test_expect_success 'explicit fetch should update tracking' ' - cd "$D" && git branch -f side && ( cd three && @@ -818,7 +792,6 @@ test_expect_success 'explicit fetch should update tracking' ' test_expect_success 'explicit pull should update tracking' ' - cd "$D" && git branch -f side && ( cd three && @@ -832,7 +805,6 @@ test_expect_success 'explicit pull should update tracking' ' ' test_expect_success 'explicit --refmap is allowed only with command-line refspec' ' - cd "$D" && ( cd three && test_must_fail git fetch --refmap="*:refs/remotes/none/*" @@ -840,7 +812,6 @@ test_expect_success 'explicit --refmap is allowed only with command-line refspec ' test_expect_success 'explicit --refmap option overrides remote.*.fetch' ' - cd "$D" && git branch -f side && ( cd three && @@ -855,7 +826,6 @@ test_expect_success 'explicit --refmap option overrides remote.*.fetch' ' ' test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' ' - cd "$D" && git branch -f side && ( cd three && @@ -870,7 +840,6 @@ test_expect_success 'explicitly empty --refmap option disables remote.*.fetch' ' test_expect_success 'configured fetch updates tracking' ' - cd "$D" && git branch -f side && ( cd three && @@ -884,7 +853,6 @@ test_expect_success 'configured fetch updates tracking' ' ' test_expect_success 'non-matching refspecs do not confuse tracking update' ' - cd "$D" && git update-ref refs/odd/location HEAD && ( cd three && @@ -901,14 +869,12 @@ test_expect_success 'non-matching refspecs do not confuse tracking update' ' test_expect_success 'pushing nonexistent branch by mistake should not segv' ' - cd "$D" && test_must_fail git push seven no:no ' test_expect_success 'auto tag following fetches minimum' ' - cd "$D" && git clone .git follow && git checkout HEAD^0 && ( @@ -1307,7 +1273,7 @@ test_expect_success 'fetch --prune prints the remotes url' ' cd only-prunes && git fetch --prune origin 2>&1 | head -n1 >../actual ) && - echo "From ${D}/." >expect && + echo "From $(pwd)/." >expect && test_cmp expect actual ' @@ -1357,14 +1323,14 @@ test_expect_success 'fetching with auto-gc does not lock up' ' echo "$*" && false EOF - git clone "file://$D" auto-gc && + git clone "file://$PWD" auto-gc && test_commit test2 && ( cd auto-gc && git config fetch.unpackLimit 1 && git config gc.autoPackLimit 1 && git config gc.autoDetach false && - GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 && + GIT_ASK_YESNO="$TRASH_DIRECTORY/askyesno" git fetch --verbose >fetch.out 2>&1 && test_grep "Auto packing the repository" fetch.out && ! grep "Should I try again" fetch.out ) From f1c2a42eacd272f7aa28ea8d017ae84547ee9ab1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Aug 2025 15:27:16 -0400 Subject: [PATCH 15/21] t5510: prefer "git -C" to subshell for followRemoteHEAD tests These tests set config within a sub-repo using (cd two && git config), and then a separate test_when_finished outside the subshell to clean it up. We can't use test_config to do this, because the cleanup command it registers inside the subshell would be lost. Nor can we do it before entering the subshell, because the config has to be set after some other commands are run. Let's switch these tests to use "git -C" for each command instead of a subshell. That lets us use test_config (with -C also) at the appropriate part of the test. And we no longer need the manual cleanup command. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5510-fetch.sh | 202 +++++++++++++++++++---------------------------- 1 file changed, 83 insertions(+), 119 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 93e309e2130b17..24379ec7aa9ec7 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -123,149 +123,113 @@ test_expect_success "fetch test remote HEAD change" ' ' test_expect_success "fetch test followRemoteHEAD never" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git update-ref --no-deref -d refs/remotes/origin/HEAD && - git config set remote.origin.followRemoteHEAD "never" && - GIT_TRACE_PACKET=$PWD/trace.out git fetch && - # Confirm that we do not even ask for HEAD when we are - # not going to act on it. - test_grep ! "ref-prefix HEAD" trace.out && - test_must_fail git rev-parse --verify refs/remotes/origin/HEAD - ) + git -C two update-ref --no-deref -d refs/remotes/origin/HEAD && + test_config -C two remote.origin.followRemoteHEAD "never" && + GIT_TRACE_PACKET=$PWD/trace.out git -C two fetch && + # Confirm that we do not even ask for HEAD when we are + # not going to act on it. + test_grep ! "ref-prefix HEAD" trace.out && + test_must_fail git -C two rev-parse --verify refs/remotes/origin/HEAD ' test_expect_success "fetch test followRemoteHEAD warn no change" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git config set remote.origin.followRemoteHEAD "warn" && - git fetch >output && - echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ - "but we have ${SQ}other${SQ} locally." >expect && - test_cmp expect output && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/other) && - test "z$head" = "z$branch" - ) + git -C two rev-parse --verify refs/remotes/origin/other && + git -C two remote set-head origin other && + git -C two rev-parse --verify refs/remotes/origin/HEAD && + git -C two rev-parse --verify refs/remotes/origin/main && + test_config -C two remote.origin.followRemoteHEAD "warn" && + git -C two fetch >output && + echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ + "but we have ${SQ}other${SQ} locally." >expect && + test_cmp expect output && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" ' test_expect_success "fetch test followRemoteHEAD warn create" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git update-ref --no-deref -d refs/remotes/origin/HEAD && - git config set remote.origin.followRemoteHEAD "warn" && - git rev-parse --verify refs/remotes/origin/main && - output=$(git fetch) && - test "z" = "z$output" && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/main) && - test "z$head" = "z$branch" - ) + git -C two update-ref --no-deref -d refs/remotes/origin/HEAD && + test_config -C two remote.origin.followRemoteHEAD "warn" && + git -C two rev-parse --verify refs/remotes/origin/main && + output=$(git -C two fetch) && + test "z" = "z$output" && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/main) && + test "z$head" = "z$branch" ' test_expect_success "fetch test followRemoteHEAD warn detached" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git update-ref --no-deref -d refs/remotes/origin/HEAD && - git update-ref refs/remotes/origin/HEAD HEAD && - HEAD=$(git log --pretty="%H") && - git config set remote.origin.followRemoteHEAD "warn" && - git fetch >output && - echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ - "but we have a detached HEAD pointing to" \ - "${SQ}${HEAD}${SQ} locally." >expect && - test_cmp expect output - ) + git -C two update-ref --no-deref -d refs/remotes/origin/HEAD && + git -C two update-ref refs/remotes/origin/HEAD HEAD && + HEAD=$(git -C two log --pretty="%H") && + test_config -C two remote.origin.followRemoteHEAD "warn" && + git -C two fetch >output && + echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ + "but we have a detached HEAD pointing to" \ + "${SQ}${HEAD}${SQ} locally." >expect && + test_cmp expect output ' test_expect_success "fetch test followRemoteHEAD warn quiet" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git config set remote.origin.followRemoteHEAD "warn" && - output=$(git fetch --quiet) && - test "z" = "z$output" && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/other) && - test "z$head" = "z$branch" - ) + git -C two rev-parse --verify refs/remotes/origin/other && + git -C two remote set-head origin other && + git -C two rev-parse --verify refs/remotes/origin/HEAD && + git -C two rev-parse --verify refs/remotes/origin/main && + test_config -C two remote.origin.followRemoteHEAD "warn" && + output=$(git -C two fetch --quiet) && + test "z" = "z$output" && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git config set remote.origin.followRemoteHEAD "warn-if-not-main" && - actual=$(git fetch) && - test "z" = "z$actual" && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/other) && - test "z$head" = "z$branch" - ) + git -C two rev-parse --verify refs/remotes/origin/other && + git -C two remote set-head origin other && + git -C two rev-parse --verify refs/remotes/origin/HEAD && + git -C two rev-parse --verify refs/remotes/origin/main && + test_config -C two remote.origin.followRemoteHEAD "warn-if-not-main" && + actual=$(git -C two fetch) && + test "z" = "z$actual" && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" ' test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git config set remote.origin.followRemoteHEAD "warn-if-not-some/different-branch" && - git fetch >actual && - echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ - "but we have ${SQ}other${SQ} locally." >expect && - test_cmp expect actual && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/other) && - test "z$head" = "z$branch" - ) + git -C two rev-parse --verify refs/remotes/origin/other && + git -C two remote set-head origin other && + git -C two rev-parse --verify refs/remotes/origin/HEAD && + git -C two rev-parse --verify refs/remotes/origin/main && + test_config -C two remote.origin.followRemoteHEAD "warn-if-not-some/different-branch" && + git -C two fetch >actual && + echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ + "but we have ${SQ}other${SQ} locally." >expect && + test_cmp expect actual && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" ' test_expect_success "fetch test followRemoteHEAD always" ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git rev-parse --verify refs/remotes/origin/other && - git remote set-head origin other && - git rev-parse --verify refs/remotes/origin/HEAD && - git rev-parse --verify refs/remotes/origin/main && - git config set remote.origin.followRemoteHEAD "always" && - git fetch && - head=$(git rev-parse refs/remotes/origin/HEAD) && - branch=$(git rev-parse refs/remotes/origin/main) && - test "z$head" = "z$branch" - ) + git -C two rev-parse --verify refs/remotes/origin/other && + git -C two remote set-head origin other && + git -C two rev-parse --verify refs/remotes/origin/HEAD && + git -C two rev-parse --verify refs/remotes/origin/main && + test_config -C two remote.origin.followRemoteHEAD "always" && + git -C two fetch && + head=$(git -C two rev-parse refs/remotes/origin/HEAD) && + branch=$(git -C two rev-parse refs/remotes/origin/main) && + test "z$head" = "z$branch" ' test_expect_success 'followRemoteHEAD does not kick in with refspecs' ' - test_when_finished "git -C two config unset remote.origin.followRemoteHEAD" && - ( - cd two && - git remote set-head origin other && - git config set remote.origin.followRemoteHEAD always && - git fetch origin refs/heads/main:refs/remotes/origin/main && - echo refs/remotes/origin/other >expect && - git symbolic-ref refs/remotes/origin/HEAD >actual && - test_cmp expect actual - ) + git -C two remote set-head origin other && + test_config -C two remote.origin.followRemoteHEAD always && + git -C two fetch origin refs/heads/main:refs/remotes/origin/main && + echo refs/remotes/origin/other >expect && + git -C two symbolic-ref refs/remotes/origin/HEAD >actual && + test_cmp expect actual ' test_expect_success 'fetch --prune on its own works as expected' ' From 450fc2bace48ce7ba07a2431175923bf2d610635 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Aug 2025 15:29:34 -0400 Subject: [PATCH 16/21] refs: do not clobber dangling symrefs When given an expected "before" state, the ref-writing code will avoid overwriting any ref that does not match that expected state. We use the null oid as a sentinel value for "nothing should exist", and likewise that is the sentinel value we get when trying to read a ref that does not exist. But there's one corner case where this is ambiguous: dangling symrefs. Trying to read them will yield the null oid, but there is potentially something of value there: the dangling symref itself. For a normal recursive write, this is OK. Imagine we have a symref "FOO_HEAD" that points to a ref "refs/heads/bar" that does not exist, and we try to write to it with a create operation like: oid=$(git rev-parse HEAD) ;# or whatever git symbolic-ref FOO_HEAD refs/heads/bar echo "create FOO_HEAD $oid" | git update-ref --stdin The attempt to resolve FOO_HEAD will actually resolve "bar", yielding the null oid. That matches our expectation, and the write proceeds. This is correct, because we are not writing FOO_HEAD at all, but writing its destination "bar", which in fact does not exist. But what if the operation asked not to dereference symrefs? Like this: echo "create FOO_HEAD $oid" | git update-ref --no-deref --stdin Resolving FOO_HEAD would still result in a null oid, and the write will proceed. But it will overwrite FOO_HEAD itself, removing the fact that it ever pointed to "bar". This case is a little esoteric; we are clobbering a symref with a no-deref write of a regular ref value. But the same problem occurs when writing symrefs. For example: echo "symref-create FOO_HEAD refs/heads/other" | git update-ref --no-deref --stdin The "create" operation asked us to create FOO_HEAD only if it did not exist. But we silently overwrite the existing value. You can trigger this without using update-ref via the fetch followRemoteHEAD code. In "create" mode, it should not overwrite an existing value. But if you manually create a symref pointing to a value that does not yet exist (either via symbolic-ref or with "remote add -m"), create mode will happily overwrite it. Instead, we should detect this case and refuse to write. The correct specification to overwrite FOO_HEAD in this case is to provide an expected target ref value, like: echo "symref-update FOO_HEAD refs/heads/other ref refs/heads/bar" | git update-ref --no-deref --stdin Note that the non-symref "update" directive does not allow you to do this (you can only specify an oid). This is a weakness in the update-ref interface, and you'd have to overwrite unconditionally, like: echo "update FOO_HEAD $oid" | git update-ref --no-deref --stdin Likewise other symref operations like symref-delete do not accept the "ref" keyword. You should be able to do: echo "symref-delete FOO_HEAD ref refs/heads/bar" but cannot (and can only delete unconditionally). This patch doesn't address those gaps. We may want to do so in a future patch for completeness, but it's not clear if anybody actually wants to perform those operations. The symref update case (specifically, via followRemoteHEAD) is what I ran into in the wild. The code for the fix is relatively straight-forward given the discussion above. But note that we have to implement it independently for the files and reftable backends. The "old oid" checks happen as part of the locking process, which is implemented separately for each system. We may want to factor this out somehow, but it's beyond the scope of this patch. (Another curiosity is that the messages in the reftable code are marked for translation, but the ones in the files backend are not. I followed local convention in each case, but we may want to harmonize this at some point). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 34 ++++++++++++++++++++++++++++++---- refs/reftable-backend.c | 30 +++++++++++++++++++++++++++--- t/t1400-update-ref.sh | 21 +++++++++++++++++++++ t/t5510-fetch.sh | 9 +++++++++ 4 files changed, 87 insertions(+), 7 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 905555365b8ff0..a4419ef62d8db2 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update, */ static enum ref_transaction_error check_old_oid(struct ref_update *update, struct object_id *oid, + struct strbuf *referent, struct strbuf *err) { if (update->flags & REF_LOG_ONLY || - !(update->flags & REF_HAVE_OLD) || - oideq(oid, &update->old_oid)) + !(update->flags & REF_HAVE_OLD)) return 0; + if (oideq(oid, &update->old_oid)) { + /* + * Normally matching the expected old oid is enough. Either we + * found the ref at the expected state, or we are creating and + * expect the null oid (and likewise found nothing). + * + * But there is one exception for the null oid: if we found a + * symref pointing to nothing we'll also get the null oid. In + * regular recursive mode, that's good (we'll write to what the + * symref points to, which doesn't exist). But in no-deref + * mode, it means we'll clobber the symref, even though the + * caller asked for this to be a creation event. So flag + * that case to preserve the dangling symref. + */ + if ((update->flags & REF_NO_DEREF) && referent->len && + is_null_oid(oid)) { + strbuf_addf(err, "cannot lock ref '%s': " + "dangling symref already exists", + ref_update_original_update_refname(update)); + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } + return 0; + } + if (is_null_oid(&update->old_oid)) { strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", @@ -2658,7 +2682,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re if (update->old_target) ret = ref_update_check_old_target(referent.buf, update, err); else - ret = check_old_oid(update, &lock->old_oid, err); + ret = check_old_oid(update, &lock->old_oid, + &referent, err); if (ret) goto out; } else { @@ -2690,7 +2715,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re ret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF; goto out; } else { - ret = check_old_oid(update, &lock->old_oid, err); + ret = check_old_oid(update, &lock->old_oid, + &referent, err); if (ret) { goto out; } diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 99fafd75ebe8ff..ef98584bf98978 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor ret = ref_update_check_old_target(referent->buf, u, err); if (ret) return ret; - } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD && - !oideq(¤t_oid, &u->old_oid)) { - if (is_null_oid(&u->old_oid)) { + } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) { + if (oideq(¤t_oid, &u->old_oid)) { + /* + * Normally matching the expected old oid is enough. Either we + * found the ref at the expected state, or we are creating and + * expect the null oid (and likewise found nothing). + * + * But there is one exception for the null oid: if we found a + * symref pointing to nothing we'll also get the null oid. In + * regular recursive mode, that's good (we'll write to what the + * symref points to, which doesn't exist). But in no-deref + * mode, it means we'll clobber the symref, even though the + * caller asked for this to be a creation event. So flag + * that case to preserve the dangling symref. + * + * Everything else is OK and we can fall through to the + * end of the conditional chain. + */ + if ((u->flags & REF_NO_DEREF) && + referent->len && + is_null_oid(&u->old_oid)) { + strbuf_addf(err, _("cannot lock ref '%s': " + "dangling symref already exists"), + ref_update_original_update_refname(u)); + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } + } else if (is_null_oid(&u->old_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference already exists"), ref_update_original_update_refname(u)); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index d29d23cb8905f8..29b31e3b9bda80 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' ' test_cmp expect actual ' +test_expect_success 'dangling symref not overwritten by creation' ' + test_when_finished "git update-ref -d refs/heads/dangling" && + git symbolic-ref refs/heads/dangling refs/heads/does-not-exist && + test_must_fail git update-ref --no-deref --stdin 2>err <<-\EOF && + create refs/heads/dangling HEAD + EOF + test_grep "cannot lock.*dangling symref already exists" err && + test_must_fail git rev-parse --verify refs/heads/dangling && + test_must_fail git rev-parse --verify refs/heads/does-not-exist +' + +test_expect_success 'dangling symref overwritten without old oid' ' + test_when_finished "git update-ref -d refs/heads/dangling" && + git symbolic-ref refs/heads/dangling refs/heads/does-not-exist && + git update-ref --no-deref --stdin <<-\EOF && + update refs/heads/dangling HEAD + EOF + git rev-parse --verify refs/heads/dangling && + test_must_fail git rev-parse --verify refs/heads/does-not-exist +' + test_done diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 24379ec7aa9ec7..83d1aadf9f50ed 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -232,6 +232,15 @@ test_expect_success 'followRemoteHEAD does not kick in with refspecs' ' test_cmp expect actual ' +test_expect_success 'followRemoteHEAD create does not overwrite dangling symref' ' + git -C two remote add -m does-not-exist custom-head ../one && + test_config -C two remote.custom-head.followRemoteHEAD create && + git -C two fetch custom-head && + echo refs/remotes/custom-head/does-not-exist >expect && + git -C two symbolic-ref refs/remotes/custom-head/HEAD >actual && + test_cmp expect actual +' + test_expect_success 'fetch --prune on its own works as expected' ' git clone . prune && ( From bcb20dda833dc71a729af50bdfb86c5d252307b1 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Wed, 20 Aug 2025 08:16:05 +0200 Subject: [PATCH 17/21] doc/gitk: update reference to the external project Gitk is now maintained by Johannes Sixt and the repository can be cloned from a new URL. b59358100c20 (Update the official repo of gitk, 2024-12-24) could have updated this instance in the manual, too, but the opportunity was missed. Update it now. Do give credit to Paul Mackerras as the inventor of the program. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- Documentation/gitk.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt index 35b39960296b69..d90396904eecda 100644 --- a/Documentation/gitk.txt +++ b/Documentation/gitk.txt @@ -163,16 +163,16 @@ used by default. If '$XDG_CONFIG_HOME' is not set it defaults to History ------- -Gitk was the first graphical repository browser. It's written in -tcl/tk. +Gitk was the first graphical repository browser, written by +Paul Mackerras in Tcl/Tk. 'gitk' is actually maintained as an independent project, but stable versions are distributed as part of the Git suite for the convenience of end users. -gitk-git/ comes from Paul Mackerras's gitk project: +`gitk-git/` comes from Johannes Sixt's gitk project: - git://ozlabs.org/~paulus/gitk + https://github.com/j6t/gitk SEE ALSO -------- From 8cfd4ac215e3711757acef8043c1c3bf3689f606 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 20 Aug 2025 02:30:34 -0400 Subject: [PATCH 18/21] describe: handle blob traversal with no commits When describing a blob, we traverse from HEAD, remembering each commit we saw, and then checking each blob to report the containing commit. But if we haven't seen any commits at all, we'll segfault (we store the "current" commit as an oid initialized to the null oid, causing lookup_commit_reference() to return NULL). This shouldn't be able to happen normally. We always start our traversal at HEAD, which must be a commit (a property which is enforced by the refs code). But you can trigger the segfault like this: blob=$(echo foo | git hash-object -w --stdin) echo $blob >.git/HEAD git describe $blob We can instead catch this case and return an empty result, which hits the usual "we didn't find $blob while traversing HEAD" error. This is a minor lie in that we did "find" the blob. And this even hints at a bigger problem in this code: what if the traversal pointed to the blob as _not_ part of a commit at all, but we had previously filled in the recorded "current commit"? One could imagine this happening due to a tag pointing directly to the blob in question. But that can't happen, because we only traverse from HEAD, never from any other refs. And the intent of the blob-describing code is to find blobs within commits. So I think this matches the original intent as closely as we can (and again, this segfault cannot be triggered without corrupting your repository!). The test here does not use the formula above, which works only for the files backend (and not reftables). Instead we use another loophole to create the bogus state using only Git commands. See the comment in the test for details. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 6 ++++-- t/t6120-describe.sh | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index f7bea3c8c5ebab..72b2e1162c71f2 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -507,8 +507,10 @@ static void process_object(struct object *obj, const char *path, void *data) if (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) { reset_revision_walk(); - describe_commit(&pcd->current_commit, pcd->dst); - strbuf_addf(pcd->dst, ":%s", path); + if (!is_null_oid(&pcd->current_commit)) { + describe_commit(&pcd->current_commit, pcd->dst); + strbuf_addf(pcd->dst, ":%s", path); + } free_commit_list(pcd->revs->commits); pcd->revs->commits = NULL; } diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index feec57bcbc577a..2c70cc561ad5f6 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -423,6 +423,22 @@ test_expect_success 'describe blob on an unborn branch' ' test_grep "cannot search .* on an unborn branch" actual ' +# This test creates a repository state that we generally try to disallow: HEAD +# is pointing to an object that is not a commit. The ref update code forbids +# non-commit writes directly to HEAD or to any branch in refs/heads/. But we +# can use the loophole of pointing HEAD to another non-branch ref (something we +# should forbid, but don't for historical reasons). +# +# Do not take this test as an endorsement of the loophole! If we ever tighten +# it, it is reasonable to just drop this test entirely. +test_expect_success 'describe blob on a non-commit HEAD' ' + oldbranch=$(git symbolic-ref HEAD) && + test_when_finished "git symbolic-ref HEAD $oldbranch" && + git symbolic-ref HEAD refs/tags/test-blob && + test_must_fail git describe test-blob 2>actual && + test_grep "blob .* not reachable from HEAD" actual +' + test_expect_success ULIMIT_STACK_SIZE 'name-rev works in a deep repo' ' i=1 && while test $i -lt 8000 From 7c10e48e81ae63974e3badf3b7df71df74a0640b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Aug 2025 17:04:17 -0400 Subject: [PATCH 19/21] describe: pass commit to describe_commit() There's a call in describe_commit() to lookup_commit_reference(), but we don't check the return value. If it returns NULL, we'll segfault as we immediately dereference the result. In practice this can never happen, since all callers pass an oid which came from a "struct commit" already. So we can make this more obvious by just taking that commit struct in the first place. Reported-by: Cheng Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 72b2e1162c71f2..04df89d56b91e7 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -313,9 +313,9 @@ static void append_suffix(int depth, const struct object_id *oid, struct strbuf repo_find_unique_abbrev(the_repository, oid, abbrev)); } -static void describe_commit(struct object_id *oid, struct strbuf *dst) +static void describe_commit(struct commit *cmit, struct strbuf *dst) { - struct commit *cmit, *gave_up_on = NULL; + struct commit *gave_up_on = NULL; struct commit_list *list; struct commit_name *n; struct possible_tag all_matches[MAX_TAGS]; @@ -323,8 +323,6 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) unsigned long seen_commits = 0; unsigned int unannotated_cnt = 0; - cmit = lookup_commit_reference(the_repository, oid); - n = find_commit_name(&cmit->object.oid); if (n && (tags || all || n->prio == 2)) { /* @@ -332,7 +330,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) */ append_name(n, dst); if (n->misnamed || longformat) - append_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst); + append_suffix(0, n->tag ? get_tagged_oid(n->tag) : &cmit->object.oid, dst); if (suffix) strbuf_addstr(dst, suffix); return; @@ -489,7 +487,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) } struct process_commit_data { - struct object_id current_commit; + struct commit *current_commit; const struct object_id *looking_for; struct strbuf *dst; struct rev_info *revs; @@ -498,7 +496,7 @@ struct process_commit_data { static void process_commit(struct commit *commit, void *data) { struct process_commit_data *pcd = data; - pcd->current_commit = commit->object.oid; + pcd->current_commit = commit; } static void process_object(struct object *obj, const char *path, void *data) @@ -507,8 +505,8 @@ static void process_object(struct object *obj, const char *path, void *data) if (oideq(pcd->looking_for, &obj->oid) && !pcd->dst->len) { reset_revision_walk(); - if (!is_null_oid(&pcd->current_commit)) { - describe_commit(&pcd->current_commit, pcd->dst); + if (pcd->current_commit) { + describe_commit(pcd->current_commit, pcd->dst); strbuf_addf(pcd->dst, ":%s", path); } free_commit_list(pcd->revs->commits); @@ -521,7 +519,7 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst) struct rev_info revs; struct strvec args = STRVEC_INIT; struct object_id head_oid; - struct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs}; + struct process_commit_data pcd = { NULL, oid, dst, &revs}; if (repo_get_oid(the_repository, "HEAD", &head_oid)) die(_("cannot search for blob '%s' on an unborn branch"), @@ -562,7 +560,7 @@ static void describe(const char *arg, int last_one) cmit = lookup_commit_reference_gently(the_repository, &oid, 1); if (cmit) - describe_commit(&oid, &sb); + describe_commit(cmit, &sb); else if (odb_read_object_info(the_repository->objects, &oid, NULL) == OBJ_BLOB) describe_blob(&oid, &sb); From 716d342c53715500ae0ce032e6cfd65806639691 Mon Sep 17 00:00:00 2001 From: Daniele Sassoli Date: Wed, 20 Aug 2025 08:36:58 +0000 Subject: [PATCH 20/21] doc: add discord to ways of getting help Discord is a great way of receiving help for members of the community that are not on the mailing list or not familiar with Libera. Adding it to the official documentation will aid discoverability of it. The link is the same as the one at https://git-scm.com/community. Signed-off-by: Daniele Sassoli Signed-off-by: Junio C Hamano --- Documentation/MyFirstContribution.adoc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc index aca7212cfe2a42..d786176bba2234 100644 --- a/Documentation/MyFirstContribution.adoc +++ b/Documentation/MyFirstContribution.adoc @@ -52,6 +52,15 @@ respond to you. It's better to ask your questions in the channel so that you can be answered if you disconnect and so that others can learn from the conversation. +==== https://discord.gg/GRFVkzgxRd[#discord] on Discord +This is an unofficial Git Discord server for everyone, from people just +starting out with Git to those who develop it. It's a great place to ask +questions, share tips, and connect with the broader Git community in real time. + +The server has channels for general discussions and specific channels for those +who use Git and those who develop it. The server's search functionality also +allows you to find previous conversations and answers to common questions. + [[getting-started]] == Getting Started From 6ad802182101d622e6a4132f48292ddfa79e2024 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 29 Aug 2025 09:43:39 -0700 Subject: [PATCH 21/21] The fifth batch Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.52.0.adoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Documentation/RelNotes/2.52.0.adoc b/Documentation/RelNotes/2.52.0.adoc index 3625db515ef963..4e8dbd0fc2b3b6 100644 --- a/Documentation/RelNotes/2.52.0.adoc +++ b/Documentation/RelNotes/2.52.0.adoc @@ -37,6 +37,9 @@ Performance, Internal Implementation, Development Support etc. * Remove dependency on the_repository and other globals from the commit-graph code, and other changes unrelated to de-globaling. + * Discord has been added to the first contribution documentation as + another way to ask for help. + Fixes since v2.51 ----------------- @@ -110,6 +113,18 @@ including security updates, are included in this release. been corrected. (merge 8f32a5a6c0 jk/fetch-check-graph-objects-fix later to maint). + * "git fetch" can clobber a symref that is dangling when the + remote-tracking HEAD is set to auto update, which has been + corrected. + + * "git describe " misbehaves and/or crashes in some corner + cases, which has been taught to exit with failure gracefully. + (merge 7c10e48e81 jk/describe-blob later to maint). + + * Manual page for "gitk" is updated with the current maintainer's + name. + (merge bcb20dda83 js/doc-gitk-history later to maint). + * Other code cleanup, docfix, build fix, etc. (merge 823d537fa7 kh/doc-git-log-markup-fix later to maint). (merge cf7efa4f33 rj/t6137-cygwin-fix later to maint). @@ -118,3 +133,4 @@ including security updates, are included in this release. (merge 741f36c7d9 kr/clone-synopsis-fix later to maint). (merge a60203a015 dk/t7005-editor-updates later to maint). (merge 7d4a5fef7d ds/doc-count-objects-fix later to maint). + (merge 16684b6fae ps/reftable-libgit2-cleanup later to maint).