From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 15 Sep 2026 04:46:18 GMT
Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects
Message-ID: <CAOLa=ZRCyowPgMABwsQBYTbW1cEf8PBBSszOEYQ4TKwLVKQfFA@mail.gmail.com>
In-Reply-To: <18a1798d958d7f089614ec588346096c10b0666a.1789328612.git.jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

> A "files" ODB transaction creates a temporary directory to stage newly
> written objects in when configured to batch fsync loose objects. Once
> the temporary directory is created, it is configured as the primary ODB
> and all object are written to it accordingly. This also includes
> packfiles containing blobs that exceed `core.bigFileThreshold` written
> via `odb_transaction_files_write_object_stream()`.
>
> If a "large" blob packfile is written to the ODB transaction temporary
> directory after other loose objects, the ODB transaction fails to commit
> as a result of the temporary directory being migrated prior to the
> packfile being flushed. Fix this bug by always flushing the packfile
> transaction before objects are migrated to the main ODB.

Okay this makes sense.

>
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  object-file.c    |  4 ++--
>  t/t1050-large.sh | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>
> 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);
>
> 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 &&
> +
> +	git -C batch config core.bigFileThreshold 5 &&

Nit: we have `test_config` which automatically unsets after the test.
Perhaps not really needed here, as we drop 'batch' anyways.

> +	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.
> +	git -C batch cat-file -e :1-small &&
> +	git -C batch cat-file -e :2-large
> +'
>

Looks good.

> +
>  test_expect_success 'checkout a large file' '
>  	large1=$(git rev-parse :large1) &&
>  	git update-index --add --cacheinfo 100644 $large1 another &&
> --
> 2.55.0