From 1ce26936d17dd8a95365b1194568def378753471 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 5 Oct 2026 23:52:39 -0400 Subject: [PATCH 01/37] test-lib: allow lazy prerequisite snippets as here-docs Commit 1d133ae91f (test-lib: allow test snippets as here-docs, 2024-07-10) let test_expect_success and test_expect_failure read their snippets from stdin, making it easier to use single quotes within them. I mentioned there that we could extend this to lazy prerequisites, but left it for later. Let's finish that off now. Since test_body_or_stdin() takes the name of the variable to fill, we can use it directly to populate the saved prereq snippet. We read the body when the prereq is declared, but still evaluate it only when the prereq is used. Converting the curl version check in t5551 shows how this can reduce awkward quoting. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5551-http-fetch-smart.sh | 8 ++++---- t/test-lib-functions.sh | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 1ec971cdbafdb4..130053c57aa141 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -21,14 +21,14 @@ start_httpd # authentication after an early HTTP/2 response. This bug was introduced # in cURL v7.88.0 (8c762f5998 (http2: minor buffer and error path fixes, # 2023-02-08)) and fixed in v8.3.0 (https://github.com/curl/curl/pull/11756). -test_lazy_prereq HAVE_CURL_HTTP2_BUG ' +test_lazy_prereq HAVE_CURL_HTTP2_BUG - <<\EOT test_have_prereq HTTP2 && build_option libcurl | - awk -F. '\'' + awk -F. ' ($1 == 7 && $2 >= 88) || ($1 == 8 && $2 < 3) { broken = 1 } END { exit !broken } - '\'' -' + ' +EOT test_expect_success HTTP2 'enable client-side http/2' ' git config --global http.version HTTP/2 diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index ed9779b0f73310..3056e937e3e957 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -760,7 +760,7 @@ lazily_testable_prereq= lazily_tested_prereq= # Usage: test_lazy_prereq PREREQ 'script' test_lazy_prereq () { lazily_testable_prereq="$lazily_testable_prereq$1 " - eval test_prereq_lazily_$1=\$2 + test_body_or_stdin "test_prereq_lazily_$1" "$2" } test_run_lazy_prereq_ () { From e4bf62f7f7015ee73bef6932d2fae7af3ffac401 Mon Sep 17 00:00:00 2001 From: Thomas Bachem Date: Tue, 13 Oct 2026 12:07:52 +0000 Subject: [PATCH 02/37] rerere: wait for MERGE_RR.lock before giving up setup_rerere() takes MERGE_RR.lock with LOCK_DIE_ON_ERROR, so when two processes want it at the same time, the second one dies. That used to be rare. Since 452b12c2e0 (builtin/maintenance: use "geometric" strategy by default, 2026-02-24) the auto maintenance after a commit runs "git rerere gc" whenever rr-cache has enough stale entries, and the gc holds the lock while it prunes. If a rebase's next pick conflicts while the gc holds the lock, the rebase dies inside repo_rerere(), before the sequencer has written what "git rebase --continue" needs. Every later "git rebase --continue" then fails with "you have staged changes in your working tree". Wait for the lock for up to rerere.lockTimeout milliseconds, 1000 by default, and only then fail as before. Pruning a few thousand entries takes well under a second, so the default covers a rebase that runs into the gc. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem Signed-off-by: Junio C Hamano --- Documentation/config/rerere.adoc | 7 +++++ rerere.c | 22 ++++++++++--- t/t4200-rerere.sh | 53 ++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc index 3a78b5ebb1dc02..ab035e2d7dc13f 100644 --- a/Documentation/config/rerere.adoc +++ b/Documentation/config/rerere.adoc @@ -10,3 +10,10 @@ rerere.enabled:: enabled if there is an `rr-cache` directory under the `$GIT_DIR`, e.g. if "rerere" was previously used in the repository. + +rerere.lockTimeout:: + The length of time, in milliseconds, to wait for the rerere + lock when another process holds it, typically a background + `git rerere gc`. Value 0 means not to wait at all; -1 means + to wait indefinitely. Default is 1000 (i.e., wait for 1 + second). diff --git a/rerere.c b/rerere.c index 1c3745d9e3279a..64fac07c71d9d8 100644 --- a/rerere.c +++ b/rerere.c @@ -33,6 +33,9 @@ static int rerere_enabled = -1; /* automatically update cleanly resolved paths to the index */ static int rerere_autoupdate; +/* how long to wait for MERGE_RR.lock, in milliseconds */ +static int rerere_lock_timeout_ms = 1000; + #define RR_HAS_POSTIMAGE 1 #define RR_HAS_PREIMAGE 2 struct rerere_dir { @@ -850,6 +853,8 @@ static void git_rerere_config(void) { repo_config_get_bool(the_repository, "rerere.enabled", &rerere_enabled); repo_config_get_bool(the_repository, "rerere.autoupdate", &rerere_autoupdate); + repo_config_get_int(the_repository, "rerere.locktimeout", + &rerere_lock_timeout_ms); repo_config(the_repository, git_default_config, NULL); } @@ -882,12 +887,19 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE); - if (flags & RERERE_READONLY) + if (flags & RERERE_READONLY) { fd = 0; - else - fd = repo_hold_lock_file_for_update(r, &write_lock, - git_path_merge_rr(r), - LOCK_DIE_ON_ERROR); + } else { + /* + * Another process may hold the lock for a while, e.g. + * "git rerere gc" while it prunes rr-cache, so wait for + * it instead of dying right away. + */ + fd = repo_hold_lock_file_for_update_timeout(r, &write_lock, + git_path_merge_rr(r), + LOCK_DIE_ON_ERROR, + rerere_lock_timeout_ms); + } read_rr(r, merge_rr); return fd; } diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh index 7bb601e117ec35..94165c8417ffc4 100755 --- a/t/t4200-rerere.sh +++ b/t/t4200-rerere.sh @@ -242,6 +242,59 @@ test_expect_success 'old records rest in peace' ' test_path_is_missing $rr2/preimage ' +test_expect_success 'a held lock is waited out within rerere.lockTimeout' ' + git reset --hard && + rm -rf $rr && + test_when_finished "wait && rm -f .git/MERGE_RR.lock" && + >.git/MERGE_RR.lock && + { + ( sleep 1 && rm -f .git/MERGE_RR.lock ) & + } && + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err && + test_grep ! "Unable to create.*MERGE_RR\.lock" err && + test_grep "^=======\$" $rr/preimage +' + +test_expect_success 'merge fails on a lock it cannot take' ' + git reset --hard && + rm -rf $rr && + test_when_finished "rm -f .git/MERGE_RR.lock" && + >.git/MERGE_RR.lock && + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err && + test_grep "^=======\$" a1 && + test_path_is_missing $rr/preimage +' + +test_expect_success 'rerere, forget, clear and gc fail on a lock they cannot take' ' + test_when_finished "rm -f .git/MERGE_RR.lock" && + >.git/MERGE_RR.lock && + test_must_fail git -c rerere.lockTimeout=0 rerere 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err && + test_must_fail git -c rerere.lockTimeout=0 rerere forget a1 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err && + test_must_fail git -c rerere.lockTimeout=0 rerere clear 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err && + test_must_fail git -c rerere.lockTimeout=0 rerere gc 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err +' + +test_expect_success 'rebase --abort fails on a lock it cannot take' ' + git reset --hard && + git checkout -b lock-held-abort third && + test_when_finished "git checkout third && git branch -D lock-held-abort" && + test_must_fail git rebase first && + test_when_finished "test_might_fail git rebase --abort" && + test_when_finished "rm -f .git/MERGE_RR.lock" && + >.git/MERGE_RR.lock && + test_must_fail git -c rerere.lockTimeout=0 rebase --abort 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err && + test_path_is_dir .git/rebase-merge && + rm .git/MERGE_RR.lock && + git rebase --abort && + test_path_is_missing .git/rebase-merge +' + rerere_gc_custom_expiry_test () { five_days="$1" right_now="$2" test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" ' From 5f6b6aa3ab4ab9f68271055da51da6746cba707e Mon Sep 17 00:00:00 2001 From: Thomas Bachem Date: Tue, 13 Oct 2026 12:07:53 +0000 Subject: [PATCH 03/37] rerere: add "gc --skip-locked" for auto maintenance Starting with the preceding commit, processes that want to acquire the rerere cache's MERGE_RR.lock by default know to wait up to one second until that lock has been released. This is a sensible default for many commands that happen to write rerere entries, as we would otherwise die immediately when the lock is taken by another process. But for repository maintenance it's a bit more complicated, as there are two cases that we have to care about. When the user explicitly asks us to garbage collect rerere entries via `git rerere gc` they probably want us to try our best to perform this operation. It's thus sensible to wait for the lock and then die if we weren't able to acquire it. But we also prune rerere entries as part of auto-maintenance, which is only executed on a best-effort basis anyway. Delaying the whole operation to acquire the lock is somewhat heavy-handed, and neither does it make sense to die in case we haven't been able to garbage collect rerere entries as that would impede other housekeeping tasks. Furthermore, it's totally fine to skip the operation when the rerere cache is locked already, as we will retry during the next run anyway. But we do not have an easy way to tell `git rerere gc` to skip the operation in case the cache is locked already. Add a new "--skip-locked" flag to plug that gap and have auto-maintenance pass that flag. Only auto-maintenance needs that flag, so hide it, like the "--skip-foreground-tasks" flag that `git maintenance run` passes to `git gc`. Helped-by: Patrick Steinhardt Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem Signed-off-by: Junio C Hamano --- Documentation/config/rerere.adoc | 4 +++- builtin/gc.c | 4 +++- builtin/rerere.c | 10 ++++++++-- rerere.c | 23 +++++++++++++++++------ rerere.h | 4 +++- t/t4200-rerere.sh | 26 ++++++++++++++++++++++++++ t/t7900-maintenance.sh | 26 +++++++++++++++++++++++++- 7 files changed, 85 insertions(+), 12 deletions(-) diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc index ab035e2d7dc13f..60e78e499fbd4d 100644 --- a/Documentation/config/rerere.adoc +++ b/Documentation/config/rerere.adoc @@ -16,4 +16,6 @@ rerere.lockTimeout:: lock when another process holds it, typically a background `git rerere gc`. Value 0 means not to wait at all; -1 means to wait indefinitely. Default is 1000 (i.e., wait for 1 - second). + second). A `git rerere gc` run by `git maintenance run --auto` + or `git gc --auto` does not wait and does nothing while the + lock is held. diff --git a/builtin/gc.c b/builtin/gc.c index 57a3520263d7be..7ad3987b717ff5 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -385,12 +385,14 @@ static int worktree_prune_condition(struct gc_config *cfg) return should_prune; } -static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts, struct gc_config *cfg UNUSED) { struct child_process rerere_cmd = CHILD_PROCESS_INIT; rerere_cmd.git_cmd = 1; strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); + if (opts->auto_flag) + strvec_push(&rerere_cmd.args, "--skip-locked"); return run_command(&rerere_cmd); } diff --git a/builtin/rerere.c b/builtin/rerere.c index a056cb791b4c80..445f1df03211ce 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c @@ -56,16 +56,21 @@ int cmd_rerere(int argc, struct repository *repo UNUSED) { struct string_list merge_rr = STRING_LIST_INIT_DUP; - int autoupdate = -1, flags = 0; + int autoupdate = -1, skip_locked = 0, flags = 0; struct option options[] = { OPT_SET_INT(0, "rerere-autoupdate", &autoupdate, N_("register clean resolutions in index"), 1), + OPT_HIDDEN_BOOL(0, "skip-locked", &skip_locked, + N_("skip gc while another process holds the lock")), OPT_END(), }; argc = parse_options(argc, argv, prefix, options, rerere_usage, 0); + if (skip_locked && (argc < 1 || strcmp(argv[0], "gc"))) + die(_("the option '%s' requires '%s'"), "--skip-locked", "gc"); + repo_config(the_repository, git_xmerge_config, NULL); if (autoupdate == 1) @@ -94,7 +99,8 @@ int cmd_rerere(int argc, if (!strcmp(argv[0], "clear")) { rerere_clear(the_repository, &merge_rr); } else if (!strcmp(argv[0], "gc")) - rerere_gc(the_repository, &merge_rr); + rerere_gc(the_repository, &merge_rr, + skip_locked ? RERERE_NOWAIT : 0); else if (!strcmp(argv[0], "status")) { if (setup_rerere(the_repository, &merge_rr, flags | RERERE_READONLY) < 0) diff --git a/rerere.c b/rerere.c index 64fac07c71d9d8..75b99f0f818614 100644 --- a/rerere.c +++ b/rerere.c @@ -887,18 +887,29 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE); + if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT)) + BUG("RERERE_NOWAIT does not apply with RERERE_READONLY"); if (flags & RERERE_READONLY) { fd = 0; } else { + int lock_flags = LOCK_DIE_ON_ERROR; + int timeout_ms = rerere_lock_timeout_ms; + + if (flags & RERERE_NOWAIT) { + lock_flags = 0; + timeout_ms = 0; + } + /* * Another process may hold the lock for a while, e.g. - * "git rerere gc" while it prunes rr-cache, so wait for - * it instead of dying right away. + * "git rerere gc" while it prunes rr-cache, so by default + * wait for it instead of dying right away. */ fd = repo_hold_lock_file_for_update_timeout(r, &write_lock, git_path_merge_rr(r), - LOCK_DIE_ON_ERROR, - rerere_lock_timeout_ms); + lock_flags, timeout_ms); + if (fd < 0) + return -1; } read_rr(r, merge_rr); return fd; @@ -1284,7 +1295,7 @@ bool rerere_gc_needed(struct repository *r, size_t limit) return needed; } -void rerere_gc(struct repository *r, struct string_list *rr) +void rerere_gc(struct repository *r, struct string_list *rr, int flags) { struct string_list to_remove = STRING_LIST_INIT_DUP; DIR *dir; @@ -1294,7 +1305,7 @@ void rerere_gc(struct repository *r, struct string_list *rr) timestamp_t cutoff_resolve; struct strbuf buf = STRBUF_INIT; - if (setup_rerere(r, rr, 0) < 0) + if (setup_rerere(r, rr, flags) < 0) return; rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve); diff --git a/rerere.h b/rerere.h index feeb0e2c9fe61b..1182ee3fb1d9f8 100644 --- a/rerere.h +++ b/rerere.h @@ -10,6 +10,8 @@ struct repository; #define RERERE_AUTOUPDATE 01 #define RERERE_NOAUTOUPDATE 02 #define RERERE_READONLY 04 +/* If MERGE_RR.lock is taken, return -1 as if rerere were disabled */ +#define RERERE_NOWAIT 010 /* * Marks paths that have been hand-resolved and added to the @@ -37,7 +39,7 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *, int rerere_forget(struct repository *, struct pathspec *); int rerere_remaining(struct repository *, struct string_list *); void rerere_clear(struct repository *, struct string_list *); -void rerere_gc(struct repository *, struct string_list *); +void rerere_gc(struct repository *, struct string_list *, int); /* * Check whether garbage collection for rerere entries is needed, which is diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh index 94165c8417ffc4..43795fb7a61985 100755 --- a/t/t4200-rerere.sh +++ b/t/t4200-rerere.sh @@ -242,6 +242,32 @@ test_expect_success 'old records rest in peace' ' test_path_is_missing $rr2/preimage ' +test_expect_success 'gc --skip-locked does nothing while MERGE_RR is locked' ' + mkdir -p $rr2 && + echo Hello >$rr2/preimage && + test-tool chmtime =$just_over_15_days_ago $rr2/preimage && + + test_when_finished "rm -f .git/MERGE_RR.lock" && + >.git/MERGE_RR.lock && + git rerere gc --skip-locked 2>err && + test_must_be_empty err && + test_path_is_file $rr2/preimage +' + +test_expect_success 'gc --skip-locked prunes while MERGE_RR is not locked' ' + mkdir -p $rr2 && + echo Hello >$rr2/preimage && + test-tool chmtime =$just_over_15_days_ago $rr2/preimage && + + git rerere gc --skip-locked && + test_path_is_missing $rr2/preimage +' + +test_expect_success '--skip-locked is only accepted by gc' ' + test_must_fail git rerere --skip-locked clear 2>err && + test_grep "option .--skip-locked. requires .gc." err +' + test_expect_success 'a held lock is waited out within rerere.lockTimeout' ' git reset --hard && rm -rf $rr && diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 4f65fa9439c0b8..9d610a29fe25c1 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -1007,9 +1007,17 @@ test_expect_rerere_gc () { shift fi + # An automatic run passes --skip-locked to "git rerere gc". + skip_locked= + case " $* " in + *" --auto "*) + skip_locked=--skip-locked + ;; + esac + rm -f "rerere-gc.txt" && GIT_TRACE2_EVENT="$(pwd)/rerere-gc.txt" "$@" && - test_subcommand $negate git rerere gc .git/MERGE_RR.lock && + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=-1 maintenance run --auto --task=rerere-gc +' + +test_expect_success 'rerere-gc task without --auto fails while MERGE_RR is locked' ' + test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" && + mkdir .git/rr-cache && + >.git/MERGE_RR.lock && + test_must_fail git -c rerere.lockTimeout=0 maintenance run --task=rerere-gc 2>err && + test_grep "Unable to create.*MERGE_RR\.lock" err +' + test_expect_success '--auto and --schedule incompatible' ' test_must_fail git maintenance run --auto --schedule=daily 2>err && test_grep "cannot be used together" err From 440b7035bbe0ce9a5b65722da288e82e8ef415ef Mon Sep 17 00:00:00 2001 From: Szymon Wilczek Date: Fri, 16 Oct 2026 11:16:47 +0200 Subject: [PATCH 04/37] cache-tree: do not die when the index is locked When a background process such as `git status` holds `index.lock` while `git rebase` picks a commit, the rebase can die in the middle of the pick and leave the sequencer in a state from which `git rebase --continue` refuses to run: error: you have staged changes in your working tree This happens because `try_to_commit()` calls `write_index_as_tree()` right after the pick has updated the index and written `CHERRY_PICK_HEAD`, and `write_index_as_tree()` dies when it cannot take the index lock. We then die before writing `rebase-merge/message`. Since 356ee4659b (sequencer: try to commit without forking 'git commit', 2017-11-24), this call happens in-process. Before that commit the `die()` happened in a `git commit` child process, and the sequencer stopped cleanly with its state saved. `write_index_as_tree()` does two different things: - It turns the index into a tree object. This part must succeed. - When that succeeds, it also rewrites the index to populate the cache-tree extension. This part is allowed to fail without impacting the result; as the comment next to `write_locked_index()` already notes, failing to write the index here is fine. Yet the function locks the index with `LOCK_DIE_ON_ERROR`, even though we only need the lock for the optional second part. Drop `LOCK_DIE_ON_ERROR` so that we take the lock gently, just like other callers that only refresh the index, and skip writing the index when the lock is held by someone else. This also lets `git write-tree` and `git am` proceed while another process holds the lock. Signed-off-by: Szymon Wilczek Signed-off-by: Junio C Hamano --- cache-tree.c | 10 +++++++--- t/t0090-cache-tree.sh | 13 +++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index b8cbb5da221020..671d4165bf57f0 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -809,11 +809,15 @@ struct tree *write_in_core_index_as_tree(struct repository *repo, int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix) { - int entries, was_valid; + int entries, was_valid, fd; struct lock_file lock_file = LOCK_INIT; int ret; - hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); + /* + * The lock only protects the cache-tree write-back below; if somebody + * else holds it, compute the tree without updating the index. + */ + fd = hold_lock_file_for_update(&lock_file, index_path, 0); entries = read_index_from(index_state, index_path, repo_get_git_dir(index_state->repo)); @@ -828,7 +832,7 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, ret = write_index_as_tree_internal(oid, index_state, was_valid, flags, prefix); - if (!ret && !was_valid) { + if (!ret && !was_valid && fd >= 0) { write_locked_index(index_state, &lock_file, COMMIT_LOCK); /* Not being able to write is fine -- we are only interested * in updating the cache-tree part, and if the next caller diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 0964718d7f33f5..4b7547e108fa8a 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -286,4 +286,17 @@ test_expect_success 'cache-tree is used by write-tree when valid' ' test_grep ! region_enter.*cache_tree.*update trace.output ' +test_expect_success 'write-tree succeeds while the index is locked' ' + test_when_finished "rm -f .git/index.lock" && + test_commit locked && + >uncommitted && + git add uncommitted && + >.git/index.lock && + git write-tree >actual && + test_path_is_file .git/index.lock && + rm .git/index.lock && + git write-tree >expect && + test_cmp expect actual +' + test_done From facedc705c8aaafbca31f5047ba0b877490008b3 Mon Sep 17 00:00:00 2001 From: Siddharth Shrimali Date: Sat, 17 Oct 2026 00:43:44 +0530 Subject: [PATCH 05/37] repack: do not rebuild packs on --dry-run "git repack --drop-filtered --dry-run" is documented to list the objects that would be dropped "without rebuilding any pack or deleting anything", but it does both. This is a bug in cmd_repack(): after printing the candidates, the --dry-run block falls through into the regular repack code. repack_promisor_objects() writes a new promisor pack, and with -d, existing_packs_remove_redundant() deletes the old packs. The existing guard only skips the implied "delete_redundant = 1", so it does not stop an explicit -d, and it does not stop the new pack from being written either. This has been broken since --dry-run was introduced in 1e746b00aa (builtin/repack: add --drop-filtered and --dry-run options). The --dry-run block never returned early, so a dry run has always gone on to do a regular repack. It was not broken by a later change. Fix it by returning right after the candidates are listed, and add a test that checks the pack directory is unchanged with and without -d. A dry run can no longer get past that point, so the two "!dry_run" checks that tried to keep it from deleting or dropping anything are now redundant. Reported-by: Coy Geek Helped-by: D. Ben Knoble Helped-by: Patrick Steinhardt Signed-off-by: Siddharth Shrimali Signed-off-by: Junio C Hamano --- builtin/repack.c | 14 ++++++++++---- t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index c4360382c1fce2..19894804f05f6b 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -351,10 +351,9 @@ int cmd_repack(int argc, /* * Dropping objects means rebuilding the promisor packs * without them and then removing the old packs, so the - * redundant packs must be deleted. Imply -d on a real run. + * redundant packs must be deleted. Imply -d. */ - if (!dry_run) - delete_redundant = 1; + delete_redundant = 1; ret = enumerate_promisor_blobs(repo, &po_args.filter_options, &drop_oids); @@ -391,6 +390,13 @@ int cmd_repack(int argc, oidset_iter_init(&drop_oids, &iter); while ((oid = oidset_iter_next(&iter))) printf("%s\n", oid_to_hex(oid)); + + /* + * Only list the candidates to be dropped; skip + * the actual repack + */ + ret = 0; + goto cleanup; } } @@ -505,7 +511,7 @@ int cmd_repack(int argc, if (pack_everything & ALL_INTO_ONE) { repack_promisor_objects(repo, &po_args, &names, packtmp, - (drop_filtered && !dry_run) ? &drop_oids : NULL); + drop_filtered ? &drop_oids : NULL); if (existing_packs_has_non_kept(&existing) && delete_redundant && diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh index cb361158349c13..a1c475e4ecc81b 100755 --- a/t/t7706-repack-drop-filtered.sh +++ b/t/t7706-repack-drop-filtered.sh @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' ' git -C repo cat-file -e "$BIG" ' +test_expect_success '--dry-run leaves the pack directory untouched' ' + BIG=$(cat big_oid) && + packdir=repo/.git/objects/pack && + + for opt in "" -d + do + ls $packdir >before && + + git -C repo -c repack.writeBitmaps=false \ + repack --drop-filtered --filter=blob:limit=1k \ + --dry-run -a $opt >out && + + ls $packdir >after && + test_cmp before after && + test_grep "$BIG" out && + git -C repo cat-file -e "$BIG" || return 1 + done +' + test_expect_success '--drop-filtered removes the promisor blob locally' ' BIG=$(cat big_oid) && SMALL=$(cat small_oid) && From cc7510f3dbec5828df12ff3dfae7864efc62e37b Mon Sep 17 00:00:00 2001 From: Olof Fairbrook Date: Sun, 18 Oct 2026 16:21:29 +0200 Subject: [PATCH 06/37] bundle: do not free uninitialized memory on bad arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "git bundle create" gives up with an error when setup_revisions() leaves an argument it does not understand, as in $ git bundle create private.bundle main --no-such-option error: unrecognized argument: --no-such-option Since 11ee9a75e7 (bundle: plug leaks in `create_bundle()`, 2024-06-11), that error jumps to the cleanup at the end of create_bundle(), which clears "revs_copy.pending". But "revs_copy" is only filled from "revs" further down, so the cleanup reads uninitialized stack memory and may free a garbage pointer. valgrind reports "Conditional jump or move depends on uninitialised value(s)" in object_array_clear() and in free(); whether the command crashes depends on what happens to be on the stack. Give “revs_copy” its value where it is declared, and add a test that fails under valgrind without this fix. Signed-off-by: Olof Fairbrook Signed-off-by: Junio C Hamano --- bundle.c | 2 +- t/t5607-clone-bundle.sh | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/bundle.c b/bundle.c index f55a521b2a1f12..aad5f251ca9606 100644 --- a/bundle.c +++ b/bundle.c @@ -492,7 +492,7 @@ int create_bundle(struct repository *r, const char *path, int bundle_fd = -1; int bundle_to_stdout; int ref_count = 0; - struct rev_info revs, revs_copy; + struct rev_info revs, revs_copy = { 0 }; int min_version = 2; struct bundle_prerequisites_info bpi; int ret; diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index d709bea7530445..c39db5a955a534 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -127,6 +127,13 @@ test_expect_success 'failed bundle creation does not leave cruft' ' test_path_is_missing fail.bundle.lock ' +test_expect_success 'an unknown argument after the revisions is an error' ' + test_must_fail git bundle create unknown.bundle main --no-such-option \ + 2>err && + test_grep "unrecognized argument: --no-such-option" err && + test_path_is_missing unknown.bundle +' + test_expect_success 'fetch SHA-1 from bundle' ' test_create_repo foo && test_commit -C foo x && From c3fc96de739bf1a85734677171537f2016b5e6da Mon Sep 17 00:00:00 2001 From: Andy Koppe Date: Sun, 18 Oct 2026 21:53:05 +0100 Subject: [PATCH 07/37] Makefile: pin cargo --target-dir to ./target The Makefile assumes that cargo places build artifacts under ./target. That assumption breaks if the user has a custom target dir setting, e.g. in their ~/.cargo/config.toml file or in the CARGO_TARGET_DIR env var. This results in a link error due to failing to find the gitcore library built with cargo. To fix this, pass '--target-dir target' to the cargo commands that build gitcore. Add it directly to the recipes rather than to CARGO_ARGS, so that it isn't lost if CARGO_ARGS is overridden. Leave the contrib/libgit-{sys,rs} targets alone, as nothing depends on the location of their build output. The Meson build of gitcore already passes --target-dir explicitly. Signed-off-by: Andy Koppe Signed-off-by: Junio C Hamano --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index a96be506b59f2d..c06fbe68444746 100644 --- a/Makefile +++ b/Makefile @@ -3048,7 +3048,7 @@ $(LIB_FILE): $(LIB_OBJS) ifndef NO_RUST ifeq ($(RUST_TARGETS),) $(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE) - $(QUIET_CARGO)cargo build $(CARGO_ARGS) + $(QUIET_CARGO)cargo build $(CARGO_ARGS) --target-dir target else ifneq ($(words $(RUST_TARGETS)),1) ifneq ($(uname_S),Darwin) @@ -3058,7 +3058,7 @@ endif RUST_MEMBER_LIBS = $(foreach target,$(RUST_TARGETS),target/$(target)/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME)) $(RUST_MEMBER_LIBS): target/%/$(RUST_BUILD_CONFIG)/$(RUST_LIB_NAME): Cargo.toml $(RUST_SOURCES) $(LIB_FILE) - $(QUIET_CARGO)cargo build $(CARGO_ARGS) --target $* + $(QUIET_CARGO)cargo build $(CARGO_ARGS) --target-dir target --target $* $(RUST_LIB): $(RUST_MEMBER_LIBS) $(call mkdir_p_parent_template) From 074aaaf4f03a3f291cb2f055343a9215699ec8e7 Mon Sep 17 00:00:00 2001 From: Olof Fairbrook Date: Sun, 18 Oct 2026 17:24:26 +0200 Subject: [PATCH 08/37] config: read the bytes of a blob as unsigned characters "git config --blob" and the other readers of config from memory get each byte from config_buf_fgetc(), which returns it as a plain "char". Where "char" is signed, as on x86, a byte 0xff comes back as -1, which is EOF, and any byte from 0x80 up as a negative number. So the parser stops at a byte 0xff and silently ignores the rest of the blob: $ printf '# caf\377\n[some]\n\tkey = value\n' >config $ git config --blob=$(git hash-object -w config) --list $ and a blob that starts with a UTF-8 byte order mark is a "bad config line 1", because the code that skips the mark compares with unsigned bytes. A config file has neither problem, as getc() returns unsigned bytes. ".gitmodules" read from a commit goes through the same reader. This has been so since config_buf_fgetc() was added in 1bc888193e (teach config --blob option to parse config from database, 2013-07-12). Return the byte as an unsigned char, as getc() does. config_buf_ungetc() checks that it is given back the byte it read; it now compares with the byte as an unsigned char too, or a CR followed by such a byte would hit its BUG(). Signed-off-by: Olof Fairbrook Signed-off-by: Junio C Hamano --- config.c | 4 ++-- t/t1307-config-blob.sh | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index d9019e7e6c34b0..fbe873274ee1cb 100644 --- a/config.c +++ b/config.c @@ -92,7 +92,7 @@ static long config_file_ftell(struct config_source *conf) static int config_buf_fgetc(struct config_source *conf) { if (conf->u.buf.pos < conf->u.buf.len) - return conf->u.buf.buf[conf->u.buf.pos++]; + return (unsigned char)conf->u.buf.buf[conf->u.buf.pos++]; return EOF; } @@ -101,7 +101,7 @@ static int config_buf_ungetc(int c, struct config_source *conf) { if (conf->u.buf.pos > 0) { conf->u.buf.pos--; - if (conf->u.buf.buf[conf->u.buf.pos] != c) + if ((unsigned char)conf->u.buf.buf[conf->u.buf.pos] != c) BUG("config_buf can only ungetc the same character"); return c; } diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh index 5cb546dd00c665..e2c0883350c36c 100755 --- a/t/t1307-config-blob.sh +++ b/t/t1307-config-blob.sh @@ -72,6 +72,27 @@ test_expect_success 'can parse blob ending with CR' ' test_cmp expect actual ' +test_expect_success 'a byte 0xff does not end a blob' ' + test_commit --printf FF config "# caf\\377\\n[some]\\n\\tkey = value\\n" && + echo value >expect && + git config --blob=HEAD:config some.key >actual && + test_cmp expect actual +' + +test_expect_success 'a byte 0xff after a CR in a blob' ' + test_commit --printf CRFF config "# a\\r\\377\\n[some]\\n\\tkey = value\\n" && + echo value >expect && + git config --blob=HEAD:config some.key >actual && + test_cmp expect actual +' + +test_expect_success 'can parse blob starting with a byte order mark' ' + test_commit --printf BOM config "\\357\\273\\277[some]\\n\\tkey = value\\n" && + echo value >expect && + git config --blob=HEAD:config some.key >actual && + test_cmp expect actual +' + test_expect_success 'config --blob outside of a repository is an error' ' nongit test_must_fail git config --blob=foo --list ' From 980e4de64134a1bb16b63cd9c0be9a184b532f1d Mon Sep 17 00:00:00 2001 From: Aniket Gupta Date: Mon, 19 Oct 2026 15:37:25 +0000 Subject: [PATCH 09/37] fsck: use Q_() for the verbose connectivity message The verbose "Checking connectivity" message has the word "objects" hardcoded as a plural inside a plain _(), so translators can't give the right form for the number. Use Q_() like the other count messages so gettext can pick it. English output stays the same. Signed-off-by: Aniket Gupta Signed-off-by: Junio C Hamano --- builtin/fsck.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index 9af4cc085bc3c1..fedf8f8533391a 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -379,7 +379,10 @@ static void check_connectivity(struct repository *repo) /* Look up all the requirements, warn about missing objects.. */ max = get_max_object_index(repo); if (verbose) - fprintf_ln(stderr, _("Checking connectivity (%d objects)"), max); + fprintf_ln(stderr, Q_("Checking connectivity (%d object)", + "Checking connectivity (%d objects)", + max), + max); for (i = 0; i < max; i++) { struct object *obj = get_indexed_object(repo, i); From 24e00cde8660d2973fce45bf7bd51b96fb04374b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 19 Oct 2026 19:11:21 +0200 Subject: [PATCH 10/37] archive-zip: omit UT extra field if mtime is out of range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We write an Extended Timestamp Extra Field (ID 0x5455 = "UT") for each zip entry, like Info-ZIP zip(1) does. Its time values are signed 32-bit integers (see e.g. https://libzip.org/specifications/extrafld.txt). Stop emitting them when the commit time doesn't fit in. Use a strbuf to prepare the extra field up front. Keep it empty if the time value is out of range. Like for the central zip directory we only care about the length-limited buffer aspect of the strbuf, not about NUL-termination. This simplifies the conditional output by avoiding to repeat the condition. Reported-by: Matthew E. Luallen Helped-by: Matthew E. Luallen Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- archive-zip.c | 37 +++++++++++++-------------------- t/t5004-archive-corner-cases.sh | 28 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/archive-zip.c b/archive-zip.c index 6f73ca6d588e83..785689584cb8bc 100644 --- a/archive-zip.c +++ b/archive-zip.c @@ -24,6 +24,7 @@ static int zip_time; /* We only care about the "buf" part here. */ static struct strbuf zip_dir; +static struct strbuf zip_extra_mtime; static uintmax_t zip_offset; static uint64_t zip_dir_entries; @@ -81,14 +82,6 @@ struct zip_dir_trailer { unsigned char _end[1]; }; -struct zip_extra_mtime { - unsigned char magic[2]; - unsigned char extra_size[2]; - unsigned char flags[1]; - unsigned char mtime[4]; - unsigned char _end[1]; -}; - struct zip64_extra { unsigned char magic[2]; unsigned char extra_size[2]; @@ -129,9 +122,6 @@ struct zip64_dir_trailer_locator { #define ZIP64_DATA_DESC_SIZE offsetof(struct zip64_data_desc, _end) #define ZIP_DIR_HEADER_SIZE offsetof(struct zip_dir_header, _end) #define ZIP_DIR_TRAILER_SIZE offsetof(struct zip_dir_trailer, _end) -#define ZIP_EXTRA_MTIME_SIZE offsetof(struct zip_extra_mtime, _end) -#define ZIP_EXTRA_MTIME_PAYLOAD_SIZE \ - (ZIP_EXTRA_MTIME_SIZE - offsetof(struct zip_extra_mtime, flags)) #define ZIP64_EXTRA_SIZE offsetof(struct zip64_extra, _end) #define ZIP64_EXTRA_PAYLOAD_SIZE \ (ZIP64_EXTRA_SIZE - offsetof(struct zip64_extra, size)) @@ -299,9 +289,8 @@ static int write_zip_entry(struct archiver_args *args, { struct zip_local_header header; uintmax_t offset = zip_offset; - struct zip_extra_mtime extra; struct zip64_extra extra64; - size_t header_extra_size = ZIP_EXTRA_MTIME_SIZE; + size_t header_extra_size = zip_extra_mtime.len; int need_zip64_extra = 0; unsigned long attr2; unsigned long compressed_size; @@ -315,7 +304,7 @@ static int write_zip_entry(struct archiver_args *args, const char *path_without_prefix = path + args->baselen; unsigned int creator_version = 0; unsigned int version_needed = 10; - size_t zip_dir_extra_size = ZIP_EXTRA_MTIME_SIZE; + size_t zip_dir_extra_size = zip_extra_mtime.len; size_t zip64_dir_extra_payload_size = 0; crc = crc32(0, NULL, 0); @@ -381,11 +370,6 @@ static int write_zip_entry(struct archiver_args *args, } } - copy_le16(extra.magic, 0x5455); - copy_le16(extra.extra_size, ZIP_EXTRA_MTIME_PAYLOAD_SIZE); - extra.flags[0] = 1; /* just mtime */ - copy_le32(extra.mtime, args->time); - if (size > 0xffffffff || compressed_size > 0xffffffff) need_zip64_extra = 1; if (stream && size > 0x7fffffff) @@ -412,8 +396,8 @@ static int write_zip_entry(struct archiver_args *args, zip_offset += ZIP_LOCAL_HEADER_SIZE; write_or_die(1, path, pathlen); zip_offset += pathlen; - write_or_die(1, &extra, ZIP_EXTRA_MTIME_SIZE); - zip_offset += ZIP_EXTRA_MTIME_SIZE; + write_or_die(1, zip_extra_mtime.buf, zip_extra_mtime.len); + zip_offset += zip_extra_mtime.len; if (need_zip64_extra) { copy_le16(extra64.magic, 0x0001); copy_le16(extra64.extra_size, ZIP64_EXTRA_PAYLOAD_SIZE); @@ -546,7 +530,7 @@ static int write_zip_entry(struct archiver_args *args, strbuf_add_le(&zip_dir, 4, attr2); strbuf_add_le(&zip_dir, 4, clamp32(offset)); strbuf_add(&zip_dir, path, pathlen); - strbuf_add(&zip_dir, &extra, ZIP_EXTRA_MTIME_SIZE); + strbuf_addbuf(&zip_dir, &zip_extra_mtime); if (zip64_dir_extra_payload_size) { strbuf_add_le(&zip_dir, 2, 0x0001); /* magic */ strbuf_add_le(&zip_dir, 2, zip64_dir_extra_payload_size); @@ -644,12 +628,21 @@ static int write_zip_archive(const struct archiver *ar UNUSED, dos_time(&args->time, &zip_date, &zip_time); strbuf_init(&zip_dir, 0); + strbuf_init(&zip_extra_mtime, 0); + + if (args->time <= 0x7fffffff) { + strbuf_add_le(&zip_extra_mtime, 2, 0x5455); /* magic */ + strbuf_add_le(&zip_extra_mtime, 2, 1 + 4); /* payload size */ + strbuf_add_le(&zip_extra_mtime, 1, 1); /* flags; just mtime */ + strbuf_add_le(&zip_extra_mtime, 4, args->time); + } err = write_archive_entries(args, write_zip_entry); if (!err) write_zip_trailer(args->commit_oid); strbuf_release(&zip_dir); + strbuf_release(&zip_extra_mtime); return err; } diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh index 768b0ff85d031e..c776387ad603c4 100755 --- a/t/t5004-archive-corner-cases.sh +++ b/t/t5004-archive-corner-cases.sh @@ -227,4 +227,32 @@ test_expect_success 'tar archive with long paths' ' git archive -o long_paths.tar $tree ' +test_lazy_prereq ZIPINFO_UT_EXTRA ' + "$ZIPINFO" -v "$TEST_DIRECTORY"/t5004/big-pack.zip | + grep "UT extra field modtime" +' + +test_expect_success TIME_IS_64BIT 'zip archive with 32-bit mtime' ' + blob=$(echo foo | git hash-object -w --stdin) && + tree=$(printf "100644 blob $blob\tfile" | git mktree) && + git archive --mtime="@2147483647 +0000" -o with_ut_extra.zip $tree +' + +test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'zip archive with mtime exceeding 32-bit' ' + blob=$(echo foo | git hash-object -w --stdin) && + tree=$(printf "100644 blob $blob\tfile" | git mktree) && + git archive --mtime="@2147483648 +0000" -o without_ut_extra.zip $tree +' + +test_expect_success TIME_IS_64BIT,ZIPINFO_UT_EXTRA 'UT extra field' ' + "$ZIPINFO" -v with_ut_extra.zip >actual && + test_grep "UT extra field modtime" actual +' + +test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT,ZIPINFO_UT_EXTRA \ + 'UT extra field omitted when mtime exceeds 32-bit' ' + "$ZIPINFO" -v without_ut_extra.zip >actual && + test_grep ! "UT extra field modtime" actual +' + test_done From 7f4e8cd95684bc99e5aba90b36a89d542b2bfe22 Mon Sep 17 00:00:00 2001 From: Andy Koppe Date: Mon, 19 Oct 2026 21:34:20 +0100 Subject: [PATCH 11/37] refs/files: keep root refs out of loose ref cache With the files backend, root refs such as HEAD or ORIG_HEAD are only added to the loose ref cache if REFS_FOR_EACH_INCLUDE_ROOT_REFS is passed when the cache is created. If the cache has already been created for an earlier ref iterator, root refs are silently omitted. Conversely, once they have been added, any later iterator also yields them, whether or not they were requested. This has been the case since d0f00c1ac1 (refs: introduce `refs_for_each_include_root_refs()`, 2024-02-23). For example, `git for-each-ref --include-root-refs --merged=main` omits root refs that are merged into main, because looking up the given commit loads the replace refs, which populates the loose ref cache first. The reftable backend is not affected, as it filters root refs for each iterator rather than caching them. Fix this by keeping the root refs out of the loose ref cache. Instead, read them into a separate, temporary ref cache whenever an iterator is requested with REFS_FOR_EACH_INCLUDE_ROOT_REFS, and overlay an iterator over that cache on the loose ref iterator. The cache is freed along with the iterator. Alternatively, the root refs could always be added to the loose ref cache and skipped during iteration unless requested. But that would mean reading them in every command that uses the loose ref cache, even though only two call sites actually request them. Also, keeping the root ref cache for reuse by later iterators is not worthwhile, because those two call sites only request the root refs once per command. Signed-off-by: Andy Koppe Signed-off-by: Junio C Hamano --- refs/files-backend.c | 37 ++++++++++++++++++++++++++-------- t/t6302-for-each-ref-filter.sh | 12 +++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 71628550f20a57..9d1e527114efba 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -481,8 +481,7 @@ static void add_root_refs(struct files_ref_store *refs, for_each_root_ref(refs, fill_root_ref, &data); } -static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs, - unsigned int flags) +static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs) { if (!refs->loose) { struct ref_dir *dir; @@ -499,9 +498,6 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs, dir = get_ref_dir(refs->loose->root); - if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) - add_root_refs(refs, dir); - /* * Add an incomplete entry for "refs/" (to be filled * lazily): @@ -511,6 +507,19 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs, return refs->loose; } +/* + * Root refs are kept out of the loose ref cache because ref iterators must + * only include them when requested with REFS_FOR_EACH_INCLUDE_ROOT_REFS. + */ +static struct ref_cache *create_root_ref_cache(struct files_ref_store *refs) +{ + struct ref_cache *cache = create_ref_cache(&refs->base, NULL); + + cache->root->flag &= ~REF_INCOMPLETE; + add_root_refs(refs, get_ref_dir(cache->root)); + return cache; +} + static int read_ref_internal(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno, int skip_packed_refs) @@ -1004,6 +1013,7 @@ struct files_ref_iterator { struct ref_iterator base; struct ref_iterator *iter0; + struct ref_cache *root_refs; struct repository *repo; unsigned int flags; }; @@ -1053,6 +1063,7 @@ static void files_ref_iterator_release(struct ref_iterator *ref_iterator) struct files_ref_iterator *iter = (struct files_ref_iterator *)ref_iterator; ref_iterator_free(iter->iter0); + free_ref_cache(iter->root_refs); } static struct ref_iterator_vtable files_ref_iterator_vtable = { @@ -1068,6 +1079,7 @@ static struct ref_iterator *files_ref_iterator_begin( { struct files_ref_store *refs; struct ref_iterator *loose_iter, *packed_iter, *overlay_iter; + struct ref_cache *root_refs = NULL; struct files_ref_iterator *iter; struct ref_iterator *ref_iterator; unsigned int required_flags = REF_STORE_READ; @@ -1094,9 +1106,17 @@ static struct ref_iterator *files_ref_iterator_begin( * disk, and re-reads it if not. */ - loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags), + loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), prefix, ref_store->repo, 1); + if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) { + root_refs = create_root_ref_cache(refs); + loose_iter = overlay_ref_iterator_begin( + cache_ref_iterator_begin(root_refs, prefix, + ref_store->repo, 0), + loose_iter); + } + /* * The packed-refs file might contain broken references, for * example an old version of a reference that points at an @@ -1118,6 +1138,7 @@ static struct ref_iterator *files_ref_iterator_begin( ref_iterator = &iter->base; base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable); iter->iter0 = overlay_iter; + iter->root_refs = root_refs; iter->repo = ref_store->repo; iter->flags = flags; @@ -1490,7 +1511,7 @@ static int should_pack_refs(struct files_ref_store *refs, if (limit < 16) limit = 16; - iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL, + iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, refs->base.repo, 0); while ((ret = ref_iterator_advance(iter)) == ITER_OK) { if (should_pack_ref(refs, &iter->ref, opts)) @@ -1530,7 +1551,7 @@ static int files_optimize(struct ref_store *ref_store, packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err); - iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL, + iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, refs->base.repo, 0); while ((ok = ref_iterator_advance(iter)) == ITER_OK) { /* diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 7f060d97bf509c..a2fc257f985e28 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -96,6 +96,18 @@ test_expect_success '--include-root-refs omits dangling symrefs' ' ) ' +test_expect_success '--include-root-refs with --merged' ' + cat >expect <<-\EOF && + HEAD + ORIG_HEAD + EOF + test_when_finished "git update-ref -d ORIG_HEAD" && + git update-ref ORIG_HEAD main && + git for-each-ref --format="%(refname)" --include-root-refs \ + --merged=main "*HEAD" >actual && + test_cmp expect actual +' + test_expect_success 'filtering with --points-at' ' cat >expect <<-\EOF && refs/heads/main From 071f2e36bcbaf81ed4ebed2e272afe483849b5b0 Mon Sep 17 00:00:00 2001 From: Roger Koranteng Date: Tue, 20 Oct 2026 09:26:27 +0000 Subject: [PATCH 12/37] t5301: use test_path_is_file instead of test -f Replace raw 'test -f' assertions with the 'test_path_is_file' helper, which gives diagnostic output when the assertion fails. Signed-off-by: Roger Koranteng Signed-off-by: Junio C Hamano --- t/t5301-sliding-window.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5301-sliding-window.sh b/t/t5301-sliding-window.sh index 3c3666b2781946..b6868229bf65e1 100755 --- a/t/t5301-sliding-window.sh +++ b/t/t5301-sliding-window.sh @@ -22,7 +22,7 @@ test_expect_success 'setup' ' git repack -a -d && test "$(git count-objects)" = "0 objects, 0 kilobytes" && pack1=$(ls .git/objects/pack/*.pack) && - test -f "$pack1" + test_path_is_file "$pack1" ' test_expect_success 'verify-pack -v, defaults' ' @@ -48,7 +48,7 @@ test_expect_success 'repack -a -d, packedGit{WindowSize,Limit} == 1 page' ' git repack -a -d && test "$(git count-objects)" = "0 objects, 0 kilobytes" && pack2=$(ls .git/objects/pack/*.pack) && - test -f "$pack2" && + test_path_is_file "$pack2" && test "$pack1" \!= "$pack2" ' From 16bd683014deb5361a4210a7a9b37759ae28b877 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 20 Oct 2026 10:18:31 -0700 Subject: [PATCH 13/37] github-ci: update macos jobs from macos-14 GitHub from address keeps sending Subject: [Action Required] macOS 14 hosted runner image is closing down and suggests to update to either macos-latest (macos-26) or macos-15. Use macos-15 for two of the four osx-* jobs, and macos-26 for the other two jobs. Use explicit version number "26" instead of "latest" so that we can more easily tell the differences and similarities between github-ci and gitlab-ci and compare their outcome on the same commit when needed. Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b229739be8b5ec..f37834326fc5d3 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -349,16 +349,16 @@ jobs: vector: - jobname: osx-clang cc: clang - pool: macos-14 + pool: macos-15 - jobname: osx-reftable cc: clang - pool: macos-14 + pool: macos-26 - jobname: osx-gcc cc: gcc-13 - pool: macos-14 + pool: macos-26 - jobname: osx-meson cc: clang - pool: macos-14 + pool: macos-15 env: CC: ${{matrix.vector.cc}} CC_PACKAGE: ${{matrix.vector.cc_package}} From b2608c4076803aff8e7bbd72564e189b5200a018 Mon Sep 17 00:00:00 2001 From: Cynthia Uche Date: Wed, 21 Oct 2026 04:37:23 +0100 Subject: [PATCH 14/37] t2104: avoid suppressing git's exit code The exit code of the upstream command in a pipeline is lost, so a crash in 'git ls-files' would go unnoticed. Write the output to a file and compare that instead. Signed-off-by: Cynthia Uche Signed-off-by: Junio C Hamano --- t/t2104-update-index-skip-worktree.sh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/t/t2104-update-index-skip-worktree.sh b/t/t2104-update-index-skip-worktree.sh index 7a0778ed983a50..68a4339e243d02 100755 --- a/t/t2104-update-index-skip-worktree.sh +++ b/t/t2104-update-index-skip-worktree.sh @@ -34,7 +34,8 @@ test_expect_success 'setup' ' mkdir sub && touch ./1 ./2 sub/1 sub/2 && git add 1 2 sub/1 sub/2 && - git ls-files -t | test_cmp expect.full - + git ls-files -t >actual && + test_cmp expect.full actual ' test_expect_success 'index is at version 2' ' @@ -43,7 +44,8 @@ test_expect_success 'index is at version 2' ' test_expect_success 'update-index --skip-worktree' ' git update-index --skip-worktree 1 sub/1 && - git ls-files -t | test_cmp expect.skip - + git ls-files -t >actual && + test_cmp expect.skip actual ' test_expect_success 'index is at version 3 after having some skip-worktree entries' ' @@ -51,12 +53,14 @@ test_expect_success 'index is at version 3 after having some skip-worktree entri ' test_expect_success 'ls-files -t' ' - git ls-files -t | test_cmp expect.skip - + git ls-files -t >actual && + test_cmp expect.skip actual ' test_expect_success 'update-index --no-skip-worktree' ' git update-index --no-skip-worktree 1 sub/1 && - git ls-files -t | test_cmp expect.full - + git ls-files -t >actual && + test_cmp expect.full actual ' test_expect_success 'index version is back to 2 when there is no skip-worktree entry' ' From 5e106cb22b6b0ecb2a7c45cf7a82fbfeb4ee2960 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:25 -0500 Subject: [PATCH 15/37] pack-write: make index-pack output comment hash agnostic The code comment in `index_pack_lockfile()` explains the expected format of output from git-index-pack(1). As written it explicitly mentions SHA1, but both the git-index-pack(1) output and the parsing are not specific to SHA1 and also support SHA256. Update the comment explanation to be hash agnostic accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- pack-write.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pack-write.c b/pack-write.c index 85674e4b726bab..b1311c9923384f 100644 --- a/pack-write.c +++ b/pack-write.c @@ -476,11 +476,15 @@ char *index_pack_lockfile(struct odb_source *source, int ip_out, const int len = source->odb->repo->hash_algo->hexsz + 6; /* - * The first thing we expect from index-pack's output - * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where - * %40s is the newly created pack SHA1 name. In the "keep" - * case, we need it to remove the corresponding .keep file - * later on. If we don't get that then tough luck with it. + * The first thing we expect from index-pack's output is + * "pack\t\n" or "keep\t\n", where is the hexadecimal + * name of the packfile it has just written. The line is therefore five + * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A + * "keep" prefix tells us that index-pack has also created the + * corresponding ".keep" file. + * + * In the "keep" case, we need it to remove the corresponding ".keep" + * file later on. If we don't get that then tough luck with it. */ if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') { const char *name; From 21bb64feefa3ca4a5982075781421fc4f98d92b6 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:26 -0500 Subject: [PATCH 16/37] pack-write: lift and expose index-pack report parsing Once git-index-pack(1) finishes writing a packfile, it reports the hexadecimal name of that packfile on stdout prefixed with either "pack" or "keep" depending on whether a ".keep" file was also created alongside it. Callers that are required to manage the lifecycle of ".keep" files parse this report via `index_pack_lockfile()`, which provides the path to the temporary ".keep" file. In a subsequent commit, the "files" ODB transaction begins creating the pack lockfiles during transaction commit instead of eagerly staging them in the transaction quarantine directory with the packfile. Extract the generic parsing logic for the git-index-pack(1) output into `read_index_pack_report()` in preparation for recording the names of newly written packfiles. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- pack-write.c | 56 ++++++++++++++++++++++++++++++++++++++-------------- pack.h | 10 ++++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/pack-write.c b/pack-write.c index b1311c9923384f..2ed33448bae0ec 100644 --- a/pack-write.c +++ b/pack-write.c @@ -469,11 +469,16 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo, fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name); } -char *index_pack_lockfile(struct odb_source *source, int ip_out, - int *is_well_formed) +int read_index_pack_report(struct repository *repo, int ip_out, + struct strbuf *pack_name, int *is_keep) { + const int len = repo->hash_algo->hexsz + 6; char packname[GIT_MAX_HEXSZ + 6]; - const int len = source->odb->repo->hash_algo->hexsz + 6; + int is_keep_ignored; + const char *name; + + if (!is_keep) + is_keep = &is_keep_ignored; /* * The first thing we expect from index-pack's output is @@ -482,24 +487,45 @@ char *index_pack_lockfile(struct odb_source *source, int ip_out, * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A * "keep" prefix tells us that index-pack has also created the * corresponding ".keep" file. - * - * In the "keep" case, we need it to remove the corresponding ".keep" - * file later on. If we don't get that then tough luck with it. */ - if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') { - const char *name; + if (read_in_full(ip_out, packname, len) != len || packname[len - 1] != '\n') + return -1; + + packname[len - 1] = 0; + if (skip_prefix(packname, "pack\t", &name)) + *is_keep = 0; + else if (skip_prefix(packname, "keep\t", &name)) + *is_keep = 1; + else + return -1; + + strbuf_addstr(pack_name, name); + + return 0; +} +char *index_pack_lockfile(struct odb_source *source, int ip_out, + int *is_well_formed) +{ + struct strbuf pack_name = STRBUF_INIT; + char *lockfile = NULL; + int is_keep = 0; + + if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) { if (is_well_formed) - *is_well_formed = 1; - packname[len-1] = 0; - if (skip_prefix(packname, "keep\t", &name)) - return xstrfmt("%s/pack/pack-%s.keep", - source->path, name); + *is_well_formed = 0; return NULL; } + if (is_well_formed) - *is_well_formed = 0; - return NULL; + *is_well_formed = 1; + + /* If there is no ".keep" file, then there is no lockfile to return. */ + if (is_keep) + lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf); + + strbuf_release(&pack_name); + return lockfile; } /* diff --git a/pack.h b/pack.h index ada506b5c5d0db..2f2ce97c11778f 100644 --- a/pack.h +++ b/pack.h @@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t); void fixup_pack_header_footer(const struct git_hash_algo *, int, unsigned char *, const char *, uint32_t, unsigned char *, off_t); + +/* + * Read the report that git-index-pack(1) writes to its standard output after + * the packfile is written. The hexadecimal name of the packfile is appended to + * `pack_name` and, if `is_keep` is non-NULL, it is set to indicate whether a + * ".keep" file was written. Return 0 on success and a negative error code on + * failure. + */ +int read_index_pack_report(struct repository *repo, int ip_out, + struct strbuf *pack_name, int *is_keep); char *index_pack_lockfile(struct odb_source *source, int fd, int *is_well_formed); From dfb7f49776848c90ee825be20afa1944b424c971 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:27 -0500 Subject: [PATCH 17/37] object-file: explicitly write ".keep" lockfile The `odb_transaction_write_pack()` interface for the "files" backend relies on git-index-pack(1) to write the specified packfile into an ODB transaction as a packfile. To prevent a concurrent housekeeping operation from deleting the newly written packfile prior to the references being updated, a ".keep" lockfile is also generated by git-index-pack(1) via the `--keep` option. Consequently, the ".keep" lockfile is written into the ODB transaction alongside the packfile and later migrated together to the primary ODB during transaction commit. In a subsequent commit, writing the lockfile is deferred until transaction commit and thus cannot rely on git-index-pack(1) to write it. In preparation for this, update `odb_transaction_files_write_pack()` to explicitly generate the same ".keep" lockfile itself. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- object-file.c | 107 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 85 insertions(+), 22 deletions(-) diff --git a/object-file.c b/object-file.c index 210984f825326e..74a31760bcff49 100644 --- a/object-file.c +++ b/object-file.c @@ -1256,6 +1256,47 @@ int read_loose_object(struct repository *repo, return ret; } +static int write_pack_lockfile(struct odb_source *source, const char *pack_name, + const char *keep_msg) +{ + struct strbuf lock_path = STRBUF_INIT; + struct strbuf contents = STRBUF_INIT; + int ret; + int fd; + + strbuf_addf(&lock_path, "%s/pack/pack-%s.keep", + source->path, pack_name); + strbuf_addf(&contents, "%s\n", keep_msg); + + fd = safe_create_file_with_leading_directories(source->odb->repo, + lock_path.buf); + if (fd < 0) { + ret = error_errno(_("unable to create '%s'"), lock_path.buf); + goto out; + } + + if (write_in_full(fd, contents.buf, contents.len) < 0) { + ret = error_errno(_("unable to write '%s'"), lock_path.buf); + close(fd); + unlink(lock_path.buf); + goto out; + } + + if (close(fd) < 0) { + ret = error_errno(_("unable to close '%s'"), lock_path.buf); + unlink(lock_path.buf); + goto out; + } + + ret = 0; + +out: + strbuf_release(&lock_path); + strbuf_release(&contents); + + return ret; +} + static int odb_transaction_files_commit(struct odb_transaction *base) { struct odb_transaction_files *transaction = @@ -1348,9 +1389,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, container_of(base, struct odb_transaction_files, base); struct repository *repo = base->source->odb->repo; struct child_process child = CHILD_PROCESS_INIT; + struct strbuf final_path = STRBUF_INIT; + struct strbuf pack_name = STRBUF_INIT; + struct strbuf keep_msg = STRBUF_INIT; struct pack_header hdr; const char *hdr_err; int err_fd = opts->err_fd; + int ret = -1; int status; hdr_err = parse_pack_header(&hdr, pack_fd); @@ -1390,17 +1435,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, } } else { char hostname[HOST_NAME_MAX + 1]; - char *lockfile; strvec_pushl(&child.args, "index-pack", "--stdin", NULL); push_header_arg(&child.args, &hdr); if (xgethostname(hostname, sizeof(hostname))) xsnprintf(hostname, sizeof(hostname), "localhost"); - strvec_pushf(&child.args, - "--keep=receive-pack %"PRIuMAX" on %s", - (uintmax_t)getpid(), - hostname); if (!opts->quiet && err_fd) strvec_push(&child.args, "--show-resolving-progress"); @@ -1424,34 +1464,57 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, return -1; } - /* - * The lockfile filepath is expected to be the final location of - * the ".keep" file after being migrated to the main ODB source. - * This ensures the lockfile can be found and removed later - * after the ODB transaction has been committed. - */ - lockfile = index_pack_lockfile(base->source, child.out, NULL); - if (lockfile) { - ALLOC_GROW(transaction->pack_lockfiles, - transaction->pack_lockfiles_nr + 1, - transaction->pack_lockfiles_alloc); - transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = - register_tempfile(lockfile); - free(lockfile); - } + ret = read_index_pack_report(repo, child.out, &pack_name, NULL); + close(child.out); status = finish_command(&child); if (status) { strbuf_addstr(err_msg, "index-pack abnormal exit"); - return -1; + goto out; + } + + if (ret) { + strbuf_addstr(err_msg, "index-pack failed to report pack name"); + goto out; } + strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s", + (uintmax_t)getpid(), hostname); + + if (write_pack_lockfile(transaction->quarantine, pack_name.buf, + keep_msg.buf)) { + strbuf_addstr(err_msg, "lockfile write failed"); + goto out; + } + + ALLOC_GROW(transaction->pack_lockfiles, + transaction->pack_lockfiles_nr + 1, + transaction->pack_lockfiles_alloc); + + /* + * The lockfile filepath is expected to be the final location of + * the ".keep" file after being migrated to the main ODB source. + * This ensures the lockfile can be found and removed later + * after the ODB transaction has been committed. + */ + strbuf_addf(&final_path, "%s/pack/pack-%s.keep", + base->source->path, pack_name.buf); + transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = + register_tempfile(final_path.buf); + odb_source_prepare(transaction->quarantine, ODB_PREPARE_FLUSH_CACHES); } - return 0; + ret = 0; + +out: + strbuf_release(&final_path); + strbuf_release(&pack_name); + strbuf_release(&keep_msg); + + return ret; } static int odb_transaction_files_finalize(struct odb_transaction *base) From b07099fff9401316b0e55c43f8dde46ddf0e7423 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:28 -0500 Subject: [PATCH 18/37] object-file: write ".keep" lockfiles during transaction commit The "files" ODB backend writes a ".keep" lockfile into the transaction quarantine directory via `odb_transaction_files_write_pack()`. When the ODB transaction is committed, the ".keep" lockfile is migrated to the primary ODB alongside its packfile. Writing the temporary lockfile in this manner has a couple of downsides: - Migrating quarantined objects via `tmp_objdir_migrate()` doesn't immediately stop if a ".keep" lockfile already exists for the same packfile in the primary ODB and instead allows all the remaining objects to be migrated. So even though `odb_transaction_commit()` ultimately returns an error here, the objects are needlessly written to the primary ODB anyways. - Even more concerning, the ".keep" tempfile atexit(3) handler tries to remove any lockfile of the same name regardless of whether it was created by the same process. If the ".keep" lockfile of the same name for a concurrent process gets deleted prematurely, its packfile becomes at risk of being deleted by concurrent housekeeping. Stop creating/registering the temporary lockfiles when writing the packfile to the transaction and instead do so when committing the transaction. This ensures the lockfile is properly acquired prior to migrating objects and that only the owning process can delete its own lockfiles. Reported-by: Qin ShiCheng Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- object-file.c | 76 +++++++++++++++++++++----------------- t/t5547-push-quarantine.sh | 30 +++++++++++++-- 2 files changed, 70 insertions(+), 36 deletions(-) diff --git a/object-file.c b/object-file.c index 74a31760bcff49..e67c0865a2b375 100644 --- a/object-file.c +++ b/object-file.c @@ -492,6 +492,7 @@ struct odb_transaction_files { struct transaction_packfile packfile; const char *prefix; + struct strvec pack_names; struct tempfile **pack_lockfiles; size_t pack_lockfiles_nr; size_t pack_lockfiles_alloc; @@ -1257,7 +1258,7 @@ int read_loose_object(struct repository *repo, } static int write_pack_lockfile(struct odb_source *source, const char *pack_name, - const char *keep_msg) + const char *keep_msg, struct tempfile **out) { struct strbuf lock_path = STRBUF_INIT; struct strbuf contents = STRBUF_INIT; @@ -1288,6 +1289,7 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name, goto out; } + *out = register_tempfile(lock_path.buf); ret = 0; out: @@ -1297,6 +1299,40 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name, return ret; } +static int write_pack_lockfiles(struct odb_transaction_files *transaction) +{ + struct strbuf keep_msg = STRBUF_INIT; + char hostname[HOST_NAME_MAX + 1]; + int ret = 0; + + if (!transaction->pack_names.nr) + return 0; + + if (xgethostname(hostname, sizeof(hostname))) + xsnprintf(hostname, sizeof(hostname), "localhost"); + strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s", + (uintmax_t)getpid(), hostname); + + for (size_t i = 0; i < transaction->pack_names.nr; i++) { + struct tempfile *lock; + + ret = write_pack_lockfile(transaction->base.source, + transaction->pack_names.v[i], + keep_msg.buf, &lock); + if (ret < 0) + break; + + ALLOC_GROW(transaction->pack_lockfiles, + transaction->pack_lockfiles_nr + 1, + transaction->pack_lockfiles_alloc); + transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = lock; + } + + strbuf_release(&keep_msg); + + return ret; +} + static int odb_transaction_files_commit(struct odb_transaction *base) { struct odb_transaction_files *transaction = @@ -1325,6 +1361,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) delete_tempfile(&temp); strbuf_release(&temp_path); + if (write_pack_lockfiles(transaction)) + return error(_("unable to lock packfiles")); + /* * Make the object files visible in the primary ODB after their data is * fully durable. @@ -1389,9 +1428,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, container_of(base, struct odb_transaction_files, base); struct repository *repo = base->source->odb->repo; struct child_process child = CHILD_PROCESS_INIT; - struct strbuf final_path = STRBUF_INIT; struct strbuf pack_name = STRBUF_INIT; - struct strbuf keep_msg = STRBUF_INIT; struct pack_header hdr; const char *hdr_err; int err_fd = opts->err_fd; @@ -1434,14 +1471,9 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, return -1; } } else { - char hostname[HOST_NAME_MAX + 1]; - strvec_pushl(&child.args, "index-pack", "--stdin", NULL); push_header_arg(&child.args, &hdr); - if (xgethostname(hostname, sizeof(hostname))) - xsnprintf(hostname, sizeof(hostname), "localhost"); - if (!opts->quiet && err_fd) strvec_push(&child.args, "--show-resolving-progress"); if (err_fd) @@ -1479,29 +1511,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, goto out; } - strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s", - (uintmax_t)getpid(), hostname); - - if (write_pack_lockfile(transaction->quarantine, pack_name.buf, - keep_msg.buf)) { - strbuf_addstr(err_msg, "lockfile write failed"); - goto out; - } - - ALLOC_GROW(transaction->pack_lockfiles, - transaction->pack_lockfiles_nr + 1, - transaction->pack_lockfiles_alloc); - - /* - * The lockfile filepath is expected to be the final location of - * the ".keep" file after being migrated to the main ODB source. - * This ensures the lockfile can be found and removed later - * after the ODB transaction has been committed. - */ - strbuf_addf(&final_path, "%s/pack/pack-%s.keep", - base->source->path, pack_name.buf); - transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = - register_tempfile(final_path.buf); + strvec_push(&transaction->pack_names, pack_name.buf); odb_source_prepare(transaction->quarantine, ODB_PREPARE_FLUSH_CACHES); @@ -1510,9 +1520,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, ret = 0; out: - strbuf_release(&final_path); strbuf_release(&pack_name); - strbuf_release(&keep_msg); return ret; } @@ -1527,6 +1535,7 @@ static int odb_transaction_files_finalize(struct odb_transaction *base) ret |= delete_tempfile(&transaction->pack_lockfiles[i]); free(transaction->pack_lockfiles); + strvec_clear(&transaction->pack_names); return ret; } @@ -1559,6 +1568,7 @@ int odb_transaction_files_begin(struct odb_source *source, transaction->base.write_pack = odb_transaction_files_write_pack; transaction->base.env = odb_transaction_files_env; transaction->flags = flags; + strvec_init(&transaction->pack_names); transaction->prefix = "bulk-fsync"; if (flags & ODB_TRANSACTION_RECEIVE) { diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh index 1b7097179ee980..6c14dfc42b10c8 100755 --- a/t/t5547-push-quarantine.sh +++ b/t/t5547-push-quarantine.sh @@ -77,10 +77,9 @@ test_expect_success '.keep file is removed after push' ' git -C keep.git config set receive.unpackLimit 0 && # While incoming objects are still quarantined, validate that the - # ".keep" lockfile is present in the quarantine directory. + # ".keep" lockfile is not present in the quarantine directory. test_hook -C keep.git pre-receive <<-\EOF && - keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" && - test -f "$keep" + test ! -e "$GIT_QUARANTINE_PATH"/pack/pack-*.keep EOF # After quarantined objects are migrated, validate that the ".keep" @@ -101,4 +100,29 @@ test_expect_success '.keep file is removed after push' ' test_path_is_missing "$keep" ' +test_expect_success '.keep file created outside of process is not removed' ' + test_when_finished rm -rf keep.git && + git init --bare keep.git && + + git -C keep.git config set receive.unpackLimit 0 && + + # Create a ".keep" lockfile matching the packfile to simulate a + # concurrent git-receive-pack(1) that has already written its lockfile. + test_hook -C keep.git pre-receive <<-\EOF && + pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" && + pack="${pack##*/}" && + echo "baz" >"objects/pack/${pack%.pack}.keep" + EOF + + test_commit bar && + test_must_fail git push keep.git HEAD && + + # The ".keep" lockfile of the concurrent git-receive-pack(1) should be + # left in its place. + keep="$(ls keep.git/objects/pack/pack-*.keep)" && + test_path_is_file "$keep" && + test_grep "baz" "$keep" && + test_path_is_missing "${keep%.keep}.pack" +' + test_done From 91ef176b8fb99f0c47986afbc44dbd54eaa9f880 Mon Sep 17 00:00:00 2001 From: tsukasa inoue Date: Wed, 21 Oct 2026 21:35:16 +0000 Subject: [PATCH 19/37] branch: strip refs/heads/ from the --set-upstream-to hint When git switch fails to write the tracking config, it prints a hint with the command to set the upstream by hand. For example, if .git/config.lock exists: error: could not lock config file .git/config: File exists error: unable to write upstream branch configuration hint: hint: After fixing the error cause you may try to fix up hint: the remote tracking information by invoking: hint: git branch --set-upstream-to=origin/refs/heads/feat The hint says origin/refs/heads/feat, but the correct name is origin/feat. Running the command as printed fails, because the full ref name is not a valid remote-tracking branch name here. This has been wrong since a3f40ec4b0 (branch: accept multiple upstream branches for tracking, 2021-12-20). Strip refs/heads/ from the hint, as the success message in the same function already does. The git config --add line for multiple upstreams still needs the full ref name, so it is not changed. Signed-off-by: tsukasa inoue Signed-off-by: Junio C Hamano --- branch.c | 8 +++++--- t/t3200-branch.sh | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/branch.c b/branch.c index 22f4f46b96ed3a..bb07846911aee5 100644 --- a/branch.c +++ b/branch.c @@ -186,12 +186,14 @@ static int install_branch_config_multiple_remotes(int flag, const char *local, advise(_("\nAfter fixing the error cause you may try to fix up\n" "the remote tracking information by invoking:")); - if (remotes->nr == 1) + if (remotes->nr == 1) { + shortname = remotes->items[0].string; + skip_prefix(shortname, "refs/heads/", &shortname); advise(" git branch --set-upstream-to=%s%s%s", origin ? origin : "", origin ? "/" : "", - remotes->items[0].string); - else { + shortname); + } else { advise(" git config --add branch.\"%s\".remote %s", local, origin ? origin : "."); for_each_string_list_item(item, remotes) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cdb6c6a6342a0c..ccfb53efa3e910 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1080,6 +1080,31 @@ test_expect_success '--set-upstream-to fails on locked config' ' test_grep "could not lock config file .git/config" err ' +test_expect_success '--set-upstream-to hint uses short remote-tracking branch name on locked config' ' + test_when_finished "git branch -D hint-local" && + test_when_finished "git update-ref -d refs/remotes/hintremote/hintbranch" && + test_config remote.hintremote.url . && + test_config remote.hintremote.fetch "+refs/heads/*:refs/remotes/hintremote/*" && + git update-ref refs/remotes/hintremote/hintbranch HEAD && + git branch hint-local && + test_when_finished "rm -f .git/config.lock" && + >.git/config.lock && + test_must_fail git -c core.configLockTimeout=0 \ + branch --set-upstream-to=hintremote/hintbranch hint-local 2>err && + cat >expect <<-\EOF && + hint: + hint: After fixing the error cause you may try to fix up + hint: the remote tracking information by invoking: + hint: git branch --set-upstream-to=hintremote/hintbranch + EOF + grep "^hint:" err >actual && + test_cmp expect actual && + rm -f .git/config.lock && + git branch --set-upstream-to=hintremote/hintbranch hint-local && + test_cmp_config hintremote branch.hint-local.remote && + test_cmp_config refs/heads/hintbranch branch.hint-local.merge +' + test_expect_success 'use --set-upstream-to modify HEAD' ' test_config branch.main.remote foo && test_config branch.main.merge foo && From 8f3d6ad89f419a62eef3e3335237e679e531f256 Mon Sep 17 00:00:00 2001 From: Tanga Joris Date: Thu, 22 Oct 2026 03:48:34 +0300 Subject: [PATCH 20/37] checkout: use Q_() for messages with counts Two messages in builtin/checkout.c show a count but are marked with a plain _(), so translators can only give a single form for them: - " ... and %d more." at the end of the list of orphaned commits left behind when leaving a detached HEAD, and - "only one reference expected, %d given." when more than one reference is given before "--". Use Q_() for both. Translators of a language that phrases "2 more" and "3 more" the same way now have to translate these messages twice. At that cost, however, we help translators of languages that need to phrase "2 more", "3 more", and "5 more" differently. Helped-by: Junio C Hamano Signed-off-by: Tanga Joris Signed-off-by: Junio C Hamano --- builtin/checkout.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2d575a4f57f76e..91b7c523969439 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1085,7 +1085,9 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs) if (more == 1) describe_one_orphan(&sb, last); else - strbuf_addf(&sb, _(" ... and %d more.\n"), more); + strbuf_addf(&sb, Q_(" ... and %d more.\n", + " ... and %d more.\n", + more), more); } fprintf(stderr, @@ -1489,7 +1491,9 @@ static int parse_branchname_arg(int argc, const char **argv, else if (dash_dash_pos == 1) has_dash_dash = 1; /* case (3) or (1) */ else if (dash_dash_pos >= 2) - die(_("only one reference expected, %d given."), dash_dash_pos); + die(Q_("only one reference expected, %d given.", + "only one reference expected, %d given.", + dash_dash_pos), dash_dash_pos); opts->count_checkout_paths = !opts->quiet && !has_dash_dash; if (!strcmp(arg, "-")) From 4ec526671145224633e13e4359089385d03c5ee0 Mon Sep 17 00:00:00 2001 From: tsukasa inoue Date: Thu, 22 Oct 2026 02:51:34 +0000 Subject: [PATCH 21/37] branch: return an error from setup_tracking() setup_tracking() calls exit(1) when it cannot write the tracking configuration. This exit was introduced in 27852b2c53 (branch: report errors in tracking branch setup, 2016-02-22). Because of it, callers cannot continue after a tracking failure, even though the ref has already been created. Change setup_tracking() to return an error instead of exiting, and make create_branch() and dwim_and_setup_tracking() return that value too. This does not change behavior. Callers still exit with status 1 on failure, as before. Changing checkout Signed-off-by: tsukasa inoue Signed-off-by: Junio C Hamano --- branch.c | 35 +++++++++++++++++++++-------------- branch.h | 21 +++++++++++++-------- builtin/branch.c | 17 ++++++++++++----- builtin/checkout.c | 17 +++++++++-------- 4 files changed, 55 insertions(+), 35 deletions(-) diff --git a/branch.c b/branch.c index 22f4f46b96ed3a..27be5ecc3ed9bb 100644 --- a/branch.c +++ b/branch.c @@ -249,12 +249,13 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) * will not be expanded to "refs/remotes/origin/main", so it is not safe * for 'orig_ref' to be raw user input. */ -static void setup_tracking(const char *new_ref, const char *orig_ref, - enum branch_track track, int quiet) +static int setup_tracking(const char *new_ref, const char *orig_ref, + enum branch_track track, int quiet) { struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + int ret = 0; struct find_tracked_branch_cb ftb_cb = { .tracking = &tracking, .ambiguous_remotes = STRING_LIST_INIT_DUP, @@ -343,11 +344,12 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, string_list_append(tracking.srcs, orig_ref); if (install_branch_config_multiple_remotes(config_flags, new_ref, tracking.remote, tracking.srcs) < 0) - exit(1); + ret = -1; cleanup: string_list_clear(&tracking_srcs, 0); string_list_clear(&ftb_cb.ambiguous_remotes, 0); + return ret; } int read_branch_desc(struct strbuf *buf, const char *branch_name) @@ -627,10 +629,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name, FREE_AND_NULL(real_ref); } -void create_branch(struct repository *r, - const char *name, const char *start_name, - int force, int clobber_head_ok, int reflog, - int quiet, enum branch_track track, int dry_run) +int create_branch(struct repository *r, + const char *name, const char *start_name, + int force, int clobber_head_ok, int reflog, + int quiet, enum branch_track track, int dry_run) { struct object_id oid; char *real_ref; @@ -639,6 +641,7 @@ void create_branch(struct repository *r, struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; int flags = 0; + int ret = 0; char *msg; if (track == BRANCH_TRACK_OVERRIDE) @@ -676,21 +679,25 @@ void create_branch(struct repository *r, free(msg); if (real_ref && track) - setup_tracking(ref.buf + 11, real_ref, track, quiet); + ret = setup_tracking(ref.buf + 11, real_ref, track, quiet); cleanup: strbuf_release(&ref); free(real_ref); + return ret; } -void dwim_and_setup_tracking(struct repository *r, const char *new_ref, - const char *orig_ref, enum branch_track track, - int quiet) +int dwim_and_setup_tracking(struct repository *r, const char *new_ref, + const char *orig_ref, enum branch_track track, + int quiet) { char *real_orig_ref = NULL; + int ret; + dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL); - setup_tracking(new_ref, real_orig_ref, track, quiet); + ret = setup_tracking(new_ref, real_orig_ref, track, quiet); free(real_orig_ref); + return ret; } /** @@ -840,8 +847,8 @@ void create_branches_recursively(struct repository *r, const char *name, * tedious to determine whether or not tracking was set up in the * superproject. */ - if (track) - setup_tracking(name, tracking_name, track, quiet); + if (track && setup_tracking(name, tracking_name, track, quiet)) + exit(1); for (i = 0; i < submodule_entry_list.entry_nr; i++) { if (submodule_create_branch( diff --git a/branch.h b/branch.h index e9b1f7b37df06f..4bc4527284e23b 100644 --- a/branch.h +++ b/branch.h @@ -34,10 +34,13 @@ enum branch_track { * causes new_ref to track whatever orig_ref tracks. * * - quiet suppresses tracking information. + * + * Returns 0 on success, or -1 if the tracking configuration could not be + * written. */ -void dwim_and_setup_tracking(struct repository *r, const char *new_ref, - const char *orig_ref, enum branch_track track, - int quiet); +int dwim_and_setup_tracking(struct repository *r, const char *new_ref, + const char *orig_ref, enum branch_track track, + int quiet); /* * Creates a new branch, where: @@ -63,12 +66,14 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref, * * - dry_run causes the branch to be validated but not created. * + * Returns 0 on success, or -1 if the tracking configuration could not be + * written. */ -void create_branch(struct repository *r, - const char *name, const char *start_name, - int force, int clobber_head_ok, - int reflog, int quiet, enum branch_track track, - int dry_run); +int create_branch(struct repository *r, + const char *name, const char *start_name, + int force, int clobber_head_ok, + int reflog, int quiet, enum branch_track track, + int dry_run); /* * Creates a new branch in a repository and its submodules (and its diff --git a/builtin/branch.c b/builtin/branch.c index a613148fc72786..b536b619bb03ca 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -1253,9 +1253,13 @@ int cmd_branch(int argc, die(_("branch '%s' does not exist"), branch->name); } - dwim_and_setup_tracking(the_repository, branch->name, - new_upstream, BRANCH_TRACK_OVERRIDE, - quiet); + if (dwim_and_setup_tracking(the_repository, branch->name, + new_upstream, BRANCH_TRACK_OVERRIDE, + quiet)) { + strbuf_release(&buf); + ret = 1; + goto out; + } strbuf_release(&buf); } else if (unset_upstream) { struct branch *branch; @@ -1305,8 +1309,11 @@ int cmd_branch(int argc, ret = 0; goto out; } - create_branch(the_repository, branch_name, start_name, force, 0, - reflog, quiet, track, 0); + if (create_branch(the_repository, branch_name, start_name, + force, 0, reflog, quiet, track, 0)) { + ret = 1; + goto out; + } } else usage_with_options(builtin_branch_usage, options); diff --git a/builtin/checkout.c b/builtin/checkout.c index 2d575a4f57f76e..13a2938b4258c8 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -979,14 +979,15 @@ static void update_refs_for_switch(const struct checkout_opts *opts, free(refname); } else - create_branch(the_repository, - opts->new_branch, new_branch_info->name, - opts->new_branch_force ? 1 : 0, - opts->new_branch_force ? 1 : 0, - opts->new_branch_log, - opts->quiet, - opts->track, - 0); + if (create_branch(the_repository, + opts->new_branch, new_branch_info->name, + opts->new_branch_force ? 1 : 0, + opts->new_branch_force ? 1 : 0, + opts->new_branch_log, + opts->quiet, + opts->track, + 0)) + exit(1); free(new_branch_info->name); free(new_branch_info->refname); new_branch_info->name = xstrdup(opts->new_branch); From ad74e02417f36e993aa5d1e497a29389f275986d Mon Sep 17 00:00:00 2001 From: tsukasa inoue Date: Thu, 22 Oct 2026 02:51:35 +0000 Subject: [PATCH 22/37] checkout: finish switching when the tracking config cannot be written When git switch -c / (or checkout -b) cannot write the tracking config, for example because .git/config.lock exists, the worktree, the index and the new branch ref have already been updated. Git then exits with status 1 while HEAD still points to the original branch. The changes of the switch remain staged, and the post-checkout hook does not run. create_branch() now returns an error instead of exiting. Make checkout ignore that error so that the switch completes and the command exits with status 0. The error and the hint are still printed. Match push -u, clone and fetch, which also print the error and continue. The upstream can be set later with the command shown in the hint. Keep git branch failing when the tracking config cannot be written, so its behavior does not change; t3200 covers this. Making the switch fully atomic, undoing the worktree, the index, the ref and the config when the tracking config fails, is a separate topic. Signed-off-by: tsukasa inoue Signed-off-by: Junio C Hamano --- builtin/checkout.c | 23 ++++++++++++++--------- t/t2027-checkout-track.sh | 20 ++++++++++++++++++++ t/t2060-switch.sh | 20 ++++++++++++++++++++ t/t3200-branch.sh | 13 +++++++++++++ 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 13a2938b4258c8..c5e2202026ef80 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -979,15 +979,20 @@ static void update_refs_for_switch(const struct checkout_opts *opts, free(refname); } else - if (create_branch(the_repository, - opts->new_branch, new_branch_info->name, - opts->new_branch_force ? 1 : 0, - opts->new_branch_force ? 1 : 0, - opts->new_branch_log, - opts->quiet, - opts->track, - 0)) - exit(1); + /* + * Not fatal: the error and hint have already been + * shown, and like "push -u" and "clone" we finish + * the switch instead of leaving HEAD on the old + * branch with the worktree already updated. + */ + create_branch(the_repository, + opts->new_branch, new_branch_info->name, + opts->new_branch_force ? 1 : 0, + opts->new_branch_force ? 1 : 0, + opts->new_branch_log, + opts->quiet, + opts->track, + 0); free(new_branch_info->name); free(new_branch_info->refname); new_branch_info->name = xstrdup(opts->new_branch); diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh index c01f1cd617c72b..c3be183bfcc411 100755 --- a/t/t2027-checkout-track.sh +++ b/t/t2027-checkout-track.sh @@ -47,6 +47,26 @@ test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' ' test_cmp_config refs/heads/main branch.b4.merge ' +test_expect_success 'checkout -b completes when tracking config cannot be written' ' + test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" && + test_when_finished "git branch -D lock-new" && + test_when_finished "git checkout main" && + test_config remote.lockremote.url . && + test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" && + git update-ref refs/remotes/lockremote/lockbranch one && + test_when_finished "rm -f .git/config.lock" && + >.git/config.lock && + git -c core.configLockTimeout=0 \ + checkout -b lock-new lockremote/lockbranch 2>err && + test_grep "unable to write upstream branch configuration" err && + echo refs/heads/lock-new >expect && + git symbolic-ref HEAD >actual && + test_cmp expect actual && + git diff --cached --exit-code && + rm -f .git/config.lock && + test_must_fail git config branch.lock-new.remote +' + test_expect_success 'ambiguous tracking info' ' # Set up a few remote repositories git init --bare --initial-branch=trunk src1 && diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh index cec2fe6876c2fe..b4f137acc3b6fe 100755 --- a/t/t2060-switch.sh +++ b/t/t2060-switch.sh @@ -146,6 +146,26 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' ' test_cmp_config "" --default "" branch.main2.merge ' +test_expect_success 'switch -c completes when tracking config cannot be written' ' + test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" && + test_when_finished "git branch -D lock-new" && + test_when_finished "git switch main" && + test_config remote.lockremote.url . && + test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" && + git update-ref refs/remotes/lockremote/lockbranch first-branch && + test_when_finished "rm -f .git/config.lock" && + >.git/config.lock && + git -c core.configLockTimeout=0 \ + switch -c lock-new lockremote/lockbranch 2>err && + test_grep "unable to write upstream branch configuration" err && + echo refs/heads/lock-new >expect && + git symbolic-ref HEAD >actual && + test_cmp expect actual && + git diff --cached --exit-code && + rm -f .git/config.lock && + test_must_fail git config branch.lock-new.remote +' + test_expect_success 'switch back when temporarily detached and checked out elsewhere ' ' test_when_finished " git worktree remove wt1 ||: diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cdb6c6a6342a0c..ca1a1c7f867eb3 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1080,6 +1080,19 @@ test_expect_success '--set-upstream-to fails on locked config' ' test_grep "could not lock config file .git/config" err ' +test_expect_success 'branch with start point fails when tracking config cannot be written' ' + test_when_finished "git branch -D lock-new || :" && + test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" && + test_config remote.lockremote.url . && + test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" && + git update-ref refs/remotes/lockremote/lockbranch HEAD && + test_when_finished "rm -f .git/config.lock" && + >.git/config.lock && + test_must_fail git -c core.configLockTimeout=0 \ + branch lock-new lockremote/lockbranch 2>err && + test_grep "unable to write upstream branch configuration" err +' + test_expect_success 'use --set-upstream-to modify HEAD' ' test_config branch.main.remote foo && test_config branch.main.merge foo && From d84678b8cfc384e79671d1039361aa43e6467830 Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:54 +0000 Subject: [PATCH 23/37] Documentation: describe connectivity checking Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check. Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- Documentation/Makefile | 1 + .../technical/connectivity-check.adoc | 112 ++++++++++++++++++ Documentation/technical/meson.build | 1 + 3 files changed, 114 insertions(+) create mode 100644 Documentation/technical/connectivity-check.adoc diff --git a/Documentation/Makefile b/Documentation/Makefile index f8dea4b3953250..1747f4a00be120 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -122,6 +122,7 @@ TECH_DOCS += technical/bitmap-format TECH_DOCS += technical/build-systems TECH_DOCS += technical/bundle-uri TECH_DOCS += technical/commit-graph +TECH_DOCS += technical/connectivity-check TECH_DOCS += technical/directory-rename-detection TECH_DOCS += technical/hash-function-transition TECH_DOCS += technical/large-object-promisors diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc new file mode 100644 index 00000000000000..a112d5be3e1663 --- /dev/null +++ b/Documentation/technical/connectivity-check.adoc @@ -0,0 +1,112 @@ +Connectivity checking +===================== + +After receiving new objects via fetch, push (receive-pack), clone, +or bundle, Git verifies that the new reference tips do not leave +the repository in a state where reachable objects are missing. +This verification is called the connectivity check. + +Connectivity invariant +---------------------- + +A repository is connected when every object reachable from its +references is available locally (with exceptions noted below). +The reverse does not hold: objects that exist in the object +database but are not reachable from any reference may be +incomplete or lack a connected closure. + +The connectivity check maintains this invariant when references +are updated. It trusts that all objects reachable from existing +reference tips are already fully connected and verifies that the +new reference tips do not introduce references to unavailable +objects. Verification is permitted to stop when it reaches +objects already reachable from existing references, since their +closure is already connected. These trusted references include +local references and references from alternate object stores. + +Without this check, a truncated or corrupted transfer could leave +a repository in a state where later history walks encounter +missing objects. + +Exceptions +~~~~~~~~~~ + +Gitlink entries (submodule references) are excluded from +connectivity checking. Their target objects belong to a separate +repository. + +In partial clones, objects promised by a promisor remote are +accepted as connected without requiring local existence. The +check excludes promisor objects from traversal so that it does +not trigger on-demand fetches for them. + +Full connectivity check +----------------------- + +The full connectivity check runs a `rev-list` subprocess, feeding +the new reference tips via stdin. A normal invocation is roughly: + + git rev-list --objects --stdin --not --all --quiet + --alternate-refs [--exclude-promisor-objects] + +In a partial clone, objects received from a promisor remote do +not need to be checked: anything they reference is promised by +that remote and can be fetched on demand. So if every new tip is +found in a promisor packfile, `rev-list` is not run at all. +Otherwise it runs with `--exclude-promisor-objects`, so that the +traversal stops at promisor objects instead of fetching them. + +Consider the following graph after a fetch, where all reference +tips point directly to commits. For simplicity, only local +references appear on the already-connected side; alternate refs +play the same role. N3 is a merge commit: + + /-------------L2 + / + C1---B1---C2---B2-----L1 + \ \ + N1 N3---T2 + \ / + N2-----------T1 + + L1, L2: local refs + T1, T2: incoming tips (new refs) + N1, N2, N3: incoming commits (N3 is a merge) + B1, B2: boundary commits (already connected) + C1, C2: already connected (but not boundary) + +The incoming set is the commits reachable from the incoming +tips but not from the already-connected side. Boundary commits +are the already-connected commits at the edge of that set. Here +B1 is an ancestor of B2, which happens when incoming branches +fork at different depths in the existing history. + +The `--not --all` arguments establish the trust boundary: the +incoming set is the commits reachable from the new tips but not +from any existing reference. + +The traversal then walks the trees of the boundary commits and +marks those objects uninteresting. Trees of incoming commits +are then walked, stopping at objects already marked as +uninteresting. The cost of marking boundary trees uninteresting +dominates when the boundary commits have large trees, even when +the incoming changes are small. + +Deepening fetches +~~~~~~~~~~~~~~~~~ + +For deepening fetches (where the shallow boundary moves), the +full check omits `--not --all`. There is no existing-reference +boundary at which the walk can stop. Instead, traversal follows +the effective shallow boundary supplied for the deepened +repository. The new content may be below the old shallow +boundary even when the tips themselves have not changed. + +Non-commit tips +~~~~~~~~~~~~~~~ + +When a new reference points to a non-commit object, such as a +tag, tree, or blob, that object is not part of the commit walk. +These non-commit tips are verified during the tree and blob +traversal that follows the commit walk within the same rev-list +invocation. diff --git a/Documentation/technical/meson.build b/Documentation/technical/meson.build index 9ce11d5e484d9c..c439a7a072d395 100644 --- a/Documentation/technical/meson.build +++ b/Documentation/technical/meson.build @@ -11,6 +11,7 @@ articles = [ 'build-systems.adoc', 'bundle-uri.adoc', 'commit-graph.adoc', + 'connectivity-check.adoc', 'directory-rename-detection.adoc', 'hash-function-transition.adoc', 'large-object-promisors.adoc', From 97998dcfd1d04def68cc9d06f79904937341ce4a Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:55 +0000 Subject: [PATCH 24/37] p5413: add perf tests for connectivity check Add benchmarks for the rev-list connectivity check. Each fixture varies one dimension: total tree size, incoming commit count, or files changed per commit. Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- ...enerate-repo-p5413-connectivity-check.perl | 44 ++++++++++ t/perf/p5413-connectivity-check.sh | 85 +++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 t/perf/generate-repo-p5413-connectivity-check.perl create mode 100755 t/perf/p5413-connectivity-check.sh diff --git a/t/perf/generate-repo-p5413-connectivity-check.perl b/t/perf/generate-repo-p5413-connectivity-check.perl new file mode 100644 index 00000000000000..bfa25f60fceac5 --- /dev/null +++ b/t/perf/generate-repo-p5413-connectivity-check.perl @@ -0,0 +1,44 @@ +#!/usr/bin/perl +# +# Generate a fast-import stream for p5413 connectivity check benchmarks. +# +# Usage: generate-repo-p5413-connectivity-check.perl +# [] [] +# +# Creates one initial commit with dirs*files_per_dir files, then +# additional commits each modifying +# files in directories chosen round-robin from 1... + +use strict; +use warnings; + +my ($nd, $nf, $nc, $hot, $fpc) = @ARGV; +$hot = $nd if !$hot || $hot > $nd; +$fpc = 1 if !$fpc; + +sub data { + printf "data %d\n%s\n", length($_[0]), $_[0]; +} + +# Initial tree: one commit with nd*nf files. +printf "commit refs/heads/main\n"; +printf "committer perf now\n"; +data("initial"); +for my $d (1..$nd) { + for my $f (1..$nf) { + printf "M 100644 inline d-%04d/f-%03d\n", $d, $f; + data(sprintf "%03d%03d", $d, $f); + } +} + +# Subsequent commits (auto-chained by fast-import). +for my $i (1..$nc) { + printf "commit refs/heads/main\n"; + printf "committer perf now\n"; + data(sprintf "change-%03d", $i); + for my $j (0..$fpc-1) { + my $d = (($i + $j) % $hot) + 1; + printf "M 100644 inline d-%04d/f-001\n", $d; + data(sprintf "c%d-%d", $i, $j); + } +} diff --git a/t/perf/p5413-connectivity-check.sh b/t/perf/p5413-connectivity-check.sh new file mode 100755 index 00000000000000..9e968d01067f67 --- /dev/null +++ b/t/perf/p5413-connectivity-check.sh @@ -0,0 +1,85 @@ +#!/bin/sh + +test_description='performance of connectivity check + +Benchmark the rev-list connectivity check, avoiding pack transfer +noise. + +Each repository has a flat tree of many directories with 100 files +in each. Three axes are scaled independently: tree size, commit +count, and files changed per commit.' + +. ./perf-lib.sh + +test_perf_fresh_repo + +generate="$TEST_DIRECTORY/perf/generate-repo-p5413-connectivity-check.perl" + +# $1=dirs $2=files_per_dir $3=commits $4=hot_dirs (optional, default=all) +# $5=files_per_commit (optional, default=1) +test_perf_conn () { + local nd="$1" nf="$2" nc="$3" hot="${4:-$1}" fpc="${5:-1}" + local total=$(($nd * $nf)) + local name="repo-${nd}d-${nf}f-${nc}c-${hot}h-${fpc}fpc" + local label="${total} files, ${nc} commits" + if test "$hot" -lt "$nd" + then + label="$label (${hot} hot dirs)" + fi + if test "$fpc" -gt 1 + then + label="$label (${fpc} files/commit)" + fi + + test_expect_success PERL "setup $label" ' + if test -d '"$name"' + then + true + else + git init '"$name"' && + "$PERL_PATH" '"$generate"' '"$nd"' '"$nf"' '"$nc"' '"$hot"' '"$fpc"' | + git -C '"$name"' fast-import --date-format=now --quiet && + ( + cd '"$name"' && + git rev-parse main~'"$nc"' >../'"${name}"'_old && + git rev-parse main >../'"${name}"'_new && + git update-ref refs/heads/main \ + $(cat ../'"${name}"'_old) && + git repack -ad && + git config gc.auto 0 + ) + fi + ' + + test_perf "$label" --prereq PERL ' + cat '"${name}"'_new | + git -C '"$name"' rev-list \ + --objects --stdin --not --all --quiet \ + --exclude-promisor-objects + ' +} + +# Scaling tree size (10 commits, 10 files/commit). +test_perf_conn 50 100 10 50 10 +test_perf_conn 500 100 10 500 10 +test_perf_conn 2000 100 10 2000 10 +test_perf_conn 8000 100 10 8000 10 + +# Scaling commit count (200K files, 10 files/commit). +test_perf_conn 2000 100 1 2000 10 +test_perf_conn 2000 100 10 2000 10 +test_perf_conn 2000 100 100 2000 10 +test_perf_conn 2000 100 500 2000 10 +test_perf_conn 2000 100 3000 2000 10 +test_perf_conn 2000 100 5000 2000 10 +test_perf_conn 2000 100 10000 2000 10 + +# Scaling files per commit (200K files, 10 commits). +test_perf_conn 2000 100 10 2000 1 +test_perf_conn 2000 100 10 2000 10 +test_perf_conn 2000 100 10 2000 100 +test_perf_conn 2000 100 10 2000 500 +test_perf_conn 2000 100 10 2000 1000 +test_perf_conn 2000 100 10 2000 2000 + +test_done From d6d2d8557c327170a2d4d14896f929f755dfa213 Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:56 +0000 Subject: [PATCH 25/37] connected: drop dependency on `the_repository` The next commits will need to use the repository to read configuration in check_connected(). Rather than accessing `the_repository`, refactor the code to instead require the caller to pass in a repository so that we can remove `USE_THE_REPOSITORY_VARIABLE`. Suggested-by: Junio C Hamano Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- builtin/clone.c | 2 +- builtin/fetch.c | 5 +++-- builtin/receive-pack.c | 12 ++++++++---- bundle.c | 2 +- connected.c | 22 +++++++++++----------- connected.h | 4 +++- fetch-pack.c | 3 ++- 7 files changed, 29 insertions(+), 21 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 44037dd428e402..1532361ea0ee7c 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -572,7 +572,7 @@ static void update_remote_refs(const struct ref *refs, opt.transport = transport; opt.progress = transport->progress; - if (check_connected(iterate_ref_map, &rm, &opt)) + if (check_connected(the_repository, iterate_ref_map, &rm, &opt)) die(_("remote did not send all necessary objects")); } diff --git a/builtin/fetch.c b/builtin/fetch.c index b2decc6cfdb8ae..0ef7fe92f11c36 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1250,7 +1250,8 @@ static int store_updated_refs(struct display_state *display_state, opt.exclude_hidden_refs_section = "fetch"; rm = ref_map; - if (check_connected(iterate_ref_map, &rm, &opt)) { + if (check_connected(the_repository, iterate_ref_map, + &rm, &opt)) { rc = error(_("%s did not send all necessary objects"), display_state->url); goto abort; @@ -1422,7 +1423,7 @@ static int check_exist_and_connected(struct ref *ref_map) opt.quiet = 1; opt.exclude_hidden_refs_section = "fetch"; - return check_connected(iterate_ref_map, &rm, &opt); + return check_connected(the_repository, iterate_ref_map, &rm, &opt); } static int fetch_and_consume_refs(struct display_state *display_state, diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 1d5b050beb4e72..4f8f58950a9be1 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1385,7 +1385,8 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si) oid_array_append(&extra, &si->shallow->oid[i]); setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra); - if (check_connected(command_singleton_iterator, cmd, &opt)) { + if (check_connected(the_repository, command_singleton_iterator, + cmd, &opt)) { rollback_shallow_file(the_repository, &shallow_lock); oid_array_clear(&extra); return -1; @@ -1827,8 +1828,9 @@ static void set_connectivity_errors(struct command *commands, odb_transaction_env(transaction, &env); opt.env = env.v; - if (!check_connected(command_singleton_iterator, &singleton, - &opt)) + if (!check_connected(the_repository, + command_singleton_iterator, + &singleton, &opt)) continue; cmd->error_string = "missing necessary objects"; @@ -2090,7 +2092,9 @@ static void execute_commands(struct command *commands, opt.env = env.v; opt.exclude_hidden_refs_section = "receive"; - if (check_connected(iterate_receive_command_list, &data, &opt)) + if (check_connected(the_repository, + iterate_receive_command_list, + &data, &opt)) set_connectivity_errors(commands, si, transaction); if (use_sideband) diff --git a/bundle.c b/bundle.c index f55a521b2a1f12..0a68e856f57500 100644 --- a/bundle.c +++ b/bundle.c @@ -259,7 +259,7 @@ int verify_bundle(struct repository *r, if (ret) goto cleanup; - if ((ret = check_connected(iterate_ref_map, &iter, &opts))) + if ((ret = check_connected(r, iterate_ref_map, &iter, &opts))) error(_("some prerequisite commits exist in the object store, " "but are not connected to the repository's history")); diff --git a/connected.c b/connected.c index 929b9bd28d6fab..34045f1032595b 100644 --- a/connected.c +++ b/connected.c @@ -1,5 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE - #include "git-compat-util.h" #include "gettext.h" #include "hex.h" @@ -35,23 +33,24 @@ static int promised_object_cb(const struct object_id *oid UNUSED, * objects in non-promisor packs, in which case we'll have to fall back to the * rev-list-based connectivity checks. Returns a negative error code on error. */ -static int check_connected_promisor(oid_iterate_fn fn, +static int check_connected_promisor(struct repository *repo, + oid_iterate_fn fn, void *cb_data, const struct object_id **oid) { struct odb_for_each_object_options opts = { .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY, - .prefix_hex_len = the_repository->hash_algo->hexsz, + .prefix_hex_len = repo->hash_algo->hexsz, }; int err; - odb_reprepare(the_repository->objects); + odb_reprepare(repo->objects); do { bool found = false; opts.prefix = *oid; - err = odb_for_each_object_ext(the_repository->objects, NULL, + err = odb_for_each_object_ext(repo->objects, NULL, promised_object_cb, &found, &opts); if (err < 0) return err; @@ -78,7 +77,8 @@ static int check_connected_promisor(oid_iterate_fn fn, * * Returns 0 if everything is connected, non-zero otherwise. */ -int check_connected(oid_iterate_fn fn, void *cb_data, +int check_connected(struct repository *repo, + oid_iterate_fn fn, void *cb_data, struct check_connected_options *opt) { struct child_process rev_list = CHILD_PROCESS_INIT; @@ -101,8 +101,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data, return err; } - if (repo_has_promisor_remote(the_repository)) { - err = check_connected_promisor(fn, cb_data, &oid); + if (repo_has_promisor_remote(repo)) { + err = check_connected_promisor(repo, fn, cb_data, &oid); if (err) { if (opt->err_fd) close(opt->err_fd); @@ -119,7 +119,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, strvec_push(&rev_list.args,"rev-list"); strvec_push(&rev_list.args, "--objects"); strvec_push(&rev_list.args, "--stdin"); - if (repo_has_promisor_remote(the_repository)) + if (repo_has_promisor_remote(repo)) strvec_push(&rev_list.args, "--exclude-promisor-objects"); if (!opt->is_deepening_fetch) { strvec_push(&rev_list.args, "--not"); @@ -160,7 +160,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string, base_len); strbuf_addstr(&idx_file, ".idx"); - new_pack = add_packed_git(the_repository, idx_file.buf, + new_pack = add_packed_git(repo, idx_file.buf, idx_file.len, 1); strbuf_release(&idx_file); } diff --git a/connected.h b/connected.h index 16b2c84f2e35fc..0624965baa7fb1 100644 --- a/connected.h +++ b/connected.h @@ -2,6 +2,7 @@ #define CONNECTED_H struct object_id; +struct repository; struct transport; /* @@ -66,7 +67,8 @@ struct check_connected_options { * * If "opt" is NULL, behaves as if CHECK_CONNECTED_INIT was passed. */ -int check_connected(oid_iterate_fn fn, void *cb_data, +int check_connected(struct repository *repo, + oid_iterate_fn fn, void *cb_data, struct check_connected_options *opt); #endif /* CONNECTED_H */ diff --git a/fetch-pack.c b/fetch-pack.c index 30a4dd6119049d..2cf8f8d653fc53 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -2186,7 +2186,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args, opt.shallow_file = alternate_shallow_file; if (args->deepen) opt.is_deepening_fetch = 1; - if (check_connected(iterate_ref_map, &iterator, &opt)) { + if (check_connected(the_repository, iterate_ref_map, + &iterator, &opt)) { error(_("remote did not send all necessary objects")); free_refs(ref_cpy); ref_cpy = NULL; From 9e0207b50b3f1281d8fa72e255037683c0654dd0 Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:57 +0000 Subject: [PATCH 26/37] connected: add transfer.connectivityCheck config Add the transfer.connectivityCheck configuration option to choose the algorithm used for the connectivity check. For now the only accepted value is "full", which is the existing behavior and the default. Unknown values fall back to "full" with a warning, so that configuration written for a newer version of Git does not break fetches and pushes; "full" is always a correct check. check_connected() now always tells its rev-list subprocess which algorithm to use with an internal --connectivity-check= option, which so far only accepts "full". Passing the mode explicitly means rev-list knows it is running a connectivity check, so later optimizations can be limited to that case. This prepares for an incremental check added in the next commit. Suggested-by: Patrick Steinhardt Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- Documentation/config/transfer.adoc | 16 ++++++++++++++++ Documentation/rev-list-options.adoc | 5 +++++ builtin/rev-list.c | 22 ++++++++++++++++++++++ connected.c | 26 ++++++++++++++++++++++++++ 4 files changed, 69 insertions(+) diff --git a/Documentation/config/transfer.adoc b/Documentation/config/transfer.adoc index f1ce50f4a6e6ba..811456f1bfad94 100644 --- a/Documentation/config/transfer.adoc +++ b/Documentation/config/transfer.adoc @@ -1,3 +1,19 @@ +transfer.connectivityCheck:: + Choose which algorithm to use for the connectivity check + performed during object transfer operations such as + linkgit:git-fetch[1] and linkgit:git-receive-pack[1]. + The connectivity check verifies that all objects reachable + from the incoming tips are available locally or, in a partial + clone, promised by a promisor remote. + The variants are as follows: ++ +-- +`full` (default);; + Walk the full object closure of the boundary commits. +-- ++ +Unknown values are treated as `full`, with a warning. + transfer.credentialsInUrl:: A configured URL can contain plaintext credentials in the form `://:@/`. You may want diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc index bd9f3456902c8f..3979a142a590ef 100644 --- a/Documentation/rev-list-options.adoc +++ b/Documentation/rev-list-options.adoc @@ -1102,6 +1102,11 @@ It is an error to use `--missing-only` without `--missing=print` or stronger than `--missing=allow-promisor` because it limits the traversal, rather than just silencing errors about missing objects. + +`--connectivity-check=full`:: + (For internal use only.) Used by `check_connected()` to + select the connectivity check algorithm; see + `transfer.connectivityCheck` in linkgit:git-config[1]. endif::git-rev-list[] `--no-walk[=(sorted|unsorted)]`:: diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 0faa833facfd4b..619855f4e44258 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -29,6 +29,23 @@ #include "quote.h" #include "strbuf.h" +enum connectivity_check_mode { + CONNECTIVITY_CHECK_FULL, +}; + +struct connectivity_check { + enum connectivity_check_mode mode; +}; + +static void parse_connectivity_check(const char *arg, + struct connectivity_check *check) +{ + if (!strcmp(arg, "full")) + check->mode = CONNECTIVITY_CHECK_FULL; + else + die(_("unknown --connectivity-check mode '%s'"), arg); +} + struct rev_list_info { struct rev_info *revs; int flags; @@ -727,6 +744,7 @@ int cmd_rev_list(int argc, int bisect_find_all = 0; int use_bitmap_index = 0; int filter_provided_objects = 0; + struct connectivity_check connectivity_check = { 0 }; const char *show_progress = NULL; int ret = 0; @@ -769,6 +787,8 @@ int cmd_rev_list(int argc, if (!strcmp(arg, "--exclude-promisor-objects")) { repo->fetch_if_missing = 0; revs.exclude_promisor_objects = 1; + } else if (skip_prefix(arg, "--connectivity-check=", &arg)) { + parse_connectivity_check(arg, &connectivity_check); } else if (skip_prefix(arg, "--missing=", &arg)) { parse_missing_action_value(repo, arg); } else if (!strcmp(arg, "--missing-only")) { @@ -848,6 +868,8 @@ int cmd_rev_list(int argc, if (!strcmp(arg, "--exclude-promisor-objects")) continue; /* already handled above */ + if (starts_with(arg, "--connectivity-check=")) + continue; /* already handled above */ if (skip_prefix(arg, "--missing=", &arg)) continue; /* already handled above */ diff --git a/connected.c b/connected.c index 34045f1032595b..0147b7d3b35716 100644 --- a/connected.c +++ b/connected.c @@ -1,4 +1,5 @@ #include "git-compat-util.h" +#include "config.h" #include "gettext.h" #include "hex.h" #include "odb.h" @@ -66,6 +67,27 @@ static int check_connected_promisor(struct repository *repo, return 1; } +/* + * Fills in the --connectivity-check mode to pass to rev-list. + */ +static void connectivity_check_mode(struct repository *repo, + struct strbuf *mode) +{ + const char *algorithm = NULL; + + if (repo_config_get_string_tmp(repo, + "transfer.connectivitycheck", + &algorithm)) + algorithm = "full"; + if (!strcasecmp(algorithm, "full")) + strbuf_addstr(mode, "full"); + else { + warning(_("unknown transfer.connectivityCheck algorithm '%s', " + "using 'full'"), algorithm); + strbuf_addstr(mode, "full"); + } +} + /* * If we feed all the commits we want to verify to this command * @@ -84,6 +106,7 @@ int check_connected(struct repository *repo, struct child_process rev_list = CHILD_PROCESS_INIT; FILE *rev_list_in; struct check_connected_options defaults = CHECK_CONNECTED_INIT; + struct strbuf mode = STRBUF_INIT; const struct object_id *oid; int err = 0; struct packed_git *new_pack = NULL; @@ -133,6 +156,9 @@ int check_connected(struct repository *repo, if (opt->progress) strvec_pushf(&rev_list.args, "--progress=%s", _("Checking connectivity")); + connectivity_check_mode(repo, &mode); + strvec_pushf(&rev_list.args, "--connectivity-check=%s", mode.buf); + strbuf_release(&mode); rev_list.git_cmd = 1; if (opt->env) From 43157175b549b7d72c3e79fb212769cc1f6acfa5 Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:58 +0000 Subject: [PATCH 27/37] connected: add incremental connectivity check via rev-list The full connectivity check uses rev-list to find commits reachable from the incoming tips but not from the already-connected side, then walks their object closure. Commit traversal stops at the connectivity boundary, but trees and blobs reachable from that boundary still need to be walked so they can be marked uninteresting, allocating a struct object for each one. On repositories where the boundary commits have large trees, the connectivity check for small incoming changes visits and tracks more objects than needed. Add an alternative connectivity check that verifies incoming commits incrementally against their parents. The verifier processes incoming commits with ancestors first and compares each new tree against its trusted parent trees. Entries already seen on the trusted side are skipped by OID, so unchanged subtrees need not remain at the same path to be recognized. Changed subtrees are recursively compared against same-path parent subtrees, and new subtrees without a comparison base are verified from scratch. See Documentation/technical/connectivity-check.adoc for the trust invariants and detailed algorithm. The incremental check runs inside rev-list, selected by the internal --connectivity-check option. After the normal revision walk identifies the incoming commits, the verifier consumes them before the usual object traversal; any pending non-commit tips are still handled by the existing traversal. Partial-clone semantics are preserved: objects promised by a promisor remote are accepted as connected, and on-demand fetching is prevented by excluding promisor objects from traversal. A new transfer.connectivityCheck value "incremental" selects the new mode, with "full" remaining the default. Deepening fetches fall back to the full check because they do not have the normal existing-reference boundary, and traversal instead follows the effective shallow boundary. Clones and pushes into an empty repository also use the full check. Without negative revisions there is no trusted boundary, and the incremental check would have to verify all of history from scratch like the full check does, with extra per-commit overhead on top. On git.git, checking 70K commits without a boundary takes 5.3s with the full check and 9.2s with the incremental one. p5413 results (best of 3), scaling one dimension at a time. Each fixture is a repository with a flat tree of many directories containing 100 files each. The incoming set is a chain of commits on top of the existing history, with each commit changing files in different directories round-robin. The three axes vary the total repository tree size, the number of incoming commits, and the number of files changed per incoming commit. Scaling tree size (10 incoming commits, 10 files/commit): files full incr. full/incr 5K 0.01s 0.01s 1.0x 50K 0.04s 0.01s 4.0x 200K 0.13s 0.01s 13.0x 800K 0.54s 0.03s 18.0x The full mode must walk the trees of boundary commits, which grows with overall tree size. Incremental still scans the root trees, but avoids descending into unchanged subtrees, so it grows much more slowly with repository size. Scaling incoming commit count (200K files, 10 files/commit): commits full incr. full/incr 1 0.13s 0.01s 13.0x 10 0.13s 0.01s 13.0x 100 0.16s 0.06s 2.7x 500 0.29s 0.25s 1.2x 3000 1.14s 1.44s 0.8x 5000 1.87s 2.39s 0.8x 10000 3.45s 5.13s 0.7x With many commits the per-commit overhead of scanning both the new and parent root trees accumulates and incremental becomes slower. Breakeven is between 500 and 3000 commits and the ratio stabilizes near 0.7x for this fixture. Scaling files per incoming commit (200K files, 10 commits): files/commit full incr. full/incr 1 0.13s 0.01s 13.0x 10 0.13s 0.01s 13.0x 100 0.15s 0.03s 5.0x 500 0.23s 0.13s 1.8x 1000 0.34s 0.27s 1.3x 2000 0.65s 0.56s 1.2x The advantage shrinks as more files change per commit, but incremental stays slightly faster even at 2000 files/commit, where every directory is touched. Selected peak RSS measurements from the same fixtures: case full incr. ratio 200K files, 10 commits 31 MB 12 MB 0.4x 800K files, 10 commits 110 MB 26 MB 0.2x 200K files, 5000 commits 155 MB 151 MB 1.0x Incremental uses less memory when it can prune enough of the boundary tree walk. In the long-history case the two modes visit similar object sets and memory converges. The regression cases in the CPU benchmarks are in wall-clock time rather than memory, primarily from scanning some trees more than once: a commit's tree is read as a candidate when the commit is verified, and again as a trusted parent tree when its children are verified. Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- Documentation/config/transfer.adoc | 8 + Documentation/rev-list-options.adoc | 2 +- .../technical/connectivity-check.adoc | 147 ++++ Makefile | 1 + builtin/rev-list.c | 27 + connected.c | 14 +- meson.build | 1 + t/meson.build | 1 + t/perf/p5413-connectivity-check.sh | 17 +- t/t5413-connectivity-check.sh | 774 ++++++++++++++++++ t/t6700-tree-depth.sh | 17 + tree-verify.c | 433 ++++++++++ tree-verify.h | 15 + 13 files changed, 1450 insertions(+), 7 deletions(-) create mode 100755 t/t5413-connectivity-check.sh create mode 100644 tree-verify.c create mode 100644 tree-verify.h diff --git a/Documentation/config/transfer.adoc b/Documentation/config/transfer.adoc index 811456f1bfad94..cba7597a584e0c 100644 --- a/Documentation/config/transfer.adoc +++ b/Documentation/config/transfer.adoc @@ -10,6 +10,14 @@ transfer.connectivityCheck:: -- `full` (default);; Walk the full object closure of the boundary commits. +`incremental`;; + Verify incoming commits by diffing their trees against parent + trees, recursively descending only into entries that differ. + The largest benefits occur when incoming commits change a + small fraction of a large tree closure, and it can be slower + than `full` when there are many incoming commits. + Falls back to `full` for clones, pushes into an empty + repository and deepening fetches. -- + Unknown values are treated as `full`, with a warning. diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc index 3979a142a590ef..2023ad5f754447 100644 --- a/Documentation/rev-list-options.adoc +++ b/Documentation/rev-list-options.adoc @@ -1103,7 +1103,7 @@ It is an error to use `--missing-only` without `--missing=print` or traversal, rather than just silencing errors about missing objects. -`--connectivity-check=full`:: +`--connectivity-check=(full|incremental)`:: (For internal use only.) Used by `check_connected()` to select the connectivity check algorithm; see `transfer.connectivityCheck` in linkgit:git-config[1]. diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc index a112d5be3e1663..8b7d619d4c8bc0 100644 --- a/Documentation/technical/connectivity-check.adoc +++ b/Documentation/technical/connectivity-check.adoc @@ -110,3 +110,150 @@ tag, tree, or blob, that object is not part of the commit walk. These non-commit tips are verified during the tree and blob traversal that follows the commit walk within the same rev-list invocation. + +Incremental connectivity check +------------------------------ + +The incremental mode, selected by +`transfer.connectivityCheck=incremental`, avoids traversing the +full tree walk of the boundary commits. Instead, it verifies +each incoming commit's tree against the already-trusted trees of +its parent commits. + +The same `--not --all` boundary as the full check identifies the +incoming commits. The incremental mode reuses this boundary but +replaces what happens afterwards: rather than walking boundary +trees to mark objects uninteresting, it compares each incoming +commit's tree against its parents' trees, processing commits in +topological order (ancestors first). This all happens inside +the same `rev-list` invocation, and the promisor fast path above +still runs first. + +Trust model +~~~~~~~~~~~ + +A tree is trusted when its transitive object closure is known to +be connected. + +Every parent of an incoming commit is either on the +already-connected side of the `--not --all` boundary (a boundary +parent whose tree is trusted by definition) or is an earlier +incoming commit that was already verified due to the topological +processing order. In both cases the parent's tree is trusted, so +all parent trees can be unconditionally used as comparison bases. + +Once an incoming commit's tree has been verified, it is trusted +and can serve as a comparison base for later descendants. This +gives an inductive correctness argument: the base case is the +trust boundary established by `--not --all`, and the inductive +step is that verifying a commit extends the trusted set. + +Tree states +~~~~~~~~~~~ + +The verifier tracks tree OIDs in three states: + +untrusted:: + The tree has not yet been established as connected. This + is the implicit state of an OID not present in the state + map. + +trusted:: + The tree is known to have a connected transitive closure, + but its direct entries have not yet been published into the + verifier's trusted object sets. + +expanded:: + The tree is trusted and its direct non-gitlink entries + have also been published into the trusted object sets. + +State transitions are monotonic: a tree may move from untrusted +to trusted to expanded, but never backwards. An untrusted tree +that is successfully verified goes directly to expanded. + +Blobs require only trusted/untrusted state: a blob becomes +trusted when it is found in a trusted tree or when its existence +and type have been verified directly. + +The trusted/expanded distinction is an optimization. An expanded +parent does not need to be reread merely to publish entries that +are already trusted, though it may still be read when same-path +subtree bases are needed for recursive comparison. + +Algorithm +~~~~~~~~~ + +At a high level: + + verify(commits): + sort topologically (ancestors first) + for each commit: + mark parent root trees as trusted + verify_tree(commit.tree, parent root trees) + + verify_tree(tree, base_trees): + if tree already trusted: return + read tree, collect entries not already trusted + for each available base tree: + publish its entries as trusted + record same-path subtrees as bases + for each collected entry: + if now trusted: skip + if blob: verify blob connectivity and type + if tree: verify_tree(entry, its recorded bases) + mark tree as expanded + +The important ordering within `verify_tree` is that entries from +the new tree are collected before the base trees are scanned, but +are processed only afterwards. Trust learned from any base can +therefore eliminate work before recursive verification begins. + +Same-path parent subtrees are passed down as comparison bases +during recursive descent. If no comparison base is available, +the new subtree is verified from scratch. + +Example +~~~~~~~ + +Consider a commit that changes one file under `lib/` and moves an +unchanged subtree from `src/` to `dev/`: + + Parent tree New tree + +-- src/ (aaa) +-- dev/ (aaa) + +-- lib/ (bbb) +-- lib/ (ccc) + +-- foo.c (ddd) +-- foo.c (ddd) + +-- bar.c (eee) +-- bar.c (fff) + +Scanning the parent makes `aaa` trusted even though it moved from +`src/` to `dev/`, so that subtree is skipped. The changed `ccc` +subtree is compared against its same-path parent `bbb`; scanning +`bbb` makes `ddd` and `eee` trusted, leaving only the new `fff` +blob to be checked. + +This illustrates two properties: + +* Trust is OID-based rather than path-based. An unchanged subtree + is recognized after a move. + +* Same-path parent subtrees provide recursive comparison bases. + These bases improve pruning efficiency but are not required for + correctness; a new subtree can always be verified from scratch. + +Multiple parents +~~~~~~~~~~~~~~~~ + +For a merge commit, root trees from all parents are comparison +bases. When several parents contain a same-path subtree, each +matching subtree is collected as a recursive comparison base. + +Entries published from any trusted parent become globally trusted, +so a matching tree or blob entry present in any parent can be +skipped while verifying the merge tree. + +Missing comparison bases +~~~~~~~~~~~~~~~~~~~~~~~~ + +A promised base tree may not be locally available for comparison. +In that case the verifier skips that base and verifies the new +subtree without it. The missing comparison can reduce pruning but +does not affect correctness. diff --git a/Makefile b/Makefile index a96be506b59f2d..fa42e295e22b13 100644 --- a/Makefile +++ b/Makefile @@ -1339,6 +1339,7 @@ LIB_OBJS += trailer.o LIB_OBJS += transport-helper.o LIB_OBJS += transport.o LIB_OBJS += tree-diff.o +LIB_OBJS += tree-verify.o LIB_OBJS += tree-walk.o LIB_OBJS += tree.o LIB_OBJS += unpack-trees.o diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 619855f4e44258..c07ad6e30ccd86 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -28,9 +28,11 @@ #include "commit-reach.h" #include "quote.h" #include "strbuf.h" +#include "tree-verify.h" enum connectivity_check_mode { CONNECTIVITY_CHECK_FULL, + CONNECTIVITY_CHECK_INCREMENTAL, }; struct connectivity_check { @@ -42,6 +44,8 @@ static void parse_connectivity_check(const char *arg, { if (!strcmp(arg, "full")) check->mode = CONNECTIVITY_CHECK_FULL; + else if (!strcmp(arg, "incremental")) + check->mode = CONNECTIVITY_CHECK_INCREMENTAL; else die(_("unknown --connectivity-check mode '%s'"), arg); } @@ -991,6 +995,29 @@ int cmd_rev_list(int argc, prepare_maximal_independent(&revs); + /* + * Without a boundary of negative revisions, as in a clone or a + * push into an empty repository, every incoming commit is new. + * The incremental check would still work, but it would verify + * the whole object graph like the full check, with extra + * per-commit overhead on top, so use the full check. + */ + if (!revs.limited) + connectivity_check.mode = CONNECTIVITY_CHECK_FULL; + + if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) { + struct commit *commit; + struct commit_list *new_commits = NULL; + + while ((commit = get_revision(&revs)) != NULL) + commit_list_insert(commit, &new_commits); + + if (verify_commits_incremental(repo, &new_commits, + revs.exclude_promisor_objects)) + die(_("connectivity check failed")); + commit_list_free(new_commits); + } + if (revs.tree_objects) mark_edges_uninteresting(&revs, show_edge, 0); diff --git a/connected.c b/connected.c index 0147b7d3b35716..c0329874100adf 100644 --- a/connected.c +++ b/connected.c @@ -71,16 +71,28 @@ static int check_connected_promisor(struct repository *repo, * Fills in the --connectivity-check mode to pass to rev-list. */ static void connectivity_check_mode(struct repository *repo, + struct check_connected_options *opt, struct strbuf *mode) { const char *algorithm = NULL; + /* + * Deepening fetches do not have the usual boundary of + * existing references, so use the full check for them. + */ + if (opt->is_deepening_fetch) { + strbuf_addstr(mode, "full"); + return; + } + if (repo_config_get_string_tmp(repo, "transfer.connectivitycheck", &algorithm)) algorithm = "full"; if (!strcasecmp(algorithm, "full")) strbuf_addstr(mode, "full"); + else if (!strcasecmp(algorithm, "incremental")) + strbuf_addstr(mode, "incremental"); else { warning(_("unknown transfer.connectivityCheck algorithm '%s', " "using 'full'"), algorithm); @@ -156,7 +168,7 @@ int check_connected(struct repository *repo, if (opt->progress) strvec_pushf(&rev_list.args, "--progress=%s", _("Checking connectivity")); - connectivity_check_mode(repo, &mode); + connectivity_check_mode(repo, opt, &mode); strvec_pushf(&rev_list.args, "--connectivity-check=%s", mode.buf); strbuf_release(&mode); diff --git a/meson.build b/meson.build index 0a95d90d211fd6..d94dedc26cc0c6 100644 --- a/meson.build +++ b/meson.build @@ -562,6 +562,7 @@ libgit_sources = [ 'transport-helper.c', 'transport.c', 'tree-diff.c', + 'tree-verify.c', 'tree-walk.c', 'tree.c', 'unpack-trees.c', diff --git a/t/meson.build b/t/meson.build index f65eb04684b669..b23b4c90b0427b 100644 --- a/t/meson.build +++ b/t/meson.build @@ -654,6 +654,7 @@ integration_tests = [ 't5410-receive-pack.sh', 't5411-proc-receive-hook.sh', 't5412-receive-report-hook.sh', + 't5413-connectivity-check.sh', 't5500-fetch-pack.sh', 't5501-fetch-push-alternates.sh', 't5502-quickfetch.sh', diff --git a/t/perf/p5413-connectivity-check.sh b/t/perf/p5413-connectivity-check.sh index 9e968d01067f67..250140a79abf63 100755 --- a/t/perf/p5413-connectivity-check.sh +++ b/t/perf/p5413-connectivity-check.sh @@ -1,9 +1,9 @@ #!/bin/sh -test_description='performance of connectivity check +test_description='performance of connectivity check modes -Benchmark the rev-list connectivity check, avoiding pack transfer -noise. +Compare the default and incremental rev-list connectivity modes +directly, avoiding pack transfer noise. Each repository has a flat tree of many directories with 100 files in each. Three axes are scaled independently: tree size, commit @@ -51,9 +51,16 @@ test_perf_conn () { fi ' - test_perf "$label" --prereq PERL ' + test_perf "$label (full)" --prereq PERL ' cat '"${name}"'_new | - git -C '"$name"' rev-list \ + git -C '"$name"' rev-list --connectivity-check=full \ + --objects --stdin --not --all --quiet \ + --exclude-promisor-objects + ' + + test_perf "$label (incremental)" --prereq PERL ' + cat '"${name}"'_new | + git -C '"$name"' rev-list --connectivity-check=incremental \ --objects --stdin --not --all --quiet \ --exclude-promisor-objects ' diff --git a/t/t5413-connectivity-check.sh b/t/t5413-connectivity-check.sh new file mode 100755 index 00000000000000..109ad17cfffcaf --- /dev/null +++ b/t/t5413-connectivity-check.sh @@ -0,0 +1,774 @@ +#!/bin/sh + +test_description='connectivity check (transfer.connectivityCheck)' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +test_oid_cache <<-\EOF +missing sha1:0000000000000000000000000000000000000001 +missing sha256:0000000000000000000000000000000000000000000000000000000000000001 +EOF + +set_connectivity_check () { + git -C "$1" config transfer.connectivityCheck "$2" +} + +# Run a connectivity check via rev-list directly. Uses $mode +# (set by the enclosing for-loop) to choose full or incremental. +check_connected () { + printf '%s\n' "$@" | + git rev-list --connectivity-check="$mode" \ + --objects --stdin --not --all --quiet \ + --exclude-promisor-objects +} + +# Run git with a temporary index, leaving the real index untouched. +tmpgit () { + GIT_INDEX_FILE=.git/tmp-idx git "$@" +} + +# Create a commit with one file changed, without modifying HEAD, +# index, or worktree. Prints the new commit OID on stdout. +# Usage: commit_with_change +commit_with_change () { + new_blob=$(echo "$3" | git hash-object -w --stdin) && + tmpgit read-tree "$1" && + tmpgit update-index --replace \ + --cacheinfo "100644,$new_blob,$2" && + new_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + git commit-tree "$new_tree" -p "$1" -m "modify $2" +} + +# Check OIDs and optionally verify trace2 counts. +# Usage: check_connected_trace ... +# An empty string for or skips that assertion. +check_connected_trace () { + trace_file=$1 trees=$2 blobs=$3 && + shift 3 && + test_env GIT_TRACE2_EVENT="$(pwd)/$trace_file" \ + check_connected "$@" && + if test "$mode" != incremental + then + return + fi && + if test -n "$trees" + then + test_trace2_data_singular connectivity trees_loaded "$trees" \ + <"$trace_file" + fi && + if test -n "$blobs" + then + test_trace2_data_singular connectivity blobs_checked "$blobs" \ + <"$trace_file" + fi +} + +# Shared setup: a repo with several root-level files and nested dirs. +# The unchanged/ subtree (10 dirs x 10 files = 100 blobs, 11 trees) +# acts as a canary: any test asserting small tree/blob counts would +# fail dramatically if incremental accidentally walked into it. + +test_expect_success 'setup main repo' ' + git init main-repo && + ( + cd main-repo && + for i in $(test_seq 1 5) + do + echo "file $i" >"file-$i.txt" || return 1 + done && + git add file-*.txt && + git commit -m "initial" && + + mkdir -p a/b/c && + echo deep >a/b/c/deep.txt && + echo other >a/other.txt && + git add a/b/c/deep.txt a/other.txt && + git commit -m "add nested dirs" && + + for i in $(test_seq 1 10) + do + d="unchanged/dir-$i" && + mkdir -p "$d" && + for j in $(test_seq 1 10) + do + echo "$i $j" >"$d/file-$j.txt" || return 1 + done + done && + git add unchanged/ && + git commit -m "add unchanged canary subtree" + ) +' + +test_expect_success 'setup replacement object repo' ' + git init replace-test && + ( + cd replace-test && + + test_commit --no-tag original file.txt && + original=$(git rev-parse HEAD) && + orig_blob=$(git rev-parse HEAD:file.txt) && + + # Orphan replacement commit with a different tree + replacement_tree=$(echo replaced | git hash-object -w --stdin | + xargs -I{} git mktree <<-EOF + 100644 blob {} file.txt + EOF + ) && + replacement=$(git commit-tree -m "replacement" \ + "$replacement_tree") && + + git replace "$original" "$replacement" && + + # Remove the original blob so only the replacement + # tree is complete. + rm .git/objects/$(test_oid_to_path "$orig_blob") && + + # Drop branch and HEAD so --not --all does not + # exclude the original commit. + git update-ref -d refs/heads/main && + git update-ref -d HEAD && + + echo "$original" >.git/test-oid && + echo "$orig_blob" >.git/test-blob + ) +' + +missing_oid=$(test_oid missing) +original_oid=$(cat replace-test/.git/test-oid) +orig_blob_oid=$(cat replace-test/.git/test-blob) + +for mode in full incremental +do + +# The full traversal reports a missing tree as "bad tree object". +case "$mode" in +full) + missing_tree="bad tree object" + ;; +incremental) + missing_tree="missing tree object" + ;; +esac + +# Corruption detection: craft broken object graphs and verify detection. +# All tests use main-repo without modifying its refs or worktree. + +test_expect_success "$mode: rejects commit with missing blob" ' + ( + cd main-repo && + bad_tree=$(printf "100644 blob ${missing_oid}\tfile.txt\n" | + git mktree --missing) && + bad_commit=$(git commit-tree "$bad_tree" -p HEAD -m "bad") && + test_expect_code 128 check_connected "$bad_commit" 2>err && + test_grep "missing blob object.*$missing_oid" err + ) +' + +test_expect_success "$mode: rejects commit with missing subtree" ' + ( + cd main-repo && + bad_tree=$(printf "40000 tree ${missing_oid}\tdir\n" | + git mktree --missing) && + bad_commit=$(git commit-tree "$bad_tree" -p HEAD -m "bad") && + test_expect_code 128 check_connected "$bad_commit" 2>err && + test_grep "$missing_tree $missing_oid" err + ) +' + +test_expect_success "$mode: verifies direct tree tip" ' + ( + cd main-repo && + bad_tree=$(printf "100644 blob ${missing_oid}\tfile.txt\n" | + git mktree --missing) && + test_expect_code 128 check_connected "$bad_tree" 2>err && + test_grep "missing blob object.*$missing_oid" err + ) +' + +test_expect_success "$mode: verifies direct blob tip" ' + ( + cd main-repo && + blob_oid=$(echo "hello" | git hash-object -w --stdin) && + check_connected "$blob_oid" + ) +' + +test_expect_success "$mode: rejects missing direct blob tip" ' + ( + cd main-repo && + test_expect_code 128 check_connected \ + "$missing_oid" 2>err + ) +' + +test_expect_success PERL_TEST_HELPERS \ + "$mode: rejects blob OID reused as tree entry" ' + ( + cd main-repo && + + blob_oid=$(git rev-parse HEAD:file-1.txt) && + bin_oid=$(echo "$blob_oid" | hex2oct) && + + bad_tree=$(printf "40000 subdir\0$bin_oid" | + git hash-object -t tree -w --stdin) && + bad_commit=$(git commit-tree -p HEAD -m "child" "$bad_tree") && + + test_expect_code 128 check_connected "$bad_commit" 2>err && + test_grep "not a tree" err + ) +' + +test_expect_success PERL_TEST_HELPERS \ + "$mode: rejects tree OID reused as blob entry" ' + ( + cd main-repo && + + tree_oid=$(git rev-parse HEAD:a) && + bin_oid=$(echo "$tree_oid" | hex2oct) && + + bad_tree=$(printf "100644 fakefile\0$bin_oid" | + git hash-object -t tree -w --stdin) && + bad_commit=$(git commit-tree -p HEAD -m "child" "$bad_tree") && + + test_expect_code 128 check_connected "$bad_commit" 2>err && + test_grep "not a blob" err + ) +' + +test_expect_success "$mode: checks multiple tips" ' + ( + cd main-repo && + c1=$(commit_with_change HEAD file-1.txt "tip-a") && + c2=$(commit_with_change HEAD file-2.txt "tip-b") && + git tag -a -m "tagged" multi-tag "$c1" && + tag_oid=$(git rev-parse multi-tag) && + git tag -d multi-tag && + check_connected "$c2" "$tag_oid" + ) +' + +# Tree-diff optimization: verify trace2 counts. + +test_expect_success "$mode: handles root commit (no parents)" ' + ( + cd main-repo && + blob_oid=$(echo "root-content" | git hash-object -w --stdin) && + tree_oid=$(printf "100644 blob %s\tfile.txt\n" \ + "$blob_oid" | git mktree) && + root_oid=$(git commit-tree "$tree_oid" -m "root") && + + # No parent trees, so the full tree is verified. + # 1 tree loaded (root), 1 blob checked. + check_connected_trace trace-root.txt 1 1 "$root_oid" + ) +' + +test_expect_success "$mode: handles single file change" ' + ( + cd main-repo && + oid=$(commit_with_change HEAD file-1.txt "changed") && + + # 2 trees loaded (new root + parent root), 1 blob checked. + check_connected_trace trace-flat.txt 2 1 "$oid" + ) +' + +test_expect_success "$mode: handles nested change" ' + ( + cd main-repo && + oid=$(commit_with_change HEAD a/b/c/deep.txt "deep-changed") && + # 4 new trees + 4 parent trees = 8 loaded, 1 blob checked. + check_connected_trace trace-nested.txt 8 1 "$oid" + ) +' + +test_expect_success "$mode: handles change-then-revert" ' + ( + cd main-repo && + c1=$(commit_with_change HEAD file-1.txt "revert-tmp") && + c2=$(commit_with_change "$c1" file-1.txt "file 1") && + c3=$(commit_with_change "$c2" file-1.txt "revert-final") && + + # c1: new root + parent root = 2 loads. c2: root matches + # HEAD (already trusted), skipped. c3: new root + parent + # already expanded = 1 load. Total: 3 trees, 2 blobs. + check_connected_trace trace-revert.txt 3 2 "$c3" + ) +' + +test_expect_success "$mode: handles subtree moved to another path" ' + ( + cd main-repo && + moved_tree=$(git ls-tree HEAD | + sed "s/ a$/ moved/" | + git mktree) && + moved=$(git commit-tree "$moved_tree" -p HEAD -m move) && + # New root + parent root scanned, but the moved subtree + # (same OID) is trusted and not descended into. + check_connected_trace trace-move.txt 2 0 "$moved" + ) +' + +test_expect_success "$mode: handles multi-parent merge" ' + ( + cd main-repo && + left=$(commit_with_change HEAD file-1.txt left) && + right=$(commit_with_change HEAD file-2.txt right) && + git update-ref refs/heads/left "$left" && + git update-ref refs/heads/right "$right" && + left_blob=$(git rev-parse "$left:file-1.txt") && + right_blob=$(git rev-parse "$right:file-2.txt") && + tmpgit read-tree HEAD && + tmpgit update-index --replace \ + --cacheinfo "100644,$left_blob,file-1.txt" && + tmpgit update-index --replace \ + --cacheinfo "100644,$right_blob,file-2.txt" && + merge_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + merge=$(git commit-tree "$merge_tree" \ + -p "$left" -p "$right" -m merge) && + # Both parent root trees are scanned as bases, so + # blobs from each parent are trusted without ODB checks. + check_connected_trace trace-merge.txt 3 0 "$merge" && + git update-ref -d refs/heads/left && + git update-ref -d refs/heads/right + ) +' + +test_expect_success "$mode: handles merge with incoming and boundary parents" ' + ( + cd main-repo && + # parent1 is incoming (not a ref), HEAD is boundary. + parent1=$(commit_with_change HEAD file-1.txt merge-inc) && + merge=$(git commit-tree \ + "$(git rev-parse "$parent1^{tree}")" \ + -p "$parent1" -p HEAD -m merge-mixed) && + # parent1 verified first (topo order): 2 trees, 1 blob. + # merge tree = parent1 tree (already trusted): 0 extra. + check_connected_trace trace-merge-inc.txt 2 1 "$merge" + ) +' + +test_expect_success "$mode: handles gitlink entries (submodules)" ' + ( + cd main-repo && + tmpgit read-tree HEAD && + tmpgit update-index --add \ + --cacheinfo "160000,$missing_oid,my-submodule" && + gitlink_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + gitlink_commit=$(git commit-tree "$gitlink_tree" -p HEAD \ + -m "add gitlink") && + + # Gitlink entries are skipped -- the missing submodule + # commit OID does not cause a failure. + check_connected_trace trace-gitlink.txt 2 0 "$gitlink_commit" + ) +' + +test_expect_success "$mode: handles file-to-directory transition" ' + ( + cd main-repo && + + # Parent: "foo" is a blob at root. + blob_a=$(echo "file-content" | git hash-object -w --stdin) && + tmpgit read-tree HEAD && + tmpgit update-index --add \ + --cacheinfo "100644,$blob_a,foo" && + parent_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + parent=$(git commit-tree "$parent_tree" -p HEAD \ + -m "add foo as file") && + + # Child: "foo" becomes a directory (foo/bar.txt). + blob_b=$(echo "dir-content" | git hash-object -w --stdin) && + tmpgit read-tree "$parent" && + tmpgit update-index --remove foo && + tmpgit update-index --add \ + --cacheinfo "100644,$blob_b,foo/bar.txt" && + child_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + child=$(git commit-tree "$child_tree" -p "$parent" \ + -m "foo: file to directory") && + check_connected_trace trace-f2d.txt "" "" "$child" + ) +' + +test_expect_success "$mode: handles directory-to-file transition" ' + ( + cd main-repo && + + # Parent: "bar/baz.txt" exists (bar is a directory). + blob_a=$(echo "nested" | git hash-object -w --stdin) && + tmpgit read-tree HEAD && + tmpgit update-index --add \ + --cacheinfo "100644,$blob_a,bar/baz.txt" && + parent_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + parent=$(git commit-tree "$parent_tree" -p HEAD \ + -m "add bar as directory") && + + # Child: "bar" becomes a plain file. + blob_b=$(echo "flat" | git hash-object -w --stdin) && + tmpgit read-tree "$parent" && + tmpgit update-index --remove bar/baz.txt && + tmpgit update-index --add \ + --cacheinfo "100644,$blob_b,bar" && + child_tree=$(tmpgit write-tree) && + rm -f .git/tmp-idx && + child=$(git commit-tree "$child_tree" -p "$parent" \ + -m "bar: directory to file") && + check_connected_trace trace-d2f.txt "" "" "$child" + ) +' + +# Replacement objects. + +test_expect_success "$mode: accepts with replacement objects" ' + ( + cd replace-test && + check_connected "$original_oid" + ) +' + +test_expect_success "$mode: rejects without replacement objects" ' + ( + cd replace-test && + GIT_NO_REPLACE_OBJECTS=1 && + export GIT_NO_REPLACE_OBJECTS && + test_expect_code 128 check_connected \ + "$original_oid" 2>err && + test_grep "missing blob object.*$orig_blob_oid" err + ) +' + +test_expect_success "$mode: accepts missing promised blob" ' + test_when_finished "rm -rf prom-src prom-server.git prom-client" && + git init prom-src && + test_commit -C prom-src --no-tag base file.txt original && + test_commit -C prom-src --no-tag "add file2" file2.txt extra && + git clone --bare prom-src prom-server.git && + git -C prom-server.git config uploadpack.allowfilter true && + git -C prom-server.git config uploadpack.allowanysha1inwant true && + git clone --no-checkout --filter=blob:none \ + "file://$(pwd)/prom-server.git" prom-client && + ( + cd prom-client && + promised_blob=$(git rev-parse HEAD:file2.txt) && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_blob" && + new_tree=$(printf "100644 blob %s\tnewname.txt\n" \ + "$promised_blob" | + git mktree --missing) && + new_commit=$(git commit-tree "$new_tree" \ + -p HEAD -m "reuse promised blob") && + check_connected "$new_commit" && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_blob" + ) +' + +test_expect_success "$mode: does not lazy-fetch promised blob" ' + test_when_finished "rm -rf nofetch-src nofetch-server.git nofetch-client" && + git init nofetch-src && + test_commit -C nofetch-src --no-tag base file.txt content && + git clone --bare nofetch-src nofetch-server.git && + git -C nofetch-server.git config uploadpack.allowfilter true && + git -C nofetch-server.git config uploadpack.allowanysha1inwant true && + git clone --no-checkout --filter=blob:none \ + "file://$(pwd)/nofetch-server.git" nofetch-client && + ( + cd nofetch-client && + promised_blob=$(git rev-parse HEAD:file.txt) && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_blob" && + new_tree=$(printf "100644 blob %s\tfile.txt\n" \ + "$promised_blob" | + git mktree --missing) && + new_commit=$(git commit-tree "$new_tree" \ + -m "root commit with promised blob") && + check_connected "$new_commit" && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_blob" + ) +' + +test_expect_success "$mode: accepts missing promised tree" ' + test_when_finished "rm -rf prom-tree-src prom-tree-server.git prom-tree-client" && + git init prom-tree-src && + mkdir -p prom-tree-src/a/b && + test_commit -C prom-tree-src --no-tag "nested dirs" a/b/file.txt deep && + git clone --bare prom-tree-src prom-tree-server.git && + git -C prom-tree-server.git config uploadpack.allowfilter true && + git -C prom-tree-server.git config uploadpack.allowanysha1inwant true && + git clone --no-checkout --filter=tree:1 \ + "file://$(pwd)/prom-tree-server.git" prom-tree-client && + ( + cd prom-tree-client && + promised_tree=$(git ls-tree HEAD -- a | + awk "{print \$3}") && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_tree" && + new_tree=$(printf "40000 tree %s\trenamed\n" \ + "$promised_tree" | + git mktree --missing) && + new_commit=$(git commit-tree "$new_tree" \ + -p HEAD -m "reuse promised tree") && + check_connected "$new_commit" && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$promised_tree" + ) +' + +test_expect_success "$mode: verifies new subtree when parent subtree is promised" ' + test_when_finished "rm -rf prom-base-src prom-base-server.git prom-base-client" && + git init prom-base-src && + mkdir -p prom-base-src/a && + test_commit -C prom-base-src --no-tag "base" a/file.txt deep && + git clone --bare prom-base-src prom-base-server.git && + git -C prom-base-server.git config uploadpack.allowfilter true && + git -C prom-base-server.git config uploadpack.allowanysha1inwant true && + git clone --no-checkout --filter=tree:1 \ + "file://$(pwd)/prom-base-server.git" prom-base-client && + ( + cd prom-base-client && + parent_subtree=$(git ls-tree HEAD -- a | + awk "{print \$3}") && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$parent_subtree" && + new_blob=$(echo "local-content" | git hash-object -w --stdin) && + new_subtree=$(printf "100644 blob %s\tfile.txt\n" \ + "$new_blob" | git mktree) && + new_root=$(printf "40000 tree %s\ta\n" \ + "$new_subtree" | git mktree) && + new_commit=$(git commit-tree "$new_root" \ + -p HEAD -m "replace promised subtree") && + check_connected "$new_commit" && + test_must_fail env GIT_NO_LAZY_FETCH=1 \ + git cat-file -e "$parent_subtree" + ) +' + +test_expect_success "$mode: verifies local commit in partial clone" ' + test_when_finished "rm -rf pc-src pc-server.git pc-client" && + git init pc-src && + test_commit -C pc-src --no-tag base file.txt && + git clone --bare pc-src pc-server.git && + git -C pc-server.git config uploadpack.allowfilter true && + git -C pc-server.git config uploadpack.allowanysha1inwant true && + git clone --filter=blob:none \ + "file://$(pwd)/pc-server.git" pc-client && + ( + cd pc-client && + local_commit=$(commit_with_change HEAD file.txt local-content) && + check_connected "$local_commit" + ) +' + +test_expect_success "$mode: tolerates missing promisor parent commit" ' + test_when_finished "rm -rf mp-src mp-server.git mp-client" && + git init mp-src && + test_commit -C mp-src --no-tag base file.txt && + git clone --bare mp-src mp-server.git && + git -C mp-server.git config uploadpack.allowfilter true && + git -C mp-server.git config uploadpack.allowanysha1inwant true && + git clone --filter=blob:none \ + "file://$(pwd)/mp-server.git" mp-client && + test_commit -C mp-src --no-tag c1 file.txt && + test_commit -C mp-src --no-tag c2 file.txt && + c1=$(git -C mp-src rev-parse HEAD~1) && + c2=$(git -C mp-src rev-parse HEAD) && + ( + cd mp-client && + # Add a promisor pack holding only c2, so its parent c1 + # is missing but promised (as after a lazy fetch that + # omits dependents). + echo "$c2" | + git -C ../mp-src pack-objects --stdout >c2.pack && + git index-pack --promisor --stdin 1112912053 -0700 + committer C O Mitter 1112912053 -0700 + + on top of a missing promisor commit + EOF + ) && + check_connected "$local_commit" + ) +' + +test_expect_success "$mode: respects shallow boundary" ' + test_when_finished "rm -rf shallow-src shallow" && + git init shallow-src && + test_commit -C shallow-src --no-tag base file content-1 && + mkdir shallow-src/sub && + test_commit -C shallow-src --no-tag change sub/other content-2 && + git clone --depth=1 "file://$(pwd)/shallow-src" shallow && + ( + cd shallow && + tip=$(git rev-parse HEAD) && + git for-each-ref --format="delete %(refname)" | + git update-ref --no-deref --stdin && + check_connected "$tip" + ) +' + +test_expect_success "$mode: deepening fetch succeeds" ' + test_when_finished "rm -rf deepen-src deepen-server.git deepen-client" && + git init deepen-src && + test_commit -C deepen-src --no-tag c1 file.txt && + test_commit -C deepen-src --no-tag c2 file.txt && + test_commit -C deepen-src --no-tag c3 file.txt && + git clone --bare deepen-src deepen-server.git && + git clone --depth=1 "file://$(pwd)/deepen-server.git" deepen-client && + set_connectivity_check deepen-client $mode && + test -f deepen-client/.git/shallow && + GIT_TRACE2_EVENT="$(pwd)/deepen-trace.txt" \ + git -C deepen-client fetch --deepen=2 origin main && + # Incremental falls back to full for deepening fetches, + # so the trees_loaded event should not appear. + test_grep ! trees_loaded deepen-trace.txt +' + +test_expect_success "$mode: malformed tree detected" ' + ( + cd main-repo && + echo abc >malformed-tree && + malformed_tree=$(git hash-object --literally -t tree -w \ + malformed-tree) && + malformed_commit=$(git commit-tree "$malformed_tree" \ + -p HEAD -m malformed) && + test_expect_code 128 check_connected \ + "$malformed_commit" 2>err + ) +' + +test_expect_success PERL_TEST_HELPERS \ + "$mode: mid-tree corruption detected" ' + ( + cd main-repo && + # Build a tree with one valid entry followed by garbage. + blob_oid=$(echo "valid" | git hash-object -w --stdin) && + bin_oid=$(echo "$blob_oid" | hex2oct) && + printf "100644 good\0${bin_oid}GARBAGE" >corrupt-mid-tree && + corrupt_tree=$(git hash-object --literally -t tree -w \ + corrupt-mid-tree) && + corrupt_commit=$(git commit-tree "$corrupt_tree" \ + -p HEAD -m "mid-tree corruption") && + test_expect_code 128 check_connected \ + "$corrupt_commit" 2>err && + test_grep "too-short tree object" err + ) +' + +done + +# The full check only checks that blobs exist, but the incremental +# check reads their headers and reports a corrupt blob. +test_expect_success 'incremental: rejects corrupt blob' ' + test_when_finished "rm -rf corrupt-repo" && + git init corrupt-repo && + ( + cd corrupt-repo && + test_commit base && + blob=$(echo corrupt | git hash-object -w --stdin) && + tree=$(printf "100644 blob $blob\tfile\n" | git mktree) && + commit=$(git commit-tree "$tree" -p HEAD -m corrupt) && + path=.git/objects/$(test_oid_to_path "$blob") && + chmod +w "$path" && + echo garbage >"$path" && + mode=incremental && + test_expect_code 128 check_connected "$commit" 2>err && + test_grep "unable to read blob object $blob" err + ) +' + +# Algorithm selection. + +# Run a command with its trace2 events in "trace" and its stderr +# in "err". +run_traced () { + rm -f trace && + GIT_TRACE2_EVENT="$(pwd)/trace" "$@" 2>err +} + +# Check which connectivity check the last run_traced used. +expect_mode () { + case "$1" in + full) + test_grep ! trees_loaded trace + ;; + incremental) + test_grep trees_loaded trace + ;; + *) + BUG "unknown mode: $1" + ;; + esac +} + +test_expect_success 'setup source repository' ' + git init src && + test_commit -C src base file.txt && + test_commit -C src --no-tag update file.txt updated +' + +test_expect_success 'unknown transfer.connectivityCheck falls back to full' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst fetch ../src base:refs/heads/main && + for value in bogus + do + set_connectivity_check dst "$value" && + run_traced git -C dst fetch ../src main && + expect_mode full && + test_grep "unknown transfer.connectivityCheck algorithm .$value., using .full." err || + return 1 + done +' + +test_expect_success 'push uses incremental when configured' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C src push ../dst base:refs/heads/main && + set_connectivity_check dst incremental && + run_traced git -C src push ../dst main && + expect_mode incremental +' + +test_expect_success 'fetch uses incremental when configured' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst fetch ../src base:refs/heads/main && + set_connectivity_check dst incremental && + run_traced git -C dst fetch ../src main && + expect_mode incremental +' + +for mode in incremental +do + test_expect_success "push with $mode into an empty repository uses full" ' + test_when_finished "rm -rf dst" && + git init --bare dst && + set_connectivity_check dst $mode && + run_traced git -C src push ../dst main && + expect_mode full + ' + + test_expect_success "clone with $mode uses full" ' + test_when_finished "rm -rf dst" && + run_traced git -c transfer.connectivityCheck=$mode \ + clone --no-local src dst && + expect_mode full + ' +done + +test_done diff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh index 0f6a2ad9b593f4..edb31e3914251f 100755 --- a/t/t6700-tree-depth.sh +++ b/t/t6700-tree-depth.sh @@ -82,6 +82,23 @@ test_expect_success 'default limit for rev-list fails gracefully' ' test_must_fail git rev-list --objects big >/dev/null ' +# The incremental check needs a boundary; without negative revisions +# rev-list falls back to the full check. +test_expect_success 'limit recursion of incremental check' ' + boundary=$(git commit-tree $EMPTY_TREE -m boundary) && + git $small_ok rev-list --connectivity-check=incremental \ + --objects small --not $boundary >/dev/null && + test_must_fail git $small_no rev-list \ + --connectivity-check=incremental --objects small \ + --not $boundary >/dev/null +' + +test_expect_success 'default limit for incremental check fails gracefully' ' + boundary=$(git commit-tree $EMPTY_TREE -m boundary) && + test_must_fail git rev-list --connectivity-check=incremental \ + --objects big --not $boundary >/dev/null +' + test_expect_success 'limit recursion of diff-tree -r' ' git $small_ok diff-tree -r $EMPTY_TREE small && test_must_fail git $small_no diff-tree -r $EMPTY_TREE small diff --git a/tree-verify.c b/tree-verify.c new file mode 100644 index 00000000000000..25db76d30ba58e --- /dev/null +++ b/tree-verify.c @@ -0,0 +1,433 @@ +#include "git-compat-util.h" +#include "commit.h" +#include "gettext.h" +#include "hex.h" +#include "khash.h" +#include "object.h" +#include "odb.h" +#include "oid-array.h" +#include "oidset.h" +#include "tree.h" +#include "tree-walk.h" +#include "tree-verify.h" +#include "packfile.h" +#include "trace2.h" + +enum tree_state { + TREE_UNTRUSTED = 0, + TREE_TRUSTED = 1, + TREE_EXPANDED = 2, +}; + +/* Values are enum tree_state, stored as a byte to keep the map small. */ +KHASH_INIT(oid_tree, struct object_id, unsigned char, 1, + oidhash_by_value, oideq_by_value) + +static enum tree_state tree_map_get(kh_oid_tree_t *m, + const struct object_id *oid) +{ + khint_t pos = kh_get_oid_tree(m, *oid); + if (pos == kh_end(m)) + return TREE_UNTRUSTED; + return kh_val(m, pos); +} + +static void tree_map_add(kh_oid_tree_t *m, const struct object_id *oid, + enum tree_state state) +{ + int added; + khint_t pos = kh_put_oid_tree(m, *oid, &added); + if (added) + kh_val(m, pos) = state; + else if (state > kh_val(m, pos)) + kh_val(m, pos) = state; +} + +struct work_item { + struct name_entry entry; + struct oid_array parent_trees; +}; + +/* The entries of a tree that still need to be verified. */ +struct work_list { + struct work_item *items; + size_t nr, alloc; + bool has_subtrees; +}; + +static void work_list_clear(struct work_list *list) +{ + for (size_t i = 0; i < list->nr; i++) + oid_array_clear(&list->items[i].parent_trees); + free(list->items); +} + +struct verify_state { + kh_oid_tree_t *trees; + struct oidset trusted_blobs; + size_t trees_loaded; + size_t blobs_checked; + int exclude_promisor_objects; + + /* + * Scratch space for reading one tree at a time. It lives here + * rather than on the stack to keep the frame of the recursive + * verify_tree() small. + */ + struct tree_desc desc; + struct name_entry entry; +}; + +/* + * Merge-walk the work list against one base tree entry, recording + * same-path parent subtrees as recursive comparison bases. + * Returns the updated work-list cursor. + */ +static size_t collect_subtree_bases(struct work_list *list, size_t wi, + const struct name_entry *entry) +{ + while (wi < list->nr) { + struct work_item *item = &list->items[wi]; + int cmp = base_name_compare(item->entry.path, + item->entry.pathlen, + item->entry.mode, + entry->path, entry->pathlen, + entry->mode); + if (cmp > 0) + break; + if (cmp < 0) { + wi++; + continue; + } + if (S_ISDIR(item->entry.mode) && S_ISDIR(entry->mode)) + oid_array_append(&item->parent_trees, &entry->oid); + return wi + 1; + } + return wi; +} + +/* + * Report an object that could not be read, telling a missing object + * apart from one that exists but cannot be read, e.g. because it is + * corrupt. + */ +static int unreadable_object_error(struct repository *repo, + const struct object_id *oid, + const char *type) +{ + if (!odb_has_object(repo->objects, oid, 0)) + return error(_("missing %s object %s"), type, oid_to_hex(oid)); + return error(_("unable to read %s object %s"), type, oid_to_hex(oid)); +} + +static int verify_blob(struct repository *repo, + const struct object_id *oid, + struct verify_state *vs) +{ + struct object_info oi = OBJECT_INFO_INIT; + enum object_type type; + + if (oidset_contains(&vs->trusted_blobs, oid)) + return 0; + + vs->blobs_checked++; + oi.typep = &type; + if (odb_read_object_info_extended(repo->objects, oid, &oi, + OBJECT_INFO_LOOKUP_REPLACE | + OBJECT_INFO_SKIP_FETCH_OBJECT)) { + if (vs->exclude_promisor_objects && + is_promisor_object(repo, oid)) + return 0; + return unreadable_object_error(repo, oid, "blob"); + } + if (type != OBJ_BLOB) + return error(_("object %s is a %s, not a %s"), + oid_to_hex(oid), type_name(type), "blob"); + oidset_insert(&vs->trusted_blobs, oid); + return 0; +} + +/* + * Read a tree object into *bufp. Returns -1 if the object exists + * but is not a tree. Otherwise returns 0, leaving *bufp NULL if the + * object could not be read. + */ +static int read_tree_object(struct object_database *odb, + const struct object_id *oid, + void **bufp, size_t *sizep) +{ + struct object_info oi = OBJECT_INFO_INIT; + enum object_type type; + + oi.typep = &type; + oi.sizep = sizep; + oi.contentp = bufp; + if (odb_read_object_info_extended(odb, oid, &oi, + OBJECT_INFO_LOOKUP_REPLACE | + OBJECT_INFO_SKIP_FETCH_OBJECT)) + *bufp = NULL; + if (*bufp && type != OBJ_TREE) { + FREE_AND_NULL(*bufp); + return error(_("object %s is a %s, not a %s"), + oid_to_hex(oid), type_name(type), "tree"); + } + return 0; +} + +/* List the entries of a tree that are not yet known to be trusted. */ +static void list_untrusted_entries(struct verify_state *vs, + const struct object_id *tree_oid, + void *tree_buf, size_t tree_size, + struct work_list *list) +{ + struct name_entry *entry = &vs->entry; + + init_tree_desc(&vs->desc, tree_oid, tree_buf, tree_size); + while (tree_entry(&vs->desc, entry)) { + if (S_ISGITLINK(entry->mode)) + continue; + if (S_ISDIR(entry->mode)) { + if (tree_map_get(vs->trees, &entry->oid) >= + TREE_TRUSTED) + continue; + list->has_subtrees = true; + } else { + if (oidset_contains(&vs->trusted_blobs, &entry->oid)) + continue; + } + ALLOC_GROW(list->items, list->nr + 1, list->alloc); + memset(&list->items[list->nr], 0, sizeof(list->items[0])); + list->items[list->nr++].entry = *entry; + } +} + +/* + * Scan one trusted base tree: expand it if that has not happened yet, + * and collect its same-path subtrees as bases for untrusted subtrees. + */ +static int scan_base_tree(struct repository *repo, struct verify_state *vs, + const struct object_id *base_oid, + struct work_list *list) +{ + int expanded = tree_map_get(vs->trees, base_oid) >= TREE_EXPANDED; + struct name_entry *entry = &vs->entry; + size_t wi = 0, base_size; + void *base_buf; + + if (expanded && !list->has_subtrees) + return 0; + + if (read_tree_object(repo->objects, base_oid, &base_buf, &base_size)) + return -1; + if (!base_buf) { + if (vs->exclude_promisor_objects && + is_promisor_object(repo, base_oid)) + return 0; + return unreadable_object_error(repo, base_oid, "tree"); + } + + vs->trees_loaded++; + init_tree_desc(&vs->desc, base_oid, base_buf, base_size); + + while (tree_entry(&vs->desc, entry)) { + if (S_ISGITLINK(entry->mode)) + continue; + + /* Same-path subtree as a base for an untrusted one. */ + if (list->has_subtrees) + wi = collect_subtree_bases(list, wi, entry); + + /* Expand: the base is trusted, so are its entries. */ + if (!expanded) { + if (S_ISDIR(entry->mode)) + tree_map_add(vs->trees, &entry->oid, + TREE_TRUSTED); + else + oidset_insert(&vs->trusted_blobs, + &entry->oid); + } + } + + if (!expanded) + tree_map_add(vs->trees, base_oid, TREE_EXPANDED); + + free(base_buf); + return 0; +} + +/* + * Verify a tree against the trusted base trees, loading as little as + * possible: + * + * First list the entries that are not yet known to be trusted. When + * the parent trees have been expanded before, only the changed entries + * remain. + * + * Then scan the trusted base trees (the same-path trees of the parent + * commits). A base that has not been expanded yet is read so that its + * entries can be marked trusted; this often shows that listed entries + * are trusted after all. An already expanded base is still read when + * there are untrusted subtrees, because its subtree at the same path is + * what lets the untrusted subtree skip its own unchanged entries in + * turn. + * + * Finally check that the remaining blobs exist, and recurse into the + * remaining subtrees with the same-path subtrees as their bases. + */ +static int verify_tree(struct repository *repo, + const struct object_id *tree_oid, + const struct oid_array *trusted_bases, + struct verify_state *vs, int depth) +{ + struct work_list list = { 0 }; + size_t i, tree_size; + void *tree_buf; + int ret = 0; + + /* + * Enforce core.maxTreeDepth like process_tree() in list-objects.c, + * so that the full and incremental checks reject the same trees. + */ + if (depth > repo->settings.max_allowed_tree_depth) + return error(_("exceeded maximum allowed tree depth")); + + if (tree_map_get(vs->trees, tree_oid) >= TREE_TRUSTED) + return 0; + + if (read_tree_object(repo->objects, tree_oid, &tree_buf, &tree_size)) + return -1; + if (!tree_buf) { + /* + * A missing tree that is promised can be fetched on demand, + * so it is not an error, as in the full check. + */ + if (vs->exclude_promisor_objects && + is_promisor_object(repo, tree_oid)) + return 0; + return unreadable_object_error(repo, tree_oid, "tree"); + } + + vs->trees_loaded++; + list_untrusted_entries(vs, tree_oid, tree_buf, tree_size, &list); + + /* Scan the base trees, unless nothing is left to verify. */ + for (i = 0; list.nr && i < trusted_bases->nr; i++) { + ret = scan_base_tree(repo, vs, &trusted_bases->oid[i], &list); + if (ret) + goto done; + } + + /* + * Check the remaining entries in tree order. Verifying a subtree + * can show that later entries are trusted, which then makes them + * cheap to skip. + */ + for (i = 0; i < list.nr && !ret; i++) { + struct work_item *item = &list.items[i]; + + if (S_ISDIR(item->entry.mode)) + ret = verify_tree(repo, &item->entry.oid, + &item->parent_trees, vs, depth + 1); + else + ret = verify_blob(repo, &item->entry.oid, vs); + } + if (!ret) + tree_map_add(vs->trees, tree_oid, TREE_EXPANDED); + +done: + free(tree_buf); + work_list_clear(&list); + return ret; +} + +static int verify_commit_tree(struct repository *repo, + struct commit *commit, + struct verify_state *vs) +{ + struct oid_array parent_trees = OID_ARRAY_INIT; + const struct object_id *tree_oid; + struct commit_list *p; + int ret; + + /* + * Parent trees are trusted: boundary parents are already + * connected, and earlier incoming parents were verified + * first due to the topological processing order. + * + * The revision walk skips missing parents that are promised + * (see process_parents()), so do the same here. Such a parent + * contributes no base, which only means less of the new tree + * can be skipped. + */ + for (p = commit->parents; p; p = p->next) { + if (repo_parse_commit_gently(repo, p->item, 1) < 0) { + if (vs->exclude_promisor_objects && + is_promisor_object(repo, &p->item->object.oid)) + continue; + ret = error(_("unable to parse commit %s"), + oid_to_hex(&p->item->object.oid)); + goto out; + } + tree_oid = get_commit_tree_oid(p->item); + if (!tree_oid) { + ret = error(_("unable to load root tree for commit %s"), + oid_to_hex(&p->item->object.oid)); + goto out; + } + tree_map_add(vs->trees, tree_oid, TREE_TRUSTED); + oid_array_append(&parent_trees, tree_oid); + } + + tree_oid = get_commit_tree_oid(commit); + if (!tree_oid) { + ret = error(_("unable to load root tree for commit %s"), + oid_to_hex(&commit->object.oid)); + goto out; + } + ret = verify_tree(repo, tree_oid, &parent_trees, vs, 0); +out: + oid_array_clear(&parent_trees); + return ret; +} + +int verify_commits_incremental(struct repository *repo, + struct commit_list **commits, + int exclude_promisor_objects) +{ + struct verify_state vs = { 0 }; + struct commit_list *iter; + unsigned nr_before; + int ret = 0; + + vs.trees = kh_init_oid_tree(); + vs.exclude_promisor_objects = exclude_promisor_objects; + + /* + * Ancestors must be verified before descendants so that parent + * trees can be trusted without re-verification. Sort explicitly + * rather than relying on the caller's ordering. + * + * sort_in_topological_order() silently drops cycle members, + * so explicitly check if the size has changed. + */ + nr_before = commit_list_count(*commits); + sort_in_topological_order(commits, REV_SORT_IN_GRAPH_ORDER); + if (commit_list_count(*commits) < nr_before) { + ret = error(_("cycle detected in incoming commit graph")); + goto out; + } + + *commits = commit_list_reverse(*commits); + + for (iter = *commits; iter && !ret; iter = iter->next) + ret = verify_commit_tree(repo, iter->item, &vs); + +out: + kh_destroy_oid_tree(vs.trees); + oidset_clear(&vs.trusted_blobs); + trace2_data_intmax("connectivity", repo, + "trees_loaded", (intmax_t)vs.trees_loaded); + trace2_data_intmax("connectivity", repo, + "blobs_checked", (intmax_t)vs.blobs_checked); + return ret; +} diff --git a/tree-verify.h b/tree-verify.h new file mode 100644 index 00000000000000..ac7092da1cb0c3 --- /dev/null +++ b/tree-verify.h @@ -0,0 +1,15 @@ +#ifndef TREE_VERIFY_H +#define TREE_VERIFY_H + +struct commit_list; +struct repository; + +/* + * Verify trees of commits incrementally against their parents. + * Returns 0 on success, or reports an error and returns -1. + */ +int verify_commits_incremental(struct repository *repo, + struct commit_list **commits, + int exclude_promisor_objects); + +#endif /* TREE_VERIFY_H */ From 656be5948a954422882bd498d6f5a4516326c952 Mon Sep 17 00:00:00 2001 From: Kristofer Karlsson Date: Fri, 23 Oct 2026 13:11:59 +0000 Subject: [PATCH 28/37] connected: add "auto" connectivity check mode The incremental connectivity check pays a per-commit cost. For every incoming commit it reads the new trees along the changed paths, and the trees at the same paths in its parents to compare against. A parent tree is often read again even though an earlier commit already read it: on git.git, about a third of the tree reads for 2000 incoming commits are such re-reads. The full check instead pays a mostly fixed cost for walking the boundary trees, so with many incoming commits the incremental check becomes the slower one (see the previous commit). Users who mostly receive small pushes but occasionally a large one should not have to choose between the two. Add an "auto" value for transfer.connectivityCheck that picks the mode based on the incoming history. When the revision walk is limited, which is the case whenever the receiving repository has references, the incoming commit list is already computed by limit_list(), so counting it is cheap. "auto" then uses the incremental check for up to 1000 incoming commits, and the full check above that. The cutoff is a fixed constant based on the p5413 fixture in the previous commit, where breakeven is between 500 and 3000 commits. It is conservative for repositories with large trees, where the fixed cost of the full check is higher. Measured on real repositories (rev-list against a single boundary commit, mean of at least 10 runs): repository files commits full incr. full/incr internal A 556K 1000 1.71s 0.61s 2.8x internal A 556K 4000 2.78s 2.83s 1.0x internal B 288K 1000 1.78s 0.88s 2.0x linux 95K 598 0.33s 0.12s 2.7x linux 95K 1728 0.56s 0.35s 1.6x git 4.9K 984 0.22s 0.27s 0.8x With small trees incremental can be slower within the cutoff, but both modes are fast enough there (0.22s vs 0.27s for git above) that the difference does not matter in practice. No single cutoff is optimal for every repository, so it can be set with "auto="; plain "auto" uses 1000. A malformed cutoff is treated like any other unknown value: it falls back to "full" with a warning. Suggested-by: Patrick Steinhardt Signed-off-by: Kristofer Karlsson Signed-off-by: Junio C Hamano --- Documentation/config/transfer.adoc | 6 ++++++ Documentation/rev-list-options.adoc | 2 +- builtin/rev-list.c | 28 ++++++++++++++++++++++++++-- connected.c | 9 ++++++++- t/t5413-connectivity-check.sh | 25 +++++++++++++++++++++++-- 5 files changed, 64 insertions(+), 6 deletions(-) diff --git a/Documentation/config/transfer.adoc b/Documentation/config/transfer.adoc index cba7597a584e0c..f77b25ff8b9732 100644 --- a/Documentation/config/transfer.adoc +++ b/Documentation/config/transfer.adoc @@ -18,6 +18,12 @@ transfer.connectivityCheck:: than `full` when there are many incoming commits. Falls back to `full` for clones, pushes into an empty repository and deepening fetches. +`auto`;; +`auto=`;; + Choose between `full` and `incremental` with a heuristic. + Currently `incremental` is used when there are at most + __ incoming commits (1000 by default), and `full` + otherwise. The heuristic may change in future versions. -- + Unknown values are treated as `full`, with a warning. diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc index 2023ad5f754447..285fb3809ff8f4 100644 --- a/Documentation/rev-list-options.adoc +++ b/Documentation/rev-list-options.adoc @@ -1103,7 +1103,7 @@ It is an error to use `--missing-only` without `--missing=print` or traversal, rather than just silencing errors about missing objects. -`--connectivity-check=(full|incremental)`:: +`--connectivity-check=(full|incremental|auto[=])`:: (For internal use only.) Used by `check_connected()` to select the connectivity check algorithm; see `transfer.connectivityCheck` in linkgit:git-config[1]. diff --git a/builtin/rev-list.c b/builtin/rev-list.c index c07ad6e30ccd86..071a0e3cb5ff5e 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -29,23 +29,35 @@ #include "quote.h" #include "strbuf.h" #include "tree-verify.h" +#include "parse.h" + +#define INCREMENTAL_MAX_COMMITS 1000 enum connectivity_check_mode { CONNECTIVITY_CHECK_FULL, CONNECTIVITY_CHECK_INCREMENTAL, + CONNECTIVITY_CHECK_AUTO, }; struct connectivity_check { enum connectivity_check_mode mode; + unsigned long max_commits; }; static void parse_connectivity_check(const char *arg, struct connectivity_check *check) { + const char *cutoff; + if (!strcmp(arg, "full")) check->mode = CONNECTIVITY_CHECK_FULL; else if (!strcmp(arg, "incremental")) check->mode = CONNECTIVITY_CHECK_INCREMENTAL; + else if (!strcmp(arg, "auto")) + check->mode = CONNECTIVITY_CHECK_AUTO; + else if (skip_prefix(arg, "auto=", &cutoff) && + git_parse_ulong(cutoff, &check->max_commits)) + check->mode = CONNECTIVITY_CHECK_AUTO; else die(_("unknown --connectivity-check mode '%s'"), arg); } @@ -748,7 +760,9 @@ int cmd_rev_list(int argc, int bisect_find_all = 0; int use_bitmap_index = 0; int filter_provided_objects = 0; - struct connectivity_check connectivity_check = { 0 }; + struct connectivity_check connectivity_check = { + .max_commits = INCREMENTAL_MAX_COMMITS, + }; const char *show_progress = NULL; int ret = 0; @@ -1001,9 +1015,19 @@ int cmd_rev_list(int argc, * The incremental check would still work, but it would verify * the whole object graph like the full check, with extra * per-commit overhead on top, so use the full check. + * + * Otherwise the walk is limited, and revs.commits already holds + * all incoming commits, so counting them for "auto" is cheap. */ - if (!revs.limited) + if (!revs.limited) { connectivity_check.mode = CONNECTIVITY_CHECK_FULL; + } else if (connectivity_check.mode == CONNECTIVITY_CHECK_AUTO) { + if (commit_list_count(revs.commits) <= + connectivity_check.max_commits) + connectivity_check.mode = CONNECTIVITY_CHECK_INCREMENTAL; + else + connectivity_check.mode = CONNECTIVITY_CHECK_FULL; + } if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) { struct commit *commit; diff --git a/connected.c b/connected.c index c0329874100adf..8ceaa45644b13c 100644 --- a/connected.c +++ b/connected.c @@ -3,6 +3,7 @@ #include "gettext.h" #include "hex.h" #include "odb.h" +#include "parse.h" #include "run-command.h" #include "sigchain.h" #include "connected.h" @@ -74,7 +75,8 @@ static void connectivity_check_mode(struct repository *repo, struct check_connected_options *opt, struct strbuf *mode) { - const char *algorithm = NULL; + const char *algorithm = NULL, *cutoff; + unsigned long max_commits; /* * Deepening fetches do not have the usual boundary of @@ -93,6 +95,11 @@ static void connectivity_check_mode(struct repository *repo, strbuf_addstr(mode, "full"); else if (!strcasecmp(algorithm, "incremental")) strbuf_addstr(mode, "incremental"); + else if (!strcasecmp(algorithm, "auto")) + strbuf_addstr(mode, "auto"); + else if (skip_iprefix(algorithm, "auto=", &cutoff) && + git_parse_ulong(cutoff, &max_commits)) + strbuf_addf(mode, "auto=%lu", max_commits); else { warning(_("unknown transfer.connectivityCheck algorithm '%s', " "using 'full'"), algorithm); diff --git a/t/t5413-connectivity-check.sh b/t/t5413-connectivity-check.sh index 109ad17cfffcaf..60edf99b63b363 100755 --- a/t/t5413-connectivity-check.sh +++ b/t/t5413-connectivity-check.sh @@ -725,7 +725,7 @@ test_expect_success 'unknown transfer.connectivityCheck falls back to full' ' test_when_finished "rm -rf dst" && git init --bare dst && git -C dst fetch ../src base:refs/heads/main && - for value in bogus + for value in bogus auto= auto=x do set_connectivity_check dst "$value" && run_traced git -C dst fetch ../src main && @@ -753,7 +753,7 @@ test_expect_success 'fetch uses incremental when configured' ' expect_mode incremental ' -for mode in incremental +for mode in incremental auto do test_expect_success "push with $mode into an empty repository uses full" ' test_when_finished "rm -rf dst" && @@ -771,4 +771,25 @@ do ' done +for value in auto auto=1 +do + test_expect_success "push with $value uses incremental up to the cutoff" ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C src push ../dst base:refs/heads/main && + set_connectivity_check dst $value && + run_traced git -C src push ../dst main && + expect_mode incremental + ' +done + +test_expect_success 'push with auto uses full above the cutoff' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C src push ../dst base:refs/heads/main && + set_connectivity_check dst auto=0 && + run_traced git -C src push ../dst main && + expect_mode full +' + test_done From 9dba0b87e512aee319d6b817962bfe18abe1f0cd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:18 +0200 Subject: [PATCH 29/37] rerere: stop using `the_repository` in `rerere_path()` The function `rerere_path()` computes paths of rerere cache entries contained in the "rr-cache" directory of `the_repository`. This makes us implicitly depend on global state. Add a repository parameter so that callers can specify which repository to compute the path for. Adjust the internal helpers that call it to pass down the repository they already have available in their contexts. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/rerere.c | 6 ++-- rerere.c | 82 +++++++++++++++++++++++++----------------------- rerere.h | 3 +- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/builtin/rerere.c b/builtin/rerere.c index ca695052836314..e900e302c37a57 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c @@ -129,8 +129,10 @@ int cmd_rerere(int argc, for (size_t i = 0; i < merge_rr.nr; i++) { const char *path = merge_rr.items[i].string; const struct rerere_id *id = merge_rr.items[i].util; - if (diff_two(rerere_path(&buf, id, "preimage"), path, path, path)) - die(_("unable to generate diff for '%s'"), rerere_path(&buf, id, NULL)); + if (diff_two(rerere_path(the_repository, &buf, id, "preimage"), + path, path, path)) + die(_("unable to generate diff for '%s'"), + rerere_path(the_repository, &buf, id, NULL)); } strbuf_release(&buf); diff --git a/rerere.c b/rerere.c index 7fc32f2cf017a4..356111838c24d6 100644 --- a/rerere.c +++ b/rerere.c @@ -95,17 +95,18 @@ static void assign_variant(struct rerere_id *id) id->variant = variant; } -const char *rerere_path(struct strbuf *buf, const struct rerere_id *id, const char *file) +const char *rerere_path(struct repository *repo, + struct strbuf *buf, const struct rerere_id *id, const char *file) { if (!file) - return repo_git_path_replace(the_repository, buf, "rr-cache/%s", + return repo_git_path_replace(repo, buf, "rr-cache/%s", rerere_id_hex(id)); if (id->variant <= 0) - return repo_git_path_replace(the_repository, buf, "rr-cache/%s/%s", + return repo_git_path_replace(repo, buf, "rr-cache/%s/%s", rerere_id_hex(id), file); - return repo_git_path_replace(the_repository, buf, "rr-cache/%s/%s.%d", + return repo_git_path_replace(repo, buf, "rr-cache/%s/%s.%d", rerere_id_hex(id), file, id->variant); } @@ -609,8 +610,8 @@ static int try_merge(struct index_state *istate, mmfile_t base = {NULL, 0}, other = {NULL, 0}; struct strbuf buf = STRBUF_INIT; - if (read_mmfile(&base, rerere_path(&buf, id, "preimage")) || - read_mmfile(&other, rerere_path(&buf, id, "postimage"))) { + if (read_mmfile(&base, rerere_path(istate->repo, &buf, id, "preimage")) || + read_mmfile(&other, rerere_path(istate->repo, &buf, id, "postimage"))) { ret = LL_MERGE_CONFLICT; } else { /* @@ -650,8 +651,8 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c * Normalize the conflicts in path and write it out to * "thisimage" temporary file. */ - if ((handle_file(istate, path, NULL, rerere_path(&buf, id, "thisimage")) < 0) || - read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) { + if ((handle_file(istate, path, NULL, rerere_path(istate->repo, &buf, id, "thisimage")) < 0) || + read_mmfile(&cur, rerere_path(istate->repo, &buf, id, "thisimage"))) { ret = 1; goto out; } @@ -664,9 +665,9 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c * A successful replay of recorded resolution. * Mark that "postimage" was used to help gc. */ - if (utime(rerere_path(&buf, id, "postimage"), NULL) < 0) + if (utime(rerere_path(istate->repo, &buf, id, "postimage"), NULL) < 0) warning_errno(_("failed utime() on '%s'"), - rerere_path(&buf, id, "postimage")); + rerere_path(istate->repo, &buf, id, "postimage")); /* Update "path" with the resolution */ f = fopen(path, "w"); @@ -705,11 +706,11 @@ static void update_paths(struct repository *r, struct string_list *update) die(_("unable to write new index file")); } -static void remove_variant(struct rerere_id *id) +static void remove_variant(struct repository *r, struct rerere_id *id) { struct strbuf buf = STRBUF_INIT; - unlink_or_warn(rerere_path(&buf, id, "postimage")); - unlink_or_warn(rerere_path(&buf, id, "preimage")); + unlink_or_warn(rerere_path(r, &buf, id, "postimage")); + unlink_or_warn(rerere_path(r, &buf, id, "preimage")); id->collection->status[id->variant] = 0; strbuf_release(&buf); } @@ -743,7 +744,7 @@ static void do_rerere_one_path(struct index_state *istate, const int had_postimage = id->collection->status[variant] & RR_HAS_POSTIMAGE; const char *postimage = - rerere_path(&buf, id, "postimage"); + rerere_path(istate->repo, &buf, id, "postimage"); if (copy_file(the_repository, postimage, @@ -784,7 +785,7 @@ static void do_rerere_one_path(struct index_state *istate, * cleanly, there is no point maintaining our own variant. */ if (0 <= id->variant && id->variant != variant) - remove_variant(id); + remove_variant(istate->repo, id); if (rerere_autoupdate) string_list_insert(update, path); @@ -802,10 +803,10 @@ static void do_rerere_one_path(struct index_state *istate, variant = id->variant; if (handle_file(istate, path, NULL, - rerere_path(&buf, id, "preimage")) < 0) + rerere_path(istate->repo, &buf, id, "preimage")) < 0) goto out; if (id->collection->status[variant] & RR_HAS_POSTIMAGE) { - const char *path = rerere_path(&buf, id, "postimage"); + const char *path = rerere_path(istate->repo, &buf, id, "postimage"); if (unlink(path)) die_errno(_("cannot unlink stray '%s'"), path); id->collection->status[variant] &= ~RR_HAS_POSTIMAGE; @@ -846,7 +847,7 @@ static int do_plain_rerere(struct repository *r, */ ret = handle_file(r->index, path, hash, NULL); if (ret != 0 && string_list_has_string(rr, path)) { - remove_variant(string_list_lookup(rr, path)->util); + remove_variant(r, string_list_lookup(rr, path)->util); string_list_remove(rr, path, 1); } if (ret < 1) @@ -856,7 +857,7 @@ static int do_plain_rerere(struct repository *r, string_list_insert(rr, path)->util = id; /* Ensure that the directory exists. */ - safe_create_dir_in_gitdir(the_repository, rerere_path(&buf, id, NULL)); + safe_create_dir_in_gitdir(the_repository, rerere_path(r, &buf, id, NULL)); } for (i = 0; i < rr->nr; i++) @@ -1091,8 +1092,8 @@ static int rerere_forget_one_path(struct index_state *istate, if (!has_rerere_resolution(id)) continue; - handle_cache(istate, path, hash, rerere_path(&buf, id, "thisimage")); - if (read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) { + handle_cache(istate, path, hash, rerere_path(istate->repo, &buf, id, "thisimage")); + if (read_mmfile(&cur, rerere_path(istate->repo, &buf, id, "thisimage"))) { error(_("failed to update conflicted state in '%s'"), path); goto fail_exit; } @@ -1108,7 +1109,7 @@ static int rerere_forget_one_path(struct index_state *istate, goto fail_exit; } - filename = rerere_path(&buf, id, "postimage"); + filename = rerere_path(istate->repo, &buf, id, "postimage"); if (unlink(filename)) { if (errno == ENOENT) error(_("no remembered resolution for '%s'"), path); @@ -1122,7 +1123,7 @@ static int rerere_forget_one_path(struct index_state *istate, * conflict in the working tree, run us again to record * the postimage. */ - handle_cache(istate, path, hash, rerere_path(&buf, id, "preimage")); + handle_cache(istate, path, hash, rerere_path(istate->repo, &buf, id, "preimage")); fprintf_ln(stderr, _("Updated preimage for '%s'"), path); /* @@ -1181,25 +1182,25 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec) * Garbage collection support */ -static timestamp_t rerere_created_at(struct rerere_id *id) +static timestamp_t rerere_created_at(struct repository *r, struct rerere_id *id) { struct strbuf buf = STRBUF_INIT; struct stat st; timestamp_t ret; - ret = stat(rerere_path(&buf, id, "preimage"), &st) ? (time_t) 0 : st.st_mtime; + ret = stat(rerere_path(r, &buf, id, "preimage"), &st) ? (time_t) 0 : st.st_mtime; strbuf_release(&buf); return ret; } -static timestamp_t rerere_last_used_at(struct rerere_id *id) +static timestamp_t rerere_last_used_at(struct repository *r, struct rerere_id *id) { struct strbuf buf = STRBUF_INIT; struct stat st; timestamp_t ret; - ret = stat(rerere_path(&buf, id, "postimage"), &st) ? (time_t) 0 : st.st_mtime; + ret = stat(rerere_path(r, &buf, id, "postimage"), &st) ? (time_t) 0 : st.st_mtime; strbuf_release(&buf); return ret; @@ -1208,11 +1209,11 @@ static timestamp_t rerere_last_used_at(struct rerere_id *id) /* * Remove the recorded resolution for a given conflict ID */ -static void unlink_rr_item(struct rerere_id *id) +static void unlink_rr_item(struct repository *r, struct rerere_id *id) { struct strbuf buf = STRBUF_INIT; - unlink_or_warn(rerere_path(&buf, id, "thisimage")); - remove_variant(id); + unlink_or_warn(rerere_path(r, &buf, id, "thisimage")); + remove_variant(r, id); id->collection->status[id->variant] = 0; strbuf_release(&buf); } @@ -1231,18 +1232,19 @@ static void rerere_gc_cutoffs(struct repository *r, *cutoff_noresolve = now - 15 * 86400; } -static bool rerere_id_is_stale(struct rerere_id *id, +static bool rerere_id_is_stale(struct repository *r, + struct rerere_id *id, timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) { timestamp_t then; timestamp_t cutoff; - then = rerere_last_used_at(id); + then = rerere_last_used_at(r, id); if (then) { cutoff = cutoff_resolve; } else { - then = rerere_created_at(id); + then = rerere_created_at(r, id); if (!then) return false; cutoff = cutoff_noresolve; @@ -1251,11 +1253,11 @@ static bool rerere_id_is_stale(struct rerere_id *id, return then < cutoff; } -static void prune_one(struct rerere_id *id, +static void prune_one(struct repository *r, struct rerere_id *id, timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) { - if (rerere_id_is_stale(id, cutoff_resolve, cutoff_noresolve)) - unlink_rr_item(id); + if (rerere_id_is_stale(r, id, cutoff_resolve, cutoff_noresolve)) + unlink_rr_item(r, id); } /* Does the basename in "path" look plausibly like an rr-cache entry? */ @@ -1297,7 +1299,7 @@ bool rerere_gc_needed(struct repository *r, size_t limit) for (id.variant = 0; id.variant < id.collection->status_nr; id.variant++) { - if (rerere_id_is_stale(&id, cutoff_resolve, + if (rerere_id_is_stale(r, &id, cutoff_resolve, cutoff_noresolve)) { count += 256; if (count >= limit) { @@ -1349,7 +1351,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) for (id.variant = 0, id.collection = rr_dir; id.variant < id.collection->status_nr; id.variant++) { - prune_one(&id, cutoff_resolve, cutoff_noresolve); + prune_one(r, &id, cutoff_resolve, cutoff_noresolve); if (id.collection->status[id.variant]) now_empty = 0; } @@ -1387,8 +1389,8 @@ void rerere_clear(struct repository *r, struct string_list *merge_rr) struct strbuf buf = STRBUF_INIT; if (!has_rerere_resolution(id)) { - unlink_rr_item(id); - rmdir(rerere_path(&buf, id, NULL)); + unlink_rr_item(r, id); + rmdir(rerere_path(r, &buf, id, NULL)); } strbuf_release(&buf); diff --git a/rerere.h b/rerere.h index 1182ee3fb1d9f8..ee8d97917d9c18 100644 --- a/rerere.h +++ b/rerere.h @@ -34,7 +34,8 @@ int repo_rerere(struct repository *, int); * path to that filesystem entity. With "file" specified with NULL, * return the path to the directory that houses these files. */ -const char *rerere_path(struct strbuf *buf, const struct rerere_id *, +const char *rerere_path(struct repository *, + struct strbuf *buf, const struct rerere_id *, const char *file); int rerere_forget(struct repository *, struct pathspec *); int rerere_remaining(struct repository *, struct string_list *); From f62947adf0f7b3d84b2b2f915e92dc4f3355efaa Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:19 +0200 Subject: [PATCH 30/37] rerere: stop using `the_repository` in internal helpers Most of the static helpers in "rerere.c" still rely on `the_repository` even though their callers already have a `struct repository` available. Thread the repository through those helpers so that we can get rid of references to that global variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- rerere.c | 69 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/rerere.c b/rerere.c index 356111838c24d6..c568bd5e8992c1 100644 --- a/rerere.c +++ b/rerere.c @@ -129,13 +129,13 @@ static int is_rr_file(const char *name, const char *filename, int *variant) return 1; } -static void scan_rerere_dir(struct rerere_dir *rr_dir) +static void scan_rerere_dir(struct repository *r, struct rerere_dir *rr_dir) { struct dirent *de; char *path; DIR *dir; - path = repo_git_path(the_repository, "rr-cache/%s", rr_dir->name); + path = repo_git_path(r, "rr-cache/%s", rr_dir->name); dir = opendir(path); free(path); if (!dir) @@ -154,7 +154,7 @@ static void scan_rerere_dir(struct rerere_dir *rr_dir) closedir(dir); } -static struct rerere_dir *find_rerere_dir(const char *hex) +static struct rerere_dir *find_rerere_dir(struct repository *r, const char *hex) { struct rerere_dir *rr_dir; @@ -166,7 +166,7 @@ static struct rerere_dir *find_rerere_dir(const char *hex) rr_dir->status_alloc = 0; strmap_put(&rerere_dirs, hex, rr_dir); - scan_rerere_dir(rr_dir); + scan_rerere_dir(r, rr_dir); } return rr_dir; } @@ -181,17 +181,17 @@ static int has_rerere_resolution(const struct rerere_id *id) return ((id->collection->status[variant] & both) == both); } -static struct rerere_id *new_rerere_id_hex(char *hex) +static struct rerere_id *new_rerere_id_hex(struct repository *r, char *hex) { struct rerere_id *id = xmalloc(sizeof(*id)); - id->collection = find_rerere_dir(hex); + id->collection = find_rerere_dir(r, hex); id->variant = -1; /* not known yet */ return id; } -static struct rerere_id *new_rerere_id(unsigned char *hash) +static struct rerere_id *new_rerere_id(struct repository *r, unsigned char *hash) { - return new_rerere_id_hex(hash_to_hex(hash)); + return new_rerere_id_hex(r, hash_to_hex(hash)); } /* @@ -213,7 +213,7 @@ static void read_rr(struct repository *r, struct string_list *rr) unsigned char hash[GIT_MAX_RAWSZ]; struct rerere_id *id; int variant; - const unsigned hexsz = the_hash_algo->hexsz; + const unsigned hexsz = r->hash_algo->hexsz; /* There has to be the hash, tab, path and then NUL */ if (buf.len < hexsz + 2 || get_hash_hex(buf.buf, hash)) @@ -231,7 +231,7 @@ static void read_rr(struct repository *r, struct string_list *rr) if (*(path++) != '\t') die(_("corrupt MERGE_RR")); buf.buf[hexsz] = '\0'; - id = new_rerere_id_hex(buf.buf); + id = new_rerere_id_hex(r, buf.buf); id->variant = variant; /* * make sure id->collection->status has enough space @@ -411,13 +411,14 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io, * Return 1 if conflict hunks are found, 0 if there are no conflict * hunks and -1 if an error occurred. */ -static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_size) +static int handle_path(struct repository *r, + unsigned char *hash, struct rerere_io *io, int marker_size) { struct git_hash_ctx ctx; struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT; int has_conflicts = 0; if (hash) - git_hash_init(&ctx, the_hash_algo); + git_hash_init(&ctx, r->hash_algo); while (!io->getline(&buf, io)) { if (is_conflict_marker_line(buf.buf, buf.len, marker_size) == '<') { @@ -466,7 +467,7 @@ static int handle_file(struct index_state *istate, } } - has_conflicts = handle_path(hash, (struct rerere_io *)&io, marker_size); + has_conflicts = handle_path(istate->repo, hash, (struct rerere_io *)&io, marker_size); fclose(io.input); if (io.io.wrerror) @@ -746,7 +747,7 @@ static void do_rerere_one_path(struct index_state *istate, const char *postimage = rerere_path(istate->repo, &buf, id, "postimage"); - if (copy_file(the_repository, + if (copy_file(istate->repo, postimage, path, 0666)) { if (!had_postimage) @@ -853,11 +854,11 @@ static int do_plain_rerere(struct repository *r, if (ret < 1) continue; - id = new_rerere_id(hash); + id = new_rerere_id(r, hash); string_list_insert(rr, path)->util = id; /* Ensure that the directory exists. */ - safe_create_dir_in_gitdir(the_repository, rerere_path(r, &buf, id, NULL)); + safe_create_dir_in_gitdir(r, rerere_path(r, &buf, id, NULL)); } for (i = 0; i < rr->nr; i++) @@ -872,18 +873,17 @@ static int do_plain_rerere(struct repository *r, return write_rr(rr, fd); } -static void git_rerere_config(void) +static void git_rerere_config(struct repository *r) { - repo_config_get_bool(the_repository, "rerere.enabled", &rerere_enabled); - repo_config_get_bool(the_repository, "rerere.autoupdate", &rerere_autoupdate); - repo_config_get_int(the_repository, "rerere.locktimeout", - &rerere_lock_timeout_ms); - repo_config(the_repository, git_default_config, NULL); + repo_config_get_bool(r, "rerere.enabled", &rerere_enabled); + repo_config_get_bool(r, "rerere.autoupdate", &rerere_autoupdate); + repo_config_get_int(r, "rerere.locktimeout", &rerere_lock_timeout_ms); + repo_config(r, git_default_config, NULL); } static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache") -static int is_rerere_enabled(void) +static int is_rerere_enabled(struct repository *r) { int rr_cache_exists; @@ -895,7 +895,7 @@ static int is_rerere_enabled(void) return rr_cache_exists; if (!rr_cache_exists && - safe_create_dir_in_gitdir(the_repository, git_path_rr_cache())) + safe_create_dir_in_gitdir(r, git_path_rr_cache())) die(_("could not create directory '%s'"), git_path_rr_cache()); return 1; } @@ -904,8 +904,8 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) { int fd; - git_rerere_config(); - if (!is_rerere_enabled()) + git_rerere_config(r); + if (!is_rerere_enabled(r)) return -1; if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) @@ -1017,7 +1017,7 @@ static int handle_cache(struct index_state *istate, break; i = ce_stage(ce) - 1; if (!mmfile[i].ptr) { - mmfile[i].ptr = odb_read_object(the_repository->objects, + mmfile[i].ptr = odb_read_object(istate->repo->objects, &ce->oid, &type, &size); if (!mmfile[i].ptr) die(_("unable to read %s"), @@ -1053,7 +1053,8 @@ static int handle_cache(struct index_state *istate, * Grab the conflict ID and optionally write the original * contents with conflict markers out. */ - has_conflicts = handle_path(hash, (struct rerere_io *)&io, marker_size); + has_conflicts = handle_path(istate->repo, hash, + (struct rerere_io *)&io, marker_size); strbuf_release(&io.input); if (io.io.output) fclose(io.io.output); @@ -1080,7 +1081,7 @@ static int rerere_forget_one_path(struct index_state *istate, return error(_("could not parse conflict hunks in '%s'"), path); /* Nuke the recorded resolution for the conflict */ - id = new_rerere_id(hash); + id = new_rerere_id(istate->repo, hash); for (id->variant = 0; id->variant < id->collection->status_nr; @@ -1295,7 +1296,7 @@ bool rerere_gc_needed(struct repository *r, size_t limit) !is_rr_cache_dirname(e->d_name)) continue; - id.collection = find_rerere_dir(e->d_name); + id.collection = find_rerere_dir(r, e->d_name); for (id.variant = 0; id.variant < id.collection->status_nr; id.variant++) { @@ -1332,8 +1333,8 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) return; rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve); - repo_config(the_repository, git_default_config, NULL); - dir = opendir(repo_git_path_replace(the_repository, &buf, "rr-cache")); + repo_config(r, git_default_config, NULL); + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache")); if (!dir) die_errno(_("unable to open rr-cache directory")); /* Collect stale conflict IDs ... */ @@ -1345,7 +1346,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) if (!is_rr_cache_dirname(e->d_name)) continue; /* or should we remove e->d_name? */ - rr_dir = find_rerere_dir(e->d_name); + rr_dir = find_rerere_dir(r, e->d_name); now_empty = 1; for (id.variant = 0, id.collection = rr_dir; @@ -1362,7 +1363,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) /* ... and then remove the empty directories */ for (i = 0; i < to_remove.nr; i++) - rmdir(repo_git_path_replace(the_repository, &buf, + rmdir(repo_git_path_replace(r, &buf, "rr-cache/%s", to_remove.items[i].string)); string_list_clear(&to_remove, 0); From a8b75e8eda21f1551e5cbed4c67708afba26911e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:20 +0200 Subject: [PATCH 31/37] rerere: remove redundant calls to `repo_config()` When setting up the rerere cache we eventually end up calling `repo_config()`. This call is redundant though, as all callers of `setup_rerere()` already called `repo_config()` beforehand. Next to being redundant though it also feels like a layering violation. The intent of that function is to set up state for the rerere cache, but we potentially end up modifying other global state, too. This is usually fine, but it is surprising to say the very least. If a caller for example were to call `repo_config()` and then modifies the parsed config manually then those modifications would be lost when calling `setup_rerere()`. Despite the layering violation though it will also create problems with a subsequent commit, where we are about to cache whether or not the rerere cache is enabled. As part of that change we would stop calling `repo_config()` when the rerere cache is disabled, but continue to call it when it's enabled. So the above layering violation is essentially becoming even worse, because we may or may not clobber config values. Drop the call, as well as another redundant call in `rerere_gc()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- rerere.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rerere.c b/rerere.c index c568bd5e8992c1..56b6ef547b049b 100644 --- a/rerere.c +++ b/rerere.c @@ -878,7 +878,6 @@ static void git_rerere_config(struct repository *r) repo_config_get_bool(r, "rerere.enabled", &rerere_enabled); repo_config_get_bool(r, "rerere.autoupdate", &rerere_autoupdate); repo_config_get_int(r, "rerere.locktimeout", &rerere_lock_timeout_ms); - repo_config(r, git_default_config, NULL); } static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache") @@ -1333,7 +1332,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) return; rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve); - repo_config(r, git_default_config, NULL); + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache")); if (!dir) die_errno(_("unable to open rr-cache directory")); From 87ab289605b22640b0ed68d47391a1a0231f1fcd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:21 +0200 Subject: [PATCH 32/37] rerere: cache result of `is_rerere_enabled()` The function `setup_rerere()` reads the rerere configuration and then figures out whether the rerere cache is enabled after all, either because it has been explicitly enabled or because it's implicitly enabled and ".git/rr-cache" exists. Whether the cache is enabled will not change in a single process though, so it does not make sense to reevaluate these conditions every single time `setup_rerere()` is called. Adapt the function to instead cache the status of the rerere cache via a new `struct repository::rerere_cache_enabled` member. This allows us to get rid of the `GIT_PATH_FUNC()`, which caches the path to the rerere cache directory. But now that we already cache the enabled state in the first place it becomes moot to cache the path, as well. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- repository.c | 1 + repository.h | 6 ++++++ rerere.c | 43 +++++++++++++++++++++++-------------------- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/repository.c b/repository.c index e584d6c4054b63..f51699a6dd9b7e 100644 --- a/repository.c +++ b/repository.c @@ -76,6 +76,7 @@ void initialize_repository(struct repository *repo) repo->bare_cfg = -1; repo->fetch_if_missing = 1; repo_config_values_init(&repo->config_values_private_); + repo->rerere_cache_enabled = -1; /* * When a command runs inside a repository, it learns what diff --git a/repository.h b/repository.h index 91b1f57db7b3bc..86795b6946c9c2 100644 --- a/repository.h +++ b/repository.h @@ -185,6 +185,12 @@ struct repository { /* True if commit-graph has been disabled within this process. */ int commit_graph_disabled; + /* + * Indicates whether the rerere-cache has been enabled. -1 (the default + * value) causes us to perform auto-detection via ".git/rr-cache". + */ + int rerere_cache_enabled; + /* * Controls whether the repository should lazily fetch missing * objects from promisor remotes. Defaults to 1. diff --git a/rerere.c b/rerere.c index 56b6ef547b049b..8fa921fc8ff62d 100644 --- a/rerere.c +++ b/rerere.c @@ -27,9 +27,6 @@ #define THREE_STAGED 2 void *RERERE_RESOLVED = &RERERE_RESOLVED; -/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */ -static int rerere_enabled = -1; - /* automatically update cleanly resolved paths to the index */ static int rerere_autoupdate; @@ -873,40 +870,46 @@ static int do_plain_rerere(struct repository *r, return write_rr(rr, fd); } -static void git_rerere_config(struct repository *r) -{ - repo_config_get_bool(r, "rerere.enabled", &rerere_enabled); - repo_config_get_bool(r, "rerere.autoupdate", &rerere_autoupdate); - repo_config_get_int(r, "rerere.locktimeout", &rerere_lock_timeout_ms); -} - -static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache") - static int is_rerere_enabled(struct repository *r) { int rr_cache_exists; + char *dir; - if (!rerere_enabled) + if (r->rerere_cache_enabled != -1) + return r->rerere_cache_enabled; + + repo_config_get_bool(r, "rerere.enabled", &r->rerere_cache_enabled); + if (!r->rerere_cache_enabled) return 0; - rr_cache_exists = is_directory(git_path_rr_cache()); - if (rerere_enabled < 0) - return rr_cache_exists; + dir = repo_git_path(r, "rr-cache"); + + rr_cache_exists = is_directory(dir); + if (r->rerere_cache_enabled < 0) { + r->rerere_cache_enabled = rr_cache_exists; + goto out; + } if (!rr_cache_exists && - safe_create_dir_in_gitdir(r, git_path_rr_cache())) - die(_("could not create directory '%s'"), git_path_rr_cache()); - return 1; + safe_create_dir_in_gitdir(r, dir)) + die(_("could not create directory '%s'"), dir); + r->rerere_cache_enabled = 1; + +out: + free(dir); + return r->rerere_cache_enabled; } int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) { int fd; - git_rerere_config(r); if (!is_rerere_enabled(r)) return -1; + repo_config_get_bool(r, "rerere.autoupdate", &rerere_autoupdate); + repo_config_get_int(r, "rerere.locktimeout", &rerere_lock_timeout_ms); + if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE); if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT)) From 4f67f74fc86c775b360124bddaf626dde3c8c3e2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:22 +0200 Subject: [PATCH 33/37] rerere: stop using `the_repository` There's only a couple of trivial function calls left that implicitly make us depend on `the_repository`. Refactor those to instead use their variants that allow us to make the dependency explicit. Note that this requires us to make `get_hash_hex_algop()` public. With these changes, "rerere.c" no longer depends on `the_repository`, so drop `USE_THE_REPOSITORY_VARIABLE`. Despite that, we still carry some global state in this file that will tie a specific rerere cache to the first repository used to populate it. This will be fixed in subsequent commits. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- hex.c | 4 ++-- hex.h | 3 +++ rerere.c | 14 +++++++------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/hex.c b/hex.c index f02832140d2d43..4b7793bb8aee51 100644 --- a/hex.c +++ b/hex.c @@ -5,8 +5,8 @@ #include "hex.h" #include "strbuf.h" -static int get_hash_hex_algop(const char *hex, unsigned char *hash, - const struct git_hash_algo *algop) +int get_hash_hex_algop(const char *hex, unsigned char *hash, + const struct git_hash_algo *algop) { for (size_t i = 0; i < algop->rawsz; i++) { int val = hex2chr(hex); diff --git a/hex.h b/hex.h index f15c7e22201cea..50a00880651e98 100644 --- a/hex.h +++ b/hex.h @@ -59,6 +59,9 @@ int parse_oid_hex_algop_impl(const char *hex, struct object_id *oid, const char int get_oid_hex_any(const char *hex, struct object_id *oid); int parse_oid_hex_any(const char *hex, struct object_id *oid, const char **end); +int get_hash_hex_algop(const char *hex, unsigned char *hash, + const struct git_hash_algo *algop); + #ifdef USE_THE_REPOSITORY_VARIABLE /* Like get_oid_hex_algop, but for `the_hash_algo`. */ diff --git a/rerere.c b/rerere.c index 8fa921fc8ff62d..366797ab7e9b59 100644 --- a/rerere.c +++ b/rerere.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" @@ -188,7 +187,7 @@ static struct rerere_id *new_rerere_id_hex(struct repository *r, char *hex) static struct rerere_id *new_rerere_id(struct repository *r, unsigned char *hash) { - return new_rerere_id_hex(r, hash_to_hex(hash)); + return new_rerere_id_hex(r, hash_to_hex_algop(hash, r->hash_algo)); } /* @@ -213,7 +212,8 @@ static void read_rr(struct repository *r, struct string_list *rr) const unsigned hexsz = r->hash_algo->hexsz; /* There has to be the hash, tab, path and then NUL */ - if (buf.len < hexsz + 2 || get_hash_hex(buf.buf, hash)) + if (buf.len < hexsz + 2 || + get_hash_hex_algop(buf.buf, hash, r->hash_algo)) die(_("corrupt MERGE_RR")); if (buf.buf[hexsz] != '.') { @@ -1264,11 +1264,11 @@ static void prune_one(struct repository *r, struct rerere_id *id, } /* Does the basename in "path" look plausibly like an rr-cache entry? */ -static int is_rr_cache_dirname(const char *path) +static int is_rr_cache_dirname(struct repository *r, const char *path) { struct object_id oid; const char *end; - return !parse_oid_hex(path, &oid, &end) && !*end; + return !parse_oid_hex_algop(path, &oid, &end, r->hash_algo) && !*end; } bool rerere_gc_needed(struct repository *r, size_t limit) @@ -1295,7 +1295,7 @@ bool rerere_gc_needed(struct repository *r, size_t limit) * use for estimating the number of loose objects. */ if (!starts_with(e->d_name, "17") || - !is_rr_cache_dirname(e->d_name)) + !is_rr_cache_dirname(r, e->d_name)) continue; id.collection = find_rerere_dir(r, e->d_name); @@ -1345,7 +1345,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) struct rerere_id id; int now_empty; - if (!is_rr_cache_dirname(e->d_name)) + if (!is_rr_cache_dirname(r, e->d_name)) continue; /* or should we remove e->d_name? */ rr_dir = find_rerere_dir(r, e->d_name); From ea54cda7c2446eb6cf840dd9e0b677f330f336ec Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:23 +0200 Subject: [PATCH 34/37] rerere: bundle global state into a struct The state of the rerere subsystem is tracked across several global variables. Move them into a struct so that we can easily de-globalize the state. Note that the lock timeout is only ever used in `setup_rerere()`. So instead of moving it into the new struct, we just keep it as a local variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- rerere.c | 44 +++++++++++++++++++------------------------- rerere.h | 11 +++++++++++ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/rerere.c b/rerere.c index 366797ab7e9b59..23bcb8bc316c25 100644 --- a/rerere.c +++ b/rerere.c @@ -21,17 +21,15 @@ #include "odb.h" #include "strmap.h" +static struct rerere_cache cache = { + .dirs = STRMAP_INIT, +}; + #define RESOLVED 0 #define PUNTED 1 #define THREE_STAGED 2 void *RERERE_RESOLVED = &RERERE_RESOLVED; -/* automatically update cleanly resolved paths to the index */ -static int rerere_autoupdate; - -/* how long to wait for MERGE_RR.lock, in milliseconds */ -static int rerere_lock_timeout_ms = 1000; - #define RR_HAS_POSTIMAGE 1 #define RR_HAS_PREIMAGE 2 struct rerere_dir { @@ -40,19 +38,17 @@ struct rerere_dir { char name[FLEX_ARRAY]; }; -static struct strmap rerere_dirs = STRMAP_INIT; - static void free_rerere_dirs(void) { struct hashmap_iter iter; struct strmap_entry *ent; - strmap_for_each_entry(&rerere_dirs, &iter, ent) { + strmap_for_each_entry(&cache.dirs, &iter, ent) { struct rerere_dir *rr_dir = ent->value; free(rr_dir->status); free(rr_dir); } - strmap_clear(&rerere_dirs, 0); + strmap_clear(&cache.dirs, 0); } static void free_rerere_id(struct string_list_item *item) @@ -154,13 +150,13 @@ static struct rerere_dir *find_rerere_dir(struct repository *r, const char *hex) { struct rerere_dir *rr_dir; - rr_dir = strmap_get(&rerere_dirs, hex); + rr_dir = strmap_get(&cache.dirs, hex); if (!rr_dir) { FLEX_ALLOC_STR(rr_dir, name, hex); rr_dir->status = NULL; rr_dir->status_nr = 0; rr_dir->status_alloc = 0; - strmap_put(&rerere_dirs, hex, rr_dir); + strmap_put(&cache.dirs, hex, rr_dir); scan_rerere_dir(r, rr_dir); } @@ -241,8 +237,6 @@ static void read_rr(struct repository *r, struct string_list *rr) fclose(in); } -static struct lock_file write_lock; - static int write_rr(struct string_list *rr, int out_fd) { int i; @@ -270,7 +264,7 @@ static int write_rr(struct string_list *rr, int out_fd) strbuf_release(&buf); } - if (commit_lock_file(&write_lock) != 0) + if (commit_lock_file(&cache.write_lock) != 0) die(_("unable to write rerere record")); return 0; } @@ -785,7 +779,7 @@ static void do_rerere_one_path(struct index_state *istate, if (0 <= id->variant && id->variant != variant) remove_variant(istate->repo, id); - if (rerere_autoupdate) + if (cache.autoupdate) string_list_insert(update, path); else fprintf_ln(stderr, @@ -902,27 +896,27 @@ static int is_rerere_enabled(struct repository *r) int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) { + int lock_timeout_ms = 1000; int fd; if (!is_rerere_enabled(r)) return -1; - repo_config_get_bool(r, "rerere.autoupdate", &rerere_autoupdate); - repo_config_get_int(r, "rerere.locktimeout", &rerere_lock_timeout_ms); + repo_config_get_bool(r, "rerere.autoupdate", &cache.autoupdate); + repo_config_get_int(r, "rerere.locktimeout", &lock_timeout_ms); if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) - rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE); + cache.autoupdate = !!(flags & RERERE_AUTOUPDATE); if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT)) BUG("RERERE_NOWAIT does not apply with RERERE_READONLY"); if (flags & RERERE_READONLY) { fd = 0; } else { int lock_flags = LOCK_DIE_ON_ERROR; - int timeout_ms = rerere_lock_timeout_ms; if (flags & RERERE_NOWAIT) { lock_flags = 0; - timeout_ms = 0; + lock_timeout_ms = 0; } /* @@ -930,9 +924,9 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) * "git rerere gc" while it prunes rr-cache, so by default * wait for it instead of dying right away. */ - fd = repo_hold_lock_file_for_update_timeout(r, &write_lock, + fd = repo_hold_lock_file_for_update_timeout(r, &cache.write_lock, git_path_merge_rr(r), - lock_flags, timeout_ms); + lock_flags, lock_timeout_ms); if (fd < 0) return -1; } @@ -1369,7 +1363,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) "rr-cache/%s", to_remove.items[i].string)); string_list_clear(&to_remove, 0); - rollback_lock_file(&write_lock); + rollback_lock_file(&cache.write_lock); strbuf_release(&buf); } @@ -1399,5 +1393,5 @@ void rerere_clear(struct repository *r, struct string_list *merge_rr) strbuf_release(&buf); } unlink_or_warn(git_path_merge_rr(r)); - rollback_lock_file(&write_lock); + rollback_lock_file(&cache.write_lock); } diff --git a/rerere.h b/rerere.h index ee8d97917d9c18..570667d866d689 100644 --- a/rerere.h +++ b/rerere.h @@ -2,7 +2,9 @@ #define RERERE_H #include "gettext.h" +#include "lockfile.h" #include "string-list.h" +#include "strmap.h" struct pathspec; struct repository; @@ -20,6 +22,15 @@ struct repository; */ extern void *RERERE_RESOLVED; +struct rerere_cache { + /* Automatically update cleanly resolved paths to the index. */ + int autoupdate; + + struct strmap dirs; + + struct lock_file write_lock; +}; + struct rerere_dir; struct rerere_id { struct rerere_dir *collection; From b948c10c8b312fa54862449bc5160bc474187595 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 23 Oct 2026 14:50:24 +0200 Subject: [PATCH 35/37] rerere: get rid of global rerere cache state The rerere cache state is tracked via a global variable. On the one hand this ties it to one specific repository. On the other hand it has the consequence that the cache may be leaking state across multiple calls into the rerere subsystem. Refactor the subsystem to instead pass around the rerere cache state explicitly and make callers responsible for setting it up and then clearing it again. For now, all callers are updated to do so ad-hoc. This of course means that state that we could carry over across multiple calls would have to be set up multiple times. But that doesn't really matter much right now anyway, as the number of times we call those functions is bounded. Note that `rerere_gc_needed()` is the only caller where we explicitly don't set up the rerere cache state. This is intentional and matches the previous status quo, where we didn't honor "rerere.enabled", either. The function only needs access to `struct rerere_cache::dirs` anyway though, so it's fine to partially initialize it, only. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/rerere.c | 15 ++++-- rerere.c | 138 ++++++++++++++++++++++++++++------------------- rerere.h | 9 +++- 3 files changed, 102 insertions(+), 60 deletions(-) diff --git a/builtin/rerere.c b/builtin/rerere.c index e900e302c37a57..2c17f02f56daae 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c @@ -106,11 +106,15 @@ int cmd_rerere(int argc, rerere_gc(the_repository, &merge_rr, skip_locked ? RERERE_NOWAIT : 0); else if (!strcmp(argv[0], "status")) { - if (setup_rerere(the_repository, &merge_rr, - flags | RERERE_READONLY) < 0) + struct rerere_cache cache; + + if (rerere_cache_setup(&cache, the_repository, &merge_rr, + flags | RERERE_READONLY) < 0) return 0; for (size_t i = 0; i < merge_rr.nr; i++) printf("%s\n", merge_rr.items[i].string); + + rerere_cache_release(&cache); } else if (!strcmp(argv[0], "remaining")) { rerere_remaining(the_repository, &merge_rr); for (size_t i = 0; i < merge_rr.nr; i++) { @@ -123,8 +127,10 @@ int cmd_rerere(int argc, } } else if (!strcmp(argv[0], "diff")) { struct strbuf buf = STRBUF_INIT; - if (setup_rerere(the_repository, &merge_rr, - flags | RERERE_READONLY) < 0) + struct rerere_cache cache; + + if (rerere_cache_setup(&cache, the_repository, &merge_rr, + flags | RERERE_READONLY) < 0) return 0; for (size_t i = 0; i < merge_rr.nr; i++) { const char *path = merge_rr.items[i].string; @@ -135,6 +141,7 @@ int cmd_rerere(int argc, rerere_path(the_repository, &buf, id, NULL)); } + rerere_cache_release(&cache); strbuf_release(&buf); } else usage_with_options(rerere_usage, options); diff --git a/rerere.c b/rerere.c index 23bcb8bc316c25..e089761208590b 100644 --- a/rerere.c +++ b/rerere.c @@ -21,10 +21,6 @@ #include "odb.h" #include "strmap.h" -static struct rerere_cache cache = { - .dirs = STRMAP_INIT, -}; - #define RESOLVED 0 #define PUNTED 1 #define THREE_STAGED 2 @@ -38,17 +34,19 @@ struct rerere_dir { char name[FLEX_ARRAY]; }; -static void free_rerere_dirs(void) +void rerere_cache_release(struct rerere_cache *cache) { struct hashmap_iter iter; struct strmap_entry *ent; - strmap_for_each_entry(&cache.dirs, &iter, ent) { + strmap_for_each_entry(&cache->dirs, &iter, ent) { struct rerere_dir *rr_dir = ent->value; free(rr_dir->status); free(rr_dir); } - strmap_clear(&cache.dirs, 0); + strmap_clear(&cache->dirs, 0); + + rollback_lock_file(&cache->write_lock); } static void free_rerere_id(struct string_list_item *item) @@ -121,13 +119,13 @@ static int is_rr_file(const char *name, const char *filename, int *variant) return 1; } -static void scan_rerere_dir(struct repository *r, struct rerere_dir *rr_dir) +static void scan_rerere_dir(struct rerere_cache *cache, struct rerere_dir *rr_dir) { struct dirent *de; char *path; DIR *dir; - path = repo_git_path(r, "rr-cache/%s", rr_dir->name); + path = repo_git_path(cache->repo, "rr-cache/%s", rr_dir->name); dir = opendir(path); free(path); if (!dir) @@ -146,19 +144,20 @@ static void scan_rerere_dir(struct repository *r, struct rerere_dir *rr_dir) closedir(dir); } -static struct rerere_dir *find_rerere_dir(struct repository *r, const char *hex) +static struct rerere_dir *find_rerere_dir(struct rerere_cache *cache, + const char *hex) { struct rerere_dir *rr_dir; - rr_dir = strmap_get(&cache.dirs, hex); + rr_dir = strmap_get(&cache->dirs, hex); if (!rr_dir) { FLEX_ALLOC_STR(rr_dir, name, hex); rr_dir->status = NULL; rr_dir->status_nr = 0; rr_dir->status_alloc = 0; - strmap_put(&cache.dirs, hex, rr_dir); + strmap_put(&cache->dirs, hex, rr_dir); - scan_rerere_dir(r, rr_dir); + scan_rerere_dir(cache, rr_dir); } return rr_dir; } @@ -173,17 +172,18 @@ static int has_rerere_resolution(const struct rerere_id *id) return ((id->collection->status[variant] & both) == both); } -static struct rerere_id *new_rerere_id_hex(struct repository *r, char *hex) +static struct rerere_id *new_rerere_id_hex(struct rerere_cache *cache, + char *hex) { struct rerere_id *id = xmalloc(sizeof(*id)); - id->collection = find_rerere_dir(r, hex); + id->collection = find_rerere_dir(cache, hex); id->variant = -1; /* not known yet */ return id; } -static struct rerere_id *new_rerere_id(struct repository *r, unsigned char *hash) +static struct rerere_id *new_rerere_id(struct rerere_cache *cache, unsigned char *hash) { - return new_rerere_id_hex(r, hash_to_hex_algop(hash, r->hash_algo)); + return new_rerere_id_hex(cache, hash_to_hex_algop(hash, cache->repo->hash_algo)); } /* @@ -193,10 +193,10 @@ static struct rerere_id *new_rerere_id(struct repository *r, unsigned char *hash * work on (i.e. what is left by the previous invocation of "git * rerere" during the current conflict resolution session). */ -static void read_rr(struct repository *r, struct string_list *rr) +static void read_rr(struct rerere_cache *cache, struct string_list *rr) { struct strbuf buf = STRBUF_INIT; - FILE *in = fopen_or_warn(git_path_merge_rr(r), "r"); + FILE *in = fopen_or_warn(git_path_merge_rr(cache->repo), "r"); if (!in) return; @@ -205,11 +205,11 @@ static void read_rr(struct repository *r, struct string_list *rr) unsigned char hash[GIT_MAX_RAWSZ]; struct rerere_id *id; int variant; - const unsigned hexsz = r->hash_algo->hexsz; + const unsigned hexsz = cache->repo->hash_algo->hexsz; /* There has to be the hash, tab, path and then NUL */ if (buf.len < hexsz + 2 || - get_hash_hex_algop(buf.buf, hash, r->hash_algo)) + get_hash_hex_algop(buf.buf, hash, cache->repo->hash_algo)) die(_("corrupt MERGE_RR")); if (buf.buf[hexsz] != '.') { @@ -224,7 +224,7 @@ static void read_rr(struct repository *r, struct string_list *rr) if (*(path++) != '\t') die(_("corrupt MERGE_RR")); buf.buf[hexsz] = '\0'; - id = new_rerere_id_hex(r, buf.buf); + id = new_rerere_id_hex(cache, buf.buf); id->variant = variant; /* * make sure id->collection->status has enough space @@ -237,7 +237,8 @@ static void read_rr(struct repository *r, struct string_list *rr) fclose(in); } -static int write_rr(struct string_list *rr, int out_fd) +static int write_rr(struct rerere_cache *cache, + struct string_list *rr, int out_fd) { int i; for (i = 0; i < rr->nr; i++) { @@ -264,7 +265,7 @@ static int write_rr(struct string_list *rr, int out_fd) strbuf_release(&buf); } - if (commit_lock_file(&cache.write_lock) != 0) + if (commit_lock_file(&cache->write_lock) != 0) die(_("unable to write rerere record")); return 0; } @@ -564,12 +565,15 @@ static int find_conflict(struct repository *r, struct string_list *conflict) */ int rerere_remaining(struct repository *r, struct string_list *merge_rr) { - int i; + struct rerere_cache cache; + int ret, i; - if (setup_rerere(r, merge_rr, RERERE_READONLY)) + if (rerere_cache_setup(&cache, r, merge_rr, RERERE_READONLY)) return 0; - if (repo_read_index(r) < 0) - return error(_("index file corrupt")); + if (repo_read_index(r) < 0) { + ret = error(_("index file corrupt")); + goto out; + } for (i = 0; i < r->index->cache_nr;) { int conflict_type; @@ -586,7 +590,12 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr) } } } - return 0; + + ret = 0; + +out: + rerere_cache_release(&cache); + return ret; } /* @@ -714,10 +723,11 @@ static void remove_variant(struct repository *r, struct rerere_id *id) * only have the preimage for that conflict, in which case the result * needs to be recorded as a resolution in a postimage file. */ -static void do_rerere_one_path(struct index_state *istate, +static void do_rerere_one_path(struct rerere_cache *cache, struct string_list_item *rr_item, struct string_list *update) { + struct index_state *istate = cache->repo->index; const char *path = rr_item->string; struct rerere_id *id = rr_item->util; struct rerere_dir *rr_dir = id->collection; @@ -779,7 +789,7 @@ static void do_rerere_one_path(struct index_state *istate, if (0 <= id->variant && id->variant != variant) remove_variant(istate->repo, id); - if (cache.autoupdate) + if (cache->autoupdate) string_list_insert(update, path); else fprintf_ln(stderr, @@ -810,12 +820,13 @@ static void do_rerere_one_path(struct index_state *istate, strbuf_release(&buf); } -static int do_plain_rerere(struct repository *r, +static int do_plain_rerere(struct rerere_cache *cache, struct string_list *rr, int fd) { struct string_list conflict = STRING_LIST_INIT_DUP; struct string_list update = STRING_LIST_INIT_DUP; struct strbuf buf = STRBUF_INIT; + struct repository *r = cache->repo; int i; find_conflict(r, &conflict); @@ -845,7 +856,7 @@ static int do_plain_rerere(struct repository *r, if (ret < 1) continue; - id = new_rerere_id(r, hash); + id = new_rerere_id(cache, hash); string_list_insert(rr, path)->util = id; /* Ensure that the directory exists. */ @@ -853,7 +864,7 @@ static int do_plain_rerere(struct repository *r, } for (i = 0; i < rr->nr; i++) - do_rerere_one_path(r->index, &rr->items[i], &update); + do_rerere_one_path(cache, &rr->items[i], &update); if (update.nr) update_paths(r, &update); @@ -861,7 +872,7 @@ static int do_plain_rerere(struct repository *r, string_list_clear(&conflict, 0); string_list_clear(&update, 0); strbuf_release(&buf); - return write_rr(rr, fd); + return write_rr(cache, rr, fd); } static int is_rerere_enabled(struct repository *r) @@ -894,7 +905,10 @@ static int is_rerere_enabled(struct repository *r) return r->rerere_cache_enabled; } -int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) +int rerere_cache_setup(struct rerere_cache *cache, + struct repository *r, + struct string_list *merge_rr, + int flags) { int lock_timeout_ms = 1000; int fd; @@ -902,11 +916,14 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) if (!is_rerere_enabled(r)) return -1; - repo_config_get_bool(r, "rerere.autoupdate", &cache.autoupdate); + memset(cache, 0, sizeof(*cache)); + cache->repo = r; + strmap_init(&cache->dirs); + repo_config_get_bool(r, "rerere.autoupdate", &cache->autoupdate); repo_config_get_int(r, "rerere.locktimeout", &lock_timeout_ms); if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) - cache.autoupdate = !!(flags & RERERE_AUTOUPDATE); + cache->autoupdate = !!(flags & RERERE_AUTOUPDATE); if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT)) BUG("RERERE_NOWAIT does not apply with RERERE_READONLY"); if (flags & RERERE_READONLY) { @@ -924,13 +941,13 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) * "git rerere gc" while it prunes rr-cache, so by default * wait for it instead of dying right away. */ - fd = repo_hold_lock_file_for_update_timeout(r, &cache.write_lock, + fd = repo_hold_lock_file_for_update_timeout(r, &cache->write_lock, git_path_merge_rr(r), lock_flags, lock_timeout_ms); if (fd < 0) return -1; } - read_rr(r, merge_rr); + read_rr(cache, merge_rr); return fd; } @@ -942,13 +959,15 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags) int repo_rerere(struct repository *r, int flags) { struct string_list merge_rr = STRING_LIST_INIT_DUP; + struct rerere_cache cache; int fd, status; - fd = setup_rerere(r, &merge_rr, flags); + fd = rerere_cache_setup(&cache, r, &merge_rr, flags); if (fd < 0) return 0; - status = do_plain_rerere(r, &merge_rr, fd); - free_rerere_dirs(); + status = do_plain_rerere(&cache, &merge_rr, fd); + + rerere_cache_release(&cache); string_list_clear(&merge_rr, 1); return status; } @@ -1057,10 +1076,11 @@ static int handle_cache(struct index_state *istate, return has_conflicts; } -static int rerere_forget_one_path(struct index_state *istate, +static int rerere_forget_one_path(struct rerere_cache *cache, const char *path, struct string_list *rr) { + struct index_state *istate = cache->repo->index; const char *filename; struct rerere_id *id; unsigned char hash[GIT_MAX_RAWSZ]; @@ -1077,7 +1097,7 @@ static int rerere_forget_one_path(struct index_state *istate, return error(_("could not parse conflict hunks in '%s'"), path); /* Nuke the recorded resolution for the conflict */ - id = new_rerere_id(istate->repo, hash); + id = new_rerere_id(cache, hash); for (id->variant = 0; id->variant < id->collection->status_nr; @@ -1145,11 +1165,12 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec) int i, fd, ret; struct string_list conflict = STRING_LIST_INIT_DUP; struct string_list merge_rr = STRING_LIST_INIT_DUP; + struct rerere_cache cache; if (repo_read_index(r) < 0) return error(_("index file corrupt")); - fd = setup_rerere(r, &merge_rr, RERERE_NOAUTOUPDATE); + fd = rerere_cache_setup(&cache, r, &merge_rr, RERERE_NOAUTOUPDATE); if (fd < 0) return 0; @@ -1165,13 +1186,14 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec) if (!match_pathspec(r->index, pathspec, it->string, strlen(it->string), 0, NULL, 0)) continue; - rerere_forget_one_path(r->index, it->string, &merge_rr); + rerere_forget_one_path(&cache, it->string, &merge_rr); } - ret = write_rr(&merge_rr, fd); + ret = write_rr(&cache, &merge_rr, fd); string_list_clear(&conflict, 0); string_list_clear(&merge_rr, 1); + rerere_cache_release(&cache); return ret; } @@ -1267,6 +1289,10 @@ static int is_rr_cache_dirname(struct repository *r, const char *path) bool rerere_gc_needed(struct repository *r, size_t limit) { + struct rerere_cache cache = { + .repo = r, + .dirs = STRMAP_INIT, + }; timestamp_t cutoff_resolve, cutoff_noresolve; struct strbuf buf = STRBUF_INIT; bool needed = false; @@ -1292,7 +1318,7 @@ bool rerere_gc_needed(struct repository *r, size_t limit) !is_rr_cache_dirname(r, e->d_name)) continue; - id.collection = find_rerere_dir(r, e->d_name); + id.collection = find_rerere_dir(&cache, e->d_name); for (id.variant = 0; id.variant < id.collection->status_nr; id.variant++) { @@ -1310,13 +1336,14 @@ bool rerere_gc_needed(struct repository *r, size_t limit) out: if (dir) closedir(dir); - free_rerere_dirs(); + rerere_cache_release(&cache); strbuf_release(&buf); return needed; } void rerere_gc(struct repository *r, struct string_list *rr, int flags) { + struct rerere_cache cache; struct string_list to_remove = STRING_LIST_INIT_DUP; DIR *dir; struct dirent *e; @@ -1325,7 +1352,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) timestamp_t cutoff_resolve; struct strbuf buf = STRBUF_INIT; - if (setup_rerere(r, rr, flags) < 0) + if (rerere_cache_setup(&cache, r, rr, flags) < 0) return; rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve); @@ -1342,7 +1369,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) if (!is_rr_cache_dirname(r, e->d_name)) continue; /* or should we remove e->d_name? */ - rr_dir = find_rerere_dir(r, e->d_name); + rr_dir = find_rerere_dir(&cache, e->d_name); now_empty = 1; for (id.variant = 0, id.collection = rr_dir; @@ -1363,7 +1390,7 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) "rr-cache/%s", to_remove.items[i].string)); string_list_clear(&to_remove, 0); - rollback_lock_file(&cache.write_lock); + rerere_cache_release(&cache); strbuf_release(&buf); } @@ -1376,9 +1403,10 @@ void rerere_gc(struct repository *r, struct string_list *rr, int flags) */ void rerere_clear(struct repository *r, struct string_list *merge_rr) { + struct rerere_cache cache; int i; - if (setup_rerere(r, merge_rr, 0) < 0) + if (rerere_cache_setup(&cache, r, merge_rr, 0) < 0) return; for (i = 0; i < merge_rr->nr; i++) { @@ -1393,5 +1421,5 @@ void rerere_clear(struct repository *r, struct string_list *merge_rr) strbuf_release(&buf); } unlink_or_warn(git_path_merge_rr(r)); - rollback_lock_file(&cache.write_lock); + rerere_cache_release(&cache); } diff --git a/rerere.h b/rerere.h index 570667d866d689..d3cbd10e13f65c 100644 --- a/rerere.h +++ b/rerere.h @@ -23,6 +23,8 @@ struct repository; extern void *RERERE_RESOLVED; struct rerere_cache { + struct repository *repo; + /* Automatically update cleanly resolved paths to the index. */ int autoupdate; @@ -37,7 +39,12 @@ struct rerere_id { int variant; }; -int setup_rerere(struct repository *,struct string_list *, int); +int rerere_cache_setup(struct rerere_cache *cache, + struct repository *r, + struct string_list *merge_rr, + int flags); +void rerere_cache_release(struct rerere_cache *cache); + int repo_rerere(struct repository *, int); /* * Given the conflict ID and the name of a "file" used for replaying From c3764d15e61ab622628b94fec204bc2acfb8aee2 Mon Sep 17 00:00:00 2001 From: Thomas Bachem Date: Fri, 23 Oct 2026 13:19:01 +0000 Subject: [PATCH 36/37] hook: make hook..enabled cover the hookdir hook hook..enabled, added by dcfb5af67e (hook: add hook..enabled switch, 2026-04-10), skips only the hooks defined in the configuration when set to false. With hook.pre-commit.enabled=false, "git commit" still runs .git/hooks/pre-commit, and "git hook list" shows that hook without the "event-disabled" mark. That was the intent, but the documentation says "no hooks fire for that event", which reads as covering the hookdir hook too. No other setting turns off the hookdir hook of a single event either: core.hooksPath=/dev/null turns off the hookdir hooks of all events. Make the switch cover the hookdir hook as well, and say so in the documentation. This changes behavior for anyone who set the switch and relied on the hookdir hook still running. The test for hook..enabled=false looks only at the standard output, but "git hook run" sends hook output to the standard error, so the test passes even when a disabled hook runs. Add a hookdir hook to it and check the standard error as well. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 3 ++- builtin/hook.c | 6 ++++-- hook.c | 14 +++++++++----- hook.h | 8 +++++++- t/t1800-hook.sh | 35 +++++++++++++++++++++++++++++++--- 5 files changed, 54 insertions(+), 12 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 083dc60a1325f2..332657e70f2f0b 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -47,7 +47,8 @@ hook..parallel:: the effective job count is greater than 1. {see-git-hook} hook..enabled:: - Switch to enable or disable all hooks for the `` hook event. + Switch to enable or disable all hooks for the `` hook event, + configured hooks and the traditional hook in the hooks directory alike. When set to `false`, no hooks fire for that event, regardless of any per-hook `hook..enabled` settings. Defaults to `true`. {see-git-hook} diff --git a/builtin/hook.c b/builtin/hook.c index cceeb3586e5daf..1ea698ecc20094 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -82,7 +82,9 @@ static int list(int argc, const char **argv, const char *prefix, switch (h->kind) { case HOOK_TRADITIONAL: - printf("%s%c", _("hook from hookdir"), line_terminator); + printf("%s%s%c", + h->event_disabled ? "event-disabled\t" : "", + _("hook from hookdir"), line_terminator); break; case HOOK_CONFIGURED: { const char *name = h->u.configured.friendly_name; @@ -95,7 +97,7 @@ static int list(int argc, const char **argv, const char *prefix, * "disabled" surfaces once the event is re-enabled. */ const char *disability = - h->u.configured.event_disabled ? "event-disabled\t" : + h->event_disabled ? "event-disabled\t" : h->u.configured.disabled ? "disabled\t" : ""; if (scope) diff --git a/hook.c b/hook.c index d10eef4763c679..5e756cb353033e 100644 --- a/hook.c +++ b/hook.c @@ -115,6 +115,9 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, h->kind = HOOK_TRADITIONAL; h->u.traditional.path = xstrdup(hook_path); + /* list_hooks_add_configured() has loaded r->disabled_events */ + h->event_disabled = !!unsorted_string_list_lookup(&r->disabled_events, + hookname); string_list_append(hook_list, hook_path)->util = h; } @@ -514,7 +517,7 @@ static void list_hooks_add_configured(struct repository *r, entry->command ? xstrdup(entry->command) : NULL; hook->u.configured.scope = entry->scope; hook->u.configured.disabled = entry->disabled; - hook->u.configured.event_disabled = event_is_disabled; + hook->event_disabled = event_is_disabled; hook->parallel = entry->parallel; string_list_append(list, friendly_name)->util = hook; @@ -559,8 +562,9 @@ int hook_exists(struct repository *r, const char *name) for (size_t i = 0; i < hooks->nr; i++) { struct hook *h = hooks->items[i].util; - if (h->kind == HOOK_TRADITIONAL || - (!h->u.configured.disabled && !h->u.configured.event_disabled)) { + if (!h->event_disabled && + (h->kind == HOOK_TRADITIONAL || + !h->u.configured.disabled)) { exists = 1; break; } @@ -583,8 +587,8 @@ static int pick_next_hook(struct child_process *cp, if (hook_cb->hook_to_run_index >= hook_list->nr) return 0; h = hook_list->items[hook_cb->hook_to_run_index++].util; - } while (h->kind == HOOK_CONFIGURED && - (h->u.configured.disabled || h->u.configured.event_disabled)); + } while (h->event_disabled || + (h->kind == HOOK_CONFIGURED && h->u.configured.disabled)); cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); diff --git a/hook.h b/hook.h index 27bb1aeb2ef465..6af24cb331ba09 100644 --- a/hook.h +++ b/hook.h @@ -32,10 +32,16 @@ struct hook { const char *command; enum config_scope scope; bool disabled; - bool event_disabled; } configured; } u; + /** + * Whether this hook's event is disabled with + * `hook..enabled = false`. This applies to configured hooks + * and the hookdir hook alike. + */ + bool event_disabled; + /** * Whether this hook may run in parallel with other hooks for the same * event. Only useful for configured (named) hooks. Traditional hooks diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 7d4b2982b4a6f7..45b99a7f048fd5 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -496,6 +496,15 @@ test_expect_success 'disabled configured hook is not reported as existing by hoo test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt ' +test_expect_success 'event-disabled hookdir hook is not reported as existing by hook_exists' ' + test_when_finished "rm -f git-bugreport-hook-exists-test.txt" && + setup_hookdir && + test_config hook.pre-commit.enabled false && + + git bugreport -s hook-exists-test && + test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt +' + test_expect_success 'globally disabled hook can be re-enabled locally' ' test_config_global hook.global-hook.event "test-hook" && test_config_global hook.global-hook.command "echo \"global-hook ran\"" && @@ -1110,9 +1119,13 @@ test_expect_success 'hook..jobs rejects values less than -1' ' test_expect_success 'hook..enabled=false skips all hooks for event' ' test_config hook.hook-1.event test-hook && test_config hook.hook-1.command "echo ran" && + test_hook test-hook <<-\EOF && + echo ran + EOF test_config hook.test-hook.enabled false && git hook run --allow-unknown-hook-name test-hook >out 2>err && - test_must_be_empty out + test_must_be_empty out && + test_must_be_empty err ' test_expect_success 'hook..enabled=true does not suppress hooks' ' @@ -1126,9 +1139,13 @@ test_expect_success 'hook..enabled=true does not suppress hooks' ' test_expect_success 'hook..enabled=false does not affect other events' ' test_config hook.hook-1.event test-hook && test_config hook.hook-1.command "echo ran" && + test_hook test-hook <<-\EOF && + echo hookdir ran + EOF test_config hook.other-event.enabled false && git hook run --allow-unknown-hook-name test-hook >out 2>err && - test_grep "ran" err + test_grep "^ran$" err && + test_grep "^hookdir ran$" err ' test_expect_success 'hook..enabled=false still disables that hook' ' @@ -1147,10 +1164,14 @@ test_expect_success 'git hook list shows event-disabled hooks as event-disabled' test_config hook.hook-1.command "echo ran" && test_config hook.hook-2.event test-hook && test_config hook.hook-2.command "echo ran" && + test_hook test-hook <<-\EOF && + echo ran + EOF test_config hook.test-hook.enabled false && git hook list --allow-unknown-hook-name test-hook >actual && test_grep "^event-disabled hook-1$" actual && - test_grep "^event-disabled hook-2$" actual + test_grep "^event-disabled hook-2$" actual && + test_grep "^event-disabled hook from hookdir$" actual ' test_expect_success 'git hook list shows scope with event-disabled' ' @@ -1169,6 +1190,14 @@ test_expect_success 'git hook list still shows hooks when event is disabled' ' test_grep "event-disabled" actual ' +test_expect_success 'commit skips the hookdir hook with hook.pre-commit.enabled=false' ' + test_hook pre-commit <<-\EOF && + exit 1 + EOF + test_must_fail git commit --allow-empty -m blocked && + git -c hook.pre-commit.enabled=false commit --allow-empty -m allowed +' + test_expect_success 'friendly-name matching known event name is rejected' ' test_config hook.pre-commit.event pre-commit && test_config hook.pre-commit.command "echo oops" && From 3a7784d0d5467ada7526356673c6653416ace302 Mon Sep 17 00:00:00 2001 From: Thomas Bachem Date: Fri, 23 Oct 2026 13:19:02 +0000 Subject: [PATCH 37/37] hook: accept .enabled and .parallel given without a value hook..enabled and hook..parallel are booleans, and git-config(1) documents a boolean without a value as true. But hook_config_lookup_all() rejects every hook.. that has no value, so "git -c hook.lint.parallel hook run pre-commit" fails with "missing value for 'hook.lint.parallel'", and so does a bare "parallel" under [hook "lint"]. The same happens with .enabled, for a hook as well as for an event. Keep rejecting a missing value for the other keys. For .enabled and .parallel, pass it to git_parse_maybe_bool(), which takes it as true. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem Signed-off-by: Junio C Hamano --- hook.c | 3 ++- t/t1800-hook.sh | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/hook.c b/hook.c index 5e756cb353033e..c500fec33ea7c7 100644 --- a/hook.c +++ b/hook.c @@ -184,7 +184,8 @@ static int hook_config_lookup_all(const char *key, const char *value, return 0; } - if (!value) + /* .enabled and .parallel are bools, where a missing value means true */ + if (!value && strcmp(subkey, "enabled") && strcmp(subkey, "parallel")) return config_error_nonbool(key); /* Extract name, ensuring it is null-terminated. */ diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 45b99a7f048fd5..fca4cfc7125f11 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -516,6 +516,17 @@ test_expect_success 'globally disabled hook can be re-enabled locally' ' test_cmp expected actual ' +test_expect_success 'hook..enabled without a value means true' ' + test_config_global hook.global-hook.event "test-hook" && + test_config_global hook.global-hook.command "echo \"global-hook ran\"" && + test_config_global hook.global-hook.enabled false && + + echo "global-hook ran" >expected && + git -c hook.global-hook.enabled \ + hook run --allow-unknown-hook-name test-hook 2>actual && + test_cmp expected actual +' + test_expect_success 'configured hooks run before hookdir hook' ' setup_hookdir && test_config hook.first.event "pre-commit" && @@ -1080,6 +1091,17 @@ test_expect_success 'hook.jobs=-1 resolves to online_cpus()' ' test_grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:$cpus\"" trace.txt ' +test_expect_success 'hook..parallel without a value means true' ' + test_config hook.hook-1.event test-hook && + test_config hook.hook-1.command "true" && + test_config hook.jobs 2 && + + GIT_TRACE2_EVENT="$(pwd)/trace-parallel.txt" \ + git -c hook.hook-1.parallel \ + hook run --allow-unknown-hook-name test-hook >out 2>err && + test_grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:2\"" trace-parallel.txt +' + test_expect_success 'hook..jobs=-1 resolves to online_cpus()' ' test_config hook.hook-1.event test-hook && test_config hook.hook-1.command "true" &&