Re: [PATCH v2 1/6] bulk-checkin: remove ODB transaction nesting

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 16, 2025, 07:57 UTC
Message-ID: <CAOLa=ZThwF0LTd17Wxa3AJnpQuEkcSyp5kHm=D-cZ=Ay0pWa0A@mail.gmail.com>
In-Reply-To: <20250915202956.3784935-2-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 11 quoted lines

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

Show 31 quoted lines

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