Re: [PATCH 3/3] bulk-checkin: wire repository variable

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 21, 2025, 20:26 UTC
Message-ID: <rhkhmaozjaogm34xfwagagy2qifsq5p4fbtb23nhdu7jfg2627@qkrv57gg2k3u>
In-Reply-To: <xmqq8qjd7coe.fsf@gitster.g>

On 25/08/20 05:15PM, Junio C Hamano wrote:

Show 6 quoted lines

> I somehow expected that odb would know what repository it was
> instanciated to work with, or in the worst case where in-core odb is
> in theory sharable among multiple in-core repositories, at least
> begin_odb_transaction() would take <repository, odb> pair and the
> transaction would know for which repository the transaction is
> working for.

The `struct odb_transaction` maintains a pointer to its parent `struct object_database`. This ODB also has a pointer to the `struct repository`. Thus, from an ongoing transaction we should be able to get the repository we want.

For `begin_odb_transaction()` we should only have to provide `struct object_database` and from that the transaction can access the corresponding repository. In the general cases, this is exactly what we do, but this doesn't work for `index_blob_bulk_checkin()` and its accompanying internal functions as they may be invoked from outside of a transaction.

> Do we need bulk_checkin_packfile as a separate structure and pass it
> around, or would these internal functions be better off passing an
> instance of odb_transaction around and learn the repository from
> odb->repo?

In its current form, the bulk-checkin mechanism for packfiles may be used outside of transactions. For example when calling `index_blob_bulk_checkin()` without a transaction, the single object is written to the packfile moved into the primary ODB. With a transaction, multiple objects may be written to a single packfile. This whole setup is rather awkward though.

Thinking about this more, we should probably just require `index_blob_bulk_checkin()` be provided a transaction. Callers will need to ensure a transaction is running so that a `struct bulk_checkin_packfile` gets set up, but this shouldn't be a big deal. With this we could easily just propagate the transaction for all these function as you suggested.

I'll do this in the next version. Thanks!

-Justin