Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 22, 2025, 16:37 UTC
- Message-ID
- <xmqqv7mfxqg2.fsf@gitster.g>
- In-Reply-To
- <20250821232249.319427-3-jltobler@gmail.com>
Justin Tobler <jltobler@gmail.com> writes:
Show 17 quoted lines
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 82a73da79e8..53a20a2d92f 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -30,11 +30,13 @@ struct bulk_checkin_packfile {
> uint32_t nr_written;
> };
>
> -static struct odb_transaction {
> +struct odb_transaction {
> + struct object_database *odb;
> +
> int nesting;
> struct tmp_objdir *objdir;
> struct bulk_checkin_packfile packfile;
> -} transaction;
> +};Very nice to see that this singleton instance is now gone.
The singleton was really handy way to allow calls into this subsystem to lazily initialize bulk_checkin_packfile in the call chain starting from deflate_blob_to_pack() -> prepare_to_stream().
We now need to be a lot more careful to make sure that everybody has access to a valid bulk_checkin_packfile struct, which makes the implementation of index_blob_bulk_checkin() a bit awkward and we need to invent one bulk_checkin_packfile instance right there. Luckily it goes away in the next step, I guess?
Show 22 quoted lines
> +int index_blob_bulk_checkin(struct odb_transaction *transaction,
> + struct object_id *oid, int fd, size_t size,
> const char *path, unsigned flags)
> {
> - int status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,
> - path, flags);
> - if (!transaction.nesting)
> - flush_bulk_checkin_packfile(&transaction.packfile);
> + int status;
> +
> + if (transaction) {
> + status = deflate_blob_to_pack(&transaction->packfile, oid, fd,
> + size, path, flags);
> + } else {
> + struct bulk_checkin_packfile state = { 0 };
> +
> + status = deflate_blob_to_pack(&state, oid, fd, size, path, flags);
> + flush_bulk_checkin_packfile(&state);
> + }
> +
> return status;
> }OK. If we do not have a transaction (i.e. if nobody called begin_odb_transaction() more times than end_odb_transaction() got called), we let the underlying machinery do the right thing on an empty state and clean up after ourselves; otherwise we follow exactly the same code path as we used to.
Show 10 quoted lines
> diff --git a/cache-tree.c b/cache-tree.c
> index 66ef2becbe0..d225554eedd 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,
>
> int cache_tree_update(struct index_state *istate, int flags)
> {
> + struct odb_transaction *transaction;
> int skip, i;OK, doing the odb-transaction here would cover both this one and write_index_as_tree and its friends, that are public interfaces into the cache-tree subsystem.
Show 22 quoted lines
> diff --git a/odb.h b/odb.h
> index 3dfc66d75a3..a89b2143909 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -84,6 +84,7 @@ struct odb_source {
>
> struct packed_git;
> struct cached_object_entry;
> +struct odb_transaction;
>
> /*
> * The object database encapsulates access to objects in a repository. It
> @@ -94,6 +95,13 @@ struct object_database {
> /* Repository that owns this database. */
> struct repository *repo;
>
> + /*
> + * State of current current object database transaction. Only one
> + * transaction may be pending at a time. Is NULL when no transaction is
> + * configured.
> + */
> + struct odb_transaction *transaction;Once dust from this topic settles, we may want to rename the bulk-checkin.[ch] to have "odb" somewhere in its name, perhaps? I usually do not like renaming file for the sake of renaming to make the result look "pretty" (people may use "consistent naming" ), though. I dunno.