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

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

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

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

> @@ -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?

> -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 ;-)