From: Justin Tobler Date: Tue, 11 Aug 2026 17:54:06 GMT Subject: [PATCH v3 0/9] builtin/receive-pack: support pluggable packfile writes Message-ID: <20260811175415.2044235-1-jltobler@gmail.com> In-Reply-To: <20260809190106.1565882-1-jltobler@gmail.com> Greetings, With bdee7b3013 (builtin/receive-pack: stage incoming objects via ODB transactions, 2026-07-10), git-receive-pack(1) started using the ODB transaction interfaces to stage incoming objects. While this brought the command closer to being ODB backend agnostic, the underlying git-index-pack(1) and git-unpack-objects(1) processes used to actually write the objects to the transaction are still fundamentally tied to the "files" backend. This series aims to address this by introducing a generic `odb_transaction_write_pack()` transaction interface to handle writing the incoming packfile to the transaction. The existing logic in git-receive-pack(1) that spawns the child processes to write the packfile becomes the "files" backend implementation of this interface. Changes since V2: - Added a patch to address a bug causing ".keep" files from not being removed. - Started handling errors at transaction commit and finalize call sites instead of ignoring them. We also make sure `odb_transaction_finalize()` runs after every successful commit callsite to ensure proper cleanup. - Updated the code handling lazy loading of unpack limit configuration to not longer cache the value. - Added a patch to begin explictly tracking the ODB source used by the "files" transaction to avoid relying on the ordering of the ODB source list. - Updated some commit messages to improve clarity. Changes since V1: - Changed the "release" interface name to "finalize" and updated it to return error codes. - Marked some function parameters as const. - Unpack limit configuration is now resolved in the ODB transaction backend instead of wiring it through the interface. - When writing a packfile to the transaction, now only the transaction source is prepared. - Updated some commit messages. - Updated some code formatting. Thanks for the review, -Justin Justin Tobler (9): builtin/receive-pack: properly clean up keep files odb/transaction: add transaction finalize interface builtin/receive-pack: pass shallow file explicitly builtin/receive-pack: read unpack limit config lazily builtin/receive-pack: lift global state out of unpack() builtin/receive-pack: report unpack errors via strbuf builtin/receive-pack: explicitly pass packfile fd odb: return temporary ODB source when set odb/transaction: add transaction interface to write packfiles builtin/add.c | 4 +- builtin/receive-pack.c | 211 ++++++++----------------------------- builtin/unpack-objects.c | 2 +- builtin/update-index.c | 4 +- cache-tree.c | 2 +- fetch-pack.c | 2 +- object-file.c | 177 ++++++++++++++++++++++++++++++- odb.c | 9 +- odb.h | 6 +- odb/transaction.c | 21 ++++ odb/transaction.h | 85 +++++++++++++++ pack-write.c | 7 +- pack.h | 4 +- read-cache.c | 2 +- t/t5547-push-quarantine.sh | 14 +++ tmp-objdir.c | 8 +- tmp-objdir.h | 6 +- 17 files changed, 376 insertions(+), 188 deletions(-) Range-diff against v2: -: ---------- > 1: 58569303f9 builtin/receive-pack: properly clean up keep files 1: 10efcc22e4 ! 2: dba9696866 odb/transaction: add transaction finalize interface @@ Commit message `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 with + no work in between and cannot meaningfully recover should either step + fail, so introduce an `odb_transaction_commit_and_finalize_or_die()` + helper that performs both and dies on error. Call sites are updated + accordingly. + Signed-off-by: Justin Tobler ## builtin/add.c ## @@ builtin/add.c: int cmd_add(int argc, repo_config(repo, add_config, NULL); @@ builtin/add.c: int cmd_add(int argc, - free(ps_matched); - dir_clear(&dir); - clear_pathspec(&pathspec); -+ odb_transaction_finalize(transaction); - return exit_status; - } + + 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, ## builtin/receive-pack.c ## @@ builtin/receive-pack.c: int cmd_receive_pack(int argc, @@ builtin/receive-pack.c: int cmd_receive_pack(int argc, ## builtin/unpack-objects.c ## @@ builtin/unpack-objects.c: static void unpack_all(void) + unpack_one(i); display_progress(progress, i + 1); } - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); +- odb_transaction_commit(transaction); ++ odb_transaction_commit_and_finalize_or_die(transaction); stop_progress(&progress); if (delta_list) ## builtin/update-index.c ## @@ builtin/update-index.c: int cmd_update_index(int argc, + * a transaction. */ if (transaction && verbose) { - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); +- odb_transaction_commit(transaction); ++ odb_transaction_commit_and_finalize_or_die(transaction); transaction = NULL; } @@ builtin/update-index.c: int cmd_update_index(int argc, + /* * By now we have added all of the new objects */ - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); +- 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) ## cache-tree.c ## @@ cache-tree.c: int cache_tree_update(struct index_state *istate, int flags) - odb_transaction_begin_or_die(the_repository->objects, &transaction, 0); i = update_one(istate->cache_tree, istate->cache, istate->cache_nr, "", 0, &skip, flags); -- if (!inflight) -+ if (!inflight) { - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); -+ } + 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) ## object-file.c ## @@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid, - &stream, xsize_t(st->st_size), oid); -- if (!inflight) -+ if (!inflight) { - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); -+ } + 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, @@ odb/transaction.h: static inline void odb_transaction_begin_or_die(struct object + * 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 @@ odb/transaction.h: static inline void odb_transaction_begin_or_die(struct object ## read-cache.c ## @@ read-cache.c: int add_files_to_cache(struct repository *repo, const char *prefix, - if (!inflight) odb_transaction_begin_or_die(repo->objects, &transaction, 0); run_diff_files(&rev, DIFF_RACY_IS_MODIFIED); -- if (!inflight) -+ if (!inflight) { - odb_transaction_commit(transaction); -+ odb_transaction_finalize(transaction); -+ } + if (!inflight) +- odb_transaction_commit(transaction); ++ odb_transaction_commit_and_finalize_or_die(transaction); release_revisions(&rev); return !!data.add_errors; 2: e1903ac32f = 3: 09bc00a070 builtin/receive-pack: pass shallow file explicitly 3: e4950c0abe ! 4: 2586ea4041 builtin/receive-pack: read unpack limit config lazily @@ builtin/receive-pack.c: static void push_header_arg(struct strvec *args, struct ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); } -+static int get_unpack_limit(struct repository *repo) ++static unsigned int get_unpack_limit(struct repository *repo) +{ -+ static int limit = -1; ++ unsigned int limit = 100; + -+ if (limit < 0) { -+ int receive_limit = -1; -+ int transfer_limit = -1; -+ -+ repo_config_get_int(repo, "receive.unpacklimit", -+ &receive_limit); -+ repo_config_get_int(repo, "transfer.unpacklimit", -+ &transfer_limit); -+ -+ if (receive_limit >= 0) -+ limit = receive_limit; -+ else if (transfer_limit >= 0) -+ limit = transfer_limit; -+ else -+ limit = 100; -+ } ++ repo_config_get_uint(repo, "transfer.unpacklimit", &limit); ++ repo_config_get_uint(repo, "receive.unpacklimit", &limit); + + return limit; +} 4: c9b4ff73ba ! 5: adf325095e builtin/receive-pack: lift global state out of unpack() @@ Commit message Signed-off-by: Justin Tobler ## builtin/receive-pack.c ## -@@ builtin/receive-pack.c: static int get_unpack_limit(struct repository *repo) +@@ builtin/receive-pack.c: static unsigned int get_unpack_limit(struct repository *repo) return limit; } 5: 7be990c2c2 ! 6: 29f407bf36 builtin/receive-pack: report unpack errors via strbuf @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transa + return -1; + } - lockfile = index_pack_lockfile(the_repository, child.out, NULL); - if (lockfile) { + /* + * The lockfile filepath is expected to be the final location of @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transaction, close(child.out); 6: 742c724943 = 7: b85f5e868c builtin/receive-pack: explicitly pass packfile fd -: ---------- > 8: 620eafe035 odb: return temporary ODB source when set 7: 7743cf242a ! 9: 2e75a8bd6c odb/transaction: add transaction interface to write packfiles @@ Commit message 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 a packfile - written via git-index-pack(1) is kept in place by a ".keep" lockfile - that must be retained until references are updated. To faciliate this in - an ODB backend agnostic manner, the "files" transaction backend takes - ownership of these lockfiles and removes them post-commit through its - release callback. + `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. @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *read - ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); -} - --static int get_unpack_limit(struct repository *repo) +-static unsigned int get_unpack_limit(struct repository *repo) -{ -- static int limit = -1; +- unsigned int limit = 100; - -- if (limit < 0) { -- int receive_limit = -1; -- int transfer_limit = -1; -- -- repo_config_get_int(repo, "receive.unpacklimit", -- &receive_limit); -- repo_config_get_int(repo, "transfer.unpacklimit", -- &transfer_limit); -- -- if (receive_limit >= 0) -- limit = receive_limit; -- else if (transfer_limit >= 0) -- limit = transfer_limit; -- else -- limit = 100; -- } +- repo_config_get_uint(repo, "transfer.unpacklimit", &limit); +- repo_config_get_uint(repo, "receive.unpacklimit", &limit); - - return limit; -} @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *read - return -1; - } - -- 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); @@ object-file.c #include "strvec.h" #include "tempfile.h" @@ object-file.c: struct odb_transaction_files { - struct tmp_objdir *objdir; + struct odb_source *quarantine; struct transaction_packfile packfile; const char *prefix; + @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b + ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries)); +} + -+static int get_unpack_limit(struct repository *repo) ++static unsigned int get_unpack_limit(struct repository *repo) +{ -+ static int limit = -1; -+ -+ if (limit < 0) { -+ int receive_limit = -1; -+ int transfer_limit = -1; -+ -+ repo_config_get_int(repo, "receive.unpacklimit", -+ &receive_limit); -+ repo_config_get_int(repo, "transfer.unpacklimit", -+ &transfer_limit); -+ -+ if (receive_limit >= 0) -+ limit = receive_limit; -+ else if (transfer_limit >= 0) -+ limit = transfer_limit; -+ else -+ limit = 100; -+ } ++ unsigned int limit = 100; ++ ++ repo_config_get_uint(repo, "transfer.unpacklimit", &limit); ++ repo_config_get_uint(repo, "receive.unpacklimit", &limit); + + return limit; +} @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b + + odb_transaction_env(base, &child.env); + -+ if (ntohl(hdr.hdr_entries) < (unsigned int)get_unpack_limit(repo)) { ++ if (ntohl(hdr.hdr_entries) < get_unpack_limit(repo)) { + strvec_push(&child.args, "unpack-objects"); + push_header_arg(&child.args, &hdr); + if (opts->quiet) @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b + return -1; + } + -+ lockfile = index_pack_lockfile(repo, 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(base->source, child.out, NULL); + if (lockfile) { + ALLOC_GROW(transaction->pack_lockfiles, + transaction->pack_lockfiles_nr + 1, @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b + return -1; + } + -+ odb_source_prepare(repo->objects->sources, ++ odb_source_prepare(transaction->quarantine, + ODB_PREPARE_FLUSH_CACHES); + } + @@ odb/transaction.h + * The max size in bytes of the incoming packfile allowed. No limit is + * enforced when set to 0. + */ -+ + off_t max_input_size; + + /* base-commit: 2c78326f810173a4f3aefd8021f1e07575412481 -- 2.55.0.424.g13c7afec21