[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