On 25/09/11 08:40AM, Patrick Steinhardt wrote:
> On Tue, Sep 09, 2025 at 02:11:34PM -0500, Justin Tobler wrote:
> > 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.
Ya, this one is a bit strange indeed. Writing a blob directly to a
packfile is done via index_blob_packfile_transaction() and currently
requires a transaction. Concerns regarding how exactly an object is
written should probably be transparently be handled by source.
In this case, writing a blob directly to a packfile should probably be
eventually moved down a layer behind the ODB object write interface.
Especially now that index_blob_packfile_transaction() is really just an
internal detail to the files object source and not part of the generic
transactional interface.
> > @@ -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?
Good point, I'll update this in the next version.
> > @@ -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?
Ya, its probably best to be consistent here. Will update.
> > 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()`.
Yup, I figure we will eventually have a function table for each source
that maps the corresponding operations similar to how its done for
reference backends.
> > 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.
Will do.
Thanks for the review,
-Justin
