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

[Justin Tobler <jltobler@gmail.com>]

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