Re: [PATCH 4/4] object-file: create ODB transaction temporary directory eagerly

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 09, 2026 at 06:22:51PM -0600, Justin Tobler wrote:
> By default, ODB transactions for the "files" backend create their
> temporary object directory lazily once the first loose object is about
> to be written, and only if batched fsync is enabled. Transactions
> configured for git-receive-pack(1) are the exception as the directory is
> needed to quarantine incoming objects up front. Lazily creating the
> temporary directory in this manner is awkward for a couple of reasons:
> 
>   - Writing loose objects has to know about the transaction internals to
>     trigger the lazy creation of the temporary object directory.
> 
>   - Whether objects are staged in a temporary object directory, and thus
>     whether they become visible at once on commit, depends on the
>     configured fsync method.

And three: the whole premise of creating ad-hoc transactions behind the
caller's back for the purpose of bulk-flushing writes to disk is
somewhat flawed in the first place. In a perfect world I'd think that
callers would manually manage their object transactions in all cases
where they know to write multiple objects at once.

But we don't live in that world (yet), and this step here brings us a
bit closer to it.

> diff --git a/object-file.c b/object-file.c
> index da189111bd..a9fb42705c 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1573,24 +1550,13 @@ int odb_transaction_files_begin(struct odb_source *source,
>  	transaction->flags = flags;
>  	strvec_init(&transaction->pack_names);
>  
> -	transaction->prefix = "transaction";
> -	if (flags & ODB_TRANSACTION_RECEIVE) {
> -		/*
> -		 * ODB transactions for git-receive-pack(1) eagerly create a
> -		 * temporary directory and use a different temporary directory
> -		 * prefix.
> -		 *
> -		 * NEEDSWORK: This transaction flag is only used by the "files"
> -		 * backend to special case temporary directory set up and
> -		 * handling. Ideally transaction users should not have to care
> -		 * though. To avoid this, we could eagerly create the temporary
> -		 * directory and use the same prefix name for all transactions.
> -		 */
> -		transaction->prefix = "incoming";
> -		if (odb_transaction_files_prepare(&transaction->base)) {
> -			free(transaction);
> -			return -1;
> -		}
> +	prefix = flags & ODB_TRANSACTION_RECEIVE ? "incoming" : "transaction";

Makes me wonder whether it's important to keep the "incoming" prefix
here or whether there is an argument to be made that it should simply be
called "transaction", as well.

That may be out of scope for this series though.

> +	transaction->objdir = tmp_objdir_create(source->odb->repo, prefix, 0);
> +	if (!transaction->objdir) {
> +		ret = error_errno(_("unable to create temporary object directory in %s"),
> +				  source->path);
> +		free(transaction);
> +		return ret;
>  	}
>  
>  	*out = &transaction->base;

Nice, this logic becomes quite a bit simpler.

> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index 50cb388c35..9330ae94b5 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -769,9 +769,6 @@ static int write_loose_object(struct odb_source_loose *loose,
>  	static struct strbuf tmp_file = STRBUF_INIT;
>  	static struct strbuf filename = STRBUF_INIT;
>  
> -	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
> -		odb_transaction_files_prepare(loose->base.odb->transaction);
> -
>  	odb_loose_path(loose, &filename, oid);
>  
>  	fd = start_loose_object_common(loose, &tmp_file, filename.buf, flags,
> @@ -860,9 +857,6 @@ static int odb_source_loose_write_object_stream(struct odb_source *source,
>  	char hdr[MAX_HEADER_LEN];
>  	int hdrlen;
>  
> -	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
> -		odb_transaction_files_prepare(loose->base.odb->transaction);
> -
>  	/* Since oid is not determined, save tmp file to odb path. */
>  	strbuf_addf(&filename, "%s/", loose->base.path);
>  	hdrlen = format_object_header(hdr, sizeof(hdr), in_stream->type, in_stream->size);

Nice to see that we can get rid of all of this leaking abstraction.

> diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
> index 3bdafbae0f..7292672a66 100755
> --- a/t/t0004-unwritable.sh
> +++ b/t/t0004-unwritable.sh
> @@ -24,8 +24,8 @@ test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable reposi
>  test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree'
>  test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' '
>  	cat >expect <<-\EOF &&
> -	error: insufficient permission for adding an object to repository database .git/objects
> -	fatal: git-write-tree: error building trees
> +	error: unable to create temporary object directory in .git/objects: Permission denied
> +	fatal: failed to start ODB transaction

Not a new issue, but this second error message is awful. An average user
won't have any idea what an "ODB" even is. s/ODB/object/ would make a
lot more sense.

Patrick