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

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 22, 2025, 19:38 UTC
Message-ID: <jd667c3ef7gkxxju5vhrwnibj4guwi5nee2o6bdtc5htgbthxd@ycyasrwbkz6r>
In-Reply-To: <xmqqjz2vxp98.fsf@gitster.g>

On 25/08/22 10:03AM, Junio C Hamano wrote:

Show 11 quoted lines

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

I'll mention this in the commit message in the next version.

Show 13 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?

That's fair. With this change we are less defensive since we are not bugging out if the object source is not set up. There are other instances throughout the codebase where the object directory is accessed directly through the repository in an open coded manner instead of going through repo_get_object_directory(). Regardless, it's probably best to just keep using repo_get_object_directory() here anyway.

I'll update in the next version.

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

In the next version, I'll perform this in the previous step instead. :)

Thanks, -Justin