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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2025, 16:44 UTC
Message-ID: <xmqqfrcmny8r.fsf@gitster.g>
In-Reply-To: <pk2cpihxk4j4ywgq3dtknybyzjeon7ajgmwq4yhknojjsfiqo2@q5dsygszdkar>

Justin Tobler <jltobler@gmail.com> writes:

Show 16 quoted lines

>> In addition, it would be useful to hear from the commit message *why*
>> this is safe to do. Justin's message suggests that nested transactions
>> are noops, so doing something like:
>> 
>>   begin_odb_transaction();
>>     begin_odb_transaction();
>>       write_object();
>>     end_odb_transaction(); <- object not yet added to the main ODB
>>   end_odb_transaction();   <- now it is
>> 
>> only results in the object being added to the main ODB when the final
>> end_odb_transaction() is called.
>
> Yes, well said. {begin,end}_odbtransaction() operations on inner
> transactions are effectively a noop. They simple manage an internal
> counter to know when a new transaction should be started/finished.

In other words, that part of the design is inherited from the old bulk-checkin infrastructure? It was done for simplicity and I was always unsure if I made the right design decisions there, so it is nice to see that I found at least somebody who seems to agree with it well enough to inherit it into a newly reinvented subsystem ;-)

> Yes, this patch removes the logic that manages the internal nested
> transaction counter in favor of requiring callers to check if a
> transaction has already been started or not.

OK. Is that related to those "make it a silent no-op to pass a NULL, instead of forcing the caller to check" changes?

Can all callers tell reliably if there is a transaction already going? One transaction may start somewhere and end in a far away place, and these two places may not be ancestor-decendant in the call graph but merely a distant relative that shares a grand grand parent.

> The ultimate goal of this patch is make it so invoking
> end_odb_transaction() on a transaction guarantees it is flushed.

Either flushed or a noop, I guess?

Show 12 quoted lines

> With
> this guarantee we would no longer need flush_odb_transaction() and the
> transaction interface eventually can be simplified to just
> {begin,end}_odb_transaction(). This also avoids a potential class of
> errors where a caller _thinks_ they have committed a transaction and
> that the objects should be visible, but they actually are not because it
> was a nested transaction.
>
> The nice thing about the current implementation though is that nested
> transactions are automatically treated as noops and the caller doesn't
> have to check if there is already a pending transaction. This is safer
> and less error-prone.

True.

Show 17 quoted lines

> Thinking about this some more though, we should be able to continue to
> have {begin,end}_odb_transaction() function as noops when there is
> already a pending transaction and also be able to drop the internal
> transaction nesting mechanism at the same time. To do this, instead of
> erroring out in begin_odb_transaction() when there is already a
> transaction, we can simply return NULL. If a caller wants to know if a
> new transaction was actually started, they can just check the return
> value afterwords. This removes the burden from the caller to explicitly
> check if there is a pending transaction beforehand. Furthermore, since
> an ODB only allows a single transaction at a time, it probably makes
> sense for operations at the ODB layer to guard against this anyway.
>
> This change would pair nicely with the change made to
> end_odb_transaction() in version 2 of this series. In this version, when
> end_odb_transaction() is provided a NULL transaction, it now functions
> as a noop as well. If a transaction _is_ provided though, it is
> guaranteed to be flushed with addresses the main goal on this patch.

OK.