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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 7, 2026, 07:03 UTC
Message-ID: <anWDKwkRp1EK9NRi@pks.im>
In-Reply-To: <20260806213859.816157-2-jltobler@gmail.com>

On Thu, Aug 06, 2026 at 04:38:54PM -0500, Justin Tobler wrote:

Show 7 quoted lines

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

Show 7 quoted lines

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

Show 12 quoted lines

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

Show 14 quoted lines

> 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