From 27be716454cc962b1c790fc845cb2d2f46a078eb Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Thu, 13 Aug 2026 21:05:02 +0200 Subject: [PATCH 01/34] completion: add 'git history' subcommands Use the parse-options completion helpers for the git history subcommands and their options. All current history subcommands take a revision as their first positional argument, so complete that argument as a revision. Once the revision is present, leave any further positional arguments to subcommand-specific completion. This allows a subcommand to complete another kind of argument, such as the pathspec accepted by git history split or another revision if a future subcommand accepts one. Signed-off-by: Vincent Mailhol Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 45 ++++++++++++++++++++++++++ t/t9902-completion.sh | 30 +++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index e8757877104eb9..17277684872388 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2137,6 +2137,51 @@ _git_help () fi } +__git_history_has_revision () +{ + local i + + for ((i = __git_cmd_idx + 2; i < cword; i++)); do + case "${words[i]}" in + -*) + ;; + *) + return 0 + ;; + esac + done + return 1 +} + +_git_history () +{ + local subcommands subcommand + + __git_resolve_builtins "history" + + subcommands="$___git_resolved_builtins" + subcommand="$(__git_find_subcommand "$subcommands")" + + if [ -z "$subcommand" ]; then + __gitcomp "$subcommands" + return + fi + + if ! __git_has_doubledash; then + case "$cur" in + --*) + __gitcomp_builtin "history_$subcommand" + return + ;; + esac + fi + + if ! __git_history_has_revision; then + __git_complete_refs + return + fi +} + _git_init () { case "$cur" in diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 9ae3c48ebdd5e9..d0d8f2ba4ab441 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -3107,6 +3107,36 @@ test_expect_success 'git clone --config= - value' ' EOF ' +test_expect_success 'git history subcommands' ' + test_completion "git history " <<-\EOF && + drop Z + fixup Z + reword Z + split Z + EOF + test_completion "git history --" "" +' + +test_expect_success 'git history subcommand options' ' + test_completion "git history split main --" <<-\EOF && + --update-refs=Z + --dry-run Z + --no-dry-run Z + EOF + test_completion "git history fixup --upd" "--update-refs=" && + test_completion "git history fixup --ree" "--reedit-message " && + test_completion "git history split --upd" "--update-refs=" && + test_completion "git history split main --dry" "--dry-run " && + test_completion "git history reword main -- --d" "" +' + +test_expect_success 'git history revisions' ' + test_completion "git history split ma" "main " && + test_completion "git history split --update-refs=head ma" "main " && + test_completion "git history fixup --empty=drop ma" "main " && + test_completion "git history reword main m" "" +' + test_expect_success 'git reflog show' ' test_when_finished "git checkout - && git branch -d shown" && git checkout -b shown && From 8231551929aa7fa2043c18753c736829c88df8d7 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Thu, 13 Aug 2026 21:05:03 +0200 Subject: [PATCH 02/34] completion: complete 'git history --empty' values The "--empty" option accepts "drop", "keep", or "abort" for the "drop" and "fixup" subcommands. Complete these values for the documented --empty= form. While parse-options also accepts the split --empty form, it is not documented. Omit it from completion as a trade-off for code simplicity. Signed-off-by: Vincent Mailhol Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 9 +++++++++ t/t9902-completion.sh | 6 +++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 17277684872388..7f3cabd595c15b 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2169,6 +2169,15 @@ _git_history () if ! __git_has_doubledash; then case "$cur" in + --empty=*) + case "$subcommand" in + drop|fixup) + __gitcomp "drop keep abort" "" \ + "${cur##--empty=}" + ;; + esac + return + ;; --*) __gitcomp_builtin "history_$subcommand" return diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index d0d8f2ba4ab441..851be383e1f158 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -3127,7 +3127,11 @@ test_expect_success 'git history subcommand options' ' test_completion "git history fixup --ree" "--reedit-message " && test_completion "git history split --upd" "--update-refs=" && test_completion "git history split main --dry" "--dry-run " && - test_completion "git history reword main -- --d" "" + test_completion "git history reword main -- --d" "" && + test_completion "git history fixup --empty=ke" "keep " && + test_completion "git history fixup --empty=drop" "drop " && + test_completion "git history drop --empty=ab" "abort " && + test_completion "git history reword --empty=ke" "" ' test_expect_success 'git history revisions' ' From 5bda733b18e838557d8956a6f5d25c5c6f7e3b34 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Thu, 13 Aug 2026 21:05:04 +0200 Subject: [PATCH 03/34] completion: complete 'git history --update-refs' values The "--update-refs" option accepts either "branches" or "head". Complete these values for the documented --update-refs= form. While parse-options also accepts the split --update-refs form, it is not documented. Omit it from completion as a trade-off for code simplicity. Signed-off-by: Vincent Mailhol Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 5 +++++ t/t9902-completion.sh | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 7f3cabd595c15b..19600940dcdecd 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2178,6 +2178,11 @@ _git_history () esac return ;; + --update-refs=*) + __gitcomp "branches head" "" \ + "${cur##--update-refs=}" + return + ;; --*) __gitcomp_builtin "history_$subcommand" return diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 851be383e1f158..b225dd3800a49f 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -3131,7 +3131,10 @@ test_expect_success 'git history subcommand options' ' test_completion "git history fixup --empty=ke" "keep " && test_completion "git history fixup --empty=drop" "drop " && test_completion "git history drop --empty=ab" "abort " && - test_completion "git history reword --empty=ke" "" + test_completion "git history reword --empty=ke" "" && + test_completion "git history fixup --update-refs=branch" "branches " && + test_completion "git history split --update-refs=he" "head " && + test_completion "git history reword main -- --update-refs=he" "" ' test_expect_success 'git history revisions' ' From 328b3b9d6288b68fdabad2e8b8b38bd7492b6228 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Thu, 13 Aug 2026 21:05:05 +0200 Subject: [PATCH 04/34] completion: complete 'git history split' pathspecs Arguments following the required revision of "git history split" are pathspecs. Complete them from tracked paths, including after an explicit "--". Signed-off-by: Vincent Mailhol Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 6 ++++++ t/t9902-completion.sh | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 19600940dcdecd..6172b6182f5089 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2194,6 +2194,12 @@ _git_history () __git_complete_refs return fi + + case "$subcommand" in + split) + __git_complete_index_file "--cached" + ;; + esac } _git_init () diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index b225dd3800a49f..194bca8d6c40f0 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -3144,6 +3144,19 @@ test_expect_success 'git history revisions' ' test_completion "git history reword main m" "" ' +test_expect_success 'git history split pathspecs' ' + test_completion "git history split main -- --update-refs=h" "" && + test_completion "git history split main -- --update-refs h" "" && + test_completion "git history split --dry-run main file" <<-\EOF && + file1Z + file2Z + EOF + test_completion "git history split main -- file" <<-\EOF + file1Z + file2Z + EOF +' + test_expect_success 'git reflog show' ' test_when_finished "git checkout - && git branch -d shown" && git checkout -b shown && From 1c46ce6dda5c58301af6a8b7a27e68fd7fb86993 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 17 Aug 2026 13:09:21 +0200 Subject: [PATCH 05/34] setup: create ref and object databases after config is written When creating a new repository we create both the reference and object databases after we have finalized the repository. This ensures that those subsystems find a fully-configured repository at the time where they are asked to create their own on-disk data structures. There is one exception though: while we have already fully configured the repository at this point, we haven't yet written both "core.sharedRepository" and "receive.denyNonFastforwards". The latter configuration doesn't really matter to us, but the first one does as the "files" object database source reads it. This doesn't cause any problems right now, but it will in a subsequent patch where we will start to read "core.ignoreCase" when creating the object database. Move the initialization of both of these data structures towards the end of `init_db()`. The only thing that now comes after is status reporting, but that's it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/setup.c b/setup.c index 20d29f31f428b1..d90654f5842deb 100644 --- a/setup.c +++ b/setup.c @@ -2880,12 +2880,6 @@ int init_db(struct repository *repo, reinit = create_default_files(repo, template_dir, original_git_dir, &repo_fmt, init_shared_repository); - if (!(flags & INIT_DB_SKIP_REFDB)) - create_reference_database(repo, initial_branch, flags & INIT_DB_QUIET); - create_object_database(repo); - - startup_info->have_repository = 1; - if (repo_settings_get_shared_repository(repo)) { char buf[10]; /* We do not spell "group" and such, so that @@ -2907,6 +2901,12 @@ int init_db(struct repository *repo, repo_config_set(repo, "receive.denyNonFastforwards", "true"); } + if (!(flags & INIT_DB_SKIP_REFDB)) + create_reference_database(repo, initial_branch, flags & INIT_DB_QUIET); + create_object_database(repo); + + startup_info->have_repository = 1; + if (!(flags & INIT_DB_QUIET)) { int len = strlen(git_dir); From 987927709135a2267abfa929a6f20ddd4302c8b7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 17 Aug 2026 13:09:22 +0200 Subject: [PATCH 06/34] odb: decouple source path comparisons from `the_repository` When registering alternates we deduplicate object database sources by their path so that the same source won't be added twice. Ever since cf2dc1c238 (speed up alt_odb_usable() with many alternates, 2021-07-07) this duplicate check is backed by a map keyed by the source's path, using `fspathhash()` and `fspatheq()` as hash and equality functions, respectively. These functions are problematic in this context for two reasons: - They implicitly depend on `the_repository` instead of the repository that owns the object database. - They derive case-sensitivity from `repo_ignore_case()`, which returns a default value in case the repository's configuration has not been parsed yet. Object database sources may be registered before that is the case, so the answer may flip depending on when a source gets registered. Fix this by making the comparison self-contained in the object database. Instead of using `fspathhash()` and `fspatheq()` we resolve "core.ignoreCase" manually and then use the correct comparison function based on the result. This requires us to migrate to a `struct hashmap`, as the khash interface does not give us the ability to pass an arbitrary payload to these functions, and hence we'd have to use global state to decide which of those to use. Note that we can unconditionally use `strihash()` to compute entry hashes regardless of case sensitivity: a hash function only needs to guarantee that equal keys have equal hashes, and a case-insensitive hash satisfies this requirement for both case-sensitive and case-insensitive equality. Overall it's quite debatable whether all of this complexity really is worth it, out of two reasons: - We could linearly search through all sources to find duplicates. But the mentioned commit cares about cases with thousands of alternates, and a linear search would of course regress performance quite a bit. This doesn't really feel like a reasonable case to care about, but I don't feel comfortable regressing it anyway. - It's dubious whether we should handle "core.ignoreCase" in the first place. The downside would be that we might add the same alternate multiple times with different casing. But this is an edge case, and it's not even fully fixed because we don't resolve symlinks or mountpoints, either. So for now, keep this infrastructure in-place while removing the global dependency on `the_repository`. We may want to revisit this in the future though. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 78 ++++++++++++++++++++++++++++++++++++++-------------- odb.h | 15 +++++++++- odb/source.h | 7 +++++ 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/odb.c b/odb.c index bd02d8ad540913..22f1425ba5c800 100644 --- a/odb.c +++ b/odb.c @@ -2,11 +2,10 @@ #include "abspath.h" #include "commit-graph.h" #include "config.h" -#include "dir.h" #include "environment.h" #include "gettext.h" +#include "hashmap.h" #include "hex.h" -#include "khash.h" #include "lockfile.h" #include "loose.h" #include "midx.h" @@ -29,8 +28,47 @@ #include "trace2.h" #include "write-or-die.h" -KHASH_INIT(odb_path_map, const char * /* key: odb_path */, - struct odb_source *, 1, fspathhash, fspatheq) +/* + * NEEDSWORK: we're using "core.ignoreCase" to deduplicate alternates that + * _may_ be the same. This requires quite a bit of boilerplate for dubious + * benefit: + * + * - Duplicating alternates should really only lead to regressed performance. + * + * - We don't properly resolve symlinks or mointpoints, so we may still end + * up duplicating alternates. + * + * - The value may be lying, in which case we might deduplicate alternates + * that are in fact not mapping to the same directory. + * + * We should investigate whether we can remove this whole mechanism outright. + */ +static int odb_source_paths_cmp(struct object_database *o, + const char *a, const char *b) +{ + if (o->source_paths_icase < 0) { + int icase = 0; + repo_config_get_bool(o->repo, "core.ignorecase", &icase); + o->source_paths_icase = icase; + } + + return o->source_paths_icase ? strcasecmp(a, b) : strcmp(a, b); +} + +static int odb_source_by_path_cmp(const void *cb_data, + const struct hashmap_entry *entry, + const struct hashmap_entry *entry_or_key, + const void *keydata) +{ + struct object_database *o = (struct object_database *)cb_data; + const struct odb_source *source = container_of(entry, const struct odb_source, by_path_entry); + const char *path = keydata; + + if (!path) + path = container_of(entry_or_key, const struct odb_source, by_path_entry)->path; + + return odb_source_paths_cmp(o, source->path, path); +} int odb_mkstemp(struct object_database *odb, struct strbuf *temp_filename, const char *pattern) @@ -58,8 +96,8 @@ int odb_mkstemp(struct object_database *odb, */ static bool odb_is_source_usable(struct object_database *o, const char *path) { - int r; struct strbuf normalized_objdir = STRBUF_INIT; + struct hashmap_entry key; bool usable = false; strbuf_realpath(&normalized_objdir, o->sources->path, 1); @@ -76,20 +114,18 @@ static bool odb_is_source_usable(struct object_database *o, const char *path) * Prevent the common mistake of listing the same * thing twice, or object directory itself. */ - if (!o->source_by_path) { - khiter_t p; - - o->source_by_path = kh_init_odb_path_map(); + if (!hashmap_get_size(&o->source_by_path)) { assert(!o->sources->next); - p = kh_put_odb_path_map(o->source_by_path, o->sources->path, &r); - assert(r == 1); /* never used */ - kh_value(o->source_by_path, p) = o->sources; + hashmap_entry_init(&o->sources->by_path_entry, + strihash(o->sources->path)); + hashmap_add(&o->source_by_path, &o->sources->by_path_entry); } - if (fspatheq(path, normalized_objdir.buf)) + if (!odb_source_paths_cmp(o, path, normalized_objdir.buf)) goto out; - if (kh_get_odb_path_map(o->source_by_path, path) < kh_end(o->source_by_path)) + hashmap_entry_init(&key, strihash(path)); + if (hashmap_get(&o->source_by_path, &key, path)) goto out; usable = true; @@ -172,8 +208,6 @@ static struct odb_source *odb_add_alternate_recursively(struct object_database * { struct odb_source *alternate = NULL; struct strvec sources = STRVEC_INIT; - khiter_t pos; - int ret; if (!odb_is_source_usable(odb, source)) goto error; @@ -184,10 +218,11 @@ static struct odb_source *odb_add_alternate_recursively(struct object_database * *odb->sources_tail = alternate; odb->sources_tail = &(alternate->next); - pos = kh_put_odb_path_map(odb->source_by_path, alternate->path, &ret); - if (!ret) + hashmap_entry_init(&alternate->by_path_entry, strihash(alternate->path)); + if (hashmap_get(&odb->source_by_path, &alternate->by_path_entry, + alternate->path)) BUG("source must not yet exist"); - kh_value(odb->source_by_path, pos) = alternate; + hashmap_add(&odb->source_by_path, &alternate->by_path_entry); /* recursively add alternates */ odb_source_read_alternates(alternate, &sources); @@ -1056,6 +1091,8 @@ struct object_database *odb_new(struct repository *repo, o->repo = repo; pthread_mutex_init(&o->replace_mutex, NULL); string_list_init_dup(&o->submodule_source_paths); + hashmap_init(&o->source_by_path, odb_source_by_path_cmp, o, 0); + o->source_paths_icase = -1; if (flags & ODB_NEW_HONOR_ENV) { primary_source = xstrdup_or_null(getenv(DB_ENVIRONMENT)); @@ -1094,8 +1131,7 @@ static void odb_free_sources(struct object_database *o) odb_source_free(o->inmemory_objects); o->inmemory_objects = NULL; - kh_destroy_odb_path_map(o->source_by_path); - o->source_by_path = NULL; + hashmap_clear(&o->source_by_path); } void odb_free(struct object_database *o) diff --git a/odb.h b/odb.h index 8eb4e85d6481a7..71af7450a91308 100644 --- a/odb.h +++ b/odb.h @@ -1,6 +1,7 @@ #ifndef ODB_H #define ODB_H +#include "hashmap.h" #include "object.h" #include "oidset.h" #include "oidmap.h" @@ -54,7 +55,19 @@ struct object_database { */ struct odb_source *sources; struct odb_source **sources_tail; - struct kh_odb_path_map *source_by_path; + + /* + * Map of object database sources, keyed by their respective paths. + * This map is used to detect the case where the same source is + * registered multiple times. + */ + struct hashmap source_by_path; + + /* + * Whether source paths shall be compared case-insensitively, as + * determined by "core.ignoreCase". + */ + int source_paths_icase; int loaded_alternates; diff --git a/odb/source.h b/odb/source.h index 4bc037b8d69ace..82cda8ad756343 100644 --- a/odb/source.h +++ b/odb/source.h @@ -1,6 +1,7 @@ #ifndef ODB_SOURCE_H #define ODB_SOURCE_H +#include "hashmap.h" #include "object.h" #include "odb.h" #include "odb/transaction.h" @@ -50,6 +51,12 @@ struct strvec; struct odb_source { struct odb_source *next; + /* + * Entry in the object database's map of sources, keyed by this + * source's path. + */ + struct hashmap_entry by_path_entry; + /* Object database that owns this object source. */ struct object_database *odb; From f978f560dd7f0c92ed1834198747da621400350a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 17 Aug 2026 13:09:23 +0200 Subject: [PATCH 07/34] odb: eagerly initialize alternates When creating the object database we initialize the main object database source, but we don't yet initialize its alternates. Instead, we have many calls to `odb_prepare_alternates()` cluttered around the code base whenever we are about to iterate through the sources. This lazy loading doesn't really add much value: the moment where we read any object we _have_ to load the alternates anyway. So given that most of our commands would access the object database this optimization is not really buying us much in the first place. Quite on the contrary, it makes the code harder to understand and is a potential source of bugs in case any callsite forgot to prepare alternates before we iterate through the sources. Historically though there was a reason why we deferred lazy-loading: it may happen that the repository has "core.ignoreCase" configured, and we use that to deduplicate the list of alternates in case we had the same alternate configured multiple times, but with different casing. We used to initialize the object database before we had fully configured the owning repository though, and consequently we couldn't access that configuration yet. This has changed in the preceding commit though where we started to parse "core.ignoreCase" manually. Eagerly prepare alternates both when creating the object database and when flushing its caches. Drop the now-unneeded calls to prepare the alternates that are scattered across the code base. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fsck.c | 3 --- builtin/pack-objects.c | 3 --- commit-graph.c | 4 ---- loose.c | 1 - object-name.c | 1 - odb.c | 26 ++++---------------------- odb.h | 6 ------ odb/streaming.c | 1 - pack-bitmap.c | 2 -- packfile.c | 1 - packfile.h | 2 -- 11 files changed, 4 insertions(+), 46 deletions(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index a6c054e45bf8c0..892c5661d93668 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -1069,7 +1069,6 @@ int cmd_fsck(int argc, odb_for_each_object(repo->objects, NULL, mark_object_for_connectivity, repo, 0); } else { - odb_prepare_alternates(repo->objects); for (source = repo->objects->sources; source; source = source->next) fsck_source(repo, source); @@ -1155,7 +1154,6 @@ int cmd_fsck(int argc, if (repo->settings.core_commit_graph) { struct child_process commit_graph_verify = CHILD_PROCESS_INIT; - odb_prepare_alternates(repo->objects); for (source = repo->objects->sources; source; source = source->next) { child_process_init(&commit_graph_verify); commit_graph_verify.git_cmd = 1; @@ -1173,7 +1171,6 @@ int cmd_fsck(int argc, if (repo->settings.core_multi_pack_index) { struct child_process midx_verify = CHILD_PROCESS_INIT; - odb_prepare_alternates(repo->objects); for (source = repo->objects->sources; source; source = source->next) { child_process_init(&midx_verify); midx_verify.git_cmd = 1; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1ec5b6f206366e..48d37e8e3235b5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1779,8 +1779,6 @@ static int want_object_in_pack_mtime(const struct object_id *oid, *found_offset = 0; } - odb_prepare_alternates(the_repository->objects); - for (source = the_repository->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); struct multi_pack_index *m = get_multi_pack_index(files->packed); @@ -4520,7 +4518,6 @@ static void add_objects_in_unpacked_packs(void) .source_infop = &source_info, }; - odb_prepare_alternates(to_pack.repo->objects); for (source = to_pack.repo->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); diff --git a/commit-graph.c b/commit-graph.c index 49e8f639305212..983c11ce853459 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -651,8 +651,6 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct object_database *odb, count = st->st_size / (odb->repo->hash_algo->hexsz + 1); CALLOC_ARRAY(oids, count); - odb_prepare_alternates(odb); - for (i = 0; i < count; i++) { struct odb_source *source; @@ -768,7 +766,6 @@ static struct commit_graph *prepare_commit_graph(struct repository *r) if (!commit_graph_compatible(r)) return NULL; - odb_prepare_alternates(r->objects); for (source = r->objects->sources; source; source = source->next) { r->objects->commit_graph = read_commit_graph_one(source); if (r->objects->commit_graph) @@ -2018,7 +2015,6 @@ static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx) _("Finding commits for commit graph among packed objects"), ctx->approx_nr_objects); - odb_prepare_alternates(ctx->r->objects); for (source = ctx->r->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); odb_source_for_each_object(&files->packed->base, &oi, add_packed_commits_oi, diff --git a/loose.c b/loose.c index aa3cb1b4fce43b..c159d29d2d2035 100644 --- a/loose.c +++ b/loose.c @@ -115,7 +115,6 @@ int repo_read_loose_object_map(struct repository *repo) { struct odb_source *source; - odb_prepare_alternates(repo->objects); for (source = repo->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); if (loose_object_map_load(files->loose) < 0) diff --git a/object-name.c b/object-name.c index 83efba0ba668e5..34a08d76dd8e34 100644 --- a/object-name.c +++ b/object-name.c @@ -280,7 +280,6 @@ static int init_object_disambiguation(struct repository *r, ds->len = len; ds->repo = r; - odb_prepare_alternates(r->objects); return 0; } diff --git a/odb.c b/odb.c index 22f1425ba5c800..d4917c36781e8b 100644 --- a/odb.c +++ b/odb.c @@ -252,11 +252,6 @@ void odb_add_to_alternates_file(struct object_database *odb, struct odb_source *odb_add_to_alternates_memory(struct object_database *odb, const char *dir) { - /* - * Make sure alternates are initialized, or else our entry may be - * overwritten when they are. - */ - odb_prepare_alternates(odb); return odb_add_alternate_recursively(odb, dir, 0); } @@ -265,12 +260,6 @@ struct odb_source *odb_set_temporary_primary_source(struct object_database *odb, { struct odb_source *source; - /* - * Make sure alternates are initialized, or else our entry may be - * overwritten when they are. - */ - odb_prepare_alternates(odb); - /* * Make a new primary odb and link the old primary ODB in as an * alternate @@ -376,7 +365,6 @@ struct odb_source *odb_find_source(struct object_database *odb, const char *obj_ char *obj_dir_real = real_pathdup(obj_dir, 1); struct strbuf odb_path_real = STRBUF_INIT; - odb_prepare_alternates(odb); for (source = odb->sources; source; source = source->next) { strbuf_realpath(&odb_path_real, source->path, 1); if (!strcmp(obj_dir_real, odb_path_real.buf)) @@ -510,7 +498,6 @@ int odb_for_each_alternate(struct object_database *odb, struct odb_source *alternate; int r = 0; - odb_prepare_alternates(odb); for (alternate = odb->sources->next; alternate; alternate = alternate->next) { r = cb(alternate, payload); if (r) @@ -519,7 +506,7 @@ int odb_for_each_alternate(struct object_database *odb, return r; } -void odb_prepare_alternates(struct object_database *odb) +static void odb_prepare_alternates(struct object_database *odb) { struct strvec sources = STRVEC_INIT; @@ -538,7 +525,6 @@ void odb_prepare_alternates(struct object_database *odb) int odb_has_alternates(struct object_database *odb) { - odb_prepare_alternates(odb); return !!odb->sources->next; } @@ -598,8 +584,6 @@ static int do_oid_object_info_extended(struct object_database *odb, if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags)) return 0; - odb_prepare_alternates(odb); - while (1) { struct odb_source *source; @@ -862,7 +846,6 @@ int odb_freshen_object(struct object_database *odb, const struct object_id *oid) { struct odb_source *source; - odb_prepare_alternates(odb); for (source = odb->sources; source; source = source->next) if (odb_source_freshen_object(source, oid, NULL)) return 1; @@ -877,7 +860,6 @@ int odb_for_each_object_ext(struct object_database *odb, { int ret; - odb_prepare_alternates(odb); for (struct odb_source *source = odb->sources; source; source = source->next) { if (opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY && !source->local) continue; @@ -915,7 +897,6 @@ int odb_count_objects(struct object_database *odb, return 0; } - odb_prepare_alternates(odb); for (source = odb->sources; source; source = source->next) { unsigned long c; @@ -995,7 +976,6 @@ int odb_find_abbrev_len(struct object_database *odb, goto out; } - odb_prepare_alternates(odb); for (struct odb_source *source = odb->sources; source; source = source->next) { ret = odb_source_find_abbrev_len(source, oid, len, &len); if (ret) @@ -1106,6 +1086,8 @@ struct object_database *odb_new(struct repository *repo, o->alternate_db = secondary_sources; o->inmemory_objects = &odb_source_inmemory_new(o)->base; + odb_prepare_alternates(o); + free(primary_source); return o; } @@ -1166,10 +1148,10 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags) */ if (flags & ODB_PREPARE_FLUSH_CACHES) { o->loaded_alternates = 0; + odb_prepare_alternates(o); o->object_count_valid = 0; } - odb_prepare_alternates(o); for (source = o->sources; source; source = source->next) odb_source_prepare(source, flags); diff --git a/odb.h b/odb.h index 71af7450a91308..fbafee174bc779 100644 --- a/odb.h +++ b/odb.h @@ -273,12 +273,6 @@ void odb_for_each_alternate_ref(struct object_database *odb, int odb_mkstemp(struct object_database *odb, struct strbuf *temp_filename, const char *pattern); -/* - * Prepare alternate object sources for the given database by reading - * "objects/info/alternates" and opening the respective sources. - */ -void odb_prepare_alternates(struct object_database *odb); - /* * Check whether the object database has any alternates. The primary object * source does not count as alternate. diff --git a/odb/streaming.c b/odb/streaming.c index 20531e864c9561..37642768e92c44 100644 --- a/odb/streaming.c +++ b/odb/streaming.c @@ -184,7 +184,6 @@ static int istream_source(struct odb_read_stream **out, { struct odb_source *source; - odb_prepare_alternates(odb); for (source = odb->sources; source; source = source->next) if (!odb_source_read_object_stream(out, source, oid)) return 0; diff --git a/pack-bitmap.c b/pack-bitmap.c index e85bd69ba446aa..e0fb57d3321889 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -717,7 +717,6 @@ static int open_bitmap(struct repository *r, assert(!bitmap_git->map); - odb_prepare_alternates(r->objects); for (source = r->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); @@ -3417,7 +3416,6 @@ int verify_bitmap_files(struct repository *r) struct packed_git *p; int res = 0; - odb_prepare_alternates(r->objects); for (source = r->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); struct multi_pack_index *m = get_multi_pack_index(files->packed); diff --git a/packfile.c b/packfile.c index 0eee45055f833e..d870de90ed044b 100644 --- a/packfile.c +++ b/packfile.c @@ -1938,7 +1938,6 @@ int has_object_pack(struct repository *r, const struct object_id *oid) { struct odb_source *source; - odb_prepare_alternates(r->objects); for (source = r->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); if (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0)) diff --git a/packfile.h b/packfile.h index e1f77152b5c4bf..10de24f47739c2 100644 --- a/packfile.h +++ b/packfile.h @@ -77,8 +77,6 @@ static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct { struct repo_for_each_pack_data data = { 0 }; - odb_prepare_alternates(repo->objects); - for (struct odb_source *source = repo->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); struct packfile_list_entry *entry = packfile_store_get_packs(files->packed); From 0e67428c8580c461317768f7fe9916c71435d9e3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 17 Aug 2026 13:09:24 +0200 Subject: [PATCH 08/34] odb: drop `loaded_alternates` field The `struct object_database::loaded_alternates` field tells us whether or not alternates have been loaded already. This field was useful before the preceding commit as we were indeed lazy-loading alternates. But now that we started to eagerly load them we can assume them to be loaded after `odb_new()`, and hence the field does not serve any purpose anymore. Remove it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 9 +-------- odb.h | 2 -- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/odb.c b/odb.c index d4917c36781e8b..ada42f864bcdb7 100644 --- a/odb.c +++ b/odb.c @@ -245,8 +245,7 @@ void odb_add_to_alternates_file(struct object_database *odb, int ret = odb_source_write_alternate(odb->sources, dir); if (ret < 0) die(NULL); - if (odb->loaded_alternates) - odb_add_alternate_recursively(odb, dir, 0); + odb_add_alternate_recursively(odb, dir, 0); } struct odb_source *odb_add_to_alternates_memory(struct object_database *odb, @@ -510,16 +509,11 @@ static void odb_prepare_alternates(struct object_database *odb) { struct strvec sources = STRVEC_INIT; - if (odb->loaded_alternates) - return; - parse_alternates(odb->alternate_db, PATH_SEP, NULL, &sources); odb_source_read_alternates(odb->sources, &sources); for (size_t i = 0; i < sources.nr; i++) odb_add_alternate_recursively(odb, sources.v[i], 0); - odb->loaded_alternates = 1; - strvec_clear(&sources); } @@ -1147,7 +1141,6 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags) * the lifetime of the process. */ if (flags & ODB_PREPARE_FLUSH_CACHES) { - o->loaded_alternates = 0; odb_prepare_alternates(o); o->object_count_valid = 0; } diff --git a/odb.h b/odb.h index fbafee174bc779..aefb34213f4bbf 100644 --- a/odb.h +++ b/odb.h @@ -69,8 +69,6 @@ struct object_database { */ int source_paths_icase; - int loaded_alternates; - /* * A list of alternate object directories loaded from the environment; * this should not generally need to be accessed directly, but will From 0076dc9f8141bd864e24a4d160b58be7c0597ce7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 17 Aug 2026 13:09:25 +0200 Subject: [PATCH 09/34] odb: drop `alternates_db` field The `struct object_database::alternates_db` field tracks the value of the "GIT_ALTERNATE_OBJECT_DIRECTORIES" environment variable and is used in `odb_prepare_alternates()`. It's not necessary to store it as a separate field anymore though, as we stopped lazy-loading alternates. Consequently, we can simply pass it to `odb_prepare_alternates()` via `odb_new()` now. Do so and remove the field. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 17 +++++++++-------- odb.h | 7 ------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/odb.c b/odb.c index ada42f864bcdb7..115957e983c73b 100644 --- a/odb.c +++ b/odb.c @@ -505,12 +505,14 @@ int odb_for_each_alternate(struct object_database *odb, return r; } -static void odb_prepare_alternates(struct object_database *odb) +static void odb_prepare_alternates(struct object_database *odb, + const char *alternate_db) { struct strvec sources = STRVEC_INIT; - parse_alternates(odb->alternate_db, PATH_SEP, NULL, &sources); + parse_alternates(alternate_db, PATH_SEP, NULL, &sources); odb_source_read_alternates(odb->sources, &sources); + for (size_t i = 0; i < sources.nr; i++) odb_add_alternate_recursively(odb, sources.v[i], 0); @@ -1077,11 +1079,11 @@ struct object_database *odb_new(struct repository *repo, o->sources = odb_source_new(o, primary_source, true); o->sources_tail = &o->sources->next; - o->alternate_db = secondary_sources; o->inmemory_objects = &odb_source_inmemory_new(o)->base; - odb_prepare_alternates(o); + odb_prepare_alternates(o, secondary_sources); + free(secondary_sources); free(primary_source); return o; } @@ -1115,8 +1117,6 @@ void odb_free(struct object_database *o) if (!o) return; - free(o->alternate_db); - oidmap_clear(&o->replace_map, 1); pthread_mutex_destroy(&o->replace_mutex); @@ -1138,10 +1138,11 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags) * Reprepare alt odbs, in case the alternates file was modified * during the course of this process. This only _adds_ odbs to * the linked list, so existing odbs will continue to exist for - * the lifetime of the process. + * the lifetime of the process. Consequently, we don't have to + * reprocess GIT_ALTERNATE_OBJECT_DIRECTORIES here. */ if (flags & ODB_PREPARE_FLUSH_CACHES) { - odb_prepare_alternates(o); + odb_prepare_alternates(o, NULL); o->object_count_valid = 0; } diff --git a/odb.h b/odb.h index aefb34213f4bbf..748366a61007e5 100644 --- a/odb.h +++ b/odb.h @@ -69,13 +69,6 @@ struct object_database { */ int source_paths_icase; - /* - * A list of alternate object directories loaded from the environment; - * this should not generally need to be accessed directly, but will - * populate the "sources" list when odb_prepare_alternates() is run. - */ - char *alternate_db; - /* * Objects that should be substituted by other objects * (see git-replace(1)). From 47ef2193b319d7ee4fffbcbd56ba912420a3460a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 19 Aug 2026 14:17:19 +0200 Subject: [PATCH 10/34] odb/source-packed: flag known-bad objects as corrupt and not missing When reading packed objects we know to tell apart missing objects and corrupt objects by returning a positive error code in the former case, and a negative one in the latter case. We do that by distinguishing between errors returned by `find_pack_entry()`, which yields the offset of the object, and `packed_object_info()`, which reads the object contents. But even though we already distinguish those cases when reading packed objects, the logic is broken in case a caller tries to read an object that has been marked as corrupt. In that case, `find_pack_entry()` will tell us that the object in question does not exist, and consequently we'll not flag the object as corrupt but as missing. Fix this issue by bubbling up whether the object is corrupt and, if so, which packfile contains the corrupted object. Note that we don't yet need the information about the specific packfile, so we could've just as well made this a `bool *corrupted` pointer. But we'll need information about the containing packfile in a subsequent commit so that we can generate a proper error message telling the user which packfile contains the broken object. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 2 +- midx.c | 10 +++++++--- midx.h | 3 ++- odb/source-packed.c | 22 ++++++++++++++++------ packfile.c | 10 +++++++--- packfile.h | 3 ++- t/helper/test-read-midx.c | 2 +- 7 files changed, 36 insertions(+), 16 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1ec5b6f206366e..10c2471024b74a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid, struct multi_pack_index *m = get_multi_pack_index(files->packed); struct pack_entry e; - if (m && fill_midx_entry(m, oid, &e)) { + if (m && fill_midx_entry(m, oid, &e, NULL)) { want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime); if (want != -1) return want; diff --git a/midx.c b/midx.c index 76c3f92cc374b8..37f082dbdd5558 100644 --- a/midx.c +++ b/midx.c @@ -591,7 +591,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos) int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, - struct pack_entry *e) + struct pack_entry *e, + struct packed_git **bad_pack) { uint32_t pos; uint32_t pack_int_id; @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m, return 0; if (oidset_size(&p->bad_objects) && - oidset_contains(&p->bad_objects, oid)) + oidset_contains(&p->bad_objects, oid)) { + if (bad_pack && !*bad_pack) + *bad_pack = p; return 0; + } e->offset = nth_midxed_offset(m, pos); e->p = p; @@ -1028,7 +1032,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) nth_midxed_object_oid(&oid, m, pairs[i].pos); - if (!fill_midx_entry(m, &oid, &e)) { + if (!fill_midx_entry(m, &oid, &e, NULL)) { midx_report(_("failed to load pack entry for oid[%d] = %s"), pairs[i].pos, oid_to_hex(&oid)); continue; diff --git a/midx.h b/midx.h index 939c18e5885e6f..1f2f2d53214da5 100644 --- a/midx.h +++ b/midx.h @@ -117,7 +117,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos); struct object_id *nth_midxed_object_oid(struct object_id *oid, struct multi_pack_index *m, uint32_t n); -int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, struct pack_entry *e); +int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, + struct pack_entry *e, struct packed_git **bad_pack); int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name); int midx_layer_contains_pack(struct multi_pack_index *m, diff --git a/odb/source-packed.c b/odb/source-packed.c index 0890704e76879b..16fa4f57699eca 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -13,18 +13,19 @@ static int find_pack_entry(struct odb_source_packed *store, const struct object_id *oid, - struct pack_entry *e) + struct pack_entry *e, + struct packed_git **bad_pack) { struct packfile_list_entry *l; odb_source_prepare(&store->base, 0); - if (store->midx && fill_midx_entry(store->midx, oid, e)) + if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack)) return 1; for (l = store->packs.head; l; l = l->next) { struct packed_git *p = l->pack; - if (!p->multi_pack_index && packfile_fill_entry(p, oid, e)) { + if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) { if (!store->skip_mru_updates) packfile_list_prepend(&store->packs, p); return 1; @@ -40,6 +41,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source, enum object_info_flags flags) { struct odb_source_packed *packed = odb_source_packed_downcast(source); + struct packed_git *bad_pack = NULL; struct pack_entry e; int ret; @@ -51,8 +53,16 @@ static int odb_source_packed_read_object_info(struct odb_source *source, if (flags & OBJECT_INFO_SECOND_READ) odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES); - if (!find_pack_entry(packed, oid, &e)) + if (!find_pack_entry(packed, oid, &e, &bad_pack)) { + /* + * The lookup may have failed because the object is known to be + * corrupt in one of the packfiles. Report the object as + * corrupt instead of missing in that case. + */ + if (bad_pack) + return -1; return 1; + } /* * We know that the caller doesn't actually need the @@ -77,7 +87,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out, struct odb_source_packed *packed = odb_source_packed_downcast(source); struct pack_entry e; - if (!find_pack_entry(packed, oid, &e)) + if (!find_pack_entry(packed, oid, &e, NULL)) return -1; return packfile_read_object_stream(out, oid, e.p, e.offset); @@ -583,7 +593,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source, timesp = × } - if (!find_pack_entry(packed, oid, &e)) + if (!find_pack_entry(packed, oid, &e, NULL)) return 0; if (e.p->is_cruft) return 0; diff --git a/packfile.c b/packfile.c index 0eee45055f833e..34e2f9bb8b1bd5 100644 --- a/packfile.c +++ b/packfile.c @@ -1859,13 +1859,17 @@ int is_pack_valid(struct packed_git *p) int packfile_fill_entry(struct packed_git *p, const struct object_id *oid, - struct pack_entry *e) + struct pack_entry *e, + struct packed_git **bad_pack) { off_t offset; if (oidset_size(&p->bad_objects) && - oidset_contains(&p->bad_objects, oid)) + oidset_contains(&p->bad_objects, oid)) { + if (bad_pack && !*bad_pack) + *bad_pack = p; return 0; + } offset = find_pack_entry_one(oid, p); if (!offset) @@ -1962,7 +1966,7 @@ int has_object_kept_pack(struct repository *r, const struct object_id *oid, for (; *cache; cache++) { struct packed_git *p = *cache; - if (packfile_fill_entry(p, oid, &e)) + if (packfile_fill_entry(p, oid, &e, NULL)) return 1; } } diff --git a/packfile.h b/packfile.h index e1f77152b5c4bf..3229a6ed472666 100644 --- a/packfile.h +++ b/packfile.h @@ -294,7 +294,8 @@ off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *); int packfile_fill_entry(struct packed_git *p, const struct object_id *oid, - struct pack_entry *e); + struct pack_entry *e, + struct packed_git **bad_pack); int is_pack_valid(struct packed_git *); void *unpack_entry(struct repository *r, struct packed_git *, off_t, diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c index fb16ec0176d7b4..27a05da957afc2 100644 --- a/t/helper/test-read-midx.c +++ b/t/helper/test-read-midx.c @@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum, for (i = 0; i < m->num_objects; i++) { nth_midxed_object_oid(&oid, m, i + m->num_objects_in_base); - fill_midx_entry(m, &oid, &e); + fill_midx_entry(m, &oid, &e, NULL); printf("%s %"PRIu64"\t%s\n", oid_to_hex(&oid), e.offset, e.p->pack_name); From d55f3629e613af08c3520b65b7a13d030134be6e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 19 Aug 2026 14:17:20 +0200 Subject: [PATCH 11/34] odb/source: introduce error status when reading objects The `read_object_info()` callback of `struct odb_source` is documented to return a negative error code in case reading the object has failed, and zero otherwise. This is overly broad though, as there are two very different kinds of failures: - The object may not exist in the source at all. - The object exists, but reading it has failed, for example because its on-disk state is corrupt. This distinction matters to callers: when an object is corrupt in one source we may still find a good copy of it in another source, so we may still be able to proceed with a given operation. The "packed" source already distinguishes these cases by returning a positive value for missing objects and a negative value in case reading the object has failed. But it is the only such source that distinguishes those cases, and the returned value is translated into a negative error code by the "files" backend anyway. Introduce a new error status that is specific to reading objects and adapt the infrastructure to return it. For now, we only discern successful reads from generic failures, which mostly matches the status quo. In subsequent commits though we're about to add an error that explicitly tells the caller that an object does not exist. Note that we keep the "packed" backend as-is with its positive return code for missing objects. This will be fixed in the next commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 16 ++++++++-------- odb.h | 15 +++++++++++---- odb/source-files.c | 8 ++++---- odb/source-inmemory.c | 8 ++++---- odb/source-loose.c | 8 ++++---- odb/source-packed.c | 8 ++++---- odb/source.h | 22 +++++++++++----------- 7 files changed, 46 insertions(+), 39 deletions(-) diff --git a/odb.c b/odb.c index caf1d0f5424d29..1b37b2637680d5 100644 --- a/odb.c +++ b/odb.c @@ -547,9 +547,9 @@ static int register_all_submodule_sources(struct object_database *odb) return ret; } -static int do_oid_object_info_extended(struct object_database *odb, - const struct object_id *oid, - struct object_info *oi, unsigned flags) +static enum odb_read_status do_oid_object_info_extended(struct object_database *odb, + const struct object_id *oid, + struct object_info *oi, unsigned flags) { const struct object_id *real = oid; int already_retried = 0; @@ -696,12 +696,12 @@ static int oid_object_info_convert(struct repository *r, return ret; } -int odb_read_object_info_extended(struct object_database *odb, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags) +enum odb_read_status odb_read_object_info_extended(struct object_database *odb, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags) { - int ret; + enum odb_read_status ret; if (oid->algo && (hash_algo_by_ptr(odb->repo->hash_algo) != oid->algo)) return oid_object_info_convert(odb->repo, oid, oi, flags); diff --git a/odb.h b/odb.h index fca67e8253e7ad..43cbcc3abafeea 100644 --- a/odb.h +++ b/odb.h @@ -435,14 +435,21 @@ enum object_info_flags { OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK), }; +enum odb_read_status { + /* The read was successful. */ + ODB_READ_OK = 0, + /* The read resulted in a generic error. */ + ODB_READ_ERROR = -1, +}; + /* * Read object info from the object database and populate the `object_info` * structure. Returns 0 on success, a negative error code otherwise. */ -int odb_read_object_info_extended(struct object_database *odb, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags); +enum odb_read_status odb_read_object_info_extended(struct object_database *odb, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags); /* * Read a subset of object info for the given object ID. Returns an `enum diff --git a/odb/source-files.c b/odb/source-files.c index 5a68af7d84c250..a28aa5042dd8d6 100644 --- a/odb/source-files.c +++ b/odb/source-files.c @@ -59,10 +59,10 @@ static void odb_source_files_prepare(struct odb_source *source, odb_source_prepare(&files->packed->base, flags); } -static int odb_source_files_read_object_info(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags) +static enum odb_read_status odb_source_files_read_object_info(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags) { struct odb_source_files *files = odb_source_files_downcast(source); diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c index 3e71611b8e0071..53d2e3a8521783 100644 --- a/odb/source-inmemory.c +++ b/odb/source-inmemory.c @@ -56,10 +56,10 @@ static void populate_object_info(struct odb_source_inmemory *source, oi->source_infop->source = &source->base; } -static int odb_source_inmemory_read_object_info(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags UNUSED) +static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags UNUSED) { struct odb_source_inmemory *inmemory = odb_source_inmemory_downcast(source); const struct inmemory_object *object; diff --git a/odb/source-loose.c b/odb/source-loose.c index ef0e9192777c4a..ad8662842dffaa 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -206,10 +206,10 @@ static int read_object_info_from_path(struct odb_source_loose *loose, return ret; } -static int odb_source_loose_read_object_info(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags) +static enum odb_read_status odb_source_loose_read_object_info(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags) { struct odb_source_loose *loose = odb_source_loose_downcast(source); static struct strbuf buf = STRBUF_INIT; diff --git a/odb/source-packed.c b/odb/source-packed.c index 16fa4f57699eca..dce68a57f7da68 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -35,10 +35,10 @@ static int find_pack_entry(struct odb_source_packed *store, return 0; } -static int odb_source_packed_read_object_info(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags) +static enum odb_read_status odb_source_packed_read_object_info(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags) { struct odb_source_packed *packed = odb_source_packed_downcast(source); struct packed_git *bad_pack = NULL; diff --git a/odb/source.h b/odb/source.h index d69f8e2d1cf626..7b8ff3d19de761 100644 --- a/odb/source.h +++ b/odb/source.h @@ -110,13 +110,13 @@ struct odb_source { * second read in case they know that the first read would have * already surfaced the object without reloading any on-disk state. * - * The callback is expected to return a negative error code in case - * reading the object has failed, 0 otherwise. + * The callback is expected to return an `enum odb_read_status`. Please + * refer to the individual values that can be returned. */ - int (*read_object_info)(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags); + enum odb_read_status (*read_object_info)(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags); /* * This callback is expected to create a new read stream that can be @@ -340,12 +340,12 @@ static inline void odb_source_prepare(struct odb_source *source, /* * Read an object from the object database source identified by its object ID. - * Returns 0 on success, a negative error code otherwise. + * Please refer to `enum odb_read_status` for the individual error codes. */ -static inline int odb_source_read_object_info(struct odb_source *source, - const struct object_id *oid, - struct object_info *oi, - enum object_info_flags flags) +static inline enum odb_read_status odb_source_read_object_info(struct odb_source *source, + const struct object_id *oid, + struct object_info *oi, + enum object_info_flags flags) { return source->read_object_info(source, oid, oi, flags); } From 3295c347c3af27d046b179f60c5f28fddcfa8fbd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 19 Aug 2026 14:17:21 +0200 Subject: [PATCH 12/34] odb/source: let callers discern missing and corrupt objects As explained in the preceding commits, reading objects can either fail because the object truly does not exist or because it exists, but its data is corrupt. Some callers do care about this distinction, but there is no way to tell these two cases apart right now. Introduce a new `ODB_READ_NOT_FOUND` value that ought to be returned by the backends in case the object truly does not exist and adapt backends to use it. Note that we don't yet return this error from `odb_read_object_info()` itself. This will be fixed in a subsequent commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.h | 2 ++ odb/source-files.c | 20 +++++++++++++++++--- odb/source-inmemory.c | 2 +- odb/source-loose.c | 31 +++++++++++++++++++------------ odb/source-packed.c | 2 +- t/unit-tests/u-odb-inmemory.c | 3 ++- 6 files changed, 42 insertions(+), 18 deletions(-) diff --git a/odb.h b/odb.h index 43cbcc3abafeea..1264d4ce7d116a 100644 --- a/odb.h +++ b/odb.h @@ -440,6 +440,8 @@ enum odb_read_status { ODB_READ_OK = 0, /* The read resulted in a generic error. */ ODB_READ_ERROR = -1, + /* The object could not be found. */ + ODB_READ_NOT_FOUND = -2, }; /* diff --git a/odb/source-files.c b/odb/source-files.c index a28aa5042dd8d6..e88fd1d3993d90 100644 --- a/odb/source-files.c +++ b/odb/source-files.c @@ -65,12 +65,26 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source enum object_info_flags flags) { struct odb_source_files *files = odb_source_files_downcast(source); + enum odb_read_status ret_packed, ret_loose; - if (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) || - !odb_source_read_object_info(&files->loose->base, oid, oi, flags)) + ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags); + if (!ret_packed) return 0; - return -1; + ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags); + if (!ret_loose) + return 0; + + /* + * Reading the packed object may have failed even though the object + * exists, for example because it is corrupt. Report this failure to + * the caller in case neither of the sources was able to read the + * object, and prefer the error of the packed source in case both + * reads have failed. + */ + if (ret_packed != ODB_READ_NOT_FOUND) + return ret_packed; + return ret_loose; } static int odb_source_files_read_object_stream(struct odb_read_stream **out, diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c index 53d2e3a8521783..3f3bd12de3e2ca 100644 --- a/odb/source-inmemory.c +++ b/odb/source-inmemory.c @@ -66,7 +66,7 @@ static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_sour object = find_cached_object(inmemory, oid); if (!object) - return -1; + return ODB_READ_NOT_FOUND; populate_object_info(inmemory, oi, object); return 0; diff --git a/odb/source-loose.c b/odb/source-loose.c index ad8662842dffaa..3c942a1069f587 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose, struct stat st; if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) { - ret = quick_has_loose(loose, oid) ? 0 : -1; + ret = quick_has_loose(loose, oid) ? 0 : ODB_READ_NOT_FOUND; goto out; } if (lstat(path, &st) < 0) { + if (errno == ENOENT) { + ret = ODB_READ_NOT_FOUND; + goto out; + } + ret = -1; goto out; } @@ -113,9 +118,12 @@ static int read_object_info_from_path(struct odb_source_loose *loose, fd = git_open(path); if (fd < 0) { - if (errno != ENOENT) - error_errno(_("unable to open loose object %s"), oid_to_hex(oid)); - ret = -1; + if (errno == ENOENT) { + ret = ODB_READ_NOT_FOUND; + goto out; + } + + ret = error_errno(_("unable to open loose object %s"), oid_to_hex(oid)); goto out; } @@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose, if (parse_loose_header(hdr, oi) < 0) { ret = error(_("unable to parse %s header"), oid_to_hex(oid)); - goto corrupt; + goto out; } if (*oi->typep < 0) @@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose, *oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid); if (!*oi->contentp) { ret = -1; - goto corrupt; + goto out; } } @@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose, case ULHR_BAD: ret = error(_("unable to unpack %s header"), oid_to_hex(oid)); - goto corrupt; + goto out; case ULHR_TOO_LONG: ret = error(_("header for %s too long, exceeds %d bytes"), oid_to_hex(oid), MAX_HEADER_LEN); - goto corrupt; + goto out; } ret = 0; -corrupt: - if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT)) +out: + if (ret && ret != ODB_READ_NOT_FOUND && (flags & OBJECT_INFO_DIE_IF_CORRUPT)) die(_("loose object %s (stored in %s) is corrupt"), oid_to_hex(oid), path); -out: if (stream_to_end) git_inflate_end(stream_to_end); if (map) @@ -221,7 +228,7 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source * second time. */ if (flags & OBJECT_INFO_SECOND_READ) - return -1; + return ODB_READ_NOT_FOUND; odb_loose_path(loose, &buf, oid); return read_object_info_from_path(loose, buf.buf, oid, oi, flags); diff --git a/odb/source-packed.c b/odb/source-packed.c index dce68a57f7da68..9b1940538029d6 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -61,7 +61,7 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source */ if (bad_pack) return -1; - return 1; + return ODB_READ_NOT_FOUND; } /* diff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c index ddf2db5c811fb8..3e5068080ca6b6 100644 --- a/t/unit-tests/u-odb-inmemory.c +++ b/t/unit-tests/u-odb-inmemory.c @@ -72,7 +72,8 @@ void test_odb_inmemory__read_missing_object(void) const char *end; cl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo)); - cl_must_fail(odb_source_read_object_info(&source->base, &oid, NULL, 0)); + cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0), + ODB_READ_NOT_FOUND); odb_source_free(&source->base); } From 63a3257352868dae9e842f2aa9637ee36651ad38 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 19 Aug 2026 14:17:22 +0200 Subject: [PATCH 13/34] odb/source: allow `read_object_info()` to bubble up error messages When reading an object fails even though it exists, the sources know best what exactly went wrong and where the corrupt object is located. This information is lost though when bubbling up the error to the object database layer, which forces that layer to reconstruct it after the fact. This is exactly what `do_oid_object_info_extended()` does via `has_packed_and_bad()`, but that function only really knows to handle the "files" backend by reaching into its internals. Introduce a new `errmsg` parameter for the `read_object_info()` callback that sources are expected to populate with a human-readable message in case reading the object has failed. Adapt the packed and loose sources to populate the buffer with the messages that we ultimately want to surface to the user. For now, all callers are adapted to pass a `NULL` pointer. We will add a user of this new infrastructure in a subsequent commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 6 +++--- odb.c | 7 ++++--- odb/source-files.c | 9 ++++++--- odb/source-inmemory.c | 3 ++- odb/source-loose.c | 23 +++++++++++++++-------- odb/source-packed.c | 34 ++++++++++++++++++++++++++-------- odb/source.h | 18 ++++++++++++++---- packfile.c | 2 +- t/unit-tests/u-odb-inmemory.c | 4 ++-- 9 files changed, 73 insertions(+), 33 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 10c2471024b74a..399acd0f225d93 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1759,7 +1759,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid, struct odb_source *source = the_repository->objects->sources->next; for (; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); - if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0)) + if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL)) return 0; } } @@ -4171,7 +4171,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type for (; !found && source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); - if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0)) + if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL)) found = 1; } @@ -4637,7 +4637,7 @@ static int force_object_loose(struct odb_source *source, for (struct odb_source *s = source->odb->sources; s; s = s->next) { struct odb_source_files *files = odb_source_files_downcast(s); - if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0)) + if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL)) return 0; } diff --git a/odb.c b/odb.c index 1b37b2637680d5..83a53f7f6b716e 100644 --- a/odb.c +++ b/odb.c @@ -560,7 +560,7 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * if (is_null_oid(real)) return -1; - if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags)) + if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL)) return 0; odb_prepare_alternates(odb); @@ -569,7 +569,7 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * struct odb_source *source; for (source = odb->sources; source; source = source->next) - if (!odb_source_read_object_info(source, real, oi, flags)) + if (!odb_source_read_object_info(source, real, oi, flags, NULL)) return 0; /* @@ -580,7 +580,8 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * if (!(flags & OBJECT_INFO_QUICK)) { for (source = odb->sources; source; source = source->next) if (!odb_source_read_object_info(source, real, oi, - flags | OBJECT_INFO_SECOND_READ)) + flags | OBJECT_INFO_SECOND_READ, + NULL)) return 0; } diff --git a/odb/source-files.c b/odb/source-files.c index e88fd1d3993d90..aafba358e4b8b6 100644 --- a/odb/source-files.c +++ b/odb/source-files.c @@ -62,16 +62,19 @@ static void odb_source_files_prepare(struct odb_source *source, static enum odb_read_status odb_source_files_read_object_info(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags) + enum object_info_flags flags, + struct strbuf *errmsg) { struct odb_source_files *files = odb_source_files_downcast(source); enum odb_read_status ret_packed, ret_loose; - ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags); + ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, + flags, errmsg); if (!ret_packed) return 0; - ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags); + ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags, + ret_packed == ODB_READ_NOT_FOUND ? errmsg : NULL); if (!ret_loose) return 0; diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c index 3f3bd12de3e2ca..12f91e594a00e9 100644 --- a/odb/source-inmemory.c +++ b/odb/source-inmemory.c @@ -59,7 +59,8 @@ static void populate_object_info(struct odb_source_inmemory *source, static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags UNUSED) + enum object_info_flags flags UNUSED, + struct strbuf *errmsg UNUSED) { struct odb_source_inmemory *inmemory = odb_source_inmemory_downcast(source); const struct inmemory_object *object; diff --git a/odb/source-loose.c b/odb/source-loose.c index 3c942a1069f587..b57ee2701a5010 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -67,7 +67,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose, const char *path, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags) + enum object_info_flags flags, + struct strbuf *errmsg) { int ret; int fd; @@ -191,9 +192,14 @@ static int read_object_info_from_path(struct odb_source_loose *loose, ret = 0; out: - if (ret && ret != ODB_READ_NOT_FOUND && (flags & OBJECT_INFO_DIE_IF_CORRUPT)) - die(_("loose object %s (stored in %s) is corrupt"), - oid_to_hex(oid), path); + if (ret && ret != ODB_READ_NOT_FOUND) { + if ((flags & OBJECT_INFO_DIE_IF_CORRUPT)) + die(_("loose object %s (stored in %s) is corrupt"), + oid_to_hex(oid), path); + if (errmsg) + strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"), + oid_to_hex(oid), path); + } if (stream_to_end) git_inflate_end(stream_to_end); @@ -216,7 +222,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose, static enum odb_read_status odb_source_loose_read_object_info(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags) + enum object_info_flags flags, + struct strbuf *errmsg) { struct odb_source_loose *loose = odb_source_loose_downcast(source); static struct strbuf buf = STRBUF_INIT; @@ -231,7 +238,7 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source return ODB_READ_NOT_FOUND; odb_loose_path(loose, &buf, oid); - return read_object_info_from_path(loose, buf.buf, oid, oi, flags); + return read_object_info_from_path(loose, buf.buf, oid, oi, flags, errmsg); } /* @@ -428,7 +435,7 @@ static int for_each_object_wrapper_cb(const struct object_id *oid, if (data->request) { struct object_info oi = *data->request; - if (read_object_info_from_path(data->loose, path, oid, &oi, 0) < 0) + if (read_object_info_from_path(data->loose, path, oid, &oi, 0, NULL) < 0) return -1; return data->cb(oid, &oi, data->cb_data); @@ -446,7 +453,7 @@ static int for_each_prefixed_object_wrapper_cb(const struct object_id *oid, struct object_info oi = *data->request; if (odb_source_read_object_info(&data->loose->base, - oid, &oi, 0) < 0) + oid, &oi, 0, NULL) < 0) return -1; return data->cb(oid, &oi, data->cb_data); diff --git a/odb/source-packed.c b/odb/source-packed.c index 9b1940538029d6..1a12a605dbc62e 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -2,7 +2,9 @@ #include "abspath.h" #include "chdir-notify.h" #include "dir.h" +#include "gettext.h" #include "git-zlib.h" +#include "hex.h" #include "list-objects-filter-options.h" #include "mergesort.h" #include "midx.h" @@ -10,6 +12,7 @@ #include "odb/streaming.h" #include "packfile.h" #include "pack-bitmap.h" +#include "strbuf.h" static int find_pack_entry(struct odb_source_packed *store, const struct object_id *oid, @@ -38,7 +41,8 @@ static int find_pack_entry(struct odb_source_packed *store, static enum odb_read_status odb_source_packed_read_object_info(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags) + enum object_info_flags flags, + struct strbuf *errmsg) { struct odb_source_packed *packed = odb_source_packed_downcast(source); struct packed_git *bad_pack = NULL; @@ -59,25 +63,39 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source * corrupt in one of the packfiles. Report the object as * corrupt instead of missing in that case. */ - if (bad_pack) - return -1; - return ODB_READ_NOT_FOUND; + if (bad_pack) { + ret = -1; + goto out; + } + + ret = ODB_READ_NOT_FOUND; + goto out; } /* * We know that the caller doesn't actually need the * information below, so return early. */ - if (!oi) - return 0; + if (!oi) { + ret = 0; + goto out; + } ret = packed_object_info(packed, e.p, e.offset, oi); if (ret < 0) { + bad_pack = e.p; mark_bad_packed_object(e.p, oid); - return -1; + goto out; } - return 0; + ret = 0; + +out: + if (ret < 0 && bad_pack && errmsg) + strbuf_addf(errmsg, _("packed object %s (stored in %s) is corrupt"), + oid_to_hex(oid), bad_pack->pack_name); + + return ret; } static int odb_source_packed_read_object_stream(struct odb_read_stream **out, diff --git a/odb/source.h b/odb/source.h index 7b8ff3d19de761..4d13e4cfaf1931 100644 --- a/odb/source.h +++ b/odb/source.h @@ -27,6 +27,7 @@ enum odb_source_type { struct object_id; struct odb_read_stream; +struct strbuf; struct strvec; /* @@ -111,12 +112,16 @@ struct odb_source { * already surfaced the object without reloading any on-disk state. * * The callback is expected to return an `enum odb_read_status`. Please - * refer to the individual values that can be returned. + * refer to the individual values that can be returned. In case reading + * the object has failed with a generic error and `errmsg` is non-NULL, + * the callback is expected to populate it with a human-readable + * message that describes the failure. */ enum odb_read_status (*read_object_info)(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags); + enum object_info_flags flags, + struct strbuf *errmsg); /* * This callback is expected to create a new read stream that can be @@ -341,13 +346,18 @@ static inline void odb_source_prepare(struct odb_source *source, /* * Read an object from the object database source identified by its object ID. * Please refer to `enum odb_read_status` for the individual error codes. + * + * In case reading the object has failed with a generic error and `errmsg` is + * non-NULL it will be populated with a human-readable message that describes + * the failure. */ static inline enum odb_read_status odb_source_read_object_info(struct odb_source *source, const struct object_id *oid, struct object_info *oi, - enum object_info_flags flags) + enum object_info_flags flags, + struct strbuf *errmsg) { - return source->read_object_info(source, oid, oi, flags); + return source->read_object_info(source, oid, oi, flags, errmsg); } /* diff --git a/packfile.c b/packfile.c index 34e2f9bb8b1bd5..3cde39a01c9ad3 100644 --- a/packfile.c +++ b/packfile.c @@ -1945,7 +1945,7 @@ int has_object_pack(struct repository *r, const struct object_id *oid) odb_prepare_alternates(r->objects); for (source = r->objects->sources; source; source = source->next) { struct odb_source_files *files = odb_source_files_downcast(source); - if (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0)) + if (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0, NULL)) return 1; } diff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c index 3e5068080ca6b6..095c20ba918c68 100644 --- a/t/unit-tests/u-odb-inmemory.c +++ b/t/unit-tests/u-odb-inmemory.c @@ -29,7 +29,7 @@ static void cl_assert_object_info(struct odb_source_inmemory *source, .contentp = &actual_content, }; - cl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0)); + cl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0, NULL)); cl_assert_equal_u(actual_size, strlen(expected_content)); cl_assert_equal_u(actual_type, expected_type); cl_assert_equal_s((char *) actual_content, expected_content); @@ -72,7 +72,7 @@ void test_odb_inmemory__read_missing_object(void) const char *end; cl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo)); - cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0), + cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0, NULL), ODB_READ_NOT_FOUND); odb_source_free(&source->base); From 2135b14863642bbcec02996e7f5e54ac1f77b03a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 19 Aug 2026 14:17:23 +0200 Subject: [PATCH 14/34] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically When a lookup with `OBJECT_INFO_DIE_IF_CORRUPT` fails we want to die in case the object exists, but cannot be read. This flag is handled in two different spots right now: - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to check whether the object is known to be corrupt in any packfile. This function reaches into the internals of the packed source and thus breaks the abstraction provided by our object sources. - The loose source handles the flag itself and dies directly in `read_object_info_from_path()`, which means that we die even in cases where another source may still have a good copy of the object. Besides being inconsistent, it also ties us to the specific backend used by the database sources because `has_packed_and_bad()` assumes that they use the "files" backend. Any other backend will instead cause us to die when calling `odb_source_files_downcast()`, even if the object was simply nonexistent. In the preceding commits we've carved out the infrastructure to make this mechanism fully generic. On the one hand, all backends now tell us whether the object is missing or corrupt via their return values. And on the other hand, they have been taught to provide a readable error message to the caller. Adapt `do_oid_object_info_extended()` to use those new mechanisms. This means that we won't die immediately anymore when a loose object is corrupt, and we properly handle backends other than the "files" backend. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 46 +++++++++++++++++++++++++----------- odb/source-loose.c | 10 ++------ packfile.c | 17 ------------- packfile.h | 1 - t/t1060-object-corruption.sh | 18 ++++++++++++++ 5 files changed, 52 insertions(+), 40 deletions(-) diff --git a/odb.c b/odb.c index 83a53f7f6b716e..6bbea640334305 100644 --- a/odb.c +++ b/odb.c @@ -15,7 +15,6 @@ #include "object-name.h" #include "odb.h" #include "odb/source-inmemory.h" -#include "packfile.h" #include "path.h" #include "promisor-remote.h" #include "quote.h" @@ -551,8 +550,11 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * const struct object_id *oid, struct object_info *oi, unsigned flags) { + struct strbuf corrupt_err = STRBUF_INIT; const struct object_id *real = oid; + enum odb_read_status ret; int already_retried = 0; + bool corrupt = false; if (flags & OBJECT_INFO_LOOKUP_REPLACE) real = lookup_replace_object(odb->repo, oid); @@ -568,9 +570,14 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * while (1) { struct odb_source *source; - for (source = odb->sources; source; source = source->next) - if (!odb_source_read_object_info(source, real, oi, flags, NULL)) - return 0; + for (source = odb->sources; source; source = source->next) { + ret = odb_source_read_object_info(source, real, oi, flags, + corrupt_err.len ? NULL : &corrupt_err); + if (!ret) + goto out; + if (ret != ODB_READ_NOT_FOUND) + corrupt = true; + } /* * When the object hasn't been found we try a second read and @@ -578,11 +585,15 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * * caches or reload on-disk state. */ if (!(flags & OBJECT_INFO_QUICK)) { - for (source = odb->sources; source; source = source->next) - if (!odb_source_read_object_info(source, real, oi, - flags | OBJECT_INFO_SECOND_READ, - NULL)) - return 0; + for (source = odb->sources; source; source = source->next) { + ret = odb_source_read_object_info(source, real, oi, + flags | OBJECT_INFO_SECOND_READ, + corrupt_err.len ? NULL : &corrupt_err); + if (!ret) + goto out; + if (ret != ODB_READ_NOT_FOUND) + corrupt = true; + } } /* @@ -605,16 +616,23 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database * } if (flags & OBJECT_INFO_DIE_IF_CORRUPT) { - const struct packed_git *p; if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid)) die(_("replacement %s not found for %s"), oid_to_hex(real), oid_to_hex(oid)); - if ((p = has_packed_and_bad(odb->repo, real))) - die(_("packed object %s (stored in %s) is corrupt"), - oid_to_hex(real), p->pack_name); + if (corrupt) { + if (corrupt_err.len) + die("%s", corrupt_err.buf); + die(_("object %s is corrupt"), oid_to_hex(real)); + } } - return -1; + + ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND; + goto out; } + +out: + strbuf_release(&corrupt_err); + return ret; } static int oid_object_info_convert(struct repository *r, diff --git a/odb/source-loose.c b/odb/source-loose.c index b57ee2701a5010..540b2dd40da28c 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -192,15 +192,9 @@ static int read_object_info_from_path(struct odb_source_loose *loose, ret = 0; out: - if (ret && ret != ODB_READ_NOT_FOUND) { - if ((flags & OBJECT_INFO_DIE_IF_CORRUPT)) - die(_("loose object %s (stored in %s) is corrupt"), + if (ret && ret != ODB_READ_NOT_FOUND && errmsg) + strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"), oid_to_hex(oid), path); - if (errmsg) - strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"), - oid_to_hex(oid), path); - } - if (stream_to_end) git_inflate_end(stream_to_end); if (map) diff --git a/packfile.c b/packfile.c index 3cde39a01c9ad3..cd38be088dcee8 100644 --- a/packfile.c +++ b/packfile.c @@ -985,23 +985,6 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid) oidset_insert(&p->bad_objects, oid); } -const struct packed_git *has_packed_and_bad(struct repository *r, - const struct object_id *oid) -{ - struct odb_source *source; - - for (source = r->objects->sources; source; source = source->next) { - struct odb_source_files *files = odb_source_files_downcast(source); - struct packfile_list_entry *e; - - for (e = files->packed->packs.head; e; e = e->next) - if (oidset_contains(&e->pack->bad_objects, oid)) - return e->pack; - } - - return NULL; -} - off_t get_delta_base(struct packed_git *p, struct pack_window **w_curs, off_t *curpos, diff --git a/packfile.h b/packfile.h index 3229a6ed472666..573fe003d03b17 100644 --- a/packfile.h +++ b/packfile.h @@ -329,7 +329,6 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source, uint32_t *maybe_index_pos, struct object_info *oi); void mark_bad_packed_object(struct packed_git *, const struct object_id *); -const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *); int has_object_pack(struct repository *r, const struct object_id *oid); int has_object_kept_pack(struct repository *r, const struct object_id *oid, diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index 502a5ea1c51e3a..d2ef468b4528ea 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -145,4 +145,22 @@ test_expect_success 'partial clone of corrupted repository' ' test_must_fail git -C corrupt-partial checkout --force ' +test_expect_success 'corrupted loose commit can be read from alternate' ' + git init repo-a && + tree=$(git -C repo-a write-tree) && + commit=$(git -C repo-a commit-tree $tree .git/objects/info/alternates && + corrupt_byte "$commit" 1 + ) && + + git -C repo-a cat-file -p "$commit" >expect && + git -C repo-b cat-file -p "$commit" >actual 2>err && + test_cmp expect actual && + test_grep "inflate: data stream error" err +' + test_done From a6c983765fa2005436475eff3aa5b9b5fe5181da Mon Sep 17 00:00:00 2001 From: Friel Date: Wed, 19 Aug 2026 16:28:10 -0700 Subject: [PATCH 15/34] pack-objects: trace pack bytes written We want to measure how compression settings affect push performance on the client. Different settings can produce different-sized packs from the same objects. Trace2 records the object count, but we also need the pack size to compare those settings. Add a write_pack_file/wrote_bytes Trace2 datum alongside write_pack_file/wrote. Count packs written to stdout or disk, including each pack's header and trailing checksum. When pack.packSizeLimit splits the output, report the sum of the pack sizes. Signed-off-by: Friel Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 5 +++++ t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1ec5b6f206366e..252530172c3009 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1337,6 +1337,7 @@ static void write_pack_file(void) uint32_t nr_remaining = nr_result; time_t last_mtime = 0; struct object_entry **write_order; + off_t bytes_written = 0; if (progress > pack_to_stdout) progress_state = start_progress(the_repository, @@ -1389,6 +1390,8 @@ static void write_pack_file(void) display_progress(progress_state, written); } + bytes_written += hashfile_total(f) + + the_repository->hash_algo->rawsz; if (pack_to_stdout) { /* * We never fsync when writing to stdout since we may @@ -1510,6 +1513,8 @@ static void write_pack_file(void) written, nr_result); trace2_data_intmax("pack-objects", the_repository, "write_pack_file/wrote", nr_result); + trace2_data_intmax("pack-objects", the_repository, + "write_pack_file/wrote_bytes", bytes_written); } static int no_try_delta(const char *path) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 9dabb3615aff56..aac139e6a096eb 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -33,6 +33,30 @@ test_expect_success 'setup' ' } >expect ' +test_expect_success 'pack-object traces bytes written to stdout' ' + test_when_finished "rm -f pack.trace pack.pack" && + GIT_TRACE2_EVENT="$PWD/pack.trace" \ + git pack-objects --quiet --revs --stdout >pack.pack <<-EOF && + $commit + EOF + bytes=$(test_file_size pack.pack) && + test_grep "\"key\":\"write_pack_file/wrote_bytes\",\"value\":\"$bytes\"" pack.trace +' + +test_expect_success 'pack-object traces bytes written to split pack files' ' + test_when_finished "rm -f split.trace traced-pack-*" && + GIT_TRACE2_EVENT="$PWD/split.trace" \ + git -c pack.packSizeLimit=3m pack-objects --quiet traced-pack Date: Thu, 20 Aug 2026 10:31:28 +0000 Subject: [PATCH 16/34] worktree add: shouldn't dwim if -b or -B is given 'git worktree add ' DWIMs to a remote-tracking branch when neither -b, -B, nor --detach is given. However, 'git worktree add -b ' can still DWIM , causing to be ignored. This is a regression introduced by 128e5496b3 (worktree add: extend DWIM to infer --orphan, 2023-05-17), which appeared in Git 2.42. Signed-off-by: Yoichi NAKAYAMA Signed-off-by: Junio C Hamano --- builtin/worktree.c | 3 +++ t/t2400-worktree-add.sh | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/builtin/worktree.c b/builtin/worktree.c index d21c43fde38b5e..dcd20c77412b8f 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -896,6 +896,9 @@ static int add(int ac, const char **av, const char *prefix, /* DWIM: Infer --orphan when repo has no refs. */ opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1); + } else if (ac == 2 && new_branch) { + if (!strcmp(branch, "HEAD")) + can_use_local_refs(&opts); } else if (ac == 2) { struct object_id oid; struct commit *commit; diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 58b4445cc441a8..fb6a02f7498884 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -621,6 +621,16 @@ test_expect_success '"add" dwims' ' ) ' +test_expect_success '"add" does not dwim with -b' ' + test_when_finished rm -rf repo_upstream repo_dwim wt && + setup_remote_repo repo_upstream repo_dwim && + ( + cd repo_dwim && + test_must_fail git worktree add -b branch ../wt foo 2>actual && + test_grep "^fatal: invalid reference: foo" actual + ) +' + test_expect_success '"add" dwims with checkout.defaultRemote' ' test_when_finished rm -rf repo_upstream repo_dwim foo && setup_remote_repo repo_upstream repo_dwim && From 7874b2c7e1889a28482d50905b5d2ba7e480bfb8 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:32 -0500 Subject: [PATCH 17/34] builtin/receive-pack: properly clean up keep files When git-receive-pack(1) stores an incoming packfile with git-index-pack(1), a ".keep" file is written alongside it in the transaction quarantine directory and also gets migrated to the main ODB when the ODB transaction is committed. This keep lockfile ensures the packfile remains in place until the references have been updated and is removed afterwards. The path used to remove it is derived via `index_pack_lockfile()` from the repository's primary object directory. In bdee7b3013 (builtin/receive-pack: stage incoming objects via ODB transactions, 2026-07-10), git-receive-pack(1) started using the ODB transaction interfaces instead of managing a temporary directory directly. When starting an ODB transaction, the sources list is reordered to insert the newly created transaction source first as the primary to ensure writes are routed to it accordingly. Prior to using ODB transactions, git-receive-pack(1) would only set the temporary directory as the primary source for the child git-index-pack(1) and git-unpack-objects(1) processes it spawned and the parent process would set the temporary directory set as an alternate only. By using ODB transactions, the ODB source list is also reordered for the parent process which results in `index_pack_lockfile()` deriving the ".keep" path relative to the temporary directory instead of the actual main ODB source path. Consequently, this prevents the ".keep" file from being properly removed after being migrated into the main ODB source post-commit. Update `index_pack_lockfile()` to operate on an ODB source explicitly provided to it and update call sites accordingly to pass the expected ODB source. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 8 +++++++- fetch-pack.c | 2 +- pack-write.c | 7 ++++--- pack.h | 4 +++- t/t5547-push-quarantine.sh | 31 +++++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 6 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 86933d8d7e4b33..d74b7871485606 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2412,7 +2412,13 @@ static const char *unpack(int err_fd, struct shallow_info *si, if (status) return "index-pack fork failed"; - lockfile = index_pack_lockfile(the_repository, child.out, NULL); + /* + * 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(transaction->source, child.out, NULL); if (lockfile) { pack_lockfile = register_tempfile(lockfile); free(lockfile); diff --git a/fetch-pack.c b/fetch-pack.c index 922a9b25812c68..6df5813b33230e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1075,7 +1075,7 @@ static int get_pack(struct fetch_pack_args *args, die(_("fetch-pack: unable to fork off %s"), cmd_name); if (do_keep && (pack_lockfiles || fsck_objects)) { int is_well_formed; - char *pack_lockfile = index_pack_lockfile(the_repository, + char *pack_lockfile = index_pack_lockfile(the_repository->objects->sources, cmd.out, &is_well_formed); diff --git a/pack-write.c b/pack-write.c index 24033a9101545a..85674e4b726bab 100644 --- a/pack-write.c +++ b/pack-write.c @@ -469,10 +469,11 @@ 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 repository *r, int ip_out, int *is_well_formed) +char *index_pack_lockfile(struct odb_source *source, int ip_out, + int *is_well_formed) { char packname[GIT_MAX_HEXSZ + 6]; - const int len = r->hash_algo->hexsz + 6; + const int len = source->odb->repo->hash_algo->hexsz + 6; /* * The first thing we expect from index-pack's output @@ -489,7 +490,7 @@ char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed) packname[len-1] = 0; if (skip_prefix(packname, "keep\t", &name)) return xstrfmt("%s/pack/pack-%s.keep", - repo_get_object_directory(r), name); + source->path, name); return NULL; } if (is_well_formed) diff --git a/pack.h b/pack.h index 1cde92082be903..ada506b5c5d0db 100644 --- a/pack.h +++ b/pack.h @@ -7,6 +7,7 @@ struct packed_git; struct pack_window; struct repository; +struct odb_source; /* * Packed object header @@ -105,7 +106,8 @@ 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); -char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed); +char *index_pack_lockfile(struct odb_source *source, int fd, + int *is_well_formed); struct ref; diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh index 0798ddab02bc8b..1b7097179ee980 100755 --- a/t/t5547-push-quarantine.sh +++ b/t/t5547-push-quarantine.sh @@ -70,4 +70,35 @@ test_expect_success 'updating a ref from quarantine is forbidden' ' git -C update.git fsck ' +test_expect_success '.keep file is removed after push' ' + test_when_finished rm -rf keep.git && + git init --bare keep.git && + + 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. + test_hook -C keep.git pre-receive <<-\EOF && + keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" && + test -f "$keep" + EOF + + # After quarantined objects are migrated, validate that the ".keep" + # lockfile is migrated and present in the main ODB. + test_hook -C keep.git reference-transaction <<-\EOF && + keep="$(ls objects/pack/pack-*.keep)" && + test -f "$keep" + EOF + + test_commit foo && + git push keep.git HEAD && + + # Once the operation is complete, validate that the ".keep" lockfile has + # been removed. + pack="$(ls keep.git/objects/pack/pack-*.pack)" && + keep="${pack%.pack}.keep" && + test_path_is_file "$pack" && + test_path_is_missing "$keep" +' + test_done From ecdda043e0e200a18fabf99b7e793fc33367e6c4 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:33 -0500 Subject: [PATCH 18/34] odb/transaction: add transaction finalize interface When committing an ODB transaction via `odb_transaction_commit()`, the staged objects are made visible and the underlying transaction is freed at the same time. Coupling these two steps does not leave room for any post-commit transaction operations to be introduced though. Such a capability is useful if an ODB transaction backend needs to hold on to lockfiles after transaction commit until references are updated, as is the case with the existing "files" backend in git-receive-pack(1). Stop freeing the transaction in `odb_transaction_commit()` and introduce `odb_transaction_finalize()` to explicitly clean up the transaction accordingly. Note that the finalize interface also provides an optional callback for any backend-specific deferred cleanup. In a subsequent commit, the "files" transaction backend will use this to remove ".keep" files generated for packfiles received via git-receive-pack(1) after references have been updated. In preparation for this, the `odb_transaction_finalize()` call site in git-receive-pack(1) is made after the reference updates are finished. All other callers commit a transaction and immediately finalize it without any work happening in between those two operations. Consequently, they cannot meaningfully recover in case either of them would fail, and spelling out these two separate steps with proper error handling would be quite repetitive and pointless. Introduce a helper `odb_transaction_commit_and_finalize_or_die()` for those call sites and update them accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/add.c | 4 ++-- builtin/receive-pack.c | 1 + builtin/unpack-objects.c | 2 +- builtin/update-index.c | 4 ++-- cache-tree.c | 2 +- object-file.c | 2 +- odb/transaction.c | 14 ++++++++++++++ odb/transaction.h | 23 +++++++++++++++++++++++ read-cache.c | 2 +- 9 files changed, 46 insertions(+), 8 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 60ffbede2be58a..ad418a595251d3 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -393,7 +393,7 @@ int cmd_add(int argc, char *seen = NULL; char *ps_matched = NULL; struct lock_file lock_file = LOCK_INIT; - struct odb_transaction *transaction; + struct odb_transaction *transaction = NULL; repo_config(repo, add_config, NULL); @@ -600,7 +600,7 @@ int cmd_add(int argc, if (chmod_arg && pathspec.nr) exit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only); - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); finish: if (write_locked_index(repo->index, &lock_file, diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index d74b7871485606..ed1edcbe93c208 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2720,6 +2720,7 @@ int cmd_receive_pack(int argc, use_keepalive = KEEPALIVE_ALWAYS; execute_commands(commands, unpack_status, &si, transaction, &push_options); + odb_transaction_finalize(transaction); delete_tempfile(&pack_lockfile); sigchain_push(SIGPIPE, SIG_IGN); if (report_status_v2) diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 4263edfbecdd39..d6a2d616d968a2 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -603,7 +603,7 @@ static void unpack_all(void) unpack_one(i); display_progress(progress, i + 1); } - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); stop_progress(&progress); if (delta_list) diff --git a/builtin/update-index.c b/builtin/update-index.c index 241abd4332dcf9..b25d4ecb1091ba 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1156,7 +1156,7 @@ int cmd_update_index(int argc, * a transaction. */ if (transaction && verbose) { - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); transaction = NULL; } @@ -1224,7 +1224,7 @@ int cmd_update_index(int argc, /* * By now we have added all of the new objects */ - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); if (split_index > 0) { if (repo_config_get_split_index(the_repository) == 0) diff --git a/cache-tree.c b/cache-tree.c index d92f5132865f13..a220372a420197 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -538,7 +538,7 @@ int cache_tree_update(struct index_state *istate, int flags) i = update_one(istate->cache_tree, istate->cache, istate->cache_nr, "", 0, &skip, flags); if (!inflight) - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); trace2_region_leave("cache_tree", "update", istate->repo); trace_performance_leave("cache_tree_update"); if (i < 0) diff --git a/object-file.c b/object-file.c index ec35c318bc9fe7..4d03c167d5a0b2 100644 --- a/object-file.c +++ b/object-file.c @@ -965,7 +965,7 @@ int index_fd(struct index_state *istate, struct object_id *oid, xsize_t(st->st_size), oid); if (!inflight) - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); } else { ret = hash_blob_stream(&stream, the_repository->hash_algo, oid, diff --git a/odb/transaction.c b/odb/transaction.c index dab7da6a9a4f55..9e9a98277872d8 100644 --- a/odb/transaction.c +++ b/odb/transaction.c @@ -33,6 +33,20 @@ int odb_transaction_commit(struct odb_transaction *transaction) ret = transaction->commit(transaction); transaction->source->odb->transaction = NULL; + + return ret; +} + +int odb_transaction_finalize(struct odb_transaction *transaction) +{ + int ret = 0; + + if (!transaction) + return 0; + + if (transaction->finalize) + ret = transaction->finalize(transaction); + free(transaction); return ret; diff --git a/odb/transaction.h b/odb/transaction.h index 4cb2eafcbf08f5..6ed39b3d0e9fb1 100644 --- a/odb/transaction.h +++ b/odb/transaction.h @@ -22,6 +22,13 @@ struct odb_transaction { */ int (*commit)(struct odb_transaction *transaction); + /* + * Optional ODB source specific callback invoked when the transaction + * needs to perform any deferred cleanup after objects have been + * committed. Returns 0 on success, a negative error code otherwise. + */ + int (*finalize)(struct odb_transaction *transaction); + /* * This callback is expected to write the given object stream into * the ODB transaction. Note that for now, only blobs support streaming. @@ -75,6 +82,22 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb, */ int odb_transaction_commit(struct odb_transaction *transaction); +/* + * Finalizes an ODB transaction, performing any deferred cleanup and freeing it. + * Must be called for every successfully started transaction. Note that, if the + * specified transaction is NULL, the function is a no-op. Returns 0 on success, + * a negative error code otherwise. + */ +int odb_transaction_finalize(struct odb_transaction *transaction); + +static inline void odb_transaction_commit_and_finalize_or_die(struct odb_transaction *transaction) +{ + if (odb_transaction_commit(transaction)) + die(_("failed to commit ODB transaction")); + if (odb_transaction_finalize(transaction)) + die(_("failed to finalize ODB transaction")); +} + /* * Writes the object in the provided stream into the transaction. The resulting * object ID is written into the out pointer. Returns 0 on success, a negative diff --git a/read-cache.c b/read-cache.c index 6c449f393d8d4d..0cd0ef85ecdf74 100644 --- a/read-cache.c +++ b/read-cache.c @@ -4049,7 +4049,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix, odb_transaction_begin_or_die(repo->objects, &transaction, 0); run_diff_files(&rev, DIFF_RACY_IS_MODIFIED); if (!inflight) - odb_transaction_commit(transaction); + odb_transaction_commit_and_finalize_or_die(transaction); release_revisions(&rev); return !!data.add_errors; From 727b99cdb11395ad6932bdb3fb4923a98c23c803 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:34 -0500 Subject: [PATCH 19/34] builtin/receive-pack: pass shallow file explicitly If shallow information is provided during `unpack()`, a temporary shallow file is created and stored in global state. In a subsequent commit, the `unpack()` logic is moved behind a generic ODB transaction interface to handle writing packfiles and thus can no longer rely on such global state. Lift the setup of the temporary shallow file out of `unpack()` and wire it through to its call sites explicitly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ed1edcbe93c208..135105deaea844 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -86,7 +86,6 @@ static const char *head_name; static void *head_name_to_free; static int sent_capabilities; static int shallow_update; -static const char *alt_shallow_file; static struct strbuf push_cert = STRBUF_INIT; static struct object_id push_cert_oid; static struct signature_check sigcheck; @@ -2334,8 +2333,8 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr) ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); } -static const char *unpack(int err_fd, struct shallow_info *si, - struct odb_transaction *transaction) +static const char *unpack(struct odb_transaction *transaction, + const char *shallow_file, int err_fd) { struct pack_header hdr; const char *hdr_err; @@ -2354,10 +2353,9 @@ static const char *unpack(int err_fd, struct shallow_info *si, return hdr_err; } - if (si->nr_ours || si->nr_theirs) { - alt_shallow_file = setup_temporary_shallow(si->shallow); + if (shallow_file) { strvec_push(&child.args, "--shallow-file"); - strvec_push(&child.args, alt_shallow_file); + strvec_push(&child.args, shallow_file); } odb_transaction_env(transaction, &child.env); @@ -2433,14 +2431,14 @@ static const char *unpack(int err_fd, struct shallow_info *si, return NULL; } -static const char *unpack_with_sideband(struct shallow_info *si, - struct odb_transaction *transaction) +static const char *unpack_with_sideband(struct odb_transaction *transaction, + const char *shallow_file) { struct async muxer; const char *ret; if (!use_sideband) - return unpack(0, si, transaction); + return unpack(transaction, shallow_file, 0); use_keepalive = KEEPALIVE_AFTER_NUL; memset(&muxer, 0, sizeof(muxer)); @@ -2449,13 +2447,14 @@ static const char *unpack_with_sideband(struct shallow_info *si, if (start_async(&muxer)) return NULL; - ret = unpack(muxer.in, si, transaction); + ret = unpack(transaction, shallow_file, muxer.in); finish_async(&muxer); return ret; } -static void prepare_shallow_update(struct shallow_info *si) +static void prepare_shallow_update(struct shallow_info *si, + const char *shallow_file) { int i, j, k, bitmap_size = DIV_ROUND_UP(si->ref->nr, 32); @@ -2495,12 +2494,13 @@ static void prepare_shallow_update(struct shallow_info *si) * command. check_connected() will be done with * true .git/shallow though. */ - setenv(GIT_SHALLOW_FILE_ENVIRONMENT, alt_shallow_file, 1); + setenv(GIT_SHALLOW_FILE_ENVIRONMENT, shallow_file, 1); } static void update_shallow_info(struct command *commands, struct shallow_info *si, - struct oid_array *ref) + struct oid_array *ref, + const char *shallow_file) { struct command *cmd; int *ref_status; @@ -2519,7 +2519,7 @@ static void update_shallow_info(struct command *commands, si->ref = ref; if (shallow_update) { - prepare_shallow_update(si); + prepare_shallow_update(si, shallow_file); return; } @@ -2711,11 +2711,17 @@ int cmd_receive_pack(int argc, if (!si.nr_ours && !si.nr_theirs) shallow_update = 0; if (!delete_only(commands)) { + const char *alt_shallow_file = NULL; + + if (si.nr_ours || si.nr_theirs) + alt_shallow_file = setup_temporary_shallow(si.shallow); + if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE)) unpack_status = "unable to start object transaction"; else - unpack_status = unpack_with_sideband(&si, transaction); - update_shallow_info(commands, &si, &ref); + unpack_status = unpack_with_sideband(transaction, alt_shallow_file); + + update_shallow_info(commands, &si, &ref, alt_shallow_file); } use_keepalive = KEEPALIVE_ALWAYS; execute_commands(commands, unpack_status, &si, transaction, From c59466d4cfcc4b0cf364bebc32ede9da53abf9a3 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:35 -0500 Subject: [PATCH 20/34] builtin/receive-pack: read unpack limit config lazily In git-receive-pack(1), the `receive.unpackLimit` and `transfer.unpackLimit` configuration decides whether an incoming packfile should be exploded into loose objects or kept as a packfile on-disk. In a subsequent commit, the logic to write the incoming packfile is made ODB backend agnostic and moved behind a pluggable ODB transaction interface. Consequently, whether to explode a packfile is a detail of how a particular backend stores objects and should not be a part of the generic interface itself. In preparation for this, instead resolve the unpack limit lazily inside `unpack()` by reading the configuration directly. The now-unused unpack limit globals are dropped accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 135105deaea844..971dc3f52edb09 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -62,12 +62,9 @@ static enum deny_action deny_delete_current = DENY_UNCONFIGURED; static int receive_fsck_objects = -1; static int transfer_fsck_objects = -1; static struct strbuf fsck_msg_types = STRBUF_INIT; -static int receive_unpack_limit = -1; -static int transfer_unpack_limit = -1; static int advertise_atomic_push = 1; static int advertise_push_options; static int advertise_sid; -static int unpack_limit = 100; static off_t max_input_size; static int report_status; static int report_status_v2; @@ -157,16 +154,6 @@ static int receive_pack_config(const char *var, const char *value, return 0; } - if (strcmp(var, "receive.unpacklimit") == 0) { - receive_unpack_limit = git_config_int(var, value, ctx->kvi); - return 0; - } - - if (strcmp(var, "transfer.unpacklimit") == 0) { - transfer_unpack_limit = git_config_int(var, value, ctx->kvi); - return 0; - } - if (strcmp(var, "receive.fsck.skiplist") == 0) { char *path; @@ -2333,6 +2320,16 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr) ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); } +static unsigned int get_unpack_limit(struct repository *repo) +{ + unsigned int limit = 100; + + repo_config_get_uint(repo, "transfer.unpacklimit", &limit); + repo_config_get_uint(repo, "receive.unpacklimit", &limit); + + return limit; +} + static const char *unpack(struct odb_transaction *transaction, const char *shallow_file, int err_fd) { @@ -2360,7 +2357,7 @@ static const char *unpack(struct odb_transaction *transaction, odb_transaction_env(transaction, &child.env); - if (ntohl(hdr.hdr_entries) < unpack_limit) { + if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) { strvec_push(&child.args, "unpack-objects"); push_header_arg(&child.args, &hdr); if (quiet) @@ -2658,11 +2655,6 @@ int cmd_receive_pack(int argc, if (cert_nonce_seed) push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL)); - if (0 <= receive_unpack_limit) - unpack_limit = receive_unpack_limit; - else if (0 <= transfer_unpack_limit) - unpack_limit = transfer_unpack_limit; - switch (determine_protocol_version_server()) { case protocol_v2: /* From 255f3a7a5318b7c2dc97c15845af3e4ec5f51d9a Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:36 -0500 Subject: [PATCH 21/34] builtin/receive-pack: lift global state out of unpack() In git-receive-pack(1), writing the packfile to the transaction is handled via `unpack()` which relies on global variables to decide how to invoke the underlying git-index-pack(1) or git-unpack-objects(1) child processes. In a subsequent commit, the `unpack()` logic is moved behind a generic ODB transaction interface to handle writing packfiles and thus can no longer rely on these globals. Lift the global state out of `unpack()` by instead storing this state in a `struct unpack_opts` that gets passed to the function explicitly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 63 +++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 971dc3f52edb09..f062b93b8da948 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2330,18 +2330,24 @@ static unsigned int get_unpack_limit(struct repository *repo) return limit; } +struct unpack_opts { + const char *fsck_msg_types; + const char *shallow_file; + off_t max_input_size; + int fsck_objects; + int reject_thin; + int err_fd; + int quiet; +}; + static const char *unpack(struct odb_transaction *transaction, - const char *shallow_file, int err_fd) + const struct unpack_opts *opts) { struct pack_header hdr; const char *hdr_err; int status; struct child_process child = CHILD_PROCESS_INIT; - int fsck_objects = (receive_fsck_objects >= 0 - ? receive_fsck_objects - : transfer_fsck_objects >= 0 - ? transfer_fsck_objects - : 0); + int err_fd = opts->err_fd; hdr_err = parse_pack_header(&hdr); if (hdr_err) { @@ -2350,9 +2356,9 @@ static const char *unpack(struct odb_transaction *transaction, return hdr_err; } - if (shallow_file) { + if (opts->shallow_file) { strvec_push(&child.args, "--shallow-file"); - strvec_push(&child.args, shallow_file); + strvec_push(&child.args, opts->shallow_file); } odb_transaction_env(transaction, &child.env); @@ -2360,14 +2366,14 @@ static const char *unpack(struct odb_transaction *transaction, if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) { strvec_push(&child.args, "unpack-objects"); push_header_arg(&child.args, &hdr); - if (quiet) + if (opts->quiet) strvec_push(&child.args, "-q"); - if (fsck_objects) + if (opts->fsck_objects) strvec_pushf(&child.args, "--strict%s", - fsck_msg_types.buf); - if (max_input_size) + opts->fsck_msg_types); + if (opts->max_input_size) strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, - (uintmax_t)max_input_size); + (uintmax_t)opts->max_input_size); child.no_stdout = 1; child.err = err_fd; child.git_cmd = 1; @@ -2388,18 +2394,18 @@ static const char *unpack(struct odb_transaction *transaction, (uintmax_t)getpid(), hostname); - if (!quiet && err_fd) + if (!opts->quiet && err_fd) strvec_push(&child.args, "--show-resolving-progress"); - if (use_sideband) + if (err_fd) strvec_push(&child.args, "--report-end-of-input"); - if (fsck_objects) + if (opts->fsck_objects) strvec_pushf(&child.args, "--strict%s", - fsck_msg_types.buf); - if (!reject_thin) + opts->fsck_msg_types); + if (!opts->reject_thin) strvec_push(&child.args, "--fix-thin"); - if (max_input_size) + if (opts->max_input_size) strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, - (uintmax_t)max_input_size); + (uintmax_t)opts->max_input_size); child.out = -1; child.err = err_fd; child.git_cmd = 1; @@ -2431,11 +2437,23 @@ static const char *unpack(struct odb_transaction *transaction, static const char *unpack_with_sideband(struct odb_transaction *transaction, const char *shallow_file) { + struct unpack_opts opts = { + .fsck_objects = (receive_fsck_objects >= 0 + ? receive_fsck_objects + : transfer_fsck_objects >= 0 + ? transfer_fsck_objects + : 0), + .fsck_msg_types = fsck_msg_types.buf, + .max_input_size = max_input_size, + .shallow_file = shallow_file, + .reject_thin = reject_thin, + .quiet = quiet, + }; struct async muxer; const char *ret; if (!use_sideband) - return unpack(transaction, shallow_file, 0); + return unpack(transaction, &opts); use_keepalive = KEEPALIVE_AFTER_NUL; memset(&muxer, 0, sizeof(muxer)); @@ -2444,7 +2462,8 @@ static const char *unpack_with_sideband(struct odb_transaction *transaction, if (start_async(&muxer)) return NULL; - ret = unpack(transaction, shallow_file, muxer.in); + opts.err_fd = muxer.in; + ret = unpack(transaction, &opts); finish_async(&muxer); return ret; From 429dd07aa0bc2082d93edc6dea15da426e8807cb Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:37 -0500 Subject: [PATCH 22/34] builtin/receive-pack: report unpack errors via strbuf When writing packfiles via `unpack()`, error messages are returned directly by the function. In preparation for `unpack()` logic being moved behind a generic ODB transaction interface, update the function to instead write any error messages to a caller provided strbuf and return a negative value on error. Call sites are updated to use the error strbuf accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 63 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f062b93b8da948..6df872697ba48e 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2015,7 +2015,7 @@ static void execute_commands_atomic(struct command *commands, } static void execute_commands(struct command *commands, - const char *unpacker_error, + int unpacker_error, struct shallow_info *si, struct odb_transaction *transaction, const struct string_list *push_options) @@ -2340,8 +2340,8 @@ struct unpack_opts { int quiet; }; -static const char *unpack(struct odb_transaction *transaction, - const struct unpack_opts *opts) +static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg, + const struct unpack_opts *opts) { struct pack_header hdr; const char *hdr_err; @@ -2353,7 +2353,8 @@ static const char *unpack(struct odb_transaction *transaction, if (hdr_err) { if (err_fd > 0) close(err_fd); - return hdr_err; + strbuf_addstr(err_msg, hdr_err); + return -1; } if (opts->shallow_file) { @@ -2378,8 +2379,10 @@ static const char *unpack(struct odb_transaction *transaction, child.err = err_fd; child.git_cmd = 1; status = run_command(&child); - if (status) - return "unpack-objects abnormal exit"; + if (status) { + strbuf_addstr(err_msg, "unpack-objects abnormal exit"); + return -1; + } } else { char hostname[HOST_NAME_MAX + 1]; char *lockfile; @@ -2410,8 +2413,10 @@ static const char *unpack(struct odb_transaction *transaction, child.err = err_fd; child.git_cmd = 1; status = start_command(&child); - if (status) - return "index-pack fork failed"; + if (status) { + strbuf_addstr(err_msg, "index-pack fork failed"); + return -1; + } /* * The lockfile filepath is expected to be the final location of @@ -2427,15 +2432,18 @@ static const char *unpack(struct odb_transaction *transaction, close(child.out); status = finish_command(&child); - if (status) - return "index-pack abnormal exit"; + if (status) { + strbuf_addstr(err_msg, "index-pack abnormal exit"); + return -1; + } odb_reprepare(the_repository->objects); } - return NULL; + return 0; } -static const char *unpack_with_sideband(struct odb_transaction *transaction, - const char *shallow_file) +static int unpack_with_sideband(struct odb_transaction *transaction, + const char *shallow_file, + struct strbuf *err_msg) { struct unpack_opts opts = { .fsck_objects = (receive_fsck_objects >= 0 @@ -2450,20 +2458,20 @@ static const char *unpack_with_sideband(struct odb_transaction *transaction, .quiet = quiet, }; struct async muxer; - const char *ret; + int ret; if (!use_sideband) - return unpack(transaction, &opts); + return unpack(transaction, err_msg, &opts); use_keepalive = KEEPALIVE_AFTER_NUL; memset(&muxer, 0, sizeof(muxer)); muxer.proc = copy_to_sideband; muxer.in = -1; if (start_async(&muxer)) - return NULL; + return 0; opts.err_fd = muxer.in; - ret = unpack(transaction, &opts); + ret = unpack(transaction, err_msg, &opts); finish_async(&muxer); return ret; @@ -2552,13 +2560,13 @@ static void update_shallow_info(struct command *commands, free(ref_status); } -static void report(struct command *commands, const char *unpack_status) +static void report(struct command *commands, const struct strbuf *unpack_status) { struct command *cmd; struct strbuf buf = STRBUF_INIT; packet_buf_write(&buf, "unpack %s\n", - unpack_status ? unpack_status : "ok"); + unpack_status->len ? unpack_status->buf : "ok"); for (cmd = commands; cmd; cmd = cmd->next) { if (!cmd->error_string) packet_buf_write(&buf, "ok %s\n", @@ -2576,14 +2584,14 @@ static void report(struct command *commands, const char *unpack_status) strbuf_release(&buf); } -static void report_v2(struct command *commands, const char *unpack_status) +static void report_v2(struct command *commands, const struct strbuf *unpack_status) { struct command *cmd; struct strbuf buf = STRBUF_INIT; struct ref_push_report *report; packet_buf_write(&buf, "unpack %s\n", - unpack_status ? unpack_status : "ok"); + unpack_status->len ? unpack_status->buf : "ok"); for (cmd = commands; cmd; cmd = cmd->next) { int count = 0; @@ -2707,8 +2715,8 @@ int cmd_receive_pack(int argc, PACKET_READ_DIE_ON_ERR_PACKET); if ((commands = read_head_info(&reader, &shallow))) { - const char *unpack_status = NULL; struct string_list push_options = STRING_LIST_INIT_DUP; + struct strbuf unpack_status = STRBUF_INIT; if (use_push_options) read_push_options(&reader, &push_options); @@ -2728,22 +2736,22 @@ int cmd_receive_pack(int argc, alt_shallow_file = setup_temporary_shallow(si.shallow); if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE)) - unpack_status = "unable to start object transaction"; + strbuf_addstr(&unpack_status, "unable to start object transaction"); else - unpack_status = unpack_with_sideband(transaction, alt_shallow_file); + unpack_with_sideband(transaction, alt_shallow_file, &unpack_status); update_shallow_info(commands, &si, &ref, alt_shallow_file); } use_keepalive = KEEPALIVE_ALWAYS; - execute_commands(commands, unpack_status, &si, transaction, + execute_commands(commands, !!unpack_status.len, &si, transaction, &push_options); odb_transaction_finalize(transaction); delete_tempfile(&pack_lockfile); sigchain_push(SIGPIPE, SIG_IGN); if (report_status_v2) - report_v2(commands, unpack_status); + report_v2(commands, &unpack_status); else if (report_status) - report(commands, unpack_status); + report(commands, &unpack_status); sigchain_pop(SIGPIPE); run_receive_hook(commands, "post-receive", 1, NULL, &push_options); @@ -2768,6 +2776,7 @@ int cmd_receive_pack(int argc, if (auto_update_server_info) update_server_info(the_repository, 0); clear_shallow_info(&si); + strbuf_release(&unpack_status); } if (use_sideband) packet_flush(1); From 8e84d34da2bf58fe25ffcd56cb15c60232fb43de Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:38 -0500 Subject: [PATCH 23/34] builtin/receive-pack: explicitly pass packfile fd When processing the incoming packfile in git-receive-pack(1), `unpack()` assumes it should always read it from stdin. In preparation for `unpack()` logic being moved behind a generic ODB transaction interface, update the function signature to take the an explicit fd provided by callers to read the incoming packfile from instead. Call sites are updated accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 6df872697ba48e..b36946678311fe 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2292,9 +2292,9 @@ static void read_push_options(struct packet_reader *reader, } } -static const char *parse_pack_header(struct pack_header *hdr) +static const char *parse_pack_header(struct pack_header *hdr, int pack_fd) { - switch (read_pack_header(0, hdr)) { + switch (read_pack_header(pack_fd, hdr)) { case PH_ERROR_EOF: return "eof before pack header was fully read"; @@ -2340,8 +2340,8 @@ struct unpack_opts { int quiet; }; -static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg, - const struct unpack_opts *opts) +static int unpack(struct odb_transaction *transaction, int pack_fd, + struct strbuf *err_msg, const struct unpack_opts *opts) { struct pack_header hdr; const char *hdr_err; @@ -2349,7 +2349,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg, struct child_process child = CHILD_PROCESS_INIT; int err_fd = opts->err_fd; - hdr_err = parse_pack_header(&hdr); + hdr_err = parse_pack_header(&hdr, pack_fd); if (hdr_err) { if (err_fd > 0) close(err_fd); @@ -2376,6 +2376,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg, strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, (uintmax_t)opts->max_input_size); child.no_stdout = 1; + child.in = pack_fd; child.err = err_fd; child.git_cmd = 1; status = run_command(&child); @@ -2410,6 +2411,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg, strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, (uintmax_t)opts->max_input_size); child.out = -1; + child.in = pack_fd; child.err = err_fd; child.git_cmd = 1; status = start_command(&child); @@ -2461,7 +2463,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction, int ret; if (!use_sideband) - return unpack(transaction, err_msg, &opts); + return unpack(transaction, 0, err_msg, &opts); use_keepalive = KEEPALIVE_AFTER_NUL; memset(&muxer, 0, sizeof(muxer)); @@ -2471,7 +2473,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction, return 0; opts.err_fd = muxer.in; - ret = unpack(transaction, err_msg, &opts); + ret = unpack(transaction, 0, err_msg, &opts); finish_async(&muxer); return ret; From 40932d0a7e35fa31b1ac237ccc77a0a346ce887d Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:39 -0500 Subject: [PATCH 24/34] odb: return temporary ODB source when set When invoked, `odb_set_temporary_primary_source()` installs a temporary object directory as the new primary ODB source. A caller that wants to operate on the ODB source of the open transaction must assume that it is the first entry in the ODB source list which is a bit awkward and fragile. Instead, return the newly installed source directly and report the previous primary source via a new `prev_source` out parameter. Propagate the installed source through `tmp_objdir_replace_primary_odb()` and start storing it in the "files" ODB transaction so a subsequent commit can easily access it without relying on the ODB source list ordering. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- object-file.c | 3 ++- odb.c | 9 +++++++-- odb.h | 6 ++++-- tmp-objdir.c | 8 +++++--- tmp-objdir.h | 6 ++++-- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/object-file.c b/object-file.c index 4d03c167d5a0b2..db63587f6d7564 100644 --- a/object-file.c +++ b/object-file.c @@ -485,6 +485,7 @@ struct odb_transaction_files { struct odb_transaction base; struct tmp_objdir *objdir; + struct odb_source *quarantine; struct transaction_packfile packfile; const char *prefix; }; @@ -507,7 +508,7 @@ int odb_transaction_files_prepare(struct odb_transaction *base) if (!transaction->objdir) return error(_("unable to create temporary object directory")); - tmp_objdir_replace_primary_odb(transaction->objdir, 0); + transaction->quarantine = tmp_objdir_replace_primary_odb(transaction->objdir, 0); return 0; } diff --git a/odb.c b/odb.c index caf1d0f5424d29..8afcb6b637686a 100644 --- a/odb.c +++ b/odb.c @@ -226,7 +226,8 @@ struct odb_source *odb_add_to_alternates_memory(struct object_database *odb, } struct odb_source *odb_set_temporary_primary_source(struct object_database *odb, - const char *dir, int will_destroy) + const char *dir, int will_destroy, + struct odb_source **prev_source) { struct odb_source *source; @@ -250,7 +251,11 @@ struct odb_source *odb_set_temporary_primary_source(struct object_database *odb, source->will_destroy = will_destroy; source->next = odb->sources; odb->sources = source; - return source->next; + + if (prev_source) + *prev_source = source->next; + + return source; } void odb_restore_primary_source(struct object_database *odb, diff --git a/odb.h b/odb.h index fca67e8253e7ad..bdfcb9509afce6 100644 --- a/odb.h +++ b/odb.h @@ -199,10 +199,12 @@ struct odb_source *odb_find_source_or_die(struct object_database *odb, const cha /* * Replace the current writable object directory with the specified temporary - * object directory; returns the former primary source. + * object directory and return the newly installed primary source. The former + * primary source is reported via `prev_source` when non-NULL. */ struct odb_source *odb_set_temporary_primary_source(struct object_database *odb, - const char *dir, int will_destroy); + const char *dir, int will_destroy, + struct odb_source **prev_source); /* * Restore the primary source that was previously replaced by diff --git a/tmp-objdir.c b/tmp-objdir.c index d199d39e7c9d51..e633d97e0efc04 100644 --- a/tmp-objdir.c +++ b/tmp-objdir.c @@ -327,11 +327,13 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *t) odb_add_to_alternates_memory(t->repo->objects, t->path.buf); } -void tmp_objdir_replace_primary_odb(struct tmp_objdir *t, int will_destroy) +struct odb_source *tmp_objdir_replace_primary_odb(struct tmp_objdir *t, + int will_destroy) { if (t->prev_source) BUG("the primary object database is already replaced"); - t->prev_source = odb_set_temporary_primary_source(t->repo->objects, - t->path.buf, will_destroy); t->will_destroy = will_destroy; + + return odb_set_temporary_primary_source(t->repo->objects, t->path.buf, + will_destroy, &t->prev_source); } diff --git a/tmp-objdir.h b/tmp-objdir.h index ccf800faa7c6b9..81eb9274136e69 100644 --- a/tmp-objdir.h +++ b/tmp-objdir.h @@ -64,8 +64,10 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *); /* * Replaces the writable object store in the current process with the temporary * object directory and makes the former main object store an alternate. - * If will_destroy is nonzero, the object directory may not be migrated. + * If will_destroy is nonzero, the object directory may not be migrated. Returns + * the newly installed primary source. */ -void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy); +struct odb_source *tmp_objdir_replace_primary_odb(struct tmp_objdir *, + int will_destroy); #endif /* TMP_OBJDIR_H */ From 2154d88f3aef51dda4cd75216dc5749bdafc8852 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Thu, 20 Aug 2026 18:49:40 -0500 Subject: [PATCH 25/34] odb/transaction: add transaction interface to write packfiles In git-receive-pack(1), the incoming packfile is written to the ODB via `unpack()`, which spawns git-index-pack(1) or git-unpack-objects(1) directly. With pluggable object databases, an alternative backend may need to handle writing packfile data differently though. Introduce `odb_transaction_write_pack()` as a generic interface to handle writing a packfile to a transaction and use the logic from `unpack()` as the "files" backend implementation. Note that when storing the objects as a packfile, git-index-pack(1) also writes a ".keep" lockfile next to it to prevent a concurrent repack from removing the new pack prior to reference updates being performed. The "files" transaction backend is responsible for managing these ".keep" files and removes them post-commit once the transaction is finalized. Call sites in git-receive-pack(1) are updated accordingly. Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 160 +----------------------------------- object-file.c | 178 +++++++++++++++++++++++++++++++++++++++++ odb/transaction.c | 7 ++ odb/transaction.h | 62 ++++++++++++++ 4 files changed, 250 insertions(+), 157 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index b36946678311fe..e6e54ba55f7a8c 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -15,7 +15,6 @@ #include "gpg-interface.h" #include "hex.h" #include "hook.h" -#include "lockfile.h" #include "object.h" #include "object-file.h" #include "object-name.h" @@ -23,7 +22,6 @@ #include "oid-array.h" #include "oidset.h" #include "pack.h" -#include "packfile.h" #include "parse-options.h" #include "pkt-line.h" #include "protocol.h" @@ -2292,162 +2290,11 @@ static void read_push_options(struct packet_reader *reader, } } -static const char *parse_pack_header(struct pack_header *hdr, int pack_fd) -{ - switch (read_pack_header(pack_fd, hdr)) { - case PH_ERROR_EOF: - return "eof before pack header was fully read"; - - case PH_ERROR_PACK_SIGNATURE: - return "protocol error (pack signature mismatch detected)"; - - case PH_ERROR_PROTOCOL: - return "protocol error (pack version unsupported)"; - - default: - return "unknown error in parse_pack_header"; - - case 0: - return NULL; - } -} - -static struct tempfile *pack_lockfile; - -static void push_header_arg(struct strvec *args, struct pack_header *hdr) -{ - strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32, - ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); -} - -static unsigned int get_unpack_limit(struct repository *repo) -{ - unsigned int limit = 100; - - repo_config_get_uint(repo, "transfer.unpacklimit", &limit); - repo_config_get_uint(repo, "receive.unpacklimit", &limit); - - return limit; -} - -struct unpack_opts { - const char *fsck_msg_types; - const char *shallow_file; - off_t max_input_size; - int fsck_objects; - int reject_thin; - int err_fd; - int quiet; -}; - -static int unpack(struct odb_transaction *transaction, int pack_fd, - struct strbuf *err_msg, const struct unpack_opts *opts) -{ - struct pack_header hdr; - const char *hdr_err; - int status; - struct child_process child = CHILD_PROCESS_INIT; - int err_fd = opts->err_fd; - - hdr_err = parse_pack_header(&hdr, pack_fd); - if (hdr_err) { - if (err_fd > 0) - close(err_fd); - strbuf_addstr(err_msg, hdr_err); - return -1; - } - - if (opts->shallow_file) { - strvec_push(&child.args, "--shallow-file"); - strvec_push(&child.args, opts->shallow_file); - } - - odb_transaction_env(transaction, &child.env); - - if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) { - strvec_push(&child.args, "unpack-objects"); - push_header_arg(&child.args, &hdr); - if (opts->quiet) - strvec_push(&child.args, "-q"); - if (opts->fsck_objects) - strvec_pushf(&child.args, "--strict%s", - opts->fsck_msg_types); - if (opts->max_input_size) - strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, - (uintmax_t)opts->max_input_size); - child.no_stdout = 1; - child.in = pack_fd; - child.err = err_fd; - child.git_cmd = 1; - status = run_command(&child); - if (status) { - strbuf_addstr(err_msg, "unpack-objects abnormal exit"); - return -1; - } - } 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"); - if (err_fd) - strvec_push(&child.args, "--report-end-of-input"); - if (opts->fsck_objects) - strvec_pushf(&child.args, "--strict%s", - opts->fsck_msg_types); - if (!opts->reject_thin) - strvec_push(&child.args, "--fix-thin"); - if (opts->max_input_size) - strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, - (uintmax_t)opts->max_input_size); - child.out = -1; - child.in = pack_fd; - child.err = err_fd; - child.git_cmd = 1; - status = start_command(&child); - if (status) { - strbuf_addstr(err_msg, "index-pack fork failed"); - 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(transaction->source, child.out, NULL); - if (lockfile) { - pack_lockfile = register_tempfile(lockfile); - free(lockfile); - } - close(child.out); - - status = finish_command(&child); - if (status) { - strbuf_addstr(err_msg, "index-pack abnormal exit"); - return -1; - } - odb_reprepare(the_repository->objects); - } - return 0; -} - static int unpack_with_sideband(struct odb_transaction *transaction, const char *shallow_file, struct strbuf *err_msg) { - struct unpack_opts opts = { + struct odb_transaction_write_pack_opts opts = { .fsck_objects = (receive_fsck_objects >= 0 ? receive_fsck_objects : transfer_fsck_objects >= 0 @@ -2463,7 +2310,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction, int ret; if (!use_sideband) - return unpack(transaction, 0, err_msg, &opts); + return odb_transaction_write_pack(transaction, 0, err_msg, &opts); use_keepalive = KEEPALIVE_AFTER_NUL; memset(&muxer, 0, sizeof(muxer)); @@ -2473,7 +2320,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction, return 0; opts.err_fd = muxer.in; - ret = unpack(transaction, 0, err_msg, &opts); + ret = odb_transaction_write_pack(transaction, 0, err_msg, &opts); finish_async(&muxer); return ret; @@ -2748,7 +2595,6 @@ int cmd_receive_pack(int argc, execute_commands(commands, !!unpack_status.len, &si, transaction, &push_options); odb_transaction_finalize(transaction); - delete_tempfile(&pack_lockfile); sigchain_push(SIGPIPE, SIG_IGN); if (report_status_v2) report_v2(commands, &unpack_status); diff --git a/object-file.c b/object-file.c index db63587f6d7564..265c5f7a3cc0bd 100644 --- a/object-file.c +++ b/object-file.c @@ -10,6 +10,7 @@ #define USE_THE_REPOSITORY_VARIABLE #include "git-compat-util.h" +#include "config.h" #include "convert.h" #include "dir.h" #include "environment.h" @@ -26,6 +27,7 @@ #include "packfile.h" #include "path.h" #include "read-cache-ll.h" +#include "run-command.h" #include "setup.h" #include "strvec.h" #include "tempfile.h" @@ -483,11 +485,16 @@ struct transaction_packfile { struct odb_transaction_files { struct odb_transaction base; + enum odb_transaction_flags flags; struct tmp_objdir *objdir; struct odb_source *quarantine; struct transaction_packfile packfile; const char *prefix; + + struct tempfile **pack_lockfiles; + size_t pack_lockfiles_nr; + size_t pack_lockfiles_alloc; }; int odb_transaction_files_prepare(struct odb_transaction *base) @@ -1291,6 +1298,174 @@ static int odb_transaction_files_commit(struct odb_transaction *base) return 0; } +static const char *parse_pack_header(struct pack_header *hdr, int pack_fd) +{ + switch (read_pack_header(pack_fd, hdr)) { + case PH_ERROR_EOF: + return "eof before pack header was fully read"; + + case PH_ERROR_PACK_SIGNATURE: + return "protocol error (pack signature mismatch detected)"; + + case PH_ERROR_PROTOCOL: + return "protocol error (pack version unsupported)"; + + default: + return "unknown error in parse_pack_header"; + + case 0: + return NULL; + } +} + +static void push_header_arg(struct strvec *args, struct pack_header *hdr) +{ + strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32, + ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); +} + +static unsigned int get_unpack_limit(struct repository *repo, + enum odb_transaction_flags flags) +{ + unsigned int limit = 0; + + if (flags & ODB_TRANSACTION_RECEIVE) { + limit = 100; + repo_config_get_uint(repo, "transfer.unpacklimit", &limit); + repo_config_get_uint(repo, "receive.unpacklimit", &limit); + } + + return limit; +} + +static int odb_transaction_files_write_pack(struct odb_transaction *base, + int pack_fd, struct strbuf *err_msg, + const struct odb_transaction_write_pack_opts *opts) +{ + struct odb_transaction_files *transaction = + container_of(base, struct odb_transaction_files, base); + struct repository *repo = base->source->odb->repo; + struct child_process child = CHILD_PROCESS_INIT; + struct pack_header hdr; + const char *hdr_err; + int err_fd = opts->err_fd; + int status; + + hdr_err = parse_pack_header(&hdr, pack_fd); + if (hdr_err) { + if (err_fd > 0) + close(err_fd); + strbuf_addstr(err_msg, hdr_err); + return -1; + } + + if (opts->shallow_file) { + strvec_push(&child.args, "--shallow-file"); + strvec_push(&child.args, opts->shallow_file); + } + + odb_transaction_env(base, &child.env); + + if (ntohl(hdr.hdr_entries) < get_unpack_limit(repo, transaction->flags)) { + strvec_push(&child.args, "unpack-objects"); + push_header_arg(&child.args, &hdr); + if (opts->quiet) + strvec_push(&child.args, "-q"); + if (opts->fsck_objects) + strvec_pushf(&child.args, "--strict%s", + opts->fsck_msg_types); + if (opts->max_input_size) + strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, + (uintmax_t)opts->max_input_size); + child.no_stdout = 1; + child.in = pack_fd; + child.err = err_fd; + child.git_cmd = 1; + status = run_command(&child); + if (status) { + strbuf_addstr(err_msg, "unpack-objects abnormal exit"); + return -1; + } + } 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"); + if (err_fd) + strvec_push(&child.args, "--report-end-of-input"); + if (opts->fsck_objects) + strvec_pushf(&child.args, "--strict%s", + opts->fsck_msg_types); + if (!opts->reject_thin) + strvec_push(&child.args, "--fix-thin"); + if (opts->max_input_size) + strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX, + (uintmax_t)opts->max_input_size); + child.out = -1; + child.in = pack_fd; + child.err = err_fd; + child.git_cmd = 1; + status = start_command(&child); + if (status) { + strbuf_addstr(err_msg, "index-pack fork failed"); + 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); + } + close(child.out); + + status = finish_command(&child); + if (status) { + strbuf_addstr(err_msg, "index-pack abnormal exit"); + return -1; + } + + odb_source_prepare(transaction->quarantine, + ODB_PREPARE_FLUSH_CACHES); + } + + return 0; +} + +static int odb_transaction_files_finalize(struct odb_transaction *base) +{ + struct odb_transaction_files *transaction = + container_of(base, struct odb_transaction_files, base); + int ret = 0; + + for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++) + ret |= delete_tempfile(&transaction->pack_lockfiles[i]); + + free(transaction->pack_lockfiles); + + return ret; +} + static int odb_transaction_files_env(struct odb_transaction *base, struct strvec *env) { @@ -1314,8 +1489,11 @@ int odb_transaction_files_begin(struct odb_source *source, transaction = xcalloc(1, sizeof(*transaction)); transaction->base.source = source; transaction->base.commit = odb_transaction_files_commit; + transaction->base.finalize = odb_transaction_files_finalize; transaction->base.write_object_stream = odb_transaction_files_write_object_stream; + transaction->base.write_pack = odb_transaction_files_write_pack; transaction->base.env = odb_transaction_files_env; + transaction->flags = flags; transaction->prefix = "bulk-fsync"; if (flags & ODB_TRANSACTION_RECEIVE) { diff --git a/odb/transaction.c b/odb/transaction.c index 9e9a98277872d8..c9144e6cd6cca3 100644 --- a/odb/transaction.c +++ b/odb/transaction.c @@ -59,6 +59,13 @@ int odb_transaction_write_object_stream(struct odb_transaction *transaction, return transaction->write_object_stream(transaction, stream, len, oid); } +int odb_transaction_write_pack(struct odb_transaction *transaction, int pack_fd, + struct strbuf *err_msg, + const struct odb_transaction_write_pack_opts *opts) +{ + return transaction->write_pack(transaction, pack_fd, err_msg, opts); +} + int odb_transaction_env(struct odb_transaction *transaction, struct strvec *env) { if (!transaction) diff --git a/odb/transaction.h b/odb/transaction.h index 6ed39b3d0e9fb1..8cb06c11914a5e 100644 --- a/odb/transaction.h +++ b/odb/transaction.h @@ -4,6 +4,50 @@ #include "gettext.h" #include "odb.h" +/* + * Options controlling how odb_transaction_write_pack() ingests a packfile. + */ +struct odb_transaction_write_pack_opts { + /* + * Optional fsck severity configuration to apply when incoming objects + * are verified. + */ + const char *fsck_msg_types; + + /* + * Path to an alternative shallow file describing the shallow boundaries + * to honor while ingesting the pack. + */ + const char *shallow_file; + + /* + * The max size in bytes of the incoming packfile allowed. No limit is + * enforced when set to 0. + */ + off_t max_input_size; + + /* + * Whether the validity of incoming objects should be verified. + */ + int fsck_objects; + + /* + * Whether to reject an incoming packfile if it is "thin". + */ + int reject_thin; + + /* + * Optional file descriptor for reporting progress and errors. Set to 0 + * for none. + */ + int err_fd; + + /* + * Suppresses progress reporting. + */ + int quiet; +}; + /* * A transaction may be started for an object database prior to writing new * objects via odb_transaction_begin(). These objects are not committed until @@ -40,6 +84,15 @@ struct odb_transaction { int (*write_object_stream)(struct odb_transaction *transaction, struct odb_write_stream *stream, size_t len, struct object_id *oid); + /* + * This callback is expected to ingest the packfile readable via + * `pack_fd` into the transaction. Returns 0 on success, a negative + * error code otherwise. On failure, a human-readable description is + * appended to `err_msg`. + */ + int (*write_pack)(struct odb_transaction *transaction, int pack_fd, + struct strbuf *err_msg, + const struct odb_transaction_write_pack_opts *opts); /* * This callback is expected to populate the provided strvec with the @@ -107,6 +160,15 @@ int odb_transaction_write_object_stream(struct odb_transaction *transaction, struct odb_write_stream *stream, size_t len, struct object_id *oid); +/* + * Ingests the packfile readable via `pack_fd` into the transaction. Returns 0 + * on success, a negative error code otherwise. On failure, a human-readable + * description is appended to `err_msg`. + */ +int odb_transaction_write_pack(struct odb_transaction *transaction, int pack_fd, + struct strbuf *err_msg, + const struct odb_transaction_write_pack_opts *opts); + /* * Populates the provided strvec with the environment variables that a child * process should inherit so that its object writes participate in the From d122e37976050ba91a5ababdb54ee328380e9a9b Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Fri, 21 Aug 2026 07:26:04 +0200 Subject: [PATCH 26/34] trailers: stop recognizing URLs as trailers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An HTTPS URL starts with an alphanumeric scheme followed by a colon. That means that they will be recognized as trailers in a trailer block. That turns out to be a problem in practice. Let’s stop recognizing these as trailers by failing the trailer parsing when we: 1. find the separator; 2. the separator and the next two characters form `://`; and 3. we haven’t parsed any whitespace yet. The simplest example of how this can be a problem is for people who do not use trailers but may leave URLs at the end of the commit message. Now, while these authors might not use trailers themselves, other authors may have used trailers and this metadata confusion can become a problem once someone tries to extract that metadata (and non-metadata). Let’s now look at some examples in the Linux Kernel[1] to see how this is a problem in practice. There are commits which contain intended non-trailer lines which start with URLs. These are comments. Example with just the trailers:[2] Signed-off-by: Shuai Xue [bhelgaas: squash fixes: https://lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com https://lore.kernel.org/r/20260108013956.14351-3-bagasdotme@gmail.com] Signed-off-by: Bjorn Helgaas Reviewed-by: Ilpo Järvinen Link: https://patch.msgid.link/20251210132907.58799-4-xueshuai@linux.alibaba.com Those `[]` pairs delimit the “squash fixes” comment. Now, any of these two commands: git log --format='%(trailers:only)' -1 git log -1 --format=%B | git interpret-trailers --only-trailers Will both wrongly (according to the surmised user intent) include these two URL lines as trailers and also mangle the URLs, e.g.: https: //lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com Because the `--only-trailers` mode (or `only` for the git-log(1) format) normalizes the output to a colon and a space. Another example is linewrapping mistakes; a `Link` trailer with a URL where the URL ended up on the next line, presumably because the user’s editor linewrapped the “too long” line. Example with just the trailers:[3] Link: https://patch.msgid.link/20260216-work-xattr-socket-v1-4-c2efa4f74cb7@kernel.org Link: https://lore.kernel.org/3cnmtqmakpbb2uwhenrj7kdqu3uefykiykjllgfbtpkiwhaa4s@sghkevv7jned [1] Acked-by: Darrick J. Wong Reviewed-by: Jan Kara Signed-off-by: Christian Brauner Now, this intended trailer is already ruined, but interpreting the URL as a standalone trailer only compounds the mistake. Yet another example is the trailer machinery normalizing the trailer block before application, resulting in a `https` trailer key in the commit message itself. Example with just the trailers:[4] https: //sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") Signed-off-by: Holger Brunck Link: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com Signed-off-by: Jakub Kicinski We have a helpful `Link` that points to the original patch.[5] Following it we can see that that `https` trailer was indeed a URL originally (again just the trailer block here): https://sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") Signed-off-by: Holger Brunck So how did it end up as a `https` trailer? My theory is that the trailer block was normalized on patch application, causing a URL comment to be wrongly normalized and cemented in the commit message as a trailer.[6] † 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ † 2: commit 8236fc613d44e59f6736d6c3e9efffaf26ab7f00 † 3: commit 5bd97f5c5f241a5610c4412d1b93995a26241f81 † 4: commit 496c0c4c53bbe1bad97e82cd12103df61a6e459d † 5: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com † 6: There are only four commits in the Linux Kernel of this kind, and three of them have the same recurring person in the signoff chain. *** Note that this check has some benign false positives. A trailer key can start with a digit, but a URL scheme can not start with a digit. That means that a line that starts with `1://` will be rejected even though it cannot be a URL. I don’t think this will reject any real trailers, so I think the implementation simplicity is worth it. And these false positives are just for a limited start fragment check; a mere heuristic, not a URL parser. Helped-by: Jeff King Acked-by: Jeff King Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- Documentation/git-interpret-trailers.adoc | 13 ++++-- t/t7513-interpret-trailers.sh | 19 +++++++++ t/unit-tests/u-trailer.c | 52 +++++++++++++++++++++++ trailer.c | 6 ++- 4 files changed, 86 insertions(+), 4 deletions(-) diff --git a/Documentation/git-interpret-trailers.adoc b/Documentation/git-interpret-trailers.adoc index b4988d39eab0e9..903d598dcb0595 100644 --- a/Documentation/git-interpret-trailers.adoc +++ b/Documentation/git-interpret-trailers.adoc @@ -123,9 +123,16 @@ OTHER RULES What was covered in the previous section are the rules that are relevant for regular use. The following points are included for completeness. -This command ignores comment lines (see `core.commentString` in -linkgit:git-config[1]). This is for use with the `prepare-commit-msg` -and `commit-msg` hooks. +-- +* This command ignores comment lines (see `core.commentString` in + linkgit:git-config[1]). This is for use with the `prepare-commit-msg` + and `commit-msg` hooks. + +* Candidate trailer lines that have `:` as the separator, that have no + whitespace before the value part, and that start with `//` are not + recognized as trailers. This is to avoid accidentally interpreting + URLs as trailers (e.g. lines that start with `https://`). +-- OPTIONS ------- diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 818a8dafbd29e5..e3555b6d51d134 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -1989,4 +1989,23 @@ test_expect_success 'handling of --- lines in conjunction with cut-lines' ' test_cmp expected actual ' +test_expect_success 'URLs and lines that are not quite URLs' ' + cat >expect <<-\EOF && + https: //www.a-trailer.org + https: //www.another-trailer.org + Signed-off-by: somebody + EOF + git interpret-trailers --only-trailers >actual <<-\EOF && + subject + + body + + https://www.not-a-trailer.org + https ://www.a-trailer.org + https: //www.another-trailer.org + Signed-off-by: somebody + EOF + test_cmp expect actual +' + test_done diff --git a/t/unit-tests/u-trailer.c b/t/unit-tests/u-trailer.c index 3d60ea1603dbda..7404b165fac8fd 100644 --- a/t/unit-tests/u-trailer.c +++ b/t/unit-tests/u-trailer.c @@ -318,3 +318,55 @@ void test_trailer__one_non_trailer_no_git_trailers(void) 0, expected_contents); } + +void test_trailer__URL(void) +{ + struct contents expected_contents[] = { 0 }; + + t_trailer_iterator("Subject: foo bar\n" + "\n" + /* + * We do not want to match URLs as trailers. + */ + "https://www.example.org\n", + 0, + expected_contents); +} + +void test_trailer__not_a_URL_space_after_separator(void) +{ + struct contents expected_contents[] = { + { .raw = "https: //www.example.org\n", + .key = "https", + .val = "//www.example.org" }, + { 0 }, + }; + + t_trailer_iterator("Subject: foo bar\n" + "\n" + /* + * This has a space after ':' so it's not a URL. + */ + "https: //www.example.org\n", + 1, + expected_contents); +} + +void test_trailer__not_a_URL_space_before_separator(void) +{ + struct contents expected_contents[] = { + { .raw = "https ://www.example.org\n", + .key = "https", + .val = "//www.example.org" }, + { 0 }, + }; + + t_trailer_iterator("Subject: foo bar\n" + "\n" + /* + * This has a space before ':' so it's not a URL. + */ + "https ://www.example.org\n", + 1, + expected_contents); +} diff --git a/trailer.c b/trailer.c index 6d8ec7fa8d88b5..10b1abebfbe906 100644 --- a/trailer.c +++ b/trailer.c @@ -635,8 +635,12 @@ static ssize_t find_separator(const char *line, const char *separators) int whitespace_found = 0; const char *c; for (c = line; *c; c++) { - if (strchr(separators, *c)) + if (strchr(separators, *c)) { + /* avoid accidental URL matches */ + if (!whitespace_found && starts_with(c, "://")) + return -1; return c - line; + } if (!whitespace_found && (isalnum(*c) || *c == '-')) continue; if (c != line && (*c == ' ' || *c == '\t')) { From a316d615dac5bcecd0fb7b1c71854d931079d199 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 21 Aug 2026 08:30:01 +0200 Subject: [PATCH 27/34] odb: introduce interface to generate packfiles Packfiles have two primary use cases: - They are used to store objects at rest in a Git repository. - They are used on the transport layer to transfer objects between two repositories. The first class is closely tied to a given object database backend, and as such this use is highly specific to how such a backend decides to store its data. This shows in git-pack-objects(1), which is used by git-repack(1) et al to optimize the object database, which supports lots of options that are closely coupled with how data is stored. But the second class is quite a lot more generic: we don't care about specifics of how the object database stores its objects, but to generate the packfiles we only care about the object graph itself. Still, this use case is also coupled with git-pack-objects(1). Unfortunately, because git-pack-objects(1) covers both classes, the result is that it is very hard to port the whole command to properly support pluggable object databases. There are simply way too many options that an alternative implementation will have a very hard time to support in the first place. And despite being hard to implement, it's also quite unnecessary to implement those backend-specific options. Optimizing the object database has already been made pluggable, and an alternative implementation is unlikely to care about cruft packs, unpacked objects, keep packs and the like. But we still need to make at least _parts_ of the packfile generation pluggable so that backends can generate packfiles for the transport layer itself. Introduce a new interface that lets backends generate a new packfile and implement that interface for the "files" backend. The options supported by the callback are exactly the set of options that are required for the transport layer, but nothing more. This means that git-pack-objects(1) itself cannot be ported over to this new interface, but as explained above that's a hard feat to pull off due to the backend-specific features. Ideally though, we should expose the ability to generate arbitrary packfiles using this interface. The intent of this is to eventually introduce a git-objects(1) subcommand (similar to git-refs(1)) that exposes generic interfaces for accessing everything related to the object database. In that case, we are able to expose only those options that are generic. Subsequent commits will convert git-upload-pack(1), git-send-pack(1) and git-bundle(1) to use this interface. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 21 +++++++ odb.h | 152 +++++++++++++++++++++++++++++++++++++++++++++ odb/source-files.c | 149 ++++++++++++++++++++++++++++++++++++++++++++ odb/source.h | 33 ++++++++++ 4 files changed, 355 insertions(+) diff --git a/odb.c b/odb.c index caf1d0f5424d29..cd9d5b48bc8f11 100644 --- a/odb.c +++ b/odb.c @@ -1046,6 +1046,27 @@ bool odb_optimize_required(struct object_database *odb, return odb_source_optimize_required(odb->sources, opts); } +void odb_generate_pack_options_release(struct odb_generate_pack_options *opts) +{ + oid_array_clear(&opts->wants); + oid_array_clear(&opts->haves); + oid_array_clear(&opts->shallows); +} + +int odb_generate_pack(struct object_database *odb, + struct odb_pack_generator **out, + const struct odb_generate_pack_options *opts) +{ + if (!odb->sources->generate_pack) + return error(_("primary object source does not support generating packfiles")); + return odb_source_generate_pack(odb->sources, out, opts); +} + +int odb_pack_generator_finish(struct odb_pack_generator *generator) +{ + return generator->finish(generator); +} + struct object_database *odb_new(struct repository *repo, const char *primary_source, const char *secondary_sources) diff --git a/odb.h b/odb.h index fca67e8253e7ad..fc1442f243271c 100644 --- a/odb.h +++ b/odb.h @@ -2,6 +2,7 @@ #define ODB_H #include "object.h" +#include "oid-array.h" #include "oidset.h" #include "oidmap.h" #include "string-list.h" @@ -677,6 +678,157 @@ int odb_write_object_stream(struct object_database *odb, struct odb_write_stream *stream, size_t len, struct object_id *oid); +/* + * Options for generating a packfile via `odb_generate_pack()`. + */ +struct odb_generate_pack_options { + /* Tips of the object graph that shall be packed. */ + struct oid_array wants; + + /* + * Boundary of the object graph. Objects reachable from any of these + * tips are expected to already be available to whoever consumes the + * pack and shall thus not be packed. + */ + struct oid_array haves; + + /* + * The shallow boundary that shall be used when computing object + * reachability. When set, any shallow information of the repository + * itself shall be ignored in favor of these objects. + */ + struct oid_array shallows; + + /* + * Pre-expanded object filter specification that limits the set of + * objects that shall be packed. May be `NULL` in case no filter shall + * be applied. + */ + const char *filter_spec; + + /* + * Protocols that may be used to offload objects via packfile URIs. + * May be `NULL` in case packfile URIs shall not be used. + */ + const struct string_list *uri_protocols; + + /* + * Hook command that shall be executed instead of the internal + * machinery to generate the pack. It is up to the specific backend + * whether or not this hook is supported. May be `NULL` in case no + * hook shall be executed. + */ + const char *pack_objects_hook; + + /* + * File descriptor that the generated pack shall be written to. If set + * to `-1`, a pipe will be created and exposed via the pack generator's + * `out` field. If set to `0`, the pack will be written to the standard + * output stream. Otherwise, the provided descriptor will be written to + * and is consumed by the generator. + */ + int pack_fd; + + /* + * File descriptor that progress output shall be written to. The same + * semantics as for `pack_fd` apply, except that `0` will cause the + * generator to write to stderr instead of stdout. + */ + int progress_fd; + + /* Whether to print progress or not. */ + enum { + /* Don't print progress output. */ + ODB_GENERATE_PACK_PROGRESS_NONE, + + /* + * Print progress while computing the packfile, but stop + * printing progress once starting to write it. + */ + ODB_GENERATE_PACK_PROGRESS_STANDARD, + + /* + * Similar to STANDARD, but also print progress when writing + * the packfile. + */ + ODB_GENERATE_PACK_PROGRESS_VERBOSE, + } progress; + + /* Allow the pack to contain deltas against unpacked objects. */ + unsigned thin:1; + + /* Use offset deltas instead of reference deltas. */ + unsigned ofs_delta:1; + + /* Include unasked-for annotated tags of packed objects. */ + unsigned include_tag:1; + + /* The generated pack is destined for a shallow consumer. */ + unsigned shallow:1; + + /* Allow objects that may be missing due to a promisor remote. */ + unsigned missing_allow_promisor:1; + + /* Do not use bitmap indices when computing reachability. */ + unsigned disable_bitmaps:1; +}; + +#define ODB_GENERATE_PACK_OPTIONS_INIT { \ + .wants = OID_ARRAY_INIT, \ + .haves = OID_ARRAY_INIT, \ + .shallows = OID_ARRAY_INIT, \ + .pack_fd = -1, \ +} + +/* Release resources associated with the options. */ +void odb_generate_pack_options_release(struct odb_generate_pack_options *opts); + +/* + * A handle for an ongoing packfile generation as started via + * `odb_generate_pack()`. + */ +struct odb_pack_generator { + /* + * File descriptor from which the generated pack can be read. Only set + * when the pack generation was started with `pack_fd == -1`. The + * caller is responsible for closing the descriptor. + */ + int out; + + /* + * File descriptor from which progress output can be read. Only set + * when the pack generation was started with `progress_fd == -1`. The + * caller is responsible for closing the descriptor. + */ + int err; + + /* + * Callback function to finish this generator. This callback is + * expected to wait for the packfile generation to complete and to then + * free the generator itself. + */ + int (*finish)(struct odb_pack_generator *); +}; + +/* + * Start generating a packfile from the object database with the given + * options. The pack is generated asynchronously; the caller is expected to + * consume the file descriptors exposed via the pack generator and to then + * wait for completion via `odb_pack_generator_finish()`. + * + * Returns 0 on success and populates the `out` pointer with the pack + * generator. Returns a negative error code otherwise. + */ +int odb_generate_pack(struct object_database *odb, + struct odb_pack_generator **out, + const struct odb_generate_pack_options *opts); + +/* + * Wait for the packfile generation to complete and free the pack generator. + * Returns 0 on success, a negative error code otherwise. + */ +int odb_pack_generator_finish(struct odb_pack_generator *generator); + void parse_alternates(const char *string, int sep, const char *relative_base, diff --git a/odb/source-files.c b/odb/source-files.c index 5a68af7d84c250..a33e01fbed0c23 100644 --- a/odb/source-files.c +++ b/odb/source-files.c @@ -4,6 +4,7 @@ #include "chdir-notify.h" #include "config.h" #include "gettext.h" +#include "hex.h" #include "lockfile.h" #include "object-file.h" #include "odb.h" @@ -729,6 +730,153 @@ int odb_source_files_optimize(struct odb_source *source, return ret; } +struct odb_pack_generator_files { + struct odb_pack_generator base; + struct child_process cp; +}; + +static int odb_pack_generator_files_finish(struct odb_pack_generator *_generator) +{ + struct odb_pack_generator_files *generator = + (struct odb_pack_generator_files *)_generator; + int ret; + + ret = finish_command(&generator->cp); + free(generator); + + if (ret) { + /* + * On failure, pack-objects is expected to have written a + * useful error message to its standard error stream already. + * Death by signal is worth mentioning, though, with the + * exception of SIGPIPE: that is a normal occurrence when the + * consumer of the pack hangs up. + */ + if (ret > 128 && ret - 128 == SIGPIPE) + return -1; + if (ret > 128) + error(_("pack-objects died of signal %d"), ret - 128); + return -1; + } + + return 0; +} + +static int odb_source_files_generate_pack(struct odb_source *source UNUSED, + struct odb_pack_generator **out, + const struct odb_generate_pack_options *opts) +{ + struct odb_pack_generator_files *generator; + struct child_process *cp; + FILE *in; + + CALLOC_ARRAY(generator, 1); + child_process_init(&generator->cp); + cp = &generator->cp; + + /* + * The hook is expected to spawn "$hook git pack-objects " + * and to behave like git-pack-objects(1) would have. This can for + * example be used to serve precomputed packfiles. + */ + if (opts->pack_objects_hook) { + strvec_push(&cp->args, opts->pack_objects_hook); + strvec_push(&cp->args, "git"); + cp->use_shell = 1; + } else { + cp->git_cmd = 1; + } + + /* + * The caller-provided shallow boundary overrides any shallow state + * that the repository itself may have, so the shallow file needs to + * be neutralized. + */ + if (opts->shallows.nr) { + strvec_push(&cp->args, "--shallow-file"); + strvec_push(&cp->args, ""); + } + strvec_push(&cp->args, "pack-objects"); + strvec_push(&cp->args, "--revs"); + strvec_push(&cp->args, "--stdout"); + if (opts->thin) + strvec_push(&cp->args, "--thin"); + if (opts->shallow) + strvec_push(&cp->args, "--shallow"); + if (opts->ofs_delta) + strvec_push(&cp->args, "--delta-base-offset"); + if (opts->include_tag) + strvec_push(&cp->args, "--include-tag"); + if (opts->missing_allow_promisor) + strvec_push(&cp->args, "--missing=allow-promisor"); + if (opts->disable_bitmaps) + strvec_push(&cp->args, "--no-use-bitmap-index"); + switch (opts->progress) { + case ODB_GENERATE_PACK_PROGRESS_NONE: + strvec_push(&cp->args, "--quiet"); + break; + case ODB_GENERATE_PACK_PROGRESS_STANDARD: + strvec_push(&cp->args, "--progress"); + break; + case ODB_GENERATE_PACK_PROGRESS_VERBOSE: + strvec_push(&cp->args, "--all-progress"); + break; + default: + BUG("unknown progress option %d", opts->progress); + } + if (opts->filter_spec) + strvec_pushf(&cp->args, "--filter=%s", opts->filter_spec); + if (opts->uri_protocols) + for (size_t i = 0; i < opts->uri_protocols->nr; i++) + strvec_pushf(&cp->args, "--uri-protocol=%s", + opts->uri_protocols->items[i].string); + + cp->in = -1; + cp->out = opts->pack_fd; + cp->err = opts->progress_fd; + cp->clean_on_exit = 1; + + if (start_command(cp)) { + free(generator); + return error(_("could not spawn pack-objects")); + } + + /* + * Feed the objects to pack-objects. This is safe to do synchronously + * because pack-objects consumes all of its standard input before it + * starts to generate the pack. + */ + in = xfdopen(cp->in, "w"); + for (size_t i = 0; i < opts->shallows.nr; i++) + fprintf(in, "--shallow %s\n", oid_to_hex(&opts->shallows.oid[i])); + for (size_t i = 0; i < opts->wants.nr; i++) + fprintf(in, "%s\n", oid_to_hex(&opts->wants.oid[i])); + fprintf(in, "--not\n"); + for (size_t i = 0; i < opts->haves.nr; i++) + fprintf(in, "%s\n", oid_to_hex(&opts->haves.oid[i])); + fprintf(in, "\n"); + fflush(in); + if (ferror(in)) { + error(_("error writing to pack-objects")); + fclose(in); + if (opts->pack_fd < 0) + close(cp->out); + if (opts->progress_fd < 0) + close(cp->err); + finish_command(cp); + free(generator); + return -1; + } + fclose(in); + + generator->base.out = opts->pack_fd < 0 ? cp->out : -1; + generator->base.err = opts->progress_fd < 0 ? cp->err : -1; + generator->base.finish = odb_pack_generator_files_finish; + + *out = &generator->base; + return 0; +} + struct odb_source_files *odb_source_files_new(struct object_database *odb, const char *path, bool local) @@ -756,6 +904,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb, files->base.write_alternate = odb_source_files_write_alternate; files->base.optimize = odb_source_files_optimize; files->base.optimize_required = odb_source_files_optimize_required; + files->base.generate_pack = odb_source_files_generate_pack; /* * Ideally, we would only ever store absolute paths in the source. This diff --git a/odb/source.h b/odb/source.h index d69f8e2d1cf626..e2129766fc573b 100644 --- a/odb/source.h +++ b/odb/source.h @@ -278,6 +278,23 @@ struct odb_source { */ bool (*optimize_required)(struct odb_source *source, const struct odb_optimize_options *opts); + + /* + * This callback is expected to start generating a packfile with the + * given options. The pack shall be generated asynchronously so that + * the caller can consume the pack data and progress output while the + * pack is being generated. + * + * This callback is optional. Sources that cannot generate packfiles + * shall leave it unset. + * + * The callback is expected to return 0 on success and populate the + * `out` pointer with the pack generator, a negative error code + * otherwise. + */ + int (*generate_pack)(struct odb_source *source, + struct odb_pack_generator **out, + const struct odb_generate_pack_options *opts); }; /* @@ -520,4 +537,20 @@ static inline bool odb_source_optimize_required(struct odb_source *source, return source->optimize_required(source, opts); } +/* + * Start generating a packfile from the given source with the given options. + * The pack is generated asynchronously; the caller is expected to consume the + * file descriptors exposed via the pack generator and to then wait for + * completion via `odb_pack_generator_finish()`. + * + * Returns 0 on success and populates the `out` pointer with the pack + * generator, a negative error code otherwise. + */ +static inline int odb_source_generate_pack(struct odb_source *source, + struct odb_pack_generator **out, + const struct odb_generate_pack_options *opts) +{ + return source->generate_pack(source, out, opts); +} + #endif From f0de4ab2480897ed1850f56c23be9d3f6deeef13 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 21 Aug 2026 08:30:02 +0200 Subject: [PATCH 28/34] upload-pack: generate packfiles via the object database When serving a fetch, git-upload-pack(1) spawns git-pack-objects(1) directly to generate the packfile that gets sent to the client. This hard-codes the assumption that the object database is able to serve packfiles via git-pack-objects(1), which is specific to the "files" backend. Convert git-upload-pack(1) to instead use the pack generation interface of the object database. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- upload-pack.c | 125 ++++++++++++++++++-------------------------------- 1 file changed, 45 insertions(+), 80 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index a52856d869891d..22573ad3651f1b 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -197,11 +197,11 @@ static void send_client_data(int fd, const char *data, ssize_t sz, write_or_die(fd, data, sz); } -static int write_one_shallow(const struct commit_graft *graft, void *cb_data) +static int append_one_shallow(const struct commit_graft *graft, void *cb_data) { - FILE *fp = cb_data; + struct oid_array *shallows = cb_data; if (graft->nr_parent == -1) - fprintf(fp, "--shallow %s\n", oid_to_hex(&graft->oid)); + oid_array_append(shallows, &graft->oid); return 0; } @@ -299,7 +299,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os, static void create_pack_file(struct upload_pack_data *pack_data, const struct string_list *uri_protocols) { - struct child_process pack_objects = CHILD_PROCESS_INIT; + struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT; + struct odb_pack_generator *generator; struct output_state *output_state = xcalloc(1, sizeof(struct output_state)); char progress[128]; char abort_msg[] = "aborting due to possible repository " @@ -307,78 +308,42 @@ static void create_pack_file(struct upload_pack_data *pack_data, uint64_t last_sent_ms = 0; ssize_t sz; int i; - FILE *pipe_fd; - - if (!pack_data->pack_objects_hook) - pack_objects.git_cmd = 1; - else { - strvec_push(&pack_objects.args, pack_data->pack_objects_hook); - strvec_push(&pack_objects.args, "git"); - pack_objects.use_shell = 1; - } if (pack_data->shallow_nr) { - strvec_push(&pack_objects.args, "--shallow-file"); - strvec_push(&pack_objects.args, ""); - } - strvec_push(&pack_objects.args, "pack-objects"); - strvec_push(&pack_objects.args, "--revs"); - if (pack_data->use_thin_pack) - strvec_push(&pack_objects.args, "--thin"); - - strvec_push(&pack_objects.args, "--stdout"); - if (pack_data->shallow_nr) - strvec_push(&pack_objects.args, "--shallow"); - if (!pack_data->no_progress) - strvec_push(&pack_objects.args, "--progress"); - if (pack_data->use_ofs_delta) - strvec_push(&pack_objects.args, "--delta-base-offset"); - if (pack_data->use_include_tag) - strvec_push(&pack_objects.args, "--include-tag"); - if (repo_has_accepted_promisor_remote(the_repository)) - strvec_push(&pack_objects.args, "--missing=allow-promisor"); - if (pack_data->filter_options.choice) { - const char *spec = - expand_list_objects_filter_spec(&pack_data->filter_options); - strvec_pushf(&pack_objects.args, "--filter=%s", spec); - } - if (uri_protocols) { - for (i = 0; i < uri_protocols->nr; i++) - strvec_pushf(&pack_objects.args, "--uri-protocol=%s", - uri_protocols->items[i].string); + for_each_commit_graft(append_one_shallow, &opts.shallows); + opts.shallow = 1; } - - pack_objects.in = -1; - pack_objects.out = -1; - pack_objects.err = -1; - pack_objects.clean_on_exit = 1; - - if (start_command(&pack_objects)) - die("git upload-pack: unable to fork git-pack-objects"); - - pipe_fd = xfdopen(pack_objects.in, "w"); - - if (pack_data->shallow_nr) - for_each_commit_graft(write_one_shallow, pipe_fd); - for (i = 0; i < pack_data->want_obj.nr; i++) - fprintf(pipe_fd, "%s\n", - oid_to_hex(&pack_data->want_obj.objects[i].item->oid)); - fprintf(pipe_fd, "--not\n"); + oid_array_append(&opts.wants, + &pack_data->want_obj.objects[i].item->oid); for (i = 0; i < pack_data->have_obj.nr; i++) - fprintf(pipe_fd, "%s\n", - oid_to_hex(&pack_data->have_obj.objects[i].item->oid)); + oid_array_append(&opts.haves, + &pack_data->have_obj.objects[i].item->oid); for (i = 0; i < pack_data->extra_edge_obj.nr; i++) - fprintf(pipe_fd, "%s\n", - oid_to_hex(&pack_data->extra_edge_obj.objects[i].item->oid)); - fprintf(pipe_fd, "\n"); - fflush(pipe_fd); - fclose(pipe_fd); - - /* We read from pack_objects.err to capture stderr output for - * progress bar, and pack_objects.out to capture the pack data. - */ + oid_array_append(&opts.haves, + &pack_data->extra_edge_obj.objects[i].item->oid); + opts.thin = pack_data->use_thin_pack; + if (!pack_data->no_progress) + opts.progress = ODB_GENERATE_PACK_PROGRESS_STANDARD; + opts.ofs_delta = pack_data->use_ofs_delta; + opts.include_tag = pack_data->use_include_tag; + opts.missing_allow_promisor = repo_has_accepted_promisor_remote(the_repository); + if (pack_data->filter_options.choice) + opts.filter_spec = expand_list_objects_filter_spec(&pack_data->filter_options); + opts.uri_protocols = uri_protocols; + opts.pack_objects_hook = pack_data->pack_objects_hook; + opts.pack_fd = -1; + opts.progress_fd = -1; + + if (odb_generate_pack(the_repository->objects, &generator, &opts)) + die("git upload-pack: unable to generate pack"); + odb_generate_pack_options_release(&opts); + + /* + * We read from generator->err to capture stderr output for the + * progress bar, and generator->out to capture the pack data. + */ while (1) { uint64_t now_ms = getnanotime() / 1000000; struct pollfd pfd[2]; @@ -393,14 +358,14 @@ static void create_pack_file(struct upload_pack_data *pack_data, pollsize = 0; pe = pu = -1; - if (0 <= pack_objects.out) { - pfd[pollsize].fd = pack_objects.out; + if (0 <= generator->out) { + pfd[pollsize].fd = generator->out; pfd[pollsize].events = POLLIN; pu = pollsize; pollsize++; } - if (0 <= pack_objects.err) { - pfd[pollsize].fd = pack_objects.err; + if (0 <= generator->err) { + pfd[pollsize].fd = generator->err; pfd[pollsize].events = POLLIN; pe = pollsize; pollsize++; @@ -437,15 +402,15 @@ static void create_pack_file(struct upload_pack_data *pack_data, /* Status ready; we ship that in the side-band * or dump to the standard error. */ - sz = xread(pack_objects.err, progress, + sz = xread(generator->err, progress, sizeof(progress)); if (0 < sz) { send_client_data(2, progress, sz, pack_data->use_sideband); last_sent_ms = now_ms; } else if (sz == 0) { - close(pack_objects.err); - pack_objects.err = -1; + close(generator->err); + generator->err = -1; } else goto fail; @@ -455,15 +420,15 @@ static void create_pack_file(struct upload_pack_data *pack_data, if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) { bool did_send_data; - int result = relay_pack_data(pack_objects.out, + int result = relay_pack_data(generator->out, output_state, pack_data->use_sideband, !!uri_protocols, &did_send_data); if (result == 0) { - close(pack_objects.out); - pack_objects.out = -1; + close(generator->out); + generator->out = -1; } else if (result < 0) { goto fail; } @@ -498,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, } } - if (finish_command(&pack_objects)) { + if (odb_pack_generator_finish(generator)) { error("git upload-pack: git-pack-objects died with error."); goto fail; } From 7d2289a23d969f87dff95d66900adc1ec09a7917 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 21 Aug 2026 08:30:03 +0200 Subject: [PATCH 29/34] send-pack: generate packfiles via the object database When pushing, git-send-pack(1) spawns git-pack-objects(1) directly to generate the packfile that gets sent to the remote. Same as with git-upload-pack(1), which has been adapted in the preceding commit, this hard-codes the assumption that objects can be packed via git-pack-objects(1), which is specific to the "files" backend. Convert git-send-pack(1) to use the pack generation interface of the object database instead. Note that this requires us to adapt t5516 because the parameters passed to git-pack-objects(1) are changing: - The order of arguments changes. - We pass "--quiet" instead of "-q". - We don't pass "--all-progress-implied" anymore when not generating output. All of these changes are benign though and should not result in a change in behaviour. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- send-pack.c | 101 ++++++++++++++---------------------------- t/t5516-fetch-push.sh | 12 ++--- 2 files changed, 40 insertions(+), 73 deletions(-) diff --git a/send-pack.c b/send-pack.c index 3bb5afc687aaf0..f20460fbf487bf 100644 --- a/send-pack.c +++ b/send-pack.c @@ -42,16 +42,17 @@ int option_parse_push_signed(const struct option *opt, die("bad %s argument: %s", opt->long_name, arg); } -static void feed_object(struct repository *r, - const struct object_id *oid, FILE *fh, int negative) +static void append_negative_object(struct repository *r, + struct oid_array *haves, + const struct object_id *oid) { - if (negative && !odb_has_object(r->objects, oid, 0)) + /* + * The remote end may have advertised objects that we do not have in + * our object database. Skip those, as we cannot use them as boundary. + */ + if (!odb_has_object(r->objects, oid, 0)) return; - - if (negative) - putc('^', fh); - fputs(oid_to_hex(oid), fh); - putc('\n', fh); + oid_array_append(haves, oid); } /* @@ -62,92 +63,58 @@ static int pack_objects(struct repository *r, struct oid_array *negotiated, struct send_pack_args *args) { - /* - * The child becomes pack-objects --revs; we feed - * the revision parameters to it via its stdin and - * let its stdout go back to the other end. - */ - struct child_process po = CHILD_PROCESS_INIT; - FILE *po_in; + struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT; + struct odb_pack_generator *generator; int rc; trace2_region_enter("send_pack", "pack_objects", r); - strvec_push(&po.args, "pack-objects"); - strvec_push(&po.args, "--all-progress-implied"); - strvec_push(&po.args, "--revs"); - strvec_push(&po.args, "--stdout"); - if (args->use_thin_pack) - strvec_push(&po.args, "--thin"); - if (args->use_ofs_delta) - strvec_push(&po.args, "--delta-base-offset"); - if (args->quiet || !args->progress) - strvec_push(&po.args, "-q"); + + opts.thin = args->use_thin_pack; + opts.ofs_delta = args->use_ofs_delta; if (args->progress) - strvec_push(&po.args, "--progress"); - if (is_repository_shallow(r)) - strvec_push(&po.args, "--shallow"); - if (args->disable_bitmaps) - strvec_push(&po.args, "--no-use-bitmap-index"); - po.in = -1; - po.out = args->stateless_rpc ? -1 : fd; - po.git_cmd = 1; - po.clean_on_exit = 1; - if (start_command(&po)) - die_errno("git pack-objects failed"); + opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE; + opts.shallow = is_repository_shallow(r); + opts.disable_bitmaps = args->disable_bitmaps; /* - * We feed the pack-objects we just spawned with revision - * parameters by writing to the pipe. + * The pack is either written directly to the remote's descriptor, or, + * in the case of a stateless RPC, read back from a pipe so that we + * can wrap the pack data into pkt-lines. */ - po_in = xfdopen(po.in, "w"); + opts.pack_fd = args->stateless_rpc ? -1 : fd; + for (size_t i = 0; i < advertised->nr; i++) - feed_object(r, &advertised->oid[i], po_in, 1); + append_negative_object(r, &opts.haves, &advertised->oid[i]); for (size_t i = 0; i < negotiated->nr; i++) - feed_object(r, &negotiated->oid[i], po_in, 1); + append_negative_object(r, &opts.haves, &negotiated->oid[i]); while (refs) { if (!is_null_oid(&refs->old_oid)) - feed_object(r, &refs->old_oid, po_in, 1); + append_negative_object(r, &opts.haves, &refs->old_oid); if (!is_null_oid(&refs->new_oid)) - feed_object(r, &refs->new_oid, po_in, 0); + oid_array_append(&opts.wants, &refs->new_oid); refs = refs->next; } - fflush(po_in); - if (ferror(po_in)) - die_errno("error writing to pack-objects"); - fclose(po_in); + if (odb_generate_pack(r->objects, &generator, &opts)) + die("git pack-objects failed"); + odb_generate_pack_options_release(&opts); if (args->stateless_rpc) { char *buf = xmalloc(LARGE_PACKET_MAX); while (1) { - ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX); + ssize_t n = xread(generator->out, buf, LARGE_PACKET_MAX); if (n <= 0) break; send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX); } free(buf); - close(po.out); - po.out = -1; + close(generator->out); } - rc = finish_command(&po); - if (rc) { - /* - * For a normal non-zero exit, we assume pack-objects wrote - * something useful to stderr. For death by signal, though, - * we should mention it to the user. The exception is SIGPIPE - * (141), because that's a normal occurrence if the remote end - * hangs up (and we'll report that by trying to read the unpack - * status). - */ - if (rc > 128 && rc != 141) - error("pack-objects died of signal %d", rc - 128); - trace2_region_leave("send_pack", "pack_objects", r); - return -1; - } + rc = odb_pack_generator_finish(generator); trace2_region_leave("send_pack", "pack_objects", r); - return 0; + return rc; } static int receive_unpack_status(struct packet_reader *reader) @@ -768,7 +735,7 @@ int send_pack(struct repository *r, goto out; } if (!args->stateless_rpc) - /* Closed by pack_objects() via start_command() */ + /* Consumed by the pack generator in pack_objects() */ fd[1] = -1; } if (args->stateless_rpc && cmds_sent) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index f3b3efc47f8a48..b982b209bfd51e 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1903,20 +1903,20 @@ test_expect_success 'push with config push.useBitmaps' ' test_unconfig push.useBitmaps && GIT_TRACE2_EVENT="$PWD/default" \ git push --quiet testrepo main:test && - test_subcommand git pack-objects --all-progress-implied --revs --stdout \ - --thin --delta-base-offset -q Date: Fri, 21 Aug 2026 08:30:04 +0200 Subject: [PATCH 30/34] builtin/bundle: refactor option handling for progress meter The git-bundle(1) command has a couple of command line options that relate to whether or not progress should be reported. These options match the options that git-pack-objects(1) expects, and consequently they mostly get passed through to it directly. This results in somewhat of a confusing interface: there are four different options that relate to whether or not progress should be displayed and how verbose it should be. But in reality, there's really only two modes: - "--progress" and "--all-progress" result in the same outcome, which is also documented as such. - "--all-progress-implied" does nothing as we pass that argument to git-pack-objects(1) unconditionally anyway. So in the end, the options only control whether or not progress should be displayed at all, nothing else. Refactor the interface to instead use a simple `progress` boolean. This makes argument handling a lot more straight-forward and it prepares us for the next commit, where we're migrating git-bundle(1) to the generic interface for generating a packfile. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/bundle.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index 1e170e92780ed4..bfafadc984e2b6 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -70,35 +70,34 @@ static int parse_options_cmd_bundle(int argc, static int cmd_bundle_create(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { struct strvec pack_opts = STRVEC_INIT; + int progress = isatty(STDERR_FILENO); int version = -1; - int ret; struct option options[] = { - OPT_PASSTHRU_ARGV('q', "quiet", &pack_opts, NULL, - N_("do not show progress meter"), - PARSE_OPT_NOARG), - OPT_PASSTHRU_ARGV(0, "progress", &pack_opts, NULL, - N_("show progress meter"), - PARSE_OPT_NOARG), - OPT_PASSTHRU_ARGV(0, "all-progress", &pack_opts, NULL, - N_("historical; same as --progress"), - PARSE_OPT_NOARG | PARSE_OPT_HIDDEN), - OPT_PASSTHRU_ARGV(0, "all-progress-implied", &pack_opts, NULL, - N_("historical; does nothing"), - PARSE_OPT_NOARG | PARSE_OPT_HIDDEN), + OPT_NEGBIT('q', "quiet", &progress, + N_("do not show progress meter"), 1), + OPT_BIT(0, "progress", &progress, + N_("show progress meter"), 1), + OPT_BIT_F(0, "all-progress", &progress, + N_("historical; same as --progress"), 1, + PARSE_OPT_HIDDEN), + OPT_NOOP_NOARG(0, "all-progress-implied"), OPT_INTEGER(0, "version", &version, N_("specify bundle format version")), OPT_END() }; char *bundle_file; - - if (isatty(STDERR_FILENO)) - strvec_push(&pack_opts, "--progress"); - strvec_push(&pack_opts, "--all-progress-implied"); + int ret; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_create_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ + if (progress) + strvec_push(&pack_opts, "--progress"); + else + strvec_push(&pack_opts, "--quiet"); + strvec_push(&pack_opts, "--all-progress-implied"); + if (!startup_info->have_repository) die(_("Need a repository to create a bundle.")); ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version); From 9e8558a31d896ca1508a8e6febb1e54b04188eef Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 21 Aug 2026 08:30:05 +0200 Subject: [PATCH 31/34] bundle: get (mostly) rid of `the_repository` Refactor "bundle.c" so that we don't depend on `the_repository` anymore. This conversion is trivial for most of the part, as we already have a repository available in all calling conexts. The only exception is that we use `get_log_output_encoding()`, which implicitly depends on `the_repository`. Add an `extern` declaration for this function so that we can drop `USE_THE_REPOSITORY_VARIABLE` and not accidentally introduce more uses of `the_repository`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bundle.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/bundle.c b/bundle.c index b64716f252b78f..a9330bf0d32e5f 100644 --- a/bundle.c +++ b/bundle.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" @@ -21,6 +20,13 @@ #include "connected.h" #include "write-or-die.h" +/* + * NEEDSWORK: this function implicitly depends on `the_repository` and is not + * available because we dropped USE_THE_REPOSITORY_VARIABLE. We can remove the + * declaration once it's accessible via `repo_config_values`. + */ +extern const char *get_log_output_encoding(void); + static const char v2_bundle_signature[] = "# v2 git bundle\n"; static const char v3_bundle_signature[] = "# v3 git bundle\n"; static struct { @@ -294,7 +300,8 @@ int list_bundle_refs(struct bundle_header *header, int argc, const char **argv) return list_refs(&header->references, argc, argv); } -static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) +static int is_tag_in_date_range(struct repository *repo, + struct object *tag, struct rev_info *revs) { size_t size; enum object_type type; @@ -305,7 +312,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) if (revs->max_age == -1 && revs->min_age == -1) goto out; - buf = odb_read_object(the_repository->objects, &tag->oid, &type, &size); + buf = odb_read_object(repo->objects, &tag->oid, &type, &size); if (!buf) goto out; line = memmem(buf, size, "\ntagger ", 8); @@ -362,7 +369,8 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs, struct strvec * struct object *object = revs->pending.objects[i].item; if (object->flags & UNINTERESTING) write_or_die(pack_objects.in, "^", 1); - write_or_die(pack_objects.in, oid_to_hex(&object->oid), the_hash_algo->hexsz); + write_or_die(pack_objects.in, oid_to_hex(&object->oid), + revs->repo->hash_algo->hexsz); write_or_die(pack_objects.in, "\n", 1); } close(pack_objects.in); @@ -395,10 +403,10 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) if (e->item->flags & UNINTERESTING) continue; - if (repo_dwim_ref(the_repository, e->name, strlen(e->name), + if (repo_dwim_ref(revs->repo, e->name, strlen(e->name), &oid, &ref, 0) != 1) goto skip_write_ref; - if (refs_read_ref_full(get_main_ref_store(the_repository), e->name, RESOLVE_REF_READING, &oid, &flag)) + if (refs_read_ref_full(get_main_ref_store(revs->repo), e->name, RESOLVE_REF_READING, &oid, &flag)) flag = 0; display_ref = (flag & REF_ISSYMREF) ? e->name : ref; @@ -406,7 +414,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) goto skip_write_ref; if (e->item->type == OBJ_TAG && - !is_tag_in_date_range(e->item, revs)) { + !is_tag_in_date_range(revs->repo, e->item, revs)) { e->item->flags |= UNINTERESTING; goto skip_write_ref; } @@ -428,7 +436,8 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) ref_count++; strset_add(&objects, display_ref); - write_or_die(bundle_fd, oid_to_hex(&e->item->oid), the_hash_algo->hexsz); + write_or_die(bundle_fd, oid_to_hex(&e->item->oid), + revs->repo->hash_algo->hexsz); write_or_die(bundle_fd, " ", 1); write_or_die(bundle_fd, display_ref, strlen(display_ref)); write_or_die(bundle_fd, "\n", 1); @@ -507,7 +516,7 @@ int create_bundle(struct repository *r, const char *path, * SHA1. * 2. @filter is required because we parsed an object filter. */ - if (the_hash_algo != &hash_algos[GIT_HASH_SHA1_LEGACY] || revs.filter.choice) + if (r->hash_algo != &hash_algos[GIT_HASH_SHA1_LEGACY] || revs.filter.choice) min_version = 3; if (argc > 1) { @@ -528,14 +537,15 @@ int create_bundle(struct repository *r, const char *path, if (version < 2 || version > 3) { die(_("unsupported bundle version %d"), version); } else if (version < min_version) { - die(_("cannot write bundle version %d with algorithm %s"), version, the_hash_algo->name); + die(_("cannot write bundle version %d with algorithm %s"), version, + r->hash_algo->name); } else if (version == 2) { write_or_die(bundle_fd, v2_bundle_signature, strlen(v2_bundle_signature)); } else { const char *capability = "@object-format="; write_or_die(bundle_fd, v3_bundle_signature, strlen(v3_bundle_signature)); write_or_die(bundle_fd, capability, strlen(capability)); - write_or_die(bundle_fd, the_hash_algo->name, strlen(the_hash_algo->name)); + write_or_die(bundle_fd, r->hash_algo->name, strlen(r->hash_algo->name)); write_or_die(bundle_fd, "\n", 1); if (revs.filter.choice) { From 5176dd3d057ac5cae8321508febef61fa88537aa Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 21 Aug 2026 08:30:06 +0200 Subject: [PATCH 32/34] bundle: generate packfiles via the object database git-bundle(1) spawns git-pack-objects(1) directly to generate the pack data that gets appended to the bundle header. While bundles are not part of the wire protocol, they are a transfer mechanism for packs all the same, so convert them to use the pack generation interface of the object database as well. This makes the pack generator the single spawn point for all pack streams that leave the repository, leaving only local maintenance tasks like git-repack(1) with direct knowledge of git-pack-objects(1). Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/bundle.c | 13 +++------ bundle.c | 69 ++++++++++++++++++++++++------------------------ bundle.h | 3 +-- 3 files changed, 39 insertions(+), 46 deletions(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index bfafadc984e2b6..5c6d8e1343c929 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -68,8 +68,8 @@ static int parse_options_cmd_bundle(int argc, } static int cmd_bundle_create(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) { - struct strvec pack_opts = STRVEC_INIT; + struct repository *repo UNUSED) +{ int progress = isatty(STDERR_FILENO); int version = -1; struct option options[] = { @@ -92,16 +92,9 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix, builtin_bundle_create_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ - if (progress) - strvec_push(&pack_opts, "--progress"); - else - strvec_push(&pack_opts, "--quiet"); - strvec_push(&pack_opts, "--all-progress-implied"); - if (!startup_info->have_repository) die(_("Need a repository to create a bundle.")); - ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version); - strvec_clear(&pack_opts); + ret = !!create_bundle(the_repository, bundle_file, argc, argv, version, progress); free(bundle_file); return ret; } diff --git a/bundle.c b/bundle.c index a9330bf0d32e5f..f55a521b2a1f12 100644 --- a/bundle.c +++ b/bundle.c @@ -332,51 +332,52 @@ static int is_tag_in_date_range(struct repository *repo, /* Write the pack data to bundle_fd */ -static int write_pack_data(int bundle_fd, struct rev_info *revs, struct strvec *pack_options) +static int write_pack_data(int bundle_fd, struct rev_info *revs, int progress) { - struct child_process pack_objects = CHILD_PROCESS_INIT; + struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT; + struct odb_pack_generator *generator; + int ret = 0; int i; - strvec_pushl(&pack_objects.args, - "pack-objects", - "--stdout", "--thin", "--delta-base-offset", - NULL); - strvec_pushv(&pack_objects.args, pack_options->v); + opts.thin = 1; + opts.ofs_delta = 1; + if (progress) + opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE; if (revs->filter.choice) - strvec_pushf(&pack_objects.args, "--filter=%s", - list_objects_filter_spec(&revs->filter)); - pack_objects.in = -1; - pack_objects.out = bundle_fd; - pack_objects.git_cmd = 1; + opts.filter_spec = list_objects_filter_spec(&revs->filter); /* - * start_command() will close our descriptor if it's >1. Duplicate it - * to avoid surprising the caller. + * The pack generator will consume our descriptor if it's >1. + * Duplicate it to avoid surprising the caller. */ - if (pack_objects.out > 1) { - pack_objects.out = dup(pack_objects.out); - if (pack_objects.out < 0) { - error_errno(_("unable to dup bundle descriptor")); - child_process_clear(&pack_objects); - return -1; - } + opts.pack_fd = bundle_fd; + if (opts.pack_fd > 1) { + opts.pack_fd = dup(bundle_fd); + if (opts.pack_fd < 0) + return error_errno(_("unable to dup bundle descriptor")); } - if (start_command(&pack_objects)) - return error(_("Could not spawn pack-objects")); - for (i = 0; i < revs->pending.nr; i++) { struct object *object = revs->pending.objects[i].item; if (object->flags & UNINTERESTING) - write_or_die(pack_objects.in, "^", 1); - write_or_die(pack_objects.in, oid_to_hex(&object->oid), - revs->repo->hash_algo->hexsz); - write_or_die(pack_objects.in, "\n", 1); + oid_array_append(&opts.haves, &object->oid); + else + oid_array_append(&opts.wants, &object->oid); } - close(pack_objects.in); - if (finish_command(&pack_objects)) - return error(_("pack-objects died")); - return 0; + + if (odb_generate_pack(revs->repo->objects, &generator, &opts)) { + ret = error(_("Could not spawn pack-objects")); + goto out; + } + + if (odb_pack_generator_finish(generator)) { + ret = error(_("pack-objects died")); + goto out; + } + +out: + odb_generate_pack_options_release(&opts); + return ret; } /* @@ -485,7 +486,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data) } int create_bundle(struct repository *r, const char *path, - int argc, const char **argv, struct strvec *pack_options, int version) + int argc, const char **argv, int version, int progress) { struct lock_file lock = LOCK_INIT; int bundle_fd = -1; @@ -594,7 +595,7 @@ int create_bundle(struct repository *r, const char *path, } /* write pack */ - if (write_pack_data(bundle_fd, &revs_copy, pack_options)) { + if (write_pack_data(bundle_fd, &revs_copy, progress)) { ret = -1; goto out; } diff --git a/bundle.h b/bundle.h index d664b2f2d61e20..471da23d1bc9e4 100644 --- a/bundle.h +++ b/bundle.h @@ -27,8 +27,7 @@ int read_bundle_header(const char *path, struct bundle_header *header); int read_bundle_header_fd(int fd, struct bundle_header *header, const char *report_path); int create_bundle(struct repository *r, const char *path, - int argc, const char **argv, struct strvec *pack_options, - int version); + int argc, const char **argv, int version, int progress); enum verify_bundle_flags { VERIFY_BUNDLE_VERBOSE = (1 << 0), From 4e00f13e7ebc914287eed00399bb8c570ca8ab93 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 11 Aug 2026 11:04:48 +0200 Subject: [PATCH 33/34] odb/files: be less aggressive with geometric repacking When performing auto-maintenance with geometric repacking we have two conditions that may trigger a repack: - Either the geometric sequence of packfiles is invalidated. - Or we have too many loose objects. The first condition shouldn't trigger all that often: it may be hit when we fetch a new packfile, but users tend to not do that all the time. The second condition is what typically triggers more regularly though, as every command that ends up writing new objects may cause us to cross the threshold of loose objects. It is thus preferable to not be too aggressive here, as otherwise we may end up repacking objects quite often. For the geometric-repacking strategy though we have a default of 100 objects, only. As we're approximating the count of objects by only reading the "objects/17/" shared, we'd only need 2 objects in there before we perform a repack by default, which is quite aggressive. git-gc(1) on the other hand has a default of 6700, so it is quite a bit more conservative here. Being this aggressive is also causing problems as reported by our users. When running lots of concurrent writers, those writes will constantly end up spawning maintenance jobs that end up repacking objects. As we also prune objects, a concurrently running process that tries to write an object may see that the sharding directories get removed under their feet. While we try re-creating such leading directories, we only do so a single time, and it may happen that the directory vanishes again before we had the chance to create the loose object. This is not a new problem, but it is exacerbated by us running maintenance this aggressively. Improve the status quo by reducing the frequency at which we pack loose objects to the same frequency that git-gc(1) uses. Reported-by: Stefan Haller Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/config/maintenance.adoc | 2 +- odb/source-files.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index b578856dde1dd4..da8be9f812c68d 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -101,7 +101,7 @@ maintenance.geometric-repack.auto:: there are packfiles that need to be merged together to retain the geometric progression, or when there are at least this many loose objects that would be written into a new packfile. The default value is - 100. + 6700. maintenance.geometric-repack.splitFactor:: This integer config option controls the factor used for the geometric diff --git a/odb/source-files.c b/odb/source-files.c index 4f8e7ad7e35e2f..2bf5d9c0b271d9 100644 --- a/odb/source-files.c +++ b/odb/source-files.c @@ -538,7 +538,7 @@ bool odb_source_files_optimize_required(struct odb_source *source, }; struct existing_packs existing_packs = EXISTING_PACKS_INIT; struct string_list kept_packs = STRING_LIST_INIT_DUP; - int auto_value = 100; + int auto_value = 6700; bool ret; repo_config_get_int(repo, "maintenance.geometric-repack.auto", From 1630431f326e15fcde608827b5ff38422528eb59 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Aug 2026 08:08:06 -0700 Subject: [PATCH 34/34] The 21st batch Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.56.0.adoc | 45 ++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/Documentation/RelNotes/2.56.0.adoc b/Documentation/RelNotes/2.56.0.adoc index 3cf754a0bdfa49..21730157b58467 100644 --- a/Documentation/RelNotes/2.56.0.adoc +++ b/Documentation/RelNotes/2.56.0.adoc @@ -131,6 +131,14 @@ UI, Workflows & Features placeholders, and a quoting inconsistency in the running text has been fixed. + * The DWIM logic in 'git worktree add' sometimes tried to infer a + remote-tracking branch when an explicit '-b' or '-B' option was + given to create a new branch, causing the explicit branch name to + be ignored, which has been corrected. + + * The command line completion (in contrib/) has been taught to handle + the experimental 'git history' command. + Performance, Internal Implementation, Development Support etc. -------------------------------------------------------------- @@ -440,6 +448,38 @@ Performance, Internal Implementation, Development Support etc. reporting failures with a relative path to a sparse directory has been corrected. + * The object database (odb) API has been refactored to distinguish + between missing objects and corrupt ones by returning more + descriptive error statuses. Both the packed and loose backends now + faithfully propagate error details using a generic strbuf error + mechanism, removing backend-specific leakage from central lookup + paths. + + * The object database layer has been simplified by eagerly loading + alternate object directories upon initialization, instead of + deferring it to the first object lookup. This eliminates the need + for scattered lazy-loading calls throughout the codebase and paves + the way for integrating alternates with the pluggable backends. + + * The threshold for geometric repacking to trigger based on loose + object count has been adjusted to match that of 'git gc --auto', + preventing over-aggressive repacking during concurrent writes. + + * The 'git receive-pack' command has been updated to use a new ODB + transaction interface for writing incoming packfiles, making it more + backend-agnostic. + + * The mechanism to generate a packfile corresponding to the result of + a fetch/push has been made pluggable through a set of object + database callback functions, removing hardcoded references to + 'pack-objects' and enabling alternative ODBs to serve packfiles + themselves. + + * The pack-objects command has been updated to record the total bytes + written to pack files in trace2 output, allowing performance + analysis of different compression settings by comparing the + resulting pack sizes. + Fixes since v2.55 ----------------- @@ -699,5 +739,10 @@ Fixes since v2.55 been corrected. (merge 05e2ab1f31 jc/complete-checkout later to maint). + * The trailer parsing machinery has been updated to avoid mistaking + lines that begin with a URL (e.g., 'https://...') as trailer lines. + This prevents intended textual URLs from being mangled or mistakenly + treated as metadata keys. + * Other code cleanup, docfix, build fix, etc. (merge 026636128f ss/submittingpatches-typofix later to maint).