[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

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

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

> 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.
> ...
> +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)
> +{
> + ...
> +		odb_source_prepare(repo->objects->sources,
> +				   ODB_PREPARE_FLUSH_CACHES);
> +	}

This assumes that we are working with the first entry in the
'repo->objects->sources' linked list.  Should we not use the source
actually associated with the current transaction (I am guessing that
it is 'base->source' but I may be wrong)?

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

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

> 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;
>  }

There is only one non-local exit between transation-begin and
transaction-finalize, which is a call ot report_path_error()
followed by exit(128).  Will _finalize() stay to be just freeing
memory and nothing else?  It may be conceptually cleaner to jump to
the bottom to make sure the clean-up sequence will always happen.

The same comment applies to other codepaths to which this patch adds
_finalize() calls.

> 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)
> +{

Curiously no callers added by this patch checks the return value
of this function.  Intended or just sloppy?  If the former, perhaps
this wants to return void instead?

The same can be said for _commit(), by the way.

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

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

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

The above is confusing and I am lost.  Care to explain a bit more?

> +		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);
> +		}

Here we add the .keep file to the list of lockfiles.  We have
finalization step laer in odb_transaction_files_finalize() that
deletes the tempfiles when we are done, which comes after
the transaction is committed.

But isn't the odb_transaction_files_commit() where the migration of
tmp_objdir_migrate() happens?  Everything in the quarantine directory
including these .keep files are "migrated" (either link-to-the-new
followed by unlink-of-the-old, or rename-old-to-new) there.

And then ...

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

... we do the deletion of tempfile but has anybody migrated the path
to these files recorded in the lockfile structure?  How are we
removing the .keep files that were "migrated" when the transaction
was committed?

Puzzled and confused...

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

[Patrick Steinhardt]

On Sun, Aug 09, 2026 at 02:00:52PM -0500, Justin Tobler wrote:
> 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.

Oh, right. I think I saw the loop in `read_head_info()` itself. My
mistake.

Patrick

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

[Patrick Steinhardt]

On Sun, Aug 09, 2026 at 02:01:02PM -0500, Justin Tobler wrote:
> 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
> @@ -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)

Shouldn't the function return `unsigned int`? We always expect it to be
a positiv value, and in the final commit we have to add a cast because
of that.

> +{
> +	static int limit = -1;

Is it really necessary to have this be a static variable? As far as I
can see we'd only call `unpack()` once. Also, the cache would become
stale if we ever tried to read the limit for multiple different repos.

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

So how about something like this instead?

	static unsigned int get_unpack_limit(struct repository *repo)
	{
		unsigned int limit = 100;
		if (!repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit) ||
		    !repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit))
			/* do nothing */;
		return limit;
	}

Patrick

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

[Justin Tobler]

On 26/08/10 07:15AM, Patrick Steinhardt wrote:
> On Sun, Aug 09, 2026 at 02:01:02PM -0500, Justin Tobler wrote:
> > 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
> > @@ -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)
> 
> Shouldn't the function return `unsigned int`? We always expect it to be
> a positiv value, and in the final commit we have to add a cast because
> of that.

Will update.

> > +{
> > +	static int limit = -1;
> 
> Is it really necessary to have this be a static variable? As far as I
> can see we'd only call `unpack()` once. Also, the cache would become
> stale if we ever tried to read the limit for multiple different repos.

Good point and yes in practice we really will only be reading this once
so there isn't really point to caching it. Will update.

> > +	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;
> > +}
> 
> So how about something like this instead?
> 
> 	static unsigned int get_unpack_limit(struct repository *repo)
> 	{
> 		unsigned int limit = 100;
> 		if (!repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit) ||
> 		    !repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit))
> 			/* do nothing */;
> 		return limit;
> 	}

Good suggestion. Because `repo_config_get_uint()` leaves the value
untouched if no config is set, we could probably simplify even further
to something like this:

  static unsigned int get_unpack_limit(struct repository *repo)
  {
  	unsigned int limit = 100;
  
  	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
  	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
  
  	return limit;
  }

Thanks,
-Justin

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

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

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

I am not sure whether this is progress.

A function that defines a 'static int' internally and sets it only
once is akin to using a global variable.  I wonder whether it would
be too much work to add a new member to either 'repo->settings' or
'repo->config_values' to make the setting truly per-repository.

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

[Justin Tobler]

On 26/08/09 08:38PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > 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;
> >  }
> 
> There is only one non-local exit between transation-begin and
> transaction-finalize, which is a call ot report_path_error()
> followed by exit(128).  Will _finalize() stay to be just freeing
> memory and nothing else?  It may be conceptually cleaner to jump to
> the bottom to make sure the clean-up sequence will always happen.

In practice, only call sites that invoke `odb_transaction_write_pack()`
(only git-receive-pack(1) for now) actually need to be concerned about
any deferred clean up outside of just freeing some memory. Conceptually
this is a bit messy though and callers shouldn't ideally have to be
aware of such specifics.

It may make sense to align the clean-up as you suggested above. I will
explore in the next version.

> The same comment applies to other codepaths to which this patch adds
> _finalize() calls.
> 
> > 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)
> > +{
> 
> Curiously no callers added by this patch checks the return value
> of this function.  Intended or just sloppy?  If the former, perhaps
> this wants to return void instead?

In version 1 I did keep `odb_transaction_finalize()` void, but decided
to at least provide the option for callers to check for errors if they
wished. The existing callers don't, but there isn't a reason most of the
couldn't be more strict here. In the next version, similar to
`odb_transaction_begin_or_die()`, I may add an
`odb_transaction_finalize_or_die()` helper and adapt some of the
existing callers.

> The same can be said for _commit(), by the way.

There is one `odb_transaction_commit()` caller in
"builtin/receive-pack.c" that does check for errors, but ya all other
callers simply ignore them. For the same reasons mentioned above, I
opted to follow the existing behavior of ignoring temporary directory
related errors, but include error reporting as part of the interface in
case callers wanted to check. I could also add an
`odb_transaction_commit_or_die()` helper here too and adapt callers
where it is reasonable to be more strict.

-Justin

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

[Justin Tobler]

On 26/08/10 10:54AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > +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;
> > +}
> 
> I am not sure whether this is progress.
> 
> A function that defines a 'static int' internally and sets it only
> once is akin to using a global variable.  I wonder whether it would
> be too much work to add a new member to either 'repo->settings' or
> 'repo->config_values' to make the setting truly per-repository.

Ya, as Patrick mentioned in [1], making it static probably isn't even
really required because in practice we just check the unpack limit once.
For now, it may just be sufficient to fetch the unpack limit value on
demand.

-Justin

[1]: <anlegs6zfUysbx0C@pks.im>

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

[Justin Tobler]

On 26/08/09 06:54PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > 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.
> > ...
> > +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)
> > +{
> > + ...
> > +		odb_source_prepare(repo->objects->sources,
> > +				   ODB_PREPARE_FLUSH_CACHES);
> > +	}
> 
> This assumes that we are working with the first entry in the
> 'repo->objects->sources' linked list.  Should we not use the source
> actually associated with the current transaction (I am guessing that
> it is 'base->source' but I may be wrong)?

Currently tracking of the transaction's ODB source relies on reading the
first source in this list which is rather awkward in my opinion and
rather fragile. The ODB source specified by `base->source` here is
actually the main ODB source that the transaction is created against so
it can't be used here.

In a future series, my plan is to stop reordering the source list when
starting a transaction and instead track the source separately which
should simplify some of this. It may be a good idea for the ODB
transaction itself to track its own source regardless though and may be
something nice to add as a part of this series. I'll explore this in my
next version of the series.

Thanks,
-Justin

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

[Justin Tobler]

On 26/08/09 09:02PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > .... 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.
> 
> The above is confusing and I am lost.  Care to explain a bit more?

It should say "finalize callback" instead of "release callback". The
idea here though is that ".keep" files are likely an implementation
detail of the "files" backend, but we still need a way to clean them up
after a transaction is committed and reference updates have been
performed via the generic ODB transaction interface.

So after `odb_transaction_commit()`, the ".keep" files are tracked by
the "files" transaction and only removed once
`odb_transaction_finalize()` is invoked. It would be up to callers to
ensure that reference updates are performed as required prior to
finalize being invoked.

I'll try to explain this better in the next version.

> > +		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);
> > +		}
> 
> Here we add the .keep file to the list of lockfiles.  We have
> finalization step laer in odb_transaction_files_finalize() that
> deletes the tempfiles when we are done, which comes after
> the transaction is committed.
> 
> But isn't the odb_transaction_files_commit() where the migration of
> tmp_objdir_migrate() happens?  Everything in the quarantine directory
> including these .keep files are "migrated" (either link-to-the-new
> followed by unlink-of-the-old, or rename-old-to-new) there.
> 
> And then ...
> 
> > +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;
> > +}
> 
> ... we do the deletion of tempfile but has anybody migrated the path
> to these files recorded in the lockfile structure?  How are we
> removing the .keep files that were "migrated" when the transaction
> was committed?

The filepath recorded by the ".keep" tempfile structure is _supposed_ to
be the final path of the ".keep" file post-commit that way it knows its
location after its been migrated and can remove it. The path is
generated by `index_pack_lockfile()` and is supposed to use the real ODB
source path and not the transaction ODB source path... but this is not
happening anymore now that we are using ODB transactions in
git-receive-pack(1) which reorders the ODB source list order when the
transaction is applied...

This is a bug and needs to be fixed. The fix itself should be fairly
straightforward, we just need to tell `index_pack_lockfile()` the
correct ODB source it should be using. I'll send a patch later today
correct this.

Thanks,
-Justin

[PATCH v3 0/9] 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 V2:
- Added a patch to address a bug causing ".keep" files from not being
  removed.
- Started handling errors at transaction commit and finalize call sites
  instead of ignoring them. We also make sure
  `odb_transaction_finalize()` runs after every successful commit
  callsite to ensure proper cleanup.
- Updated the code handling lazy loading of unpack limit configuration
  to not longer cache the value.
- Added a patch to begin explictly tracking the ODB source used by the
  "files" transaction to avoid relying on the ordering of the ODB source
  list.
- Updated some commit messages to improve clarity.

Changes since V1:
- Changed the "release" interface name to "finalize" and updated it to
  return error codes.
- Marked some function parameters as const.
- Unpack limit configuration is now resolved in the ODB transaction
  backend instead of wiring it through the interface.
- When writing a packfile to the transaction, now only the transaction
  source is prepared.
- Updated some commit messages.
- Updated some code formatting.

Thanks for the review,
-Justin

Justin Tobler (9):
  builtin/receive-pack: properly clean up keep files
  odb/transaction: add transaction finalize interface
  builtin/receive-pack: pass shallow file explicitly
  builtin/receive-pack: read unpack limit config lazily
  builtin/receive-pack: lift global state out of unpack()
  builtin/receive-pack: report unpack errors via strbuf
  builtin/receive-pack: explicitly pass packfile fd
  odb: return temporary ODB source when set
  odb/transaction: add transaction interface to write packfiles

 builtin/add.c              |   4 +-
 builtin/receive-pack.c     | 211 ++++++++-----------------------------
 builtin/unpack-objects.c   |   2 +-
 builtin/update-index.c     |   4 +-
 cache-tree.c               |   2 +-
 fetch-pack.c               |   2 +-
 object-file.c              | 177 ++++++++++++++++++++++++++++++-
 odb.c                      |   9 +-
 odb.h                      |   6 +-
 odb/transaction.c          |  21 ++++
 odb/transaction.h          |  85 +++++++++++++++
 pack-write.c               |   7 +-
 pack.h                     |   4 +-
 read-cache.c               |   2 +-
 t/t5547-push-quarantine.sh |  14 +++
 tmp-objdir.c               |   8 +-
 tmp-objdir.h               |   6 +-
 17 files changed, 376 insertions(+), 188 deletions(-)

Range-diff against v2:
 -:  ---------- >  1:  58569303f9 builtin/receive-pack: properly clean up keep files
 1:  10efcc22e4 !  2:  dba9696866 odb/transaction: add transaction finalize interface
    @@ Commit message
         `odb_transaction_finalize()` call site in git-receive-pack(1) is made
         after the reference updates are finished.
     
    +    All other callers commit a transaction and immediately finalize it with
    +    no work in between and cannot meaningfully recover should either step
    +    fail, so introduce an `odb_transaction_commit_and_finalize_or_die()`
    +    helper that performs both and dies on error. Call sites are updated
    +    accordingly.
    +
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
      ## builtin/add.c ##
    @@ builtin/add.c: int cmd_add(int argc,
      	repo_config(repo, add_config, NULL);
      
     @@ builtin/add.c: int cmd_add(int argc,
    - 	free(ps_matched);
    - 	dir_clear(&dir);
    - 	clear_pathspec(&pathspec);
    -+	odb_transaction_finalize(transaction);
    - 	return exit_status;
    - }
    + 
    + 	if (chmod_arg && pathspec.nr)
    + 		exit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);
    +-	odb_transaction_commit(transaction);
    ++	odb_transaction_commit_and_finalize_or_die(transaction);
    + 
    + finish:
    + 	if (write_locked_index(repo->index, &lock_file,
     
      ## builtin/receive-pack.c ##
     @@ builtin/receive-pack.c: int cmd_receive_pack(int argc,
    @@ builtin/receive-pack.c: int cmd_receive_pack(int argc,
     
      ## builtin/unpack-objects.c ##
     @@ builtin/unpack-objects.c: static void unpack_all(void)
    + 		unpack_one(i);
      		display_progress(progress, i + 1);
      	}
    - 	odb_transaction_commit(transaction);
    -+	odb_transaction_finalize(transaction);
    +-	odb_transaction_commit(transaction);
    ++	odb_transaction_commit_and_finalize_or_die(transaction);
      	stop_progress(&progress);
      
      	if (delta_list)
     
      ## builtin/update-index.c ##
     @@ builtin/update-index.c: int cmd_update_index(int argc,
    + 			 * a transaction.
      			 */
      			if (transaction && verbose) {
    - 				odb_transaction_commit(transaction);
    -+				odb_transaction_finalize(transaction);
    +-				odb_transaction_commit(transaction);
    ++				odb_transaction_commit_and_finalize_or_die(transaction);
      				transaction = NULL;
      			}
      
     @@ builtin/update-index.c: int cmd_update_index(int argc,
    + 	/*
      	 * By now we have added all of the new objects
      	 */
    - 	odb_transaction_commit(transaction);
    -+	odb_transaction_finalize(transaction);
    +-	odb_transaction_commit(transaction);
    ++	odb_transaction_commit_and_finalize_or_die(transaction);
      
      	if (split_index > 0) {
      		if (repo_config_get_split_index(the_repository) == 0)
     
      ## cache-tree.c ##
     @@ cache-tree.c: int cache_tree_update(struct index_state *istate, int flags)
    - 		odb_transaction_begin_or_die(the_repository->objects, &transaction, 0);
      	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
      		       "", 0, &skip, flags);
    --	if (!inflight)
    -+	if (!inflight) {
    - 		odb_transaction_commit(transaction);
    -+		odb_transaction_finalize(transaction);
    -+	}
    + 	if (!inflight)
    +-		odb_transaction_commit(transaction);
    ++		odb_transaction_commit_and_finalize_or_die(transaction);
      	trace2_region_leave("cache_tree", "update", istate->repo);
      	trace_performance_leave("cache_tree_update");
      	if (i < 0)
     
      ## object-file.c ##
     @@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid,
    - 								  &stream,
      								  xsize_t(st->st_size),
      								  oid);
    --			if (!inflight)
    -+			if (!inflight) {
    - 				odb_transaction_commit(transaction);
    -+				odb_transaction_finalize(transaction);
    -+			}
    + 			if (!inflight)
    +-				odb_transaction_commit(transaction);
    ++				odb_transaction_commit_and_finalize_or_die(transaction);
      		} else {
      			ret = hash_blob_stream(&stream,
      					       the_repository->hash_algo, oid,
    @@ odb/transaction.h: static inline void odb_transaction_begin_or_die(struct object
     + * a negative error code otherwise.
     + */
     +int odb_transaction_finalize(struct odb_transaction *transaction);
    ++
    ++static inline void odb_transaction_commit_and_finalize_or_die(struct odb_transaction *transaction)
    ++{
    ++	if (odb_transaction_commit(transaction))
    ++		die(_("failed to commit ODB transaction"));
    ++	if (odb_transaction_finalize(transaction))
    ++		die(_("failed to finalize ODB transaction"));
    ++}
     +
      /*
       * Writes the object in the provided stream into the transaction. The resulting
    @@ odb/transaction.h: static inline void odb_transaction_begin_or_die(struct object
     
      ## read-cache.c ##
     @@ read-cache.c: int add_files_to_cache(struct repository *repo, const char *prefix,
    - 	if (!inflight)
      		odb_transaction_begin_or_die(repo->objects, &transaction, 0);
      	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
    --	if (!inflight)
    -+	if (!inflight) {
    - 		odb_transaction_commit(transaction);
    -+		odb_transaction_finalize(transaction);
    -+	}
    + 	if (!inflight)
    +-		odb_transaction_commit(transaction);
    ++		odb_transaction_commit_and_finalize_or_die(transaction);
      
      	release_revisions(&rev);
      	return !!data.add_errors;
 2:  e1903ac32f =  3:  09bc00a070 builtin/receive-pack: pass shallow file explicitly
 3:  e4950c0abe !  4:  2586ea4041 builtin/receive-pack: read unpack limit config lazily
    @@ builtin/receive-pack.c: static void push_header_arg(struct strvec *args, struct
      		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
      }
      
    -+static int get_unpack_limit(struct repository *repo)
    ++static unsigned int get_unpack_limit(struct repository *repo)
     +{
    -+	static int limit = -1;
    ++	unsigned int limit = 100;
     +
    -+	if (limit < 0) {
    -+		int receive_limit = -1;
    -+		int transfer_limit = -1;
    -+
    -+		repo_config_get_int(repo, "receive.unpacklimit",
    -+				    &receive_limit);
    -+		repo_config_get_int(repo, "transfer.unpacklimit",
    -+				    &transfer_limit);
    -+
    -+		if (receive_limit >= 0)
    -+			limit = receive_limit;
    -+		else if (transfer_limit >= 0)
    -+			limit = transfer_limit;
    -+		else
    -+			limit = 100;
    -+	}
    ++	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
    ++	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
     +
     +	return limit;
     +}
 4:  c9b4ff73ba !  5:  adf325095e builtin/receive-pack: lift global state out of unpack()
    @@ Commit message
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
      ## builtin/receive-pack.c ##
    -@@ builtin/receive-pack.c: static int get_unpack_limit(struct repository *repo)
    +@@ builtin/receive-pack.c: static unsigned int get_unpack_limit(struct repository *repo)
      	return limit;
      }
      
 5:  7be990c2c2 !  6:  29f407bf36 builtin/receive-pack: report unpack errors via strbuf
    @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transa
     +			return -1;
     +		}
      
    - 		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
    - 		if (lockfile) {
    + 		/*
    + 		 * The lockfile filepath is expected to be the final location of
     @@ builtin/receive-pack.c: static const char *unpack(struct odb_transaction *transaction,
      		close(child.out);
      
 6:  742c724943 =  7:  b85f5e868c builtin/receive-pack: explicitly pass packfile fd
 -:  ---------- >  8:  620eafe035 odb: return temporary ODB source when set
 7:  7743cf242a !  9:  2e75a8bd6c odb/transaction: add transaction interface to write packfiles
    @@ Commit message
     
         Introduce `odb_transaction_write_pack()` as a generic interface to
         handle writing a packfile to a transaction and use the logic from
    -    `unpack()` as the "files" backend implementation. Note that a packfile
    -    written via git-index-pack(1) is kept in place by a ".keep" lockfile
    -    that must be retained until references are updated. To faciliate this in
    -    an ODB backend agnostic manner, the "files" transaction backend takes
    -    ownership of these lockfiles and removes them post-commit through its
    -    release callback.
    +    `unpack()` as the "files" backend implementation. Note that when storing
    +    the objects as a packfile, git-index-pack(1) also writes a ".keep"
    +    lockfile next to it to prevent a concurrent repack from removing the new
    +    pack prior to reference updates being performed. The "files" transaction
    +    backend is responsible for managing these ".keep" files and removes them
    +    post-commit once the transaction is finalized.
     
         Call sites in git-receive-pack(1) are updated accordingly.
     
    @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *read
     -		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
     -}
     -
    --static int get_unpack_limit(struct repository *repo)
    +-static unsigned int get_unpack_limit(struct repository *repo)
     -{
    --	static int limit = -1;
    +-	unsigned int limit = 100;
     -
    --	if (limit < 0) {
    --		int receive_limit = -1;
    --		int transfer_limit = -1;
    --
    --		repo_config_get_int(repo, "receive.unpacklimit",
    --				    &receive_limit);
    --		repo_config_get_int(repo, "transfer.unpacklimit",
    --				    &transfer_limit);
    --
    --		if (receive_limit >= 0)
    --			limit = receive_limit;
    --		else if (transfer_limit >= 0)
    --			limit = transfer_limit;
    --		else
    --			limit = 100;
    --	}
    +-	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
    +-	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
     -
     -	return limit;
     -}
    @@ builtin/receive-pack.c: static void read_push_options(struct packet_reader *read
     -			return -1;
     -		}
     -
    --		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
    +-		/*
    +-		 * The lockfile filepath is expected to be the final location of
    +-		 * the ".keep" file after being migrated to the main ODB source.
    +-		 * This ensures the lockfile can be found and removed later
    +-		 * after the ODB transaction has been committed.
    +-		 */
    +-		lockfile = index_pack_lockfile(transaction->source, child.out, NULL);
     -		if (lockfile) {
     -			pack_lockfile = register_tempfile(lockfile);
     -			free(lockfile);
    @@ object-file.c
      #include "strvec.h"
      #include "tempfile.h"
     @@ object-file.c: struct odb_transaction_files {
    - 	struct tmp_objdir *objdir;
    + 	struct odb_source *quarantine;
      	struct transaction_packfile packfile;
      	const char *prefix;
     +
    @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
     +		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
     +}
     +
    -+static int get_unpack_limit(struct repository *repo)
    ++static unsigned int get_unpack_limit(struct repository *repo)
     +{
    -+	static int limit = -1;
    -+
    -+	if (limit < 0) {
    -+		int receive_limit = -1;
    -+		int transfer_limit = -1;
    -+
    -+		repo_config_get_int(repo, "receive.unpacklimit",
    -+				    &receive_limit);
    -+		repo_config_get_int(repo, "transfer.unpacklimit",
    -+				    &transfer_limit);
    -+
    -+		if (receive_limit >= 0)
    -+			limit = receive_limit;
    -+		else if (transfer_limit >= 0)
    -+			limit = transfer_limit;
    -+		else
    -+			limit = 100;
    -+	}
    ++	unsigned int limit = 100;
    ++
    ++	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
    ++	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
     +
     +	return limit;
     +}
    @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
     +
     +	odb_transaction_env(base, &child.env);
     +
    -+	if (ntohl(hdr.hdr_entries) < (unsigned int)get_unpack_limit(repo)) {
    ++	if (ntohl(hdr.hdr_entries) < get_unpack_limit(repo)) {
     +		strvec_push(&child.args, "unpack-objects");
     +		push_header_arg(&child.args, &hdr);
     +		if (opts->quiet)
    @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
     +			return -1;
     +		}
     +
    -+		lockfile = index_pack_lockfile(repo, child.out, NULL);
    ++		/*
    ++		 * The lockfile filepath is expected to be the final location of
    ++		 * the ".keep" file after being migrated to the main ODB source.
    ++		 * This ensures the lockfile can be found and removed later
    ++		 * after the ODB transaction has been committed.
    ++		 */
    ++		lockfile = index_pack_lockfile(base->source, child.out, NULL);
     +		if (lockfile) {
     +			ALLOC_GROW(transaction->pack_lockfiles,
     +				   transaction->pack_lockfiles_nr + 1,
    @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
     +			return -1;
     +		}
     +
    -+		odb_source_prepare(repo->objects->sources,
    ++		odb_source_prepare(transaction->quarantine,
     +				   ODB_PREPARE_FLUSH_CACHES);
     +	}
     +
    @@ odb/transaction.h
     +	 * The max size in bytes of the incoming packfile allowed. No limit is
     +	 * enforced when set to 0.
     +	 */
    -+
     +	off_t max_input_size;
     +
     +	/*

base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
-- 
2.55.0.424.g13c7afec21

[PATCH v3 1/9] builtin/receive-pack: properly clean up keep files

[Justin Tobler]

When git-receive-pack(1) stores an incoming packfile with
git-index-pack(1), a ".keep" file is written alongside it to hold the
pack in place until the references have been updated, and is removed
afterwards. The path used to remove it is derived via
`index_pack_lockfile()` from the repository's primary object directory.

In bdee7b3013 (builtin/receive-pack: stage incoming objects via ODB
transactions, 2026-07-10), git-receive-pack(1) started using the ODB
transaction interfaces instead of managing a temporary directory
directly. When starting an ODB transaction, the sources list is
reordered to insert the newly created transaction source first as the
primary to ensure writes are routed to it accordingly.

Prior to using ODB transactions, git-receive-pack(1) would only set the
temporary directory as the primary source for the child
git-index-pack(1) and git-unpack-objects(1) processes it spawned and the
parent process would set the temporary directory set as an alternate
only. By using ODB transactions, the ODB source list is also reordered
for the parent process which results in `index_pack_lockfile()` deriving
the ".keep" path relative to the temporary directory instead the actual
main ODB source path. Consequently, this prevents the ".keep" file from
being properly removed after being migrated into the main ODB source
post-commit.

Update `index_pack_lockfile()` to operate on an ODB source explicitly
provided to it and update call sites accordingly to pass the expected
ODB source.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/receive-pack.c     |  8 +++++++-
 fetch-pack.c               |  2 +-
 pack-write.c               |  7 ++++---
 pack.h                     |  4 +++-
 t/t5547-push-quarantine.sh | 14 ++++++++++++++
 5 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 86933d8d7e..d74b787148 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2412,7 +2412,13 @@ static const char *unpack(int err_fd, struct shallow_info *si,
 		if (status)
 			return "index-pack fork failed";
 
-		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
+		/*
+		 * The lockfile filepath is expected to be the final location of
+		 * the ".keep" file after being migrated to the main ODB source.
+		 * This ensures the lockfile can be found and removed later
+		 * after the ODB transaction has been committed.
+		 */
+		lockfile = index_pack_lockfile(transaction->source, child.out, NULL);
 		if (lockfile) {
 			pack_lockfile = register_tempfile(lockfile);
 			free(lockfile);
diff --git a/fetch-pack.c b/fetch-pack.c
index 922a9b2581..6df5813b33 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1075,7 +1075,7 @@ static int get_pack(struct fetch_pack_args *args,
 		die(_("fetch-pack: unable to fork off %s"), cmd_name);
 	if (do_keep && (pack_lockfiles || fsck_objects)) {
 		int is_well_formed;
-		char *pack_lockfile = index_pack_lockfile(the_repository,
+		char *pack_lockfile = index_pack_lockfile(the_repository->objects->sources,
 							  cmd.out,
 							  &is_well_formed);
 
diff --git a/pack-write.c b/pack-write.c
index 24033a9101..85674e4b72 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -469,10 +469,11 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
+char *index_pack_lockfile(struct odb_source *source, int ip_out,
+			  int *is_well_formed)
 {
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = r->hash_algo->hexsz + 6;
+	const int len = source->odb->repo->hash_algo->hexsz + 6;
 
 	/*
 	 * The first thing we expect from index-pack's output
@@ -489,7 +490,7 @@ char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
 		packname[len-1] = 0;
 		if (skip_prefix(packname, "keep\t", &name))
 			return xstrfmt("%s/pack/pack-%s.keep",
-				       repo_get_object_directory(r), name);
+				       source->path, name);
 		return NULL;
 	}
 	if (is_well_formed)
diff --git a/pack.h b/pack.h
index 1cde92082b..68dcf08cf3 100644
--- a/pack.h
+++ b/pack.h
@@ -3,6 +3,7 @@
 
 #include "object.h"
 #include "csum-file.h"
+#include "odb/source.h"
 
 struct packed_git;
 struct pack_window;
@@ -105,7 +106,8 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
-char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);
+char *index_pack_lockfile(struct odb_source *source, int fd,
+			  int *is_well_formed);
 
 struct ref;
 
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 0798ddab02..400a597606 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -70,4 +70,18 @@ test_expect_success 'updating a ref from quarantine is forbidden' '
 	git -C update.git fsck
 '
 
+test_expect_success '.keep file is removed after push' '
+	test_when_finished rm -rf keep.git &&
+	git init --bare keep.git &&
+
+	git -C keep.git config set receive.unpackLimit 0 &&
+	test_commit foo &&
+	git push keep.git HEAD &&
+	pack="$(ls keep.git/objects/pack/pack-*.pack)" &&
+	keep="${pack%.pack}.keep" &&
+
+	test_path_is_file "$pack" &&
+	test_path_is_missing "$keep"
+'
+
 test_done
-- 
2.55.0.424.g13c7afec21

[PATCH v3 2/9] 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.

All other callers commit a transaction and immediately finalize it with
no work in between and cannot meaningfully recover should either step
fail, so introduce an `odb_transaction_commit_and_finalize_or_die()`
helper that performs both and dies on error. Call sites are updated
accordingly.

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

diff --git a/builtin/add.c b/builtin/add.c
index 60ffbede2b..ad418a5952 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -393,7 +393,7 @@ int cmd_add(int argc,
 	char *seen = NULL;
 	char *ps_matched = NULL;
 	struct lock_file lock_file = LOCK_INIT;
-	struct odb_transaction *transaction;
+	struct odb_transaction *transaction = NULL;
 
 	repo_config(repo, add_config, NULL);
 
@@ -600,7 +600,7 @@ int cmd_add(int argc,
 
 	if (chmod_arg && pathspec.nr)
 		exit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);
-	odb_transaction_commit(transaction);
+	odb_transaction_commit_and_finalize_or_die(transaction);
 
 finish:
 	if (write_locked_index(repo->index, &lock_file,
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d74b787148..ed1edcbe93 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2720,6 +2720,7 @@ int cmd_receive_pack(int argc,
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si, transaction,
 				 &push_options);
+		odb_transaction_finalize(transaction);
 		delete_tempfile(&pack_lockfile);
 		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 4263edfbec..d6a2d616d9 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -603,7 +603,7 @@ static void unpack_all(void)
 		unpack_one(i);
 		display_progress(progress, i + 1);
 	}
-	odb_transaction_commit(transaction);
+	odb_transaction_commit_and_finalize_or_die(transaction);
 	stop_progress(&progress);
 
 	if (delta_list)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 241abd4332..b25d4ecb10 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1156,7 +1156,7 @@ int cmd_update_index(int argc,
 			 * a transaction.
 			 */
 			if (transaction && verbose) {
-				odb_transaction_commit(transaction);
+				odb_transaction_commit_and_finalize_or_die(transaction);
 				transaction = NULL;
 			}
 
@@ -1224,7 +1224,7 @@ int cmd_update_index(int argc,
 	/*
 	 * By now we have added all of the new objects
 	 */
-	odb_transaction_commit(transaction);
+	odb_transaction_commit_and_finalize_or_die(transaction);
 
 	if (split_index > 0) {
 		if (repo_config_get_split_index(the_repository) == 0)
diff --git a/cache-tree.c b/cache-tree.c
index d92f513286..a220372a42 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -538,7 +538,7 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
 		       "", 0, &skip, flags);
 	if (!inflight)
-		odb_transaction_commit(transaction);
+		odb_transaction_commit_and_finalize_or_die(transaction);
 	trace2_region_leave("cache_tree", "update", istate->repo);
 	trace_performance_leave("cache_tree_update");
 	if (i < 0)
diff --git a/object-file.c b/object-file.c
index ec35c318bc..4d03c167d5 100644
--- a/object-file.c
+++ b/object-file.c
@@ -965,7 +965,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
 								  xsize_t(st->st_size),
 								  oid);
 			if (!inflight)
-				odb_transaction_commit(transaction);
+				odb_transaction_commit_and_finalize_or_die(transaction);
 		} else {
 			ret = hash_blob_stream(&stream,
 					       the_repository->hash_algo, oid,
diff --git a/odb/transaction.c b/odb/transaction.c
index 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..6ed39b3d0e 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -22,6 +22,13 @@ struct odb_transaction {
 	 */
 	int (*commit)(struct odb_transaction *transaction);
 
+	/*
+	 * Optional ODB source specific callback invoked when the transaction
+	 * needs to perform any deferred cleanup after objects have been
+	 * committed. Returns 0 on success, a negative error code otherwise.
+	 */
+	int (*finalize)(struct odb_transaction *transaction);
+
 	/*
 	 * This callback is expected to write the given object stream into
 	 * the ODB transaction. Note that for now, only blobs support streaming.
@@ -75,6 +82,22 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
  */
 int odb_transaction_commit(struct odb_transaction *transaction);
 
+/*
+ * Finalizes an ODB transaction, performing any deferred cleanup and freeing it.
+ * Must be called for every successfully started transaction. Note that, if the
+ * specified transaction is NULL, the function is a no-op. Returns 0 on success,
+ * a negative error code otherwise.
+ */
+int odb_transaction_finalize(struct odb_transaction *transaction);
+
+static inline void odb_transaction_commit_and_finalize_or_die(struct odb_transaction *transaction)
+{
+	if (odb_transaction_commit(transaction))
+		die(_("failed to commit ODB transaction"));
+	if (odb_transaction_finalize(transaction))
+		die(_("failed to finalize ODB transaction"));
+}
+
 /*
  * Writes the object in the provided stream into the transaction. The resulting
  * object ID is written into the out pointer. Returns 0 on success, a negative
diff --git a/read-cache.c b/read-cache.c
index 6c449f393d..0cd0ef85ec 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -4049,7 +4049,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
 		odb_transaction_begin_or_die(repo->objects, &transaction, 0);
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	if (!inflight)
-		odb_transaction_commit(transaction);
+		odb_transaction_commit_and_finalize_or_die(transaction);
 
 	release_revisions(&rev);
 	return !!data.add_errors;
-- 
2.55.0.424.g13c7afec21

[PATCH v3 3/9] 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 ed1edcbe93..135105deae 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -86,7 +86,6 @@ static const char *head_name;
 static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
-static const char *alt_shallow_file;
 static struct strbuf push_cert = STRBUF_INIT;
 static struct object_id push_cert_oid;
 static struct signature_check sigcheck;
@@ -2334,8 +2333,8 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
 }
 
-static const char *unpack(int err_fd, struct shallow_info *si,
-			  struct odb_transaction *transaction)
+static const char *unpack(struct odb_transaction *transaction,
+			  const char *shallow_file, int err_fd)
 {
 	struct pack_header hdr;
 	const char *hdr_err;
@@ -2354,10 +2353,9 @@ static const char *unpack(int err_fd, struct shallow_info *si,
 		return hdr_err;
 	}
 
-	if (si->nr_ours || si->nr_theirs) {
-		alt_shallow_file = setup_temporary_shallow(si->shallow);
+	if (shallow_file) {
 		strvec_push(&child.args, "--shallow-file");
-		strvec_push(&child.args, alt_shallow_file);
+		strvec_push(&child.args, shallow_file);
 	}
 
 	odb_transaction_env(transaction, &child.env);
@@ -2433,14 +2431,14 @@ static const char *unpack(int err_fd, struct shallow_info *si,
 	return NULL;
 }
 
-static const char *unpack_with_sideband(struct shallow_info *si,
-					struct odb_transaction *transaction)
+static const char *unpack_with_sideband(struct odb_transaction *transaction,
+					const char *shallow_file)
 {
 	struct async muxer;
 	const char *ret;
 
 	if (!use_sideband)
-		return unpack(0, si, transaction);
+		return unpack(transaction, shallow_file, 0);
 
 	use_keepalive = KEEPALIVE_AFTER_NUL;
 	memset(&muxer, 0, sizeof(muxer));
@@ -2449,13 +2447,14 @@ static const char *unpack_with_sideband(struct shallow_info *si,
 	if (start_async(&muxer))
 		return NULL;
 
-	ret = unpack(muxer.in, si, transaction);
+	ret = unpack(transaction, shallow_file, muxer.in);
 
 	finish_async(&muxer);
 	return ret;
 }
 
-static void prepare_shallow_update(struct shallow_info *si)
+static void prepare_shallow_update(struct shallow_info *si,
+				   const char *shallow_file)
 {
 	int i, j, k, bitmap_size = DIV_ROUND_UP(si->ref->nr, 32);
 
@@ -2495,12 +2494,13 @@ static void prepare_shallow_update(struct shallow_info *si)
 	 * command. check_connected() will be done with
 	 * true .git/shallow though.
 	 */
-	setenv(GIT_SHALLOW_FILE_ENVIRONMENT, alt_shallow_file, 1);
+	setenv(GIT_SHALLOW_FILE_ENVIRONMENT, shallow_file, 1);
 }
 
 static void update_shallow_info(struct command *commands,
 				struct shallow_info *si,
-				struct oid_array *ref)
+				struct oid_array *ref,
+				const char *shallow_file)
 {
 	struct command *cmd;
 	int *ref_status;
@@ -2519,7 +2519,7 @@ static void update_shallow_info(struct command *commands,
 	si->ref = ref;
 
 	if (shallow_update) {
-		prepare_shallow_update(si);
+		prepare_shallow_update(si, shallow_file);
 		return;
 	}
 
@@ -2711,11 +2711,17 @@ int cmd_receive_pack(int argc,
 		if (!si.nr_ours && !si.nr_theirs)
 			shallow_update = 0;
 		if (!delete_only(commands)) {
+			const char *alt_shallow_file = NULL;
+
+			if (si.nr_ours || si.nr_theirs)
+				alt_shallow_file = setup_temporary_shallow(si.shallow);
+
 			if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))
 				unpack_status = "unable to start object transaction";
 			else
-				unpack_status = unpack_with_sideband(&si, transaction);
-			update_shallow_info(commands, &si, &ref);
+				unpack_status = unpack_with_sideband(transaction, alt_shallow_file);
+
+			update_shallow_info(commands, &si, &ref, alt_shallow_file);
 		}
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si, transaction,
-- 
2.55.0.424.g13c7afec21

[PATCH v3 4/9] 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 | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 135105deae..971dc3f52e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -62,12 +62,9 @@ static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static struct strbuf fsck_msg_types = STRBUF_INIT;
-static int receive_unpack_limit = -1;
-static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
 static int advertise_sid;
-static int unpack_limit = 100;
 static off_t max_input_size;
 static int report_status;
 static int report_status_v2;
@@ -157,16 +154,6 @@ static int receive_pack_config(const char *var, const char *value,
 		return 0;
 	}
 
-	if (strcmp(var, "receive.unpacklimit") == 0) {
-		receive_unpack_limit = git_config_int(var, value, ctx->kvi);
-		return 0;
-	}
-
-	if (strcmp(var, "transfer.unpacklimit") == 0) {
-		transfer_unpack_limit = git_config_int(var, value, ctx->kvi);
-		return 0;
-	}
-
 	if (strcmp(var, "receive.fsck.skiplist") == 0) {
 		char *path;
 
@@ -2333,6 +2320,16 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
 }
 
+static unsigned int get_unpack_limit(struct repository *repo)
+{
+	unsigned int limit = 100;
+
+	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
+	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
+
+	return limit;
+}
+
 static const char *unpack(struct odb_transaction *transaction,
 			  const char *shallow_file, int err_fd)
 {
@@ -2360,7 +2357,7 @@ static const char *unpack(struct odb_transaction *transaction,
 
 	odb_transaction_env(transaction, &child.env);
 
-	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+	if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) {
 		strvec_push(&child.args, "unpack-objects");
 		push_header_arg(&child.args, &hdr);
 		if (quiet)
@@ -2658,11 +2655,6 @@ int cmd_receive_pack(int argc,
 	if (cert_nonce_seed)
 		push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL));
 
-	if (0 <= receive_unpack_limit)
-		unpack_limit = receive_unpack_limit;
-	else if (0 <= transfer_unpack_limit)
-		unpack_limit = transfer_unpack_limit;
-
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		/*
-- 
2.55.0.424.g13c7afec21

[PATCH v3 5/9] 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 971dc3f52e..f062b93b8d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2330,18 +2330,24 @@ static unsigned int get_unpack_limit(struct repository *repo)
 	return limit;
 }
 
+struct unpack_opts {
+	const char *fsck_msg_types;
+	const char *shallow_file;
+	off_t max_input_size;
+	int fsck_objects;
+	int reject_thin;
+	int err_fd;
+	int quiet;
+};
+
 static const char *unpack(struct odb_transaction *transaction,
-			  const char *shallow_file, int err_fd)
+			  const struct unpack_opts *opts)
 {
 	struct pack_header hdr;
 	const char *hdr_err;
 	int status;
 	struct child_process child = CHILD_PROCESS_INIT;
-	int fsck_objects = (receive_fsck_objects >= 0
-			    ? receive_fsck_objects
-			    : transfer_fsck_objects >= 0
-			    ? transfer_fsck_objects
-			    : 0);
+	int err_fd = opts->err_fd;
 
 	hdr_err = parse_pack_header(&hdr);
 	if (hdr_err) {
@@ -2350,9 +2356,9 @@ static const char *unpack(struct odb_transaction *transaction,
 		return hdr_err;
 	}
 
-	if (shallow_file) {
+	if (opts->shallow_file) {
 		strvec_push(&child.args, "--shallow-file");
-		strvec_push(&child.args, shallow_file);
+		strvec_push(&child.args, opts->shallow_file);
 	}
 
 	odb_transaction_env(transaction, &child.env);
@@ -2360,14 +2366,14 @@ static const char *unpack(struct odb_transaction *transaction,
 	if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) {
 		strvec_push(&child.args, "unpack-objects");
 		push_header_arg(&child.args, &hdr);
-		if (quiet)
+		if (opts->quiet)
 			strvec_push(&child.args, "-q");
-		if (fsck_objects)
+		if (opts->fsck_objects)
 			strvec_pushf(&child.args, "--strict%s",
-				     fsck_msg_types.buf);
-		if (max_input_size)
+				     opts->fsck_msg_types);
+		if (opts->max_input_size)
 			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
-				     (uintmax_t)max_input_size);
+				     (uintmax_t)opts->max_input_size);
 		child.no_stdout = 1;
 		child.err = err_fd;
 		child.git_cmd = 1;
@@ -2388,18 +2394,18 @@ static const char *unpack(struct odb_transaction *transaction,
 			     (uintmax_t)getpid(),
 			     hostname);
 
-		if (!quiet && err_fd)
+		if (!opts->quiet && err_fd)
 			strvec_push(&child.args, "--show-resolving-progress");
-		if (use_sideband)
+		if (err_fd)
 			strvec_push(&child.args, "--report-end-of-input");
-		if (fsck_objects)
+		if (opts->fsck_objects)
 			strvec_pushf(&child.args, "--strict%s",
-				     fsck_msg_types.buf);
-		if (!reject_thin)
+				     opts->fsck_msg_types);
+		if (!opts->reject_thin)
 			strvec_push(&child.args, "--fix-thin");
-		if (max_input_size)
+		if (opts->max_input_size)
 			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
-				     (uintmax_t)max_input_size);
+				     (uintmax_t)opts->max_input_size);
 		child.out = -1;
 		child.err = err_fd;
 		child.git_cmd = 1;
@@ -2431,11 +2437,23 @@ static const char *unpack(struct odb_transaction *transaction,
 static const char *unpack_with_sideband(struct odb_transaction *transaction,
 					const char *shallow_file)
 {
+	struct unpack_opts opts = {
+		.fsck_objects = (receive_fsck_objects >= 0
+				 ? receive_fsck_objects
+				 : transfer_fsck_objects >= 0
+				 ? transfer_fsck_objects
+				 : 0),
+		.fsck_msg_types = fsck_msg_types.buf,
+		.max_input_size = max_input_size,
+		.shallow_file = shallow_file,
+		.reject_thin = reject_thin,
+		.quiet = quiet,
+	};
 	struct async muxer;
 	const char *ret;
 
 	if (!use_sideband)
-		return unpack(transaction, shallow_file, 0);
+		return unpack(transaction, &opts);
 
 	use_keepalive = KEEPALIVE_AFTER_NUL;
 	memset(&muxer, 0, sizeof(muxer));
@@ -2444,7 +2462,8 @@ static const char *unpack_with_sideband(struct odb_transaction *transaction,
 	if (start_async(&muxer))
 		return NULL;
 
-	ret = unpack(transaction, shallow_file, muxer.in);
+	opts.err_fd = muxer.in;
+	ret = unpack(transaction, &opts);
 
 	finish_async(&muxer);
 	return ret;
-- 
2.55.0.424.g13c7afec21

[PATCH v3 6/9] 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 f062b93b8d..6df872697b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2015,7 +2015,7 @@ static void execute_commands_atomic(struct command *commands,
 }
 
 static void execute_commands(struct command *commands,
-			     const char *unpacker_error,
+			     int unpacker_error,
 			     struct shallow_info *si,
 			     struct odb_transaction *transaction,
 			     const struct string_list *push_options)
@@ -2340,8 +2340,8 @@ struct unpack_opts {
 	int quiet;
 };
 
-static const char *unpack(struct odb_transaction *transaction,
-			  const struct unpack_opts *opts)
+static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
+		  const struct unpack_opts *opts)
 {
 	struct pack_header hdr;
 	const char *hdr_err;
@@ -2353,7 +2353,8 @@ static const char *unpack(struct odb_transaction *transaction,
 	if (hdr_err) {
 		if (err_fd > 0)
 			close(err_fd);
-		return hdr_err;
+		strbuf_addstr(err_msg, hdr_err);
+		return -1;
 	}
 
 	if (opts->shallow_file) {
@@ -2378,8 +2379,10 @@ static const char *unpack(struct odb_transaction *transaction,
 		child.err = err_fd;
 		child.git_cmd = 1;
 		status = run_command(&child);
-		if (status)
-			return "unpack-objects abnormal exit";
+		if (status) {
+			strbuf_addstr(err_msg, "unpack-objects abnormal exit");
+			return -1;
+		}
 	} else {
 		char hostname[HOST_NAME_MAX + 1];
 		char *lockfile;
@@ -2410,8 +2413,10 @@ static const char *unpack(struct odb_transaction *transaction,
 		child.err = err_fd;
 		child.git_cmd = 1;
 		status = start_command(&child);
-		if (status)
-			return "index-pack fork failed";
+		if (status) {
+			strbuf_addstr(err_msg, "index-pack fork failed");
+			return -1;
+		}
 
 		/*
 		 * The lockfile filepath is expected to be the final location of
@@ -2427,15 +2432,18 @@ static const char *unpack(struct odb_transaction *transaction,
 		close(child.out);
 
 		status = finish_command(&child);
-		if (status)
-			return "index-pack abnormal exit";
+		if (status) {
+			strbuf_addstr(err_msg, "index-pack abnormal exit");
+			return -1;
+		}
 		odb_reprepare(the_repository->objects);
 	}
-	return NULL;
+	return 0;
 }
 
-static const char *unpack_with_sideband(struct odb_transaction *transaction,
-					const char *shallow_file)
+static int unpack_with_sideband(struct odb_transaction *transaction,
+				const char *shallow_file,
+				struct strbuf *err_msg)
 {
 	struct unpack_opts opts = {
 		.fsck_objects = (receive_fsck_objects >= 0
@@ -2450,20 +2458,20 @@ static const char *unpack_with_sideband(struct odb_transaction *transaction,
 		.quiet = quiet,
 	};
 	struct async muxer;
-	const char *ret;
+	int ret;
 
 	if (!use_sideband)
-		return unpack(transaction, &opts);
+		return unpack(transaction, err_msg, &opts);
 
 	use_keepalive = KEEPALIVE_AFTER_NUL;
 	memset(&muxer, 0, sizeof(muxer));
 	muxer.proc = copy_to_sideband;
 	muxer.in = -1;
 	if (start_async(&muxer))
-		return NULL;
+		return 0;
 
 	opts.err_fd = muxer.in;
-	ret = unpack(transaction, &opts);
+	ret = unpack(transaction, err_msg, &opts);
 
 	finish_async(&muxer);
 	return ret;
@@ -2552,13 +2560,13 @@ static void update_shallow_info(struct command *commands,
 	free(ref_status);
 }
 
-static void report(struct command *commands, const char *unpack_status)
+static void report(struct command *commands, const struct strbuf *unpack_status)
 {
 	struct command *cmd;
 	struct strbuf buf = STRBUF_INIT;
 
 	packet_buf_write(&buf, "unpack %s\n",
-			 unpack_status ? unpack_status : "ok");
+			 unpack_status->len ? unpack_status->buf : "ok");
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string)
 			packet_buf_write(&buf, "ok %s\n",
@@ -2576,14 +2584,14 @@ static void report(struct command *commands, const char *unpack_status)
 	strbuf_release(&buf);
 }
 
-static void report_v2(struct command *commands, const char *unpack_status)
+static void report_v2(struct command *commands, const struct strbuf *unpack_status)
 {
 	struct command *cmd;
 	struct strbuf buf = STRBUF_INIT;
 	struct ref_push_report *report;
 
 	packet_buf_write(&buf, "unpack %s\n",
-			 unpack_status ? unpack_status : "ok");
+			 unpack_status->len ? unpack_status->buf : "ok");
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		int count = 0;
 
@@ -2707,8 +2715,8 @@ int cmd_receive_pack(int argc,
 			   PACKET_READ_DIE_ON_ERR_PACKET);
 
 	if ((commands = read_head_info(&reader, &shallow))) {
-		const char *unpack_status = NULL;
 		struct string_list push_options = STRING_LIST_INIT_DUP;
+		struct strbuf unpack_status = STRBUF_INIT;
 
 		if (use_push_options)
 			read_push_options(&reader, &push_options);
@@ -2728,22 +2736,22 @@ int cmd_receive_pack(int argc,
 				alt_shallow_file = setup_temporary_shallow(si.shallow);
 
 			if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))
-				unpack_status = "unable to start object transaction";
+				strbuf_addstr(&unpack_status, "unable to start object transaction");
 			else
-				unpack_status = unpack_with_sideband(transaction, alt_shallow_file);
+				unpack_with_sideband(transaction, alt_shallow_file, &unpack_status);
 
 			update_shallow_info(commands, &si, &ref, alt_shallow_file);
 		}
 		use_keepalive = KEEPALIVE_ALWAYS;
-		execute_commands(commands, unpack_status, &si, transaction,
+		execute_commands(commands, !!unpack_status.len, &si, transaction,
 				 &push_options);
 		odb_transaction_finalize(transaction);
 		delete_tempfile(&pack_lockfile);
 		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
-			report_v2(commands, unpack_status);
+			report_v2(commands, &unpack_status);
 		else if (report_status)
-			report(commands, unpack_status);
+			report(commands, &unpack_status);
 		sigchain_pop(SIGPIPE);
 		run_receive_hook(commands, "post-receive", 1, NULL,
 				 &push_options);
@@ -2768,6 +2776,7 @@ int cmd_receive_pack(int argc,
 		if (auto_update_server_info)
 			update_server_info(the_repository, 0);
 		clear_shallow_info(&si);
+		strbuf_release(&unpack_status);
 	}
 	if (use_sideband)
 		packet_flush(1);
-- 
2.55.0.424.g13c7afec21

[PATCH v3 7/9] 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 6df872697b..b369466783 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2292,9 +2292,9 @@ static void read_push_options(struct packet_reader *reader,
 	}
 }
 
-static const char *parse_pack_header(struct pack_header *hdr)
+static const char *parse_pack_header(struct pack_header *hdr, int pack_fd)
 {
-	switch (read_pack_header(0, hdr)) {
+	switch (read_pack_header(pack_fd, hdr)) {
 	case PH_ERROR_EOF:
 		return "eof before pack header was fully read";
 
@@ -2340,8 +2340,8 @@ struct unpack_opts {
 	int quiet;
 };
 
-static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
-		  const struct unpack_opts *opts)
+static int unpack(struct odb_transaction *transaction, int pack_fd,
+		  struct strbuf *err_msg, const struct unpack_opts *opts)
 {
 	struct pack_header hdr;
 	const char *hdr_err;
@@ -2349,7 +2349,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
 	struct child_process child = CHILD_PROCESS_INIT;
 	int err_fd = opts->err_fd;
 
-	hdr_err = parse_pack_header(&hdr);
+	hdr_err = parse_pack_header(&hdr, pack_fd);
 	if (hdr_err) {
 		if (err_fd > 0)
 			close(err_fd);
@@ -2376,6 +2376,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
 			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
 				     (uintmax_t)opts->max_input_size);
 		child.no_stdout = 1;
+		child.in = pack_fd;
 		child.err = err_fd;
 		child.git_cmd = 1;
 		status = run_command(&child);
@@ -2410,6 +2411,7 @@ static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
 			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
 				     (uintmax_t)opts->max_input_size);
 		child.out = -1;
+		child.in = pack_fd;
 		child.err = err_fd;
 		child.git_cmd = 1;
 		status = start_command(&child);
@@ -2461,7 +2463,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction,
 	int ret;
 
 	if (!use_sideband)
-		return unpack(transaction, err_msg, &opts);
+		return unpack(transaction, 0, err_msg, &opts);
 
 	use_keepalive = KEEPALIVE_AFTER_NUL;
 	memset(&muxer, 0, sizeof(muxer));
@@ -2471,7 +2473,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction,
 		return 0;
 
 	opts.err_fd = muxer.in;
-	ret = unpack(transaction, err_msg, &opts);
+	ret = unpack(transaction, 0, err_msg, &opts);
 
 	finish_async(&muxer);
 	return ret;
-- 
2.55.0.424.g13c7afec21

[PATCH v3 8/9] odb: return temporary ODB source when set

[Justin Tobler]

When invoked, `odb_set_temporary_primary_source()` installs a temporary
object directory as the new primary ODB source. A caller that wants to
operate on the ODB source of the open transaction must assume that it is
the first entry in the ODB source list which is a bit awkward and
fragile.

Instead, return the newly installed source directly and report the
previous primary source via a new `prev_source` out parameter. Propagate
the installed source through `tmp_objdir_replace_primary_odb()` and
start storing it in the "files" ODB transaction so a subsequent commit
can easily access it without relying on the ODB source list ordering.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 3 ++-
 odb.c         | 9 +++++++--
 odb.h         | 6 ++++--
 tmp-objdir.c  | 8 +++++---
 tmp-objdir.h  | 6 ++++--
 5 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/object-file.c b/object-file.c
index 4d03c167d5..db63587f6d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -485,6 +485,7 @@ struct odb_transaction_files {
 	struct odb_transaction base;
 
 	struct tmp_objdir *objdir;
+	struct odb_source *quarantine;
 	struct transaction_packfile packfile;
 	const char *prefix;
 };
@@ -507,7 +508,7 @@ int odb_transaction_files_prepare(struct odb_transaction *base)
 	if (!transaction->objdir)
 		return error(_("unable to create temporary object directory"));
 
-	tmp_objdir_replace_primary_odb(transaction->objdir, 0);
+	transaction->quarantine = tmp_objdir_replace_primary_odb(transaction->objdir, 0);
 
 	return 0;
 }
diff --git a/odb.c b/odb.c
index caf1d0f542..8afcb6b637 100644
--- a/odb.c
+++ b/odb.c
@@ -226,7 +226,8 @@ struct odb_source *odb_add_to_alternates_memory(struct object_database *odb,
 }
 
 struct odb_source *odb_set_temporary_primary_source(struct object_database *odb,
-						    const char *dir, int will_destroy)
+						    const char *dir, int will_destroy,
+						    struct odb_source **prev_source)
 {
 	struct odb_source *source;
 
@@ -250,7 +251,11 @@ struct odb_source *odb_set_temporary_primary_source(struct object_database *odb,
 	source->will_destroy = will_destroy;
 	source->next = odb->sources;
 	odb->sources = source;
-	return source->next;
+
+	if (prev_source)
+		*prev_source = source->next;
+
+	return source;
 }
 
 void odb_restore_primary_source(struct object_database *odb,
diff --git a/odb.h b/odb.h
index fca67e8253..bdfcb9509a 100644
--- a/odb.h
+++ b/odb.h
@@ -199,10 +199,12 @@ struct odb_source *odb_find_source_or_die(struct object_database *odb, const cha
 
 /*
  * Replace the current writable object directory with the specified temporary
- * object directory; returns the former primary source.
+ * object directory and return the newly installed primary source. The former
+ * primary source is reported via `prev_source` when non-NULL.
  */
 struct odb_source *odb_set_temporary_primary_source(struct object_database *odb,
-						    const char *dir, int will_destroy);
+						    const char *dir, int will_destroy,
+						    struct odb_source **prev_source);
 
 /*
  * Restore the primary source that was previously replaced by
diff --git a/tmp-objdir.c b/tmp-objdir.c
index d199d39e7c..e633d97e0e 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -327,11 +327,13 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)
 	odb_add_to_alternates_memory(t->repo->objects, t->path.buf);
 }
 
-void tmp_objdir_replace_primary_odb(struct tmp_objdir *t, int will_destroy)
+struct odb_source *tmp_objdir_replace_primary_odb(struct tmp_objdir *t,
+						  int will_destroy)
 {
 	if (t->prev_source)
 		BUG("the primary object database is already replaced");
-	t->prev_source = odb_set_temporary_primary_source(t->repo->objects,
-							  t->path.buf, will_destroy);
 	t->will_destroy = will_destroy;
+
+	return odb_set_temporary_primary_source(t->repo->objects, t->path.buf,
+						will_destroy, &t->prev_source);
 }
diff --git a/tmp-objdir.h b/tmp-objdir.h
index ccf800faa7..81eb927413 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -64,8 +64,10 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
 /*
  * Replaces the writable object store in the current process with the temporary
  * object directory and makes the former main object store an alternate.
- * If will_destroy is nonzero, the object directory may not be migrated.
+ * If will_destroy is nonzero, the object directory may not be migrated. Returns
+ * the newly installed primary source.
  */
-void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);
+struct odb_source *tmp_objdir_replace_primary_odb(struct tmp_objdir *,
+						  int will_destroy);
 
 #endif /* TMP_OBJDIR_H */
-- 
2.55.0.424.g13c7afec21

[PATCH v3 9/9] 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 when storing
the objects as a packfile, git-index-pack(1) also writes a ".keep"
lockfile next to it to prevent a concurrent repack from removing the new
pack prior to reference updates being performed. The "files" transaction
backend is responsible for managing these ".keep" files and removes them
post-commit once the transaction is finalized.

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

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 builtin/receive-pack.c | 160 +-------------------------------------
 object-file.c          | 172 +++++++++++++++++++++++++++++++++++++++++
 odb/transaction.c      |   7 ++
 odb/transaction.h      |  62 +++++++++++++++
 4 files changed, 244 insertions(+), 157 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b369466783..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,162 +2290,11 @@ static void read_push_options(struct packet_reader *reader,
 	}
 }
 
-static const char *parse_pack_header(struct pack_header *hdr, int pack_fd)
-{
-	switch (read_pack_header(pack_fd, hdr)) {
-	case PH_ERROR_EOF:
-		return "eof before pack header was fully read";
-
-	case PH_ERROR_PACK_SIGNATURE:
-		return "protocol error (pack signature mismatch detected)";
-
-	case PH_ERROR_PROTOCOL:
-		return "protocol error (pack version unsupported)";
-
-	default:
-		return "unknown error in parse_pack_header";
-
-	case 0:
-		return NULL;
-	}
-}
-
-static struct tempfile *pack_lockfile;
-
-static void push_header_arg(struct strvec *args, struct pack_header *hdr)
-{
-	strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32,
-		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
-}
-
-static unsigned int get_unpack_limit(struct repository *repo)
-{
-	unsigned int limit = 100;
-
-	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
-	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
-
-	return limit;
-}
-
-struct unpack_opts {
-	const char *fsck_msg_types;
-	const char *shallow_file;
-	off_t max_input_size;
-	int fsck_objects;
-	int reject_thin;
-	int err_fd;
-	int quiet;
-};
-
-static int unpack(struct odb_transaction *transaction, int pack_fd,
-		  struct strbuf *err_msg, const struct unpack_opts *opts)
-{
-	struct pack_header hdr;
-	const char *hdr_err;
-	int status;
-	struct child_process child = CHILD_PROCESS_INIT;
-	int err_fd = opts->err_fd;
-
-	hdr_err = parse_pack_header(&hdr, pack_fd);
-	if (hdr_err) {
-		if (err_fd > 0)
-			close(err_fd);
-		strbuf_addstr(err_msg, hdr_err);
-		return -1;
-	}
-
-	if (opts->shallow_file) {
-		strvec_push(&child.args, "--shallow-file");
-		strvec_push(&child.args, opts->shallow_file);
-	}
-
-	odb_transaction_env(transaction, &child.env);
-
-	if (ntohl(hdr.hdr_entries) < get_unpack_limit(the_repository)) {
-		strvec_push(&child.args, "unpack-objects");
-		push_header_arg(&child.args, &hdr);
-		if (opts->quiet)
-			strvec_push(&child.args, "-q");
-		if (opts->fsck_objects)
-			strvec_pushf(&child.args, "--strict%s",
-				     opts->fsck_msg_types);
-		if (opts->max_input_size)
-			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
-				     (uintmax_t)opts->max_input_size);
-		child.no_stdout = 1;
-		child.in = pack_fd;
-		child.err = err_fd;
-		child.git_cmd = 1;
-		status = run_command(&child);
-		if (status) {
-			strbuf_addstr(err_msg, "unpack-objects abnormal exit");
-			return -1;
-		}
-	} else {
-		char hostname[HOST_NAME_MAX + 1];
-		char *lockfile;
-
-		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
-		push_header_arg(&child.args, &hdr);
-
-		if (xgethostname(hostname, sizeof(hostname)))
-			xsnprintf(hostname, sizeof(hostname), "localhost");
-		strvec_pushf(&child.args,
-			     "--keep=receive-pack %"PRIuMAX" on %s",
-			     (uintmax_t)getpid(),
-			     hostname);
-
-		if (!opts->quiet && err_fd)
-			strvec_push(&child.args, "--show-resolving-progress");
-		if (err_fd)
-			strvec_push(&child.args, "--report-end-of-input");
-		if (opts->fsck_objects)
-			strvec_pushf(&child.args, "--strict%s",
-				     opts->fsck_msg_types);
-		if (!opts->reject_thin)
-			strvec_push(&child.args, "--fix-thin");
-		if (opts->max_input_size)
-			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
-				     (uintmax_t)opts->max_input_size);
-		child.out = -1;
-		child.in = pack_fd;
-		child.err = err_fd;
-		child.git_cmd = 1;
-		status = start_command(&child);
-		if (status) {
-			strbuf_addstr(err_msg, "index-pack fork failed");
-			return -1;
-		}
-
-		/*
-		 * The lockfile filepath is expected to be the final location of
-		 * the ".keep" file after being migrated to the main ODB source.
-		 * This ensures the lockfile can be found and removed later
-		 * after the ODB transaction has been committed.
-		 */
-		lockfile = index_pack_lockfile(transaction->source, child.out, NULL);
-		if (lockfile) {
-			pack_lockfile = register_tempfile(lockfile);
-			free(lockfile);
-		}
-		close(child.out);
-
-		status = finish_command(&child);
-		if (status) {
-			strbuf_addstr(err_msg, "index-pack abnormal exit");
-			return -1;
-		}
-		odb_reprepare(the_repository->objects);
-	}
-	return 0;
-}
-
 static int unpack_with_sideband(struct odb_transaction *transaction,
 				const char *shallow_file,
 				struct strbuf *err_msg)
 {
-	struct unpack_opts opts = {
+	struct odb_transaction_write_pack_opts opts = {
 		.fsck_objects = (receive_fsck_objects >= 0
 				 ? receive_fsck_objects
 				 : transfer_fsck_objects >= 0
@@ -2463,7 +2310,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction,
 	int ret;
 
 	if (!use_sideband)
-		return unpack(transaction, 0, err_msg, &opts);
+		return odb_transaction_write_pack(transaction, 0, err_msg, &opts);
 
 	use_keepalive = KEEPALIVE_AFTER_NUL;
 	memset(&muxer, 0, sizeof(muxer));
@@ -2473,7 +2320,7 @@ static int unpack_with_sideband(struct odb_transaction *transaction,
 		return 0;
 
 	opts.err_fd = muxer.in;
-	ret = unpack(transaction, 0, err_msg, &opts);
+	ret = odb_transaction_write_pack(transaction, 0, err_msg, &opts);
 
 	finish_async(&muxer);
 	return ret;
@@ -2748,7 +2595,6 @@ int cmd_receive_pack(int argc,
 		execute_commands(commands, !!unpack_status.len, &si, transaction,
 				 &push_options);
 		odb_transaction_finalize(transaction);
-		delete_tempfile(&pack_lockfile);
 		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
 			report_v2(commands, &unpack_status);
diff --git a/object-file.c b/object-file.c
index db63587f6d..a957bc126f 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"
@@ -488,6 +490,10 @@ struct odb_transaction_files {
 	struct odb_source *quarantine;
 	struct transaction_packfile packfile;
 	const char *prefix;
+
+	struct tempfile **pack_lockfiles;
+	size_t pack_lockfiles_nr;
+	size_t pack_lockfiles_alloc;
 };
 
 int odb_transaction_files_prepare(struct odb_transaction *base)
@@ -1291,6 +1297,170 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 	return 0;
 }
 
+static const char *parse_pack_header(struct pack_header *hdr, int pack_fd)
+{
+	switch (read_pack_header(pack_fd, hdr)) {
+	case PH_ERROR_EOF:
+		return "eof before pack header was fully read";
+
+	case PH_ERROR_PACK_SIGNATURE:
+		return "protocol error (pack signature mismatch detected)";
+
+	case PH_ERROR_PROTOCOL:
+		return "protocol error (pack version unsupported)";
+
+	default:
+		return "unknown error in parse_pack_header";
+
+	case 0:
+		return NULL;
+	}
+}
+
+static void push_header_arg(struct strvec *args, struct pack_header *hdr)
+{
+	strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32,
+		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
+}
+
+static unsigned int get_unpack_limit(struct repository *repo)
+{
+	unsigned int limit = 100;
+
+	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
+	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
+
+	return limit;
+}
+
+static int odb_transaction_files_write_pack(struct odb_transaction *base,
+					    int pack_fd, struct strbuf *err_msg,
+					    const struct odb_transaction_write_pack_opts *opts)
+{
+	struct odb_transaction_files *transaction =
+		container_of(base, struct odb_transaction_files, base);
+	struct repository *repo = base->source->odb->repo;
+	struct child_process child = CHILD_PROCESS_INIT;
+	struct pack_header hdr;
+	const char *hdr_err;
+	int err_fd = opts->err_fd;
+	int status;
+
+	hdr_err = parse_pack_header(&hdr, pack_fd);
+	if (hdr_err) {
+		if (err_fd > 0)
+			close(err_fd);
+		strbuf_addstr(err_msg, hdr_err);
+		return -1;
+	}
+
+	if (opts->shallow_file) {
+		strvec_push(&child.args, "--shallow-file");
+		strvec_push(&child.args, opts->shallow_file);
+	}
+
+	odb_transaction_env(base, &child.env);
+
+	if (ntohl(hdr.hdr_entries) < get_unpack_limit(repo)) {
+		strvec_push(&child.args, "unpack-objects");
+		push_header_arg(&child.args, &hdr);
+		if (opts->quiet)
+			strvec_push(&child.args, "-q");
+		if (opts->fsck_objects)
+			strvec_pushf(&child.args, "--strict%s",
+				     opts->fsck_msg_types);
+		if (opts->max_input_size)
+			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
+				     (uintmax_t)opts->max_input_size);
+		child.no_stdout = 1;
+		child.in = pack_fd;
+		child.err = err_fd;
+		child.git_cmd = 1;
+		status = run_command(&child);
+		if (status) {
+			strbuf_addstr(err_msg, "unpack-objects abnormal exit");
+			return -1;
+		}
+	} else {
+		char hostname[HOST_NAME_MAX + 1];
+		char *lockfile;
+
+		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
+		push_header_arg(&child.args, &hdr);
+
+		if (xgethostname(hostname, sizeof(hostname)))
+			xsnprintf(hostname, sizeof(hostname), "localhost");
+		strvec_pushf(&child.args,
+			     "--keep=receive-pack %"PRIuMAX" on %s",
+			     (uintmax_t)getpid(),
+			     hostname);
+
+		if (!opts->quiet && err_fd)
+			strvec_push(&child.args, "--show-resolving-progress");
+		if (err_fd)
+			strvec_push(&child.args, "--report-end-of-input");
+		if (opts->fsck_objects)
+			strvec_pushf(&child.args, "--strict%s",
+				     opts->fsck_msg_types);
+		if (!opts->reject_thin)
+			strvec_push(&child.args, "--fix-thin");
+		if (opts->max_input_size)
+			strvec_pushf(&child.args, "--max-input-size=%"PRIuMAX,
+				     (uintmax_t)opts->max_input_size);
+		child.out = -1;
+		child.in = pack_fd;
+		child.err = err_fd;
+		child.git_cmd = 1;
+		status = start_command(&child);
+		if (status) {
+			strbuf_addstr(err_msg, "index-pack fork failed");
+			return -1;
+		}
+
+		/*
+		 * The lockfile filepath is expected to be the final location of
+		 * the ".keep" file after being migrated to the main ODB source.
+		 * This ensures the lockfile can be found and removed later
+		 * after the ODB transaction has been committed.
+		 */
+		lockfile = index_pack_lockfile(base->source, child.out, NULL);
+		if (lockfile) {
+			ALLOC_GROW(transaction->pack_lockfiles,
+				   transaction->pack_lockfiles_nr + 1,
+				   transaction->pack_lockfiles_alloc);
+			transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
+				register_tempfile(lockfile);
+			free(lockfile);
+		}
+		close(child.out);
+
+		status = finish_command(&child);
+		if (status) {
+			strbuf_addstr(err_msg, "index-pack abnormal exit");
+			return -1;
+		}
+
+		odb_source_prepare(transaction->quarantine,
+				   ODB_PREPARE_FLUSH_CACHES);
+	}
+
+	return 0;
+}
+
+static int odb_transaction_files_finalize(struct odb_transaction *base)
+{
+	struct odb_transaction_files *transaction =
+		container_of(base, struct odb_transaction_files, base);
+	int ret = 0;
+
+	for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++)
+		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
+
+	free(transaction->pack_lockfiles);
+
+	return ret;
+}
+
 static int odb_transaction_files_env(struct odb_transaction *base,
 				     struct strvec *env)
 {
@@ -1314,7 +1484,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 6ed39b3d0e..8cb06c1191 100644
--- a/odb/transaction.h
+++ b/odb/transaction.h
@@ -4,6 +4,50 @@
 #include "gettext.h"
 #include "odb.h"
 
+/*
+ * Options controlling how odb_transaction_write_pack() ingests a packfile.
+ */
+struct odb_transaction_write_pack_opts {
+	/*
+	 * Optional fsck severity configuration to apply when incoming objects
+	 * are verified.
+	 */
+	const char *fsck_msg_types;
+
+	/*
+	 * Path to an alternative shallow file describing the shallow boundaries
+	 * to honor while ingesting the pack.
+	 */
+	const char *shallow_file;
+
+	/*
+	 * The max size in bytes of the incoming packfile allowed. No limit is
+	 * enforced when set to 0.
+	 */
+	off_t max_input_size;
+
+	/*
+	 * Whether the validity of incoming objects should be verified.
+	 */
+	int fsck_objects;
+
+	/*
+	 * Whether to reject an incoming packfile if it is "thin".
+	 */
+	int reject_thin;
+
+	/*
+	 * Optional file descriptor for reporting progress and errors. Set to 0
+	 * for none.
+	 */
+	int err_fd;
+
+	/*
+	 * Suppresses progress reporting.
+	 */
+	int quiet;
+};
+
 /*
  * A transaction may be started for an object database prior to writing new
  * objects via odb_transaction_begin(). These objects are not committed until
@@ -40,6 +84,15 @@ struct odb_transaction {
 	int (*write_object_stream)(struct odb_transaction *transaction,
 				   struct odb_write_stream *stream, size_t len,
 				   struct object_id *oid);
+	/*
+	 * This callback is expected to ingest the packfile readable via
+	 * `pack_fd` into the transaction. Returns 0 on success, a negative
+	 * error code otherwise. On failure, a human-readable description is
+	 * appended to `err_msg`.
+	 */
+	int (*write_pack)(struct odb_transaction *transaction, int pack_fd,
+			  struct strbuf *err_msg,
+			  const struct odb_transaction_write_pack_opts *opts);
 
 	/*
 	 * This callback is expected to populate the provided strvec with the
@@ -107,6 +160,15 @@ int odb_transaction_write_object_stream(struct odb_transaction *transaction,
 					struct odb_write_stream *stream,
 					size_t len, struct object_id *oid);
 
+/*
+ * Ingests the packfile readable via `pack_fd` into the transaction. Returns 0
+ * on success, a negative error code otherwise. On failure, a human-readable
+ * description is appended to `err_msg`.
+ */
+int odb_transaction_write_pack(struct odb_transaction *transaction, int pack_fd,
+			       struct strbuf *err_msg,
+			       const struct odb_transaction_write_pack_opts *opts);
+
 /*
  * Populates the provided strvec with the environment variables that a child
  * process should inherit so that its object writes participate in the
-- 
2.55.0.424.g13c7afec21