From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 11 Sep 2025 06:40:52 GMT
Subject: Re: [PATCH 6/6] odb: add transaction interface
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:
> 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.

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

> @@ -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?

> @@ -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?

> 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()`.

> 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