[PATCH 0/2] object-file: fix packfile flush during transaction commit

[PATCH 0/2] object-file: fix packfile flush during transaction commit

[Justin Tobler]

Greetings,

This short series fixes a bug I found related to committing an ODB
transaction that contains both a loose object and "large" blob when also
configured to batch fsync loose objects. The issue can be reproduced
with the following:

	git init
	git config core.fsync loose-object
	git config core.fsyncMethod batch
	git config core.bigFileThreshold 5
	echo foo >1-foo && echo foobar >2-foobar
	git add 1-foo 2-foobar

and produces the following error:

	error: unable to write file .git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack: No such file or directory
	fatal: unable to rename temporary file to '.git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack'

If a "large" blob packfile is written to the transaction temporary
directory, it is unable to be flushed during transaction commit because
the underlying transaction is migrated to the main ODB before the
packfile is finalized. To avoid this, this series ensures any pending
packfile in the transaction is flushed first.

Thanks,
-Justin

Justin Tobler (2):
  object-file: lift ODB reprepare out of packfile flush
  object-file: flush transaction packfile before migrating objects

 object-file.c    | 12 ++++++++----
 t/t1050-large.sh | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)


base-commit: 47ce80527c56f462cb97db4ca8125342204d3783
-- 
2.55.0

[PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Justin Tobler]

When flushing a packfile via `flush_packfile_transaction()`,
`odb_reprepare()` is invoked so the written packfile becomes visible in
the current process. In a subsequent commit, repreparing the ODB is
slightly deferred when committing a "files" ODB transaction.

Lift ODB reprepare out of `flush_packfile_transaction()` and instead
require callers to explicitly invoke `odb_reprepare()` if required.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/object-file.c b/object-file.c
index a4cbf8b081df..0f123b79fad1 100644
--- a/object-file.c
+++ b/object-file.c
@@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
 	memset(state, 0, sizeof(*state));
 
 	strbuf_release(&packname);
-	/* Make objects we just wrote available to ourselves */
-	odb_reprepare(repo->objects);
 }
 
 /*
@@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 	 * to zlib compression and is sufficient for this check.
 	 */
 	if (state->nr_written && pack_size_limit_cfg &&
-	    pack_size_limit_cfg < state->offset + stream->size)
+	    pack_size_limit_cfg < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
+		odb_reprepare(transaction->base.source->odb);
+	}
 
 	CALLOC_ARRAY(idx, 1);
 	prepare_packfile_transaction(transaction);
@@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
 		container_of(base, struct odb_transaction_files, base);
+	int have_packfile = !!transaction->packfile.f;
 
 	if (transaction->objdir) {
 		struct strbuf temp_path = STRBUF_INIT;
@@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 
 	flush_packfile_transaction(transaction);
 
+	if (have_packfile)
+		odb_reprepare(transaction->base.source->odb);
+
 	return 0;
 }
 
-- 
2.55.0

[PATCH 2/2] object-file: flush transaction packfile before migrating objects

[Justin Tobler]

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.

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

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> When flushing a packfile via `flush_packfile_transaction()`,
> `odb_reprepare()` is invoked so the written packfile becomes visible in
> the current process. In a subsequent commit, repreparing the ODB is
> slightly deferred when committing a "files" ODB transaction.
>
> Lift ODB reprepare out of `flush_packfile_transaction()` and instead
> require callers to explicitly invoke `odb_reprepare()` if required.
>
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  object-file.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index a4cbf8b081df..0f123b79fad1 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
>  	memset(state, 0, sizeof(*state));
>
>  	strbuf_release(&packname);
> -	/* Make objects we just wrote available to ourselves */
> -	odb_reprepare(repo->objects);
>  }
>
>  /*
> @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
>  	 * to zlib compression and is sufficient for this check.
>  	 */
>  	if (state->nr_written && pack_size_limit_cfg &&
> -	    pack_size_limit_cfg < state->offset + stream->size)
> +	    pack_size_limit_cfg < state->offset + stream->size) {
>  		flush_packfile_transaction(transaction);
> +		odb_reprepare(transaction->base.source->odb);
> +	}
>
>  	CALLOC_ARRAY(idx, 1);
>  	prepare_packfile_transaction(transaction);
> @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  {
>  	struct odb_transaction_files *transaction =
>  		container_of(base, struct odb_transaction_files, base);
> +	int have_packfile = !!transaction->packfile.f;
>
>  	if (transaction->objdir) {
>  		struct strbuf temp_path = STRBUF_INIT;
> @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>
>  	flush_packfile_transaction(transaction);
>

Earlier this would unconditionally call `odb_reprepare()` within, now we
only call if needed. Which makes sense. Would it also make sense to only
call `flush_packfile_transaction(transaction)` if we have a packfile?

> +	if (have_packfile)
> +		odb_reprepare(transaction->base.source->odb);
> +
>  	return 0;
>  }
>
> --
> 2.55.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2841 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Justin Tobler]

On 26/09/15 12:40AM, Karthik Nayak wrote:
>Justin Tobler <jltobler@gmail.com> writes:
>> @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>>  {
>>  	struct odb_transaction_files *transaction =
>>  		container_of(base, struct odb_transaction_files, base);
>> +	int have_packfile = !!transaction->packfile.f;
>>
>>  	if (transaction->objdir) {
>>  		struct strbuf temp_path = STRBUF_INIT;
>> @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>>
>>  	flush_packfile_transaction(transaction);
>>
>
>Earlier this would unconditionally call `odb_reprepare()` within, now we
>only call if needed. Which makes sense. Would it also make sense to only
>call `flush_packfile_transaction(transaction)` if we have a packfile?

`flush_packfile_transaction()` already returns early if there is nothing
to do. We could make it more explicit here, but I think it is probably
fine to leave it as-is.

-Justin

Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects

[Justin Tobler]

On 26/09/15 12:46AM, Karthik Nayak wrote:
>Justin Tobler <jltobler@gmail.com> writes:
>> +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.

Ya, since we are deleting the repo here as part of this test, I don't
think there is much need to cleanup the config right before deleting.
I'll leave it as-is.

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

Thanks for the review,
-Justin

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Patrick Steinhardt]

On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:
> diff --git a/object-file.c b/object-file.c
> index a4cbf8b081df..0f123b79fad1 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
>  	 * to zlib compression and is sufficient for this check.
>  	 */
>  	if (state->nr_written && pack_size_limit_cfg &&
> -	    pack_size_limit_cfg < state->offset + stream->size)
> +	    pack_size_limit_cfg < state->offset + stream->size) {
>  		flush_packfile_transaction(transaction);
> +		odb_reprepare(transaction->base.source->odb);
> +	}
>  
>  	CALLOC_ARRAY(idx, 1);
>  	prepare_packfile_transaction(transaction);
> @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  {
>  	struct odb_transaction_files *transaction =
>  		container_of(base, struct odb_transaction_files, base);
> +	int have_packfile = !!transaction->packfile.f;
>  
>  	if (transaction->objdir) {
>  		struct strbuf temp_path = STRBUF_INIT;
> @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  
>  	flush_packfile_transaction(transaction);
>  
> +	if (have_packfile)
> +		odb_reprepare(transaction->base.source->odb);
> +
>  	return 0;
>  }

One thing that I'm curious about: we don't have any error checking for
flushing the object directory at alll. So there is actually a change in
behaviour here, where we now also reprepare in case flushing has failed.
It probably doesn't matter much, but it does raise the question whether
we may want to start checking for errors.

Patrick

Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects

[Patrick Steinhardt]

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

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Justin Tobler]

On 26/09/23 03:16PM, Patrick Steinhardt wrote:
> On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index a4cbf8b081df..0f123b79fad1 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
> >  	 * to zlib compression and is sufficient for this check.
> >  	 */
> >  	if (state->nr_written && pack_size_limit_cfg &&
> > -	    pack_size_limit_cfg < state->offset + stream->size)
> > +	    pack_size_limit_cfg < state->offset + stream->size) {
> >  		flush_packfile_transaction(transaction);
> > +		odb_reprepare(transaction->base.source->odb);
> > +	}
> >  
> >  	CALLOC_ARRAY(idx, 1);
> >  	prepare_packfile_transaction(transaction);
> > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> >  {
> >  	struct odb_transaction_files *transaction =
> >  		container_of(base, struct odb_transaction_files, base);
> > +	int have_packfile = !!transaction->packfile.f;
> >  
> >  	if (transaction->objdir) {
> >  		struct strbuf temp_path = STRBUF_INIT;
> > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> >  
> >  	flush_packfile_transaction(transaction);
> >  
> > +	if (have_packfile)
> > +		odb_reprepare(transaction->base.source->odb);
> > +
> >  	return 0;
> >  }
> 
> One thing that I'm curious about: we don't have any error checking for
> flushing the object directory at alll. So there is actually a change in
> behaviour here, where we now also reprepare in case flushing has failed.
> It probably doesn't matter much, but it does raise the question whether
> we may want to start checking for errors.

Regarding the behavior change, I'm not entirely sure I follow.
`flush_packfile_transaction()` only returns early in the case where
there is nothing to flush. In both of the above call sites,
`odb_reprepare()` is only invoked in the same circumstance.

I do agree with the sentiment that error handling could be improve here
as most errors are simply handled by die()'ing in place. I'll probably
defer doing that as part of this series though.

Thanks,
-Justin

Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects

[Justin Tobler]

On 26/09/23 03:16PM, Patrick Steinhardt wrote:
> 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.

That's fair. When I said "deferred" I really meant that
`odb_reprepare()` was now happening after and outside of
`flush_packfile_transaction()`, but logically it is really in the same
place.

I will adapt the commit message accordingly.

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

Ya, using a subshell is probably a bit easier on the eyes. Since I'm
making some small changes anyways I'll go ahead and make this change
too.

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

That fair, I'm not sure this comment is really that useful anyways so
I'll just go ahead and remove it in the next version.

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

It is always welcome and appreciated! :)

Thanks,
-Justin

[PATCH v2 0/2] object-file: fix packfile flush during transaction commit

[Justin Tobler]

Greetings,

This short series fixes a bug I found related to committing an ODB
transaction that contains both a loose object and "large" blob when also
configured to batch fsync loose objects. The issue can be reproduced
with the following:

        git init
        git config core.fsync loose-object
        git config core.fsyncMethod batch
        git config core.bigFileThreshold 5
        echo foo >1-foo && echo foobar >2-foobar
        git add 1-foo 2-foobar

and produces the following error:

        error: unable to write file .git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack: No such file or directory
        fatal: unable to rename temporary file to '.git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack'

If a "large" blob packfile is written to the transaction temporary
directory, it is unable to be flushed during transaction commit because
the underlying transaction is migrated to the main ODB before the
packfile is finalized. To avoid this, this series ensures any pending
packfile in the transaction is flushed first.

Changes since V1:
- Updated a commit message of first patch.
- Improved test readability in second patch.

Thanks,
-Justin

Justin Tobler (2):
  object-file: lift ODB reprepare out of packfile flush
  object-file: flush transaction packfile before migrating objects

 object-file.c    | 12 ++++++++----
 t/t1050-large.sh | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

Range-diff against v1:
1:  cf14416f22 ! 1:  6f74391ae8 object-file: lift ODB reprepare out of packfile flush
    @@ Commit message
     
         When flushing a packfile via `flush_packfile_transaction()`,
         `odb_reprepare()` is invoked so the written packfile becomes visible in
    -    the current process. In a subsequent commit, repreparing the ODB is
    -    slightly deferred when committing a "files" ODB transaction.
    +    the current process. In a subsequent commit, flushing the packfile is
    +    performed earlier when committing a "files" ODB transaction, but the ODB
    +    reprepare needs to remain the last step.
     
         Lift ODB reprepare out of `flush_packfile_transaction()` and instead
         require callers to explicitly invoke `odb_reprepare()` if required.
2:  18a1798d95 ! 2:  ad2fa8ee3f object-file: flush transaction packfile before migrating objects
    @@ t/t1050-large.sh: test_expect_success 'add a large file or two' '
     +test_expect_success 'add large file with loose object in batch fsync' '
     +	test_when_finished "rm -rf batch" &&
     +	git init batch &&
    ++	(
    ++		cd batch &&
    ++		git config core.bigFileThreshold 5 &&
    ++		echo foo >1-small &&
    ++		echo foobar >2-large &&
     +
    -+	git -C batch config core.bigFileThreshold 5 &&
    -+	echo foo >batch/1-small &&
    -+	echo foobar >batch/2-large &&
    ++		git -c core.fsync=loose-object -c core.fsyncMethod=batch \
    ++			add 1-small 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
    ++		git cat-file -e :1-small &&
    ++		git cat-file -e :2-large
    ++	)
     +'
     +
      test_expect_success 'checkout a large file' '

base-commit: 47ce80527c56f462cb97db4ca8125342204d3783
-- 
2.55.0.424.g13c7afec21

[PATCH v2 1/2] object-file: lift ODB reprepare out of packfile flush

[Justin Tobler]

When flushing a packfile via `flush_packfile_transaction()`,
`odb_reprepare()` is invoked so the written packfile becomes visible in
the current process. In a subsequent commit, flushing the packfile is
performed earlier when committing a "files" ODB transaction, but the ODB
reprepare needs to remain the last step.

Lift ODB reprepare out of `flush_packfile_transaction()` and instead
require callers to explicitly invoke `odb_reprepare()` if required.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/object-file.c b/object-file.c
index a4cbf8b081..0f123b79fa 100644
--- a/object-file.c
+++ b/object-file.c
@@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
 	memset(state, 0, sizeof(*state));
 
 	strbuf_release(&packname);
-	/* Make objects we just wrote available to ourselves */
-	odb_reprepare(repo->objects);
 }
 
 /*
@@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 	 * to zlib compression and is sufficient for this check.
 	 */
 	if (state->nr_written && pack_size_limit_cfg &&
-	    pack_size_limit_cfg < state->offset + stream->size)
+	    pack_size_limit_cfg < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
+		odb_reprepare(transaction->base.source->odb);
+	}
 
 	CALLOC_ARRAY(idx, 1);
 	prepare_packfile_transaction(transaction);
@@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
 		container_of(base, struct odb_transaction_files, base);
+	int have_packfile = !!transaction->packfile.f;
 
 	if (transaction->objdir) {
 		struct strbuf temp_path = STRBUF_INIT;
@@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 
 	flush_packfile_transaction(transaction);
 
+	if (have_packfile)
+		odb_reprepare(transaction->base.source->odb);
+
 	return 0;
 }
 
-- 
2.55.0.424.g13c7afec21

[PATCH v2 2/2] object-file: flush transaction packfile before migrating objects

[Justin Tobler]

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.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c    |  4 ++--
 t/t1050-large.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/object-file.c b/object-file.c
index 0f123b79fa..210984f825 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 d295c265c7..95233458b4 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -87,6 +87,23 @@ 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 &&
+	(
+		cd batch &&
+		git config core.bigFileThreshold 5 &&
+		echo foo >1-small &&
+		echo foobar >2-large &&
+
+		git -c core.fsync=loose-object -c core.fsyncMethod=batch \
+			add 1-small 2-large &&
+
+		git cat-file -e :1-small &&
+		git cat-file -e :2-large
+	)
+'
+
 test_expect_success 'checkout a large file' '
 	large1=$(git rev-parse :large1) &&
 	git update-index --add --cacheinfo 100644 $large1 another &&
-- 
2.55.0.424.g13c7afec21

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Patrick Steinhardt]

On Wed, Sep 23, 2026 at 04:04:55PM -0500, Justin Tobler wrote:
> On 26/09/23 03:16PM, Patrick Steinhardt wrote:
> > On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:
> > > diff --git a/object-file.c b/object-file.c
> > > index a4cbf8b081df..0f123b79fad1 100644
> > > --- a/object-file.c
> > > +++ b/object-file.c
> > > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
> > >  	 * to zlib compression and is sufficient for this check.
> > >  	 */
> > >  	if (state->nr_written && pack_size_limit_cfg &&
> > > -	    pack_size_limit_cfg < state->offset + stream->size)
> > > +	    pack_size_limit_cfg < state->offset + stream->size) {
> > >  		flush_packfile_transaction(transaction);
> > > +		odb_reprepare(transaction->base.source->odb);
> > > +	}
> > >  
> > >  	CALLOC_ARRAY(idx, 1);
> > >  	prepare_packfile_transaction(transaction);
> > > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> > >  {
> > >  	struct odb_transaction_files *transaction =
> > >  		container_of(base, struct odb_transaction_files, base);
> > > +	int have_packfile = !!transaction->packfile.f;
> > >  
> > >  	if (transaction->objdir) {
> > >  		struct strbuf temp_path = STRBUF_INIT;
> > > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> > >  
> > >  	flush_packfile_transaction(transaction);
> > >  
> > > +	if (have_packfile)
> > > +		odb_reprepare(transaction->base.source->odb);
> > > +
> > >  	return 0;
> > >  }
> > 
> > One thing that I'm curious about: we don't have any error checking for
> > flushing the object directory at alll. So there is actually a change in
> > behaviour here, where we now also reprepare in case flushing has failed.
> > It probably doesn't matter much, but it does raise the question whether
> > we may want to start checking for errors.
> 
> Regarding the behavior change, I'm not entirely sure I follow.
> `flush_packfile_transaction()` only returns early in the case where
> there is nothing to flush. In both of the above call sites,
> `odb_reprepare()` is only invoked in the same circumstance.

There's a second early return when `tmp_objdir_migrate()` fails, and
that early return causes us to not flush.

Patrick

Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

[Patrick Steinhardt]

On Thu, Sep 24, 2026 at 07:58:07AM +0200, Patrick Steinhardt wrote:
> On Wed, Sep 23, 2026 at 04:04:55PM -0500, Justin Tobler wrote:
> > On 26/09/23 03:16PM, Patrick Steinhardt wrote:
> > > On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:
> > > > diff --git a/object-file.c b/object-file.c
> > > > index a4cbf8b081df..0f123b79fad1 100644
> > > > --- a/object-file.c
> > > > +++ b/object-file.c
> > > > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
> > > >  	 * to zlib compression and is sufficient for this check.
> > > >  	 */
> > > >  	if (state->nr_written && pack_size_limit_cfg &&
> > > > -	    pack_size_limit_cfg < state->offset + stream->size)
> > > > +	    pack_size_limit_cfg < state->offset + stream->size) {
> > > >  		flush_packfile_transaction(transaction);
> > > > +		odb_reprepare(transaction->base.source->odb);
> > > > +	}
> > > >  
> > > >  	CALLOC_ARRAY(idx, 1);
> > > >  	prepare_packfile_transaction(transaction);
> > > > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> > > >  {
> > > >  	struct odb_transaction_files *transaction =
> > > >  		container_of(base, struct odb_transaction_files, base);
> > > > +	int have_packfile = !!transaction->packfile.f;
> > > >  
> > > >  	if (transaction->objdir) {
> > > >  		struct strbuf temp_path = STRBUF_INIT;
> > > > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> > > >  
> > > >  	flush_packfile_transaction(transaction);
> > > >  
> > > > +	if (have_packfile)
> > > > +		odb_reprepare(transaction->base.source->odb);
> > > > +
> > > >  	return 0;
> > > >  }
> > > 
> > > One thing that I'm curious about: we don't have any error checking for
> > > flushing the object directory at alll. So there is actually a change in
> > > behaviour here, where we now also reprepare in case flushing has failed.
> > > It probably doesn't matter much, but it does raise the question whether
> > > we may want to start checking for errors.
> > 
> > Regarding the behavior change, I'm not entirely sure I follow.
> > `flush_packfile_transaction()` only returns early in the case where
> > there is nothing to flush. In both of the above call sites,
> > `odb_reprepare()` is only invoked in the same circumstance.
> 
> There's a second early return when `tmp_objdir_migrate()` fails, and
> that early return causes us to not flush.

Oh, never mind. I think I've been confusing the fact that what you're
changing is actually `odb_transaction_files_commit()` itself, and that
early return of course still exists in there. So this looks good to me.

Patrick

Re: [PATCH v2 0/2] object-file: fix packfile flush during transaction commit

[Patrick Steinhardt]

On Wed, Sep 23, 2026 at 05:03:13PM -0500, Justin Tobler wrote:
> Changes since V1:
> - Updated a commit message of first patch.
> - Improved test readability in second patch.

Thanks, this version looks good to me.

Patrick

Re: [PATCH v2 1/2] object-file: lift ODB reprepare out of packfile flush

[Patrick Steinhardt]

On Wed, Sep 23, 2026 at 05:03:14PM -0500, Justin Tobler wrote:
> When flushing a packfile via `flush_packfile_transaction()`,
> `odb_reprepare()` is invoked so the written packfile becomes visible in
> the current process. In a subsequent commit, flushing the packfile is
> performed earlier when committing a "files" ODB transaction, but the ODB
> reprepare needs to remain the last step.

Yup, this is more in line with what that second commit will do.

Patrick