Re: [PATCH 3/4] odb: prepare `struct odb_transaction` to support more sources

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 29, 2026, 11:24 UTC
Message-ID: <aXtDZyFoSQahkxBa@pks.im>
In-Reply-To: <20260128234519.2721179-4-jltobler@gmail.com>

On Wed, Jan 28, 2026 at 05:45:18PM -0600, Justin Tobler wrote:

The bit about supporting "more sources" in the subject reads a bit weird to me. We still only handle a single source in a transaction, not multiple ones. I guess what you rather want to say is that we handle "generic" sources? How about:

    odb: prepare `struct odb_source` to become generic

> Each ODB transaction should be specific to the ODB source it pertains
> to.

This is a claim that should probably be backed up a bit. I myself obviously agree with it, but I think it should be noted _why_ we want to have this in the first place.

The patch itself looks as expected to me, as we split up `struct odb_transaction` into two structures:

  - `struct odb_transaction` continues to exist, but is now the generic
    part that simply contains the source and a function pointer.

  - `struct odb_transaction_loose` is the backend-specific
    implementation.

One question though: is this tansaction really specific to loose objects? We also seem to be handling packfiles there in `prepare_packfile_transaction()`, so it rather feels like this is specific to the whole "files" backend. I might be misunderstanding though.

Show 5 quoted lines

> --- a/object-file.h
> +++ b/object-file.h
> @@ -208,10 +208,4 @@ struct odb_transaction;
>   */
>  struct odb_transaction *odb_transaction_loose_begin(struct odb_source *source);

Right, we still need this function to be public right now. But eventually it'll be converted into a function pointer part of the `struct odb_source`.

Show 5 quoted lines

> -/*
> - * Tell the object database to make any objects from the
> - * current transaction visible.
> - */
> -void odb_transaction_loose_commit(struct odb_transaction *transaction);

But this doesn't have to be public anymore as we have it available to us via `tranasction->commit()`. Makes sense.

Patrick