From: Justin Tobler <jltobler@gmail.com>
Date: Tue, 16 Sep 2025 15:00:45 GMT
Subject: Re: [PATCH v2 1/6] bulk-checkin: remove ODB transaction nesting
Message-ID: <h7yqajyyilmcjb4lfco2oooqwradmyrhx4aystush3klpbgl6d@3zrii36y6kk5>
In-Reply-To: <CAOLa=ZThwF0LTd17Wxa3AJnpQuEkcSyp5kHm=D-cZ=Ay0pWa0A@mail.gmail.com>

On 25/09/16 12:57AM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > ODB transactions support being nested. Only the outermost
> > {begin,end}_odb_transaction() start and finish a transaction. This is
> > done so that certain object write codepaths that occur internally can be
> > optimized via ODB transactions without having to worry if a transaction
> > has already been started or not. This can make the interface a bit
> > awkward to use, as calling {begin,end}_odb_transaction() does not
> > guarantee that a transaction is actually started or ended. Thus, in
> > situations where a transaction must be explicitly flushed,
> > flush_odb_transaction() must be used.
> >
> > To better clarify ownership sematics around a transaction and further
> 
> s/smatics/semantics
> 
> > remove the need for flush_odb_transaction() as part of the transaction
> > interface, instead be more explicit and require callers who use ODB
> 
> The first sentence doesn't flow into the second here. Perhaps s/instead//

Thanks. I'll fix these in the next version.

> > diff --git a/cache-tree.c b/cache-tree.c
> > index d225554eed..f88555a773 100644
> > --- a/cache-tree.c
> > +++ b/cache-tree.c
> > @@ -474,7 +474,7 @@ static int update_one(struct cache_tree *it,
> >
> >  int cache_tree_update(struct index_state *istate, int flags)
> >  {
> > -	struct odb_transaction *transaction;
> > +	struct odb_transaction *transaction = NULL;
> >  	int skip, i;
> >
> >  	i = verify_cache(istate, flags);
> > @@ -490,10 +490,15 @@ int cache_tree_update(struct index_state *istate, int flags)
> >
> >  	trace_performance_enter();
> >  	trace2_region_enter("cache_tree", "update", the_repository);
> > -	transaction = begin_odb_transaction(the_repository->objects);
> > +
> > +	if (!the_repository->objects->transaction)
> > +		transaction = begin_odb_transaction(the_repository->objects);
> > +
> >  	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
> >  		       "", 0, &skip, flags);
> > +
> >  	end_odb_transaction(transaction);
> > +
> >  	trace2_region_leave("cache_tree", "update", the_repository);
> >  	trace_performance_leave("cache_tree_update");
> >  	if (i < 0)
> >
> I wish the parent transaction would be passed through to make it easier
> to understand, instead of deriving from a global variable. Nevertheless,
> this is a great improvement.

This should be cleaned up a bit in the next version by making
begin_odb_transaction() function as a noop that returns NULL when the
ODB already has a pending transaction. This allow us to at least avoid
checking the transaction state from the global here. We will still have
to derive the ODB from the_repository global for now though.

-Justin