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

[Patrick Steinhardt <ps@pks.im>]

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