Re: [PATCH v2 4/4] bulk-checkin: use repository variable from transaction

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 22, 2025, 17:03 UTC
Message-ID: <xmqqjz2vxp98.fsf@gitster.g>
In-Reply-To: <20250821232249.319427-5-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 5 quoted lines

> The bulk-checkin subsystem depends on `the_repository`. Adapt functions
> and call sites to access the repository through `struct odb_transaction`
> instead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the
> `pack_compression_level` and `pack_size_limit_cfg` globals are still
> used.

Also we grab the details of the new packfile the bulk-checkin machinery is building out of the transaction, which made some redundant parameters functions take go away, ...

Show 8 quoted lines

> -static void finish_tmp_packfile(struct strbuf *basename,
> -				const char *pack_tmp_name,
> -				struct pack_idx_entry **written_list,
> -				uint32_t nr_written,
> -				struct pack_idx_option *pack_idx_opts,
> +static void finish_tmp_packfile(struct odb_transaction *transaction,
> +				struct strbuf *basename,
>  				unsigned char hash[])

... like this one.  Very nice.

Show 7 quoted lines

> @@ -117,7 +118,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)
>  	 * to ensure that the data in each new object file is durable before
>  	 * the final name is visible.
>  	 */
> -	strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", repo_get_object_directory(the_repository));
> +	strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX",
> +		    transaction->odb->sources->path);

This is doing a lot more than a simple "the_repository" -> "odb->repo" replacement. How much confidence do we have that the internal detail of repo_get_object_directory() will stay the same and our developers in the future would spot that this open coded copy needs to be updated if they have to change it?

Show 8 quoted lines

> -static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> -				struct object_id *result_oid,
> -				int fd, size_t size,
> -				const char *path, unsigned flags)
> +int index_blob_bulk_checkin(struct odb_transaction *transaction,
> +			    struct object_id *result_oid,
> +			    int fd, size_t size,
> +			    const char *path, unsigned flags)

Ahh, OK, with the simplification to always take transaction, there is no need to have the deflate_blob_to_pack() function, which had only one caller, as an internal implementation detail anymore.

This change could have been in the previous step and it would have been less surprising; the above "Ahh, OK" is my reaction to the surprise ;-)