[PATCH 0/6] builtin/receive-pack: support pluggable packfile writes

[PATCH 0/6] builtin/receive-pack: support pluggable packfile writes

[Justin Tobler]

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.

As part of this series, the first patch also introduces the
`odb_transaction_release()` transaction interface. This is done to
decouple freeing the transaction from committing it and is used later in
the series to allow the post-commit cleanup of the ".keep" lockfile to
be deffered until after references have been updated. I'm a bit
uncertain as to whether the "release" part of the transaction lifecyle
is really the appropriate spot for such logic though. An alternative
could be to introduce a separate post-commit transaction interface that
would exist to remove any lockfiles after reference updates have been
performed. I am not certain such an explicit transaction interface is
also the best route either. In this version, I've opted to keep it
simple for now and tie the lockfile cleanup to transaction release, but
I am open to change based on feedback. :)

Most of the other patches are just structural refactorings to prepare
git-receive-pack(1) to eventually use `odb_transaction_write_pack()`.
The final patch makes the switch in git-receive-pack(1).

Thanks for the review,
-Justin

Justin Tobler (6):
  odb/transaction: add transaction release interface
  builtin/receive-pack: pass shallow file explicitly
  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/transaction: add transaction interface to write packfiles

 builtin/add.c            |   3 +-
 builtin/receive-pack.c   | 192 ++++++++++-----------------------------
 builtin/unpack-objects.c |   1 +
 builtin/update-index.c   |   2 +
 cache-tree.c             |   4 +-
 object-file.c            | 153 ++++++++++++++++++++++++++++++-
 odb/transaction.c        |  19 +++-
 odb/transaction.h        |  77 ++++++++++++++++
 read-cache.c             |   4 +-
 9 files changed, 305 insertions(+), 150 deletions(-)


base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
-- 
2.55.0.424.g13c7afec21

[PATCH 1/6] odb/transaction: add transaction release interface

[Justin Tobler]

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_release()` to explicitly clean up the transaction
accordingly. Note that the release 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.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/add.c            |  3 ++-
 builtin/receive-pack.c   |  1 +
 builtin/unpack-objects.c |  1 +
 builtin/update-index.c   |  2 ++
 cache-tree.c             |  4 +++-
 object-file.c            |  4 +++-
 odb/transaction.c        | 12 +++++++++++-
 odb/transaction.h        | 14 ++++++++++++++
 read-cache.c             |  4 +++-
 9 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 60ffbede2b..037491a51e 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);
 
@@ -610,5 +610,6 @@ int cmd_add(int argc,
 	free(ps_matched);
 	dir_clear(&dir);
 	clear_pathspec(&pathspec);
+	odb_transaction_release(transaction);
 	return exit_status;
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 86933d8d7e..420de9aa7f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2714,6 +2714,7 @@ int cmd_receive_pack(int argc,
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si, transaction,
 				 &push_options);
+		odb_transaction_release(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 4263edfbec..13d4b7f1ad 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -604,6 +604,7 @@ static void unpack_all(void)
 		display_progress(progress, i + 1);
 	}
 	odb_transaction_commit(transaction);
+	odb_transaction_release(transaction);
 	stop_progress(&progress);
 
 	if (delta_list)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 241abd4332..1484835ef0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1157,6 +1157,7 @@ int cmd_update_index(int argc,
 			 */
 			if (transaction && verbose) {
 				odb_transaction_commit(transaction);
+				odb_transaction_release(transaction);
 				transaction = NULL;
 			}
 
@@ -1225,6 +1226,7 @@ int cmd_update_index(int argc,
 	 * By now we have added all of the new objects
 	 */
 	odb_transaction_commit(transaction);
+	odb_transaction_release(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 d92f513286..5a2fa7f22d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -537,8 +537,10 @@ 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_release(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 ec35c318bc..30b4717d3e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -964,8 +964,10 @@ 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_release(transaction);
+			}
 		} else {
 			ret = hash_blob_stream(&stream,
 					       the_repository->hash_algo, oid,
diff --git a/odb/transaction.c b/odb/transaction.c
index dab7da6a9a..ce1e24f3ed 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -33,11 +33,21 @@ int odb_transaction_commit(struct odb_transaction *transaction)
 
 	ret = transaction->commit(transaction);
 	transaction->source->odb->transaction = NULL;
-	free(transaction);
 
 	return ret;
 }
 
+void odb_transaction_release(struct odb_transaction *transaction)
+{
+	if (!transaction)
+		return;
+
+	if (transaction->release)
+		transaction->release(transaction);
+
+	free(transaction);
+}
+
 int odb_transaction_write_object_stream(struct odb_transaction *transaction,
 					struct odb_write_stream *stream,
 					size_t len, struct object_id *oid)
diff --git a/odb/transaction.h b/odb/transaction.h
index 4cb2eafcbf..ec0b27c449 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.
+	 */
+	void (*release)(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,13 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
  */
 int odb_transaction_commit(struct odb_transaction *transaction);
 
+/*
+ * Releases 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.
+ */
+void odb_transaction_release(struct odb_transaction *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 6c449f393d..42623f6e10 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -4048,8 +4048,10 @@ 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_release(transaction);
+	}
 
 	release_revisions(&rev);
 	return !!data.add_errors;
-- 
2.55.0.424.g13c7afec21

[PATCH 2/6] builtin/receive-pack: pass shallow file explicitly

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 420de9aa7f..6da854fca2 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);
@@ -2427,14 +2425,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));
@@ -2443,13 +2441,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);
 
@@ -2489,12 +2488,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;
@@ -2513,7 +2513,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;
 	}
 
@@ -2705,11 +2705,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,
-- 
2.55.0.424.g13c7afec21

[PATCH 3/6] builtin/receive-pack: lift global state out of unpack()

[Justin Tobler]

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 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 <jltobler@gmail.com>
---
 builtin/receive-pack.c | 67 +++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 6da854fca2..8c2d6e5789 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2333,18 +2333,25 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
 }
 
+struct unpack_opts {
+	const char *fsck_msg_types;
+	const char *shallow_file;
+	off_t max_input_size;
+	int fsck_objects;
+	int unpack_limit;
+	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) {
@@ -2353,24 +2360,24 @@ 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);
 
-	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+	if (ntohl(hdr.hdr_entries) < opts->unpack_limit) {
 		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;
@@ -2391,18 +2398,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;
@@ -2428,11 +2435,24 @@ 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,
+		.unpack_limit = unpack_limit,
+		.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));
@@ -2441,7 +2461,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;
-- 
2.55.0.424.g13c7afec21

[PATCH 4/6] builtin/receive-pack: report unpack errors via strbuf

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 8c2d6e5789..7635b82bd3 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2028,7 +2028,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)
@@ -2344,8 +2344,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;
@@ -2357,7 +2357,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) {
@@ -2382,8 +2383,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;
@@ -2414,8 +2417,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;
+		}
 
 		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
 		if (lockfile) {
@@ -2425,15 +2430,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
@@ -2449,20 +2457,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;
@@ -2551,13 +2559,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, 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",
@@ -2575,14 +2583,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, 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;
 
@@ -2711,8 +2719,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);
@@ -2732,22 +2740,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_release(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);
@@ -2772,6 +2780,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);
-- 
2.55.0.424.g13c7afec21

[PATCH 5/6] builtin/receive-pack: explicitly pass packfile fd

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 7635b82bd3..743005f1f5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2305,9 +2305,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";
 
@@ -2344,8 +2344,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;
@@ -2353,7 +2353,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);
@@ -2380,6 +2380,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);
@@ -2414,6 +2415,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);
@@ -2460,7 +2462,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));
@@ -2470,7 +2472,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;
-- 
2.55.0.424.g13c7afec21

[PATCH 6/6] odb/transaction: add transaction interface to write packfiles

[Justin Tobler]

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 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.

Call sites in git-receive-pack(1) are updated accordingly.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/receive-pack.c | 143 +--------------------------------------
 object-file.c          | 149 +++++++++++++++++++++++++++++++++++++++++
 odb/transaction.c      |   7 ++
 odb/transaction.h      |  63 +++++++++++++++++
 4 files changed, 222 insertions(+), 140 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 743005f1f5..3069b53509 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2305,147 +2305,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));
-}
-
-struct unpack_opts {
-	const char *fsck_msg_types;
-	const char *shallow_file;
-	off_t max_input_size;
-	int fsck_objects;
-	int unpack_limit;
-	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) < opts->unpack_limit) {
-		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;
-		}
-
-		lockfile = index_pack_lockfile(the_repository, 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
@@ -2462,7 +2326,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));
@@ -2472,7 +2336,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;
@@ -2752,7 +2616,6 @@ int cmd_receive_pack(int argc,
 		execute_commands(commands, !!unpack_status.len, &si, transaction,
 				 &push_options);
 		odb_transaction_release(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 30b4717d3e..ec3b9a185e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -26,6 +26,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"
@@ -487,6 +488,10 @@ struct odb_transaction_files {
 	struct tmp_objdir *objdir;
 	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)
@@ -1292,6 +1297,148 @@ 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 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) < (unsigned int)opts->unpack_limit) {
+		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;
+		}
+
+		lockfile = index_pack_lockfile(repo, 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_reprepare(repo->objects);
+	}
+
+	return 0;
+}
+
+static void odb_transaction_files_release(struct odb_transaction *base)
+{
+	struct odb_transaction_files *transaction =
+		container_of(base, struct odb_transaction_files, base);
+
+	for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++)
+		delete_tempfile(&transaction->pack_lockfiles[i]);
+	free(transaction->pack_lockfiles);
+}
+
 static int odb_transaction_files_env(struct odb_transaction *base,
 				     struct strvec *env)
 {
@@ -1315,7 +1462,9 @@ 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.release = odb_transaction_files_release;
 	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->prefix = "bulk-fsync";
diff --git a/odb/transaction.c b/odb/transaction.c
index ce1e24f3ed..de03116ca0 100644
--- a/odb/transaction.c
+++ b/odb/transaction.c
@@ -55,6 +55,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 ec0b27c449..491026e815 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -4,6 +4,51 @@
 #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;
+	/*
+	 * The threshold for the number of incoming objects required to store
+	 * the objects in a packfile. This option may not be relevant to
+	 * backends that do not store obejcts in loose/packed formats and can be
+	 * ignored.
+	 */
+	int unpack_limit;
+	/*
+	 * 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 +85,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
@@ -98,6 +152,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
-- 
2.55.0.424.g13c7afec21

Re: [PATCH 1/6] odb/transaction: add transaction release interface

[Patrick Steinhardt]

On Thu, Aug 06, 2026 at 04:38:54PM -0500, Justin Tobler wrote:
> 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).

Right. We don't want to remove ".keep" files until references have been
updated so that the potentially still unreachable objects won't get
pruned. And consequently we have to introduce an additional phase after
the transaction has been committed but before the refs were updated.

> Stop freeing the transaction in `odb_transaction_commit()` and introduce
> `odb_transaction_release()` to explicitly clean up the transaction
> accordingly. Note that the release 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.

I'm not a 100% sure whether I like "release" as a name, as it typically
indicates that we release memory and other resources hold on by Git. On
the other hand we also kind of release state in this case here, but it
feels like the consequence of that is broader than it usually is.

How about we call this "finalize" instead?

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 86933d8d7e..420de9aa7f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2714,6 +2714,7 @@ int cmd_receive_pack(int argc,
>  		use_keepalive = KEEPALIVE_ALWAYS;
>  		execute_commands(commands, unpack_status, &si, transaction,
>  				 &push_options);
> +		odb_transaction_release(transaction);
>  		delete_tempfile(&pack_lockfile);
>  		sigchain_push(SIGPIPE, SIG_IGN);
>  		if (report_status_v2)

I think this here is the only caller that we care about where we release
the transaction not immediately after committing it. This is because
`execute_commands()` is the function that's responsible for updating the
references, and thus we don't want to delete the ".keep" files before
it.

It would make sense to single out this caller in the commit message.

> diff --git a/odb/transaction.h b/odb/transaction.h
> index 4cb2eafcbf..ec0b27c449 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -75,6 +82,13 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
>   */
>  int odb_transaction_commit(struct odb_transaction *transaction);
>  
> +/*
> + * Releases 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.
> + */
> +void odb_transaction_release(struct odb_transaction *transaction);

Should this function be able to report errors? Cleaning up ".keep" files
can fail, and I'm not sure whether we should simply ignore those.

Patrick

Re: [PATCH 2/6] builtin/receive-pack: pass shallow file explicitly

[Patrick Steinhardt]

On Thu, Aug 06, 2026 at 04:38:55PM -0500, Justin Tobler wrote:
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 420de9aa7f..6da854fca2 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;

I always like seeing less global state.

> @@ -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);

Okay, so instead of creating the shallow file here, ...

> @@ -2705,11 +2705,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);
>  		}

... we create it in a transitive caller and then pass it down the stack.
Makes sense.

It's nice that we don't have to pass the shallow information at all
anymore as a consequence.

Patrick

Re: [PATCH 3/6] builtin/receive-pack: lift global state out of unpack()

[Patrick Steinhardt]

On Thu, Aug 06, 2026 at 04:38:56PM -0500, Justin Tobler wrote:
> 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 rely on these globals.

Nit: either "can not" or "can no longer".

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 6da854fca2..8c2d6e5789 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2333,18 +2333,25 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
>  		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
>  }
>  
> +struct unpack_opts {
> +	const char *fsck_msg_types;
> +	const char *shallow_file;
> +	off_t max_input_size;
> +	int fsck_objects;
> +	int unpack_limit;
> +	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) {

It's quite hard to see that the function indeed doesn't rely on the
global variables anymore, and I'm quite certain that I'd not spot cases
that you forgot to convert to use the options structure instead. But I
assume that the function will move into a different file in a subsequent
commit, so we'd notice in that patch.

> @@ -2428,11 +2435,24 @@ 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),

This looks quite ugly, but it's no more ugly than the previous code it
replaces.

> @@ -2441,7 +2461,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);

Hm, okay. I guess this here is because we only want to manually read
stderr in case we use the sideband. It's a bit unfortunate that this
requires us to modify the passed-in options structure, but I guess I can
live with that.

Patrick

Re: [PATCH 4/6] builtin/receive-pack: report unpack errors via strbuf

[Patrick Steinhardt]

On Thu, Aug 06, 2026 at 04:38:57PM -0500, Justin Tobler wrote:
> 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.

If only Git had a structured error type, than we wouldn't have to have
such ugly workarounds. Anyway, this is a deeper issue and nothing we can
blame on this patch series.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 8c2d6e5789..7635b82bd3 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2344,8 +2344,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;

While I'm not a huge fan of error message parameters like this, this
change does make the calling convention more straight-forward. A reader
probably wouldn't have known beforehand what to do with the return value
without reading through docs.

Also, we cannot just return the equivalent of `return error("msg")`, as
we do want to use and munge the error message as part of the status
report we send to the client.

> @@ -2551,13 +2559,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, struct strbuf *unpack_status)

Should we mark this parameter as `const`?

> @@ -2575,14 +2583,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, struct strbuf *unpack_status)

And here, as well?

> @@ -2711,8 +2719,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;

Can't we reuse this buffer and reset it on every run to save some memory
allocations?

Patrick

Re: [PATCH 6/6] odb/transaction: add transaction interface to write packfiles

[Patrick Steinhardt]

On Thu, Aug 06, 2026 at 04:38:59PM -0500, Justin Tobler wrote:
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 743005f1f5..3069b53509 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
[snip]
>  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
> @@ -2462,7 +2326,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));
> @@ -2472,7 +2336,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;

Nicely done. All we need to do now is to rename the structure and the
parameters, and everything else was already taken care of in the
preceding commits.

> diff --git a/object-file.c b/object-file.c
> index 30b4717d3e..ec3b9a185e 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1292,6 +1297,148 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
[snip]
> +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) < (unsigned int)opts->unpack_limit) {
> +		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;
> +		}
> +
> +		lockfile = index_pack_lockfile(repo, 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);

A `git diff --color-moved` shows that almost all of the code was simply
moved around. The biggest change is this part here, where we now
register the packfiles as part of the transactions. Makes sense.

> +		status = finish_command(&child);
> +		if (status) {
> +			strbuf_addstr(err_msg, "index-pack abnormal exit");
> +			return -1;
> +		}
> +		odb_reprepare(repo->objects);

Now that this is part of the ODB transaction, do we really have to
reprepare the whole object database? Shouldn't it suffice to reprepare
just the one source that we've created the transaction for?

> diff --git a/odb/transaction.h b/odb/transaction.h
> index ec0b27c449..491026e815 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -4,6 +4,51 @@
>  #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;
> +	/*
> +	 * The threshold for the number of incoming objects required to store
> +	 * the objects in a packfile. This option may not be relevant to
> +	 * backends that do not store obejcts in loose/packed formats and can be
> +	 * ignored.
> +	 */
> +	int unpack_limit;

I wonder whether this option should rather be handled internal in the
backend itself, as it very likely doesn't apply to alternative backends
anyway. I don't think we allow command line options to override this, so
the backend could just read the configuration manually.

> +	/*
> +	 * 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;
> +};

Nit: I think having some spacing between the different options would
make this a bit easier to grok.

Patrick

Re: [PATCH 1/6] odb/transaction: add transaction release interface

[Justin Tobler]

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:54PM -0500, Justin Tobler wrote:
> I'm not a 100% sure whether I like "release" as a name, as it typically
> indicates that we release memory and other resources hold on by Git. On
> the other hand we also kind of release state in this case here, but it
> feels like the consequence of that is broader than it usually is.
> 
> How about we call this "finalize" instead?

Ya, that is fair. If we keep freeing the transaction and removing
lockfiles in the same lifecycle phase, "finalize" is probably a better
name. Will update in the next version.

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 86933d8d7e..420de9aa7f 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -2714,6 +2714,7 @@ int cmd_receive_pack(int argc,
> >  		use_keepalive = KEEPALIVE_ALWAYS;
> >  		execute_commands(commands, unpack_status, &si, transaction,
> >  				 &push_options);
> > +		odb_transaction_release(transaction);
> >  		delete_tempfile(&pack_lockfile);
> >  		sigchain_push(SIGPIPE, SIG_IGN);
> >  		if (report_status_v2)
> 
> I think this here is the only caller that we care about where we release
> the transaction not immediately after committing it. This is because
> `execute_commands()` is the function that's responsible for updating the
> references, and thus we don't want to delete the ".keep" files before
> it.
> 
> It would make sense to single out this caller in the commit message.

That is correct, git-receive-pack(1) is the only ODB transaction user
currently that cares about this. At this point in the series,
`odb_transaction_release()` is not yet cleaning up any lockfiles yet,
but will later on in the series. I'll explain this in the commit
message.

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index 4cb2eafcbf..ec0b27c449 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -75,6 +82,13 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
> >   */
> >  int odb_transaction_commit(struct odb_transaction *transaction);
> >  
> > +/*
> > + * Releases 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.
> > + */
> > +void odb_transaction_release(struct odb_transaction *transaction);
> 
> Should this function be able to report errors? Cleaning up ".keep" files
> can fail, and I'm not sure whether we should simply ignore those.

Good point. Will update in the next version.

-Justin

Re: [PATCH 3/6] builtin/receive-pack: lift global state out of unpack()

[Justin Tobler]

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:56PM -0500, Justin Tobler wrote:
> > 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 rely on these globals.
> 
> Nit: either "can not" or "can no longer".

Will fix.

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 6da854fca2..8c2d6e5789 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -2333,18 +2333,25 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
> >  		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
> >  }
> >  
> > +struct unpack_opts {
> > +	const char *fsck_msg_types;
> > +	const char *shallow_file;
> > +	off_t max_input_size;
> > +	int fsck_objects;
> > +	int unpack_limit;
> > +	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) {
> 
> It's quite hard to see that the function indeed doesn't rely on the
> global variables anymore, and I'm quite certain that I'd not spot cases
> that you forgot to convert to use the options structure instead. But I
> assume that the function will move into a different file in a subsequent
> commit, so we'd notice in that patch.

Ya, that is indeed the plan. :)

> > @@ -2428,11 +2435,24 @@ 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),
> 
> This looks quite ugly, but it's no more ugly than the previous code it
> replaces.

In a different version of this patch, I modified the fsck objects field
after initialization and dropped the single statement here, but I
ultimately didn't think that looked much better either.

-Justin

Re: [PATCH 4/6] builtin/receive-pack: report unpack errors via strbuf

[Justin Tobler]

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:57PM -0500, Justin Tobler wrote:
> > @@ -2551,13 +2559,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, struct strbuf *unpack_status)
> 
> Should we mark this parameter as `const`?

Yes, will do in the next version.

> 
> > @@ -2575,14 +2583,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, struct strbuf *unpack_status)
> 
> And here, as well?

Will do.

> > @@ -2711,8 +2719,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;
> 
> Can't we reuse this buffer and reset it on every run to save some memory
> allocations?

Good suggestion. I'll lift this up in the next version so we can reuse
it for each iteration.

-Justin

Re: [PATCH 6/6] odb/transaction: add transaction interface to write packfiles

[Justin Tobler]

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:59PM -0500, Justin Tobler wrote:
> > +		status = finish_command(&child);
> > +		if (status) {
> > +			strbuf_addstr(err_msg, "index-pack abnormal exit");
> > +			return -1;
> > +		}
> > +		odb_reprepare(repo->objects);
> 
> Now that this is part of the ODB transaction, do we really have to
> reprepare the whole object database? Shouldn't it suffice to reprepare
> just the one source that we've created the transaction for?

Ya, this is a good suggestion. At this point, the packfile has only been
written to the transaction source, so it should be fine to just prepare
that source. Will do in the next version.

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index ec0b27c449..491026e815 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -4,6 +4,51 @@
> >  #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;
> > +	/*
> > +	 * The threshold for the number of incoming objects required to store
> > +	 * the objects in a packfile. This option may not be relevant to
> > +	 * backends that do not store obejcts in loose/packed formats and can be
> > +	 * ignored.
> > +	 */
> > +	int unpack_limit;
> 
> I wonder whether this option should rather be handled internal in the
> backend itself, as it very likely doesn't apply to alternative backends
> anyway. I don't think we allow command line options to override this, so
> the backend could just read the configuration manually.

This was something I was also considering initially. This option doesn't
really make much sense to have as part of the generic interface though.
I'll update in the next version to have the backend read this
configuration manually.

> > +	/*
> > +	 * 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;
> > +};
> 
> Nit: I think having some spacing between the different options would
> make this a bit easier to grok.

Will do.

-Justin

Re: [PATCH 4/6] builtin/receive-pack: report unpack errors via strbuf

[Justin Tobler]

On 26/08/07 10:36AM, Justin Tobler wrote:
> On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> > > @@ -2711,8 +2719,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;
> > 
> > Can't we reuse this buffer and reset it on every run to save some memory
> > allocations?

Looking at this more closely, there isn't actually any loop we are
running this in so I don't think there is any need to change how
`unpack_status` is set up here.

-Justin

[PATCH v2 0/7] builtin/receive-pack: support pluggable packfile writes

[Justin Tobler]

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 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 (7):
  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/transaction: add transaction interface to write packfiles

 builtin/add.c            |   3 +-
 builtin/receive-pack.c   | 211 +++++++++------------------------------
 builtin/unpack-objects.c |   1 +
 builtin/update-index.c   |   2 +
 cache-tree.c             |   4 +-
 object-file.c            | 184 +++++++++++++++++++++++++++++++++-
 odb/transaction.c        |  21 ++++
 odb/transaction.h        |  78 +++++++++++++++
 read-cache.c             |   4 +-
 9 files changed, 339 insertions(+), 169 deletions(-)

Range-diff against v1:
1:  d0a4b632bd ! 1:  10efcc22e4 odb/transaction: add transaction release interface
    @@ Metadata
     Author: Justin Tobler <jltobler@gmail.com>
     
      ## Commit message ##
    -    odb/transaction: add transaction release interface
    +    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
    @@ Commit message
         the case with the existing "files" backend in git-receive-pack(1).
     
         Stop freeing the transaction in `odb_transaction_commit()` and introduce
    -    `odb_transaction_release()` to explicitly clean up the transaction
    -    accordingly. Note that the release interface also provides an optional
    +    `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.
    +    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.
     
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
    @@ builtin/add.c: int cmd_add(int argc,
      	free(ps_matched);
      	dir_clear(&dir);
      	clear_pathspec(&pathspec);
    -+	odb_transaction_release(transaction);
    ++	odb_transaction_finalize(transaction);
      	return exit_status;
      }
     
    @@ builtin/receive-pack.c: int cmd_receive_pack(int argc,
      		use_keepalive = KEEPALIVE_ALWAYS;
      		execute_commands(commands, unpack_status, &si, transaction,
      				 &push_options);
    -+		odb_transaction_release(transaction);
    ++		odb_transaction_finalize(transaction);
      		delete_tempfile(&pack_lockfile);
      		sigchain_push(SIGPIPE, SIG_IGN);
      		if (report_status_v2)
    @@ builtin/unpack-objects.c: static void unpack_all(void)
      		display_progress(progress, i + 1);
      	}
      	odb_transaction_commit(transaction);
    -+	odb_transaction_release(transaction);
    ++	odb_transaction_finalize(transaction);
      	stop_progress(&progress);
      
      	if (delta_list)
    @@ builtin/update-index.c: int cmd_update_index(int argc,
      			 */
      			if (transaction && verbose) {
      				odb_transaction_commit(transaction);
    -+				odb_transaction_release(transaction);
    ++				odb_transaction_finalize(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_release(transaction);
    ++	odb_transaction_finalize(transaction);
      
      	if (split_index > 0) {
      		if (repo_config_get_split_index(the_repository) == 0)
    @@ cache-tree.c: int cache_tree_update(struct index_state *istate, int flags)
     -	if (!inflight)
     +	if (!inflight) {
      		odb_transaction_commit(transaction);
    -+		odb_transaction_release(transaction);
    ++		odb_transaction_finalize(transaction);
     +	}
      	trace2_region_leave("cache_tree", "update", istate->repo);
      	trace_performance_leave("cache_tree_update");
    @@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid,
     -			if (!inflight)
     +			if (!inflight) {
      				odb_transaction_commit(transaction);
    -+				odb_transaction_release(transaction);
    ++				odb_transaction_finalize(transaction);
     +			}
      		} else {
      			ret = hash_blob_stream(&stream,
    @@ odb/transaction.c: int odb_transaction_commit(struct odb_transaction *transactio
      
      	ret = transaction->commit(transaction);
      	transaction->source->odb->transaction = NULL;
    --	free(transaction);
    - 
    - 	return ret;
    - }
    - 
    -+void odb_transaction_release(struct odb_transaction *transaction)
    ++
    ++	return ret;
    ++}
    ++
    ++int odb_transaction_finalize(struct odb_transaction *transaction)
     +{
    -+	if (!transaction)
    -+		return;
    ++	int ret = 0;
     +
    -+	if (transaction->release)
    -+		transaction->release(transaction);
    ++	if (!transaction)
    ++		return 0;
     +
    -+	free(transaction);
    -+}
    ++	if (transaction->finalize)
    ++		ret = transaction->finalize(transaction);
     +
    - int odb_transaction_write_object_stream(struct odb_transaction *transaction,
    - 					struct odb_write_stream *stream,
    - 					size_t len, struct object_id *oid)
    + 	free(transaction);
    + 
    + 	return ret;
     
      ## odb/transaction.h ##
     @@ odb/transaction.h: struct odb_transaction {
    @@ odb/transaction.h: struct odb_transaction {
     +	/*
     +	 * Optional ODB source specific callback invoked when the transaction
     +	 * needs to perform any deferred cleanup after objects have been
    -+	 * committed.
    ++	 * committed. Returns 0 on success, a negative error code otherwise.
     +	 */
    -+	void (*release)(struct odb_transaction *transaction);
    ++	int (*finalize)(struct odb_transaction *transaction);
     +
      	/*
      	 * This callback is expected to write the given object stream into
    @@ odb/transaction.h: static inline void odb_transaction_begin_or_die(struct object
      int odb_transaction_commit(struct odb_transaction *transaction);
      
     +/*
    -+ * Releases an ODB transaction, performing any deferred cleanup and freeing it.
    ++ * 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.
    ++ * specified transaction is NULL, the function is a no-op. Returns 0 on success,
    ++ * a negative error code otherwise.
     + */
    -+void odb_transaction_release(struct odb_transaction *transaction);
    ++int odb_transaction_finalize(struct odb_transaction *transaction);
     +
      /*
       * Writes the object in the provided stream into the transaction. The resulting
    @@ read-cache.c: int add_files_to_cache(struct repository *repo, const char *prefix
     -	if (!inflight)
     +	if (!inflight) {
      		odb_transaction_commit(transaction);
    -+		odb_transaction_release(transaction);
    ++		odb_transaction_finalize(transaction);
     +	}
      
      	release_revisions(&rev);
2:  0aff7f769e = 2:  e1903ac32f builtin/receive-pack: pass shallow file explicitly
-:  ---------- > 3:  e4950c0abe builtin/receive-pack: read unpack limit config lazily
3:  61bac2a56f ! 4:  c9b4ff73ba builtin/receive-pack: lift global state out of unpack()
    @@ Commit message
         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 rely on these globals.
    +    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.
    @@ Commit message
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
      ## builtin/receive-pack.c ##
    -@@ builtin/receive-pack.c: static void push_header_arg(struct strvec *args, struct pack_header *hdr)
    - 		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
    +@@ builtin/receive-pack.c: static int get_unpack_limit(struct repository *repo)
    + 	return limit;
      }
      
     +struct unpack_opts {
    @@ builtin/receive-pack.c: static void push_header_arg(struct strvec *args, struct
     +	const char *shallow_file;
     +	off_t max_input_size;
     +	int fsck_objects;
    -+	int unpack_limit;
     +	int reject_thin;
     +	int err_fd;
     +	int quiet;
    @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transa
      	}
      
      	odb_transaction_env(transaction, &child.env);
    - 
    --	if (ntohl(hdr.hdr_entries) < unpack_limit) {
    -+	if (ntohl(hdr.hdr_entries) < opts->unpack_limit) {
    +@@ builtin/receive-pack.c: 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)
    @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transa
     +		.fsck_msg_types = fsck_msg_types.buf,
     +		.max_input_size = max_input_size,
     +		.shallow_file = shallow_file,
    -+		.unpack_limit = unpack_limit,
     +		.reject_thin = reject_thin,
     +		.quiet = quiet,
     +	};
4:  12b83ee3bc ! 5:  7be990c2c2 builtin/receive-pack: report unpack errors via strbuf
    @@ builtin/receive-pack.c: static void update_shallow_info(struct command *commands
      }
      
     -static void report(struct command *commands, const char *unpack_status)
    -+static void report(struct command *commands, struct strbuf *unpack_status)
    ++static void report(struct command *commands, const struct strbuf *unpack_status)
      {
      	struct command *cmd;
      	struct strbuf buf = STRBUF_INIT;
    @@ builtin/receive-pack.c: static void report(struct command *commands, const char
      }
      
     -static void report_v2(struct command *commands, const char *unpack_status)
    -+static void report_v2(struct command *commands, struct strbuf *unpack_status)
    ++static void report_v2(struct command *commands, const struct strbuf *unpack_status)
      {
      	struct command *cmd;
      	struct strbuf buf = STRBUF_INIT;
    @@ builtin/receive-pack.c: int cmd_receive_pack(int argc,
     -		execute_commands(commands, unpack_status, &si, transaction,
     +		execute_commands(commands, !!unpack_status.len, &si, transaction,
      				 &push_options);
    - 		odb_transaction_release(transaction);
    + 		odb_transaction_finalize(transaction);
      		delete_tempfile(&pack_lockfile);
      		sigchain_push(SIGPIPE, SIG_IGN);
      		if (report_status_v2)
5:  8678f4cd45 = 6:  742c724943 builtin/receive-pack: explicitly pass packfile fd
6:  c390f59367 ! 7:  7743cf242a odb/transaction: add transaction interface to write packfiles
    @@ Commit message
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
      ## builtin/receive-pack.c ##
    +@@
    + #include "gpg-interface.h"
    + #include "hex.h"
    + #include "hook.h"
    +-#include "lockfile.h"
    + #include "object.h"
    + #include "object-file.h"
    + #include "object-name.h"
    +@@
    + #include "oid-array.h"
    + #include "oidset.h"
    + #include "pack.h"
    +-#include "packfile.h"
    + #include "parse-options.h"
    + #include "pkt-line.h"
    + #include "protocol.h"
     @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *reader,
      	}
      }
    @@ 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 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;
    +-	}
    +-
    +-	return limit;
    +-}
    +-
     -struct unpack_opts {
     -	const char *fsck_msg_types;
     -	const char *shallow_file;
     -	off_t max_input_size;
     -	int fsck_objects;
    --	int unpack_limit;
     -	int reject_thin;
     -	int err_fd;
     -	int quiet;
    @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *read
     -
     -	odb_transaction_env(transaction, &child.env);
     -
    --	if (ntohl(hdr.hdr_entries) < opts->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 (opts->quiet)
    @@ builtin/receive-pack.c: static int unpack_with_sideband(struct odb_transaction *
     @@ builtin/receive-pack.c: int cmd_receive_pack(int argc,
      		execute_commands(commands, !!unpack_status.len, &si, transaction,
      				 &push_options);
    - 		odb_transaction_release(transaction);
    + 		odb_transaction_finalize(transaction);
     -		delete_tempfile(&pack_lockfile);
      		sigchain_push(SIGPIPE, SIG_IGN);
      		if (report_status_v2)
      			report_v2(commands, &unpack_status);
     
      ## object-file.c ##
    +@@
    + #define USE_THE_REPOSITORY_VARIABLE
    + 
    + #include "git-compat-util.h"
    ++#include "config.h"
    + #include "convert.h"
    + #include "dir.h"
    + #include "environment.h"
     @@
      #include "packfile.h"
      #include "path.h"
    @@ 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 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;
    ++	}
    ++
    ++	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)
    @@ 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)opts->unpack_limit) {
    ++	if (ntohl(hdr.hdr_entries) < (unsigned int)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
     +			strbuf_addstr(err_msg, "index-pack abnormal exit");
     +			return -1;
     +		}
    -+		odb_reprepare(repo->objects);
    ++
    ++		odb_source_prepare(repo->objects->sources,
    ++				   ODB_PREPARE_FLUSH_CACHES);
     +	}
     +
     +	return 0;
     +}
     +
    -+static void odb_transaction_files_release(struct odb_transaction *base)
    ++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++)
    -+		delete_tempfile(&transaction->pack_lockfiles[i]);
    ++		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
    ++
     +	free(transaction->pack_lockfiles);
    ++
    ++	return ret;
     +}
     +
      static int odb_transaction_files_env(struct odb_transaction *base,
    @@ object-file.c: 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.release = odb_transaction_files_release;
    ++	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;
    @@ odb/transaction.h
     +	 * 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;
    -+	/*
    -+	 * The threshold for the number of incoming objects required to store
    -+	 * the objects in a packfile. This option may not be relevant to
    -+	 * backends that do not store obejcts in loose/packed formats and can be
    -+	 * ignored.
    -+	 */
    -+	int unpack_limit;
    ++
     +	/*
     +	 * 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.
     +	 */

base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
-- 
2.55.0.424.g13c7afec21

[PATCH v2 1/7] odb/transaction: add transaction finalize interface

[Justin Tobler]

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.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/add.c            |  3 ++-
 builtin/receive-pack.c   |  1 +
 builtin/unpack-objects.c |  1 +
 builtin/update-index.c   |  2 ++
 cache-tree.c             |  4 +++-
 object-file.c            |  4 +++-
 odb/transaction.c        | 14 ++++++++++++++
 odb/transaction.h        | 15 +++++++++++++++
 read-cache.c             |  4 +++-
 9 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 60ffbede2b..501e114ed5 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);
 
@@ -610,5 +610,6 @@ int cmd_add(int argc,
 	free(ps_matched);
 	dir_clear(&dir);
 	clear_pathspec(&pathspec);
+	odb_transaction_finalize(transaction);
 	return exit_status;
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 86933d8d7e..8720281250 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2714,6 +2714,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 4263edfbec..aee68dc42d 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -604,6 +604,7 @@ static void unpack_all(void)
 		display_progress(progress, i + 1);
 	}
 	odb_transaction_commit(transaction);
+	odb_transaction_finalize(transaction);
 	stop_progress(&progress);
 
 	if (delta_list)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 241abd4332..e422342f52 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1157,6 +1157,7 @@ int cmd_update_index(int argc,
 			 */
 			if (transaction && verbose) {
 				odb_transaction_commit(transaction);
+				odb_transaction_finalize(transaction);
 				transaction = NULL;
 			}
 
@@ -1225,6 +1226,7 @@ int cmd_update_index(int argc,
 	 * By now we have added all of the new objects
 	 */
 	odb_transaction_commit(transaction);
+	odb_transaction_finalize(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 d92f513286..cff1d4fd48 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -537,8 +537,10 @@ 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);
+	}
 	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 ec35c318bc..f993d58056 100644
--- a/object-file.c
+++ b/object-file.c
@@ -964,8 +964,10 @@ 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);
+			}
 		} else {
 			ret = hash_blob_stream(&stream,
 					       the_repository->hash_algo, oid,
diff --git a/odb/transaction.c b/odb/transaction.c
index dab7da6a9a..9e9a982778 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 4cb2eafcbf..89f6902caf 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,14 @@ 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);
+
 /*
  * 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 6c449f393d..9a3ac4646f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -4048,8 +4048,10 @@ 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);
+	}
 
 	release_revisions(&rev);
 	return !!data.add_errors;
-- 
2.55.0.424.g13c7afec21

[PATCH v2 2/7] builtin/receive-pack: pass shallow file explicitly

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 8720281250..78d2911c00 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);
@@ -2427,14 +2425,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));
@@ -2443,13 +2441,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);
 
@@ -2489,12 +2488,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;
@@ -2513,7 +2513,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;
 	}
 
@@ -2705,11 +2705,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,
-- 
2.55.0.424.g13c7afec21

[PATCH v2 3/7] builtin/receive-pack: read unpack limit config lazily

[Justin Tobler]

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 <jltobler@gmail.com>
---
 builtin/receive-pack.c | 44 ++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 78d2911c00..5264d70467 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,30 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
 }
 
+static 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;
+	}
+
+	return limit;
+}
+
 static const char *unpack(struct odb_transaction *transaction,
 			  const char *shallow_file, int err_fd)
 {
@@ -2360,7 +2371,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)
@@ -2652,11 +2663,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:
 		/*
-- 
2.55.0.424.g13c7afec21

[PATCH v2 4/7] builtin/receive-pack: lift global state out of unpack()

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 5264d70467..21dab851ad 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2344,18 +2344,24 @@ static 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) {
@@ -2364,9 +2370,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);
@@ -2374,14 +2380,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;
@@ -2402,18 +2408,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;
@@ -2439,11 +2445,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));
@@ -2452,7 +2470,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;
-- 
2.55.0.424.g13c7afec21

[PATCH v2 5/7] builtin/receive-pack: report unpack errors via strbuf

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 21dab851ad..896439d46d 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)
@@ -2354,8 +2354,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;
@@ -2367,7 +2367,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) {
@@ -2392,8 +2393,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;
@@ -2424,8 +2427,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;
+		}
 
 		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
 		if (lockfile) {
@@ -2435,15 +2440,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
@@ -2458,20 +2466,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;
@@ -2560,13 +2568,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",
@@ -2584,14 +2592,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;
 
@@ -2715,8 +2723,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);
@@ -2736,22 +2744,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);
@@ -2776,6 +2784,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);
-- 
2.55.0.424.g13c7afec21

[PATCH v2 6/7] builtin/receive-pack: explicitly pass packfile fd

[Justin Tobler]

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 <jltobler@gmail.com>
---
 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 896439d46d..76e8f4216c 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";
 
@@ -2354,8 +2354,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;
@@ -2363,7 +2363,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);
@@ -2390,6 +2390,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);
@@ -2424,6 +2425,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);
@@ -2469,7 +2471,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));
@@ -2479,7 +2481,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;
-- 
2.55.0.424.g13c7afec21

[PATCH v2 7/7] odb/transaction: add transaction interface to write packfiles

[Justin Tobler]

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 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.

Call sites in git-receive-pack(1) are updated accordingly.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/receive-pack.c | 168 +-------------------------------------
 object-file.c          | 180 +++++++++++++++++++++++++++++++++++++++++
 odb/transaction.c      |   7 ++
 odb/transaction.h      |  63 +++++++++++++++
 4 files changed, 253 insertions(+), 165 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 76e8f4216c..e6e54ba55f 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,170 +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 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;
-	}
-
-	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;
-		}
-
-		lockfile = index_pack_lockfile(the_repository, 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
@@ -2471,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));
@@ -2481,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;
@@ -2756,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 f993d58056..424f5f148c 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"
@@ -487,6 +489,10 @@ struct odb_transaction_files {
 	struct tmp_objdir *objdir;
 	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)
@@ -1292,6 +1298,178 @@ 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 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;
+	}
+
+	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) < (unsigned int)get_unpack_limit(repo)) {
+		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;
+		}
+
+		lockfile = index_pack_lockfile(repo, 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(repo->objects->sources,
+				   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)
 {
@@ -1315,7 +1493,9 @@ 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->prefix = "bulk-fsync";
diff --git a/odb/transaction.c b/odb/transaction.c
index 9e9a982778..c9144e6cd6 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 89f6902caf..e77807c593 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -4,6 +4,51 @@
 #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 +85,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
@@ -99,6 +153,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
-- 
2.55.0.424.g13c7afec21