From: Junio C Hamano Date: Tue, 16 Sep 2025 16:44:20 GMT Subject: Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting Message-ID: In-Reply-To: Justin Tobler writes: >> 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? > 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. > 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.