From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 23 Sep 2026 13:16:35 GMT
Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects
Message-ID: <arPRM191URNQGu7V@pks.im>
In-Reply-To: <18a1798d958d7f089614ec588346096c10b0666a.1789328612.git.jltobler@gmail.com>

On Sun, Sep 13, 2026 at 03:26:22PM -0500, Justin Tobler wrote:
> diff --git a/object-file.c b/object-file.c
> index 0f123b79fad1..210984f82532 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  		container_of(base, struct odb_transaction_files, base);
>  	int have_packfile = !!transaction->packfile.f;
>  
> +	flush_packfile_transaction(transaction);
> +
>  	if (transaction->objdir) {
>  		struct strbuf temp_path = STRBUF_INIT;
>  		struct tempfile *temp;
> @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  		transaction->objdir = NULL;
>  	}
>  
> -	flush_packfile_transaction(transaction);
> -
>  	if (have_packfile)
>  		odb_reprepare(transaction->base.source->odb);
>  

In the preceding commit you wrote:

    In a subsequent commit, repreparing the ODB is slightly deferred
    when committing a "files" ODB transaction.

But that's not really true -- you don't delay repreparing the object
database, but instead only flush earlier. The reprepare still happens at
the same point in time.

> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index d295c265c75c..fb83c8fba619 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' '
>  	test $count = 1
>  '
>  
> +test_expect_success 'add large file with loose object in batch fsync' '
> +	test_when_finished "rm -rf batch" &&
> +	git init batch &&

I feel like using a subshell might've helped here for readability. But,
oh well, it saves us an extra process.

> +	git -C batch config core.bigFileThreshold 5 &&
> +	echo foo >batch/1-small &&
> +	echo foobar >batch/2-large &&
> +
> +	git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \
> +		add 1-small 2-large &&
> +
> +	# Neither object may be left behind in a temporary location.

You don't really verify whether they are left behind, but rather verify
that the can be read. Which is a bit of a different thing.

Sorry, feels like I'm in a nitpicky mood today :)

Thanks!

Patrick