From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 29 Jan 2026 11:24:23 GMT
Subject: Re: [PATCH 3/4] odb: prepare `struct odb_transaction` to support more sources
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.

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

> -/*
> - * 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