From: Karthik Nayak Date: Tue, 16 Sep 2025 07:57:03 GMT Subject: Re: [PATCH v2 1/6] bulk-checkin: remove ODB transaction nesting Message-ID: In-Reply-To: <20250915202956.3784935-2-jltobler@gmail.com> Justin Tobler 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// > transactions internally to ensure there is not already a pending > transaction before beginning or ending a transaction. [snip] > 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) > If there is an ongoing transaction, we don't create a new one. If we create a new transaction, we ensure we also close it. Makes sense. 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. [snip]