Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 22, 2025, 18:07 UTC
Message-ID: <njvcqkkcotesqznalieuwhzow7lxbejxlfw3knbhalmqjcb7cs@3vwea4r2zljl>
In-Reply-To: <xmqqv7mfxqg2.fsf@gitster.g>

On 25/08/22 09:37AM, Junio C Hamano wrote:

> Justin Tobler <jltobler@gmail.com> writes:

Show 5 quoted lines

> We now need to be a lot more careful to make sure that everybody has
> access to a valid bulk_checkin_packfile struct, which makes the
> implementation of index_blob_bulk_checkin() a bit awkward and we
> need to invent one bulk_checkin_packfile instance right there.
> Luckily it goes away in the next step, I guess?

Ya, now that we cannot rely on the transaction singleton to provide a bulk_checkin_packfile, we have create one if there is no transaction. I completely agree that this is awkward which is why the next step requires index_blob_bulk_checkin() callers to ensure a transaction is created ahead of time.

If it is preferable, I can combine these two patches together. I kept them separate as I thought it would better explain the steps, but maybe that isn't the best way to structure the patch as we are largely undoing the change in the next patch anyway.

Show 28 quoted lines

> > diff --git a/odb.h b/odb.h
> > index 3dfc66d75a3..a89b2143909 100644
> > --- a/odb.h
> > +++ b/odb.h
> > @@ -84,6 +84,7 @@ struct odb_source {
> >  
> >  struct packed_git;
> >  struct cached_object_entry;
> > +struct odb_transaction;
> >  
> >  /*
> >   * The object database encapsulates access to objects in a repository. It
> > @@ -94,6 +95,13 @@ struct object_database {
> >  	/* Repository that owns this database. */
> >  	struct repository *repo;
> >  
> > +	/*
> > +	 * State of current current object database transaction. Only one
> > +	 * transaction may be pending at a time. Is NULL when no transaction is
> > +	 * configured.
> > +	 */
> > +	struct odb_transaction *transaction;
> 
> Once dust from this topic settles, we may want to rename the
> bulk-checkin.[ch] to have "odb" somewhere in its name, perhaps?  I
> usually do not like renaming file for the sake of renaming to make
> the result look "pretty" (people may use "consistent naming" ),
> though.  I dunno.

I'm hoping for an eventual state where the transactional interfaces, {begin,end}_odb_transaction(), are moved directly into odb.{h,c}. The current implementation of transaction handling is specific to the current object database source. In a pluggable object database future where we could have different types of object database sources, transaction handling will likely have to implemented separately.

With this in mind, we could move the current transaction implementation into something like object-files.c. This is already where the vast majority of its call sites are and would enable us to further simply the interface we expose.

I plan do submit a followup series to do this. :)

-Justin