Re: [PATCH 6/6] odb: add transaction interface
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 11, 2025, 06:40 UTC
- Message-ID
- <aMJu9LOAEa1NWsF0@pks.im>
- In-Reply-To
- <20250909191134.555689-7-jltobler@gmail.com>
On Tue, Sep 09, 2025 at 02:11:34PM -0500, Justin Tobler wrote:
Show 10 quoted lines
> Transactions are managed via the {begin,end}_odb_transaction() function
> in the object-file subsystem and its implementation is specific to the
> files object source. Introduce odb_transaction_{begin,commit}() in the
> odb subsystem to provide an eventual object source agnostic means to
> manage transactions.
>
> Update call sites to instead manage transactions through the odb
> subsystem. Also rename {begin,end}_odb_transaction() functions to
> object_file_transaction_{begin,end}() to clarify the object source it
> supports.Makes sense.
Show 13 quoted lines
> diff --git a/object-file.c b/object-file.c > index 91fddfc4984..aff6c6c6dbb 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1623,7 +1623,7 @@ int index_fd(struct index_state *istate, struct object_id *oid, > struct odb_transaction *transaction = NULL; > > if (!the_repository->objects->transaction) > - transaction = begin_odb_transaction(the_repository->objects); > + transaction = odb_transaction_begin(the_repository->objects); > > ret = index_blob_packfile_transaction(the_repository->objects->transaction, > oid, fd,
This function is a bit of an outlier, as it is weird that we call `odb_transaction_begin()` instead of the specific function.
But "object-file.c" currently contains two different parts: logic that is related to reading and writing objects in general, and logic that provides the actual object source implementation. This will be split up eventually once we carve out the actual "files" backend, so meanwhile we have to live with this seemingly-unclean separation of concerns.
Show 9 quoted lines
> @@ -1971,7 +1971,7 @@ int read_loose_object(struct repository *repo,
> return ret;
> }
>
> -struct odb_transaction *begin_odb_transaction(struct object_database *odb)
> +struct odb_transaction *object_file_transaction_begin(struct object_database *odb)
> {
> if (odb->transaction)
> BUG("ODB transaction already started");I would have expected that this function now gets as input an `struct odb_source` instead of the whole object database. After all, the ODB layer is the one coordinating the sources and managing which sources to tap into for a specific use case. But the actual business logic to read or write objects should then be handled on the source level, shouldn't it?
Show 9 quoted lines
> @@ -1982,7 +1982,7 @@ struct odb_transaction *begin_odb_transaction(struct object_database *odb)
> return odb->transaction;
> }
>
> -void end_odb_transaction(struct odb_transaction *transaction)
> +void object_file_transaction_end(struct odb_transaction *transaction)
> {
> flush_loose_object_transaction(transaction);
> flush_packfile_transaction(transaction);Shouldn't this also be called `object_file_transaction_commit()` to match the ODB layer?
Show 13 quoted lines
> diff --git a/odb.c b/odb.c
> index 2a92a018c42..2cd954a1040 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -1051,3 +1051,13 @@ void odb_clear(struct object_database *o)
> hashmap_clear(&o->pack_map);
> string_list_clear(&o->submodule_source_paths, 0);
> }
> +
> +struct odb_transaction *odb_transaction_begin(struct object_database *odb)
> +{
> + return object_file_transaction_begin(odb);
> +}So with the above, I would expect that we pick the source to create the transaction for here and then call `object_file_transaction_begin()` on that source. Eventually, once we have pluggable object databases, we would then not call `object_file_transaction_start()` directly anymore, but instead we'd call e.g. `source->backend.transaction_start()`.
Show 10 quoted lines
> diff --git a/odb.h b/odb.h
> index a89b2143909..c7725b3df00 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -185,6 +185,9 @@ struct object_database {
> struct object_database *odb_new(struct repository *repo);
> void odb_clear(struct object_database *o);
>
> +struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> +void odb_transaction_commit(struct odb_transaction *transaction);Let's add some documentation here what these functions do and why you'd want to use them.
Patrick