Volume XXII, number 279Tuesday, October 6, 2026Latest message 1 hour ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patch, 2 partsobject-file: fix packfile flush during transaction commit

18 messages between Sep 13, 2026 and Sep 24, 2026, from Justin Tobler, Karthik Nayak, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Justin ToblerSep 13, 2026, 20:26 UTC on lore
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
Justin ToblerSep 13, 2026, 20:26 UTC in reply to Justin Tobler on lore

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

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(-)
Show changes to object-file.c +7 −3
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
Justin ToblerSep 13, 2026, 20:26 UTC in reply to Justin Tobler on lore

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

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(-)
Show changes to 2 files +18 −2

object-file.c, t/t1050-large.sh

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
Karthik NayakSep 15, 2026, 04:40 UTC in reply to Justin Tobler on lore

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

Justin Tobler <jltobler@gmail.com> writes:
Show 50 quoted lines
> 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?

Show 8 quoted lines
> +	if (have_packfile)
> +		odb_reprepare(transaction->base.source->odb);
> +
>  	return 0;
>  }
>
> --
> 2.55.0
Karthik NayakSep 15, 2026, 04:46 UTC in reply to Justin Tobler on lore

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

Justin Tobler <jltobler@gmail.com> writes:
Show 12 quoted lines
> 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.
Show 42 quoted lines
>
> 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.
Show 11 quoted lines
> +	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.
Show 6 quoted lines
> +
>  test_expect_success 'checkout a large file' '
>  	large1=$(git rev-parse :large1) &&
>  	git update-index --add --cacheinfo 100644 $large1 another &&
> --
> 2.55.0
Justin ToblerSep 15, 2026, 08:54 UTC in reply to Karthik Nayak on lore

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

On 26/09/15 12:40AM, Karthik Nayak wrote:
Show 17 quoted lines
>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
Justin ToblerSep 15, 2026, 08:59 UTC in reply to Karthik Nayak on lore

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

On 26/09/15 12:46AM, Karthik Nayak wrote:
Show 9 quoted lines
>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.

Show 13 quoted lines
>> +	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

Patrick SteinhardtSep 23, 2026, 13:16 UTC in reply to Justin Tobler on lore

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

On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:
Show 33 quoted lines
> 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
Patrick SteinhardtSep 23, 2026, 13:16 UTC in reply to Justin Tobler on lore

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

On Sun, Sep 13, 2026 at 03:26:22PM -0500, Justin Tobler wrote:
Show 22 quoted lines
> 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.

Show 11 quoted lines
> 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.

Show 8 quoted lines
> +	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
Justin ToblerSep 23, 2026, 21:04 UTC in reply to Patrick Steinhardt on lore

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

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

Justin ToblerSep 23, 2026, 21:17 UTC in reply to Patrick Steinhardt on lore

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

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

Show 11 quoted lines
> > +	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

Justin ToblerSep 23, 2026, 22:03 UTC in reply to Justin Tobler on lore

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

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
Justin ToblerSep 23, 2026, 22:03 UTC in reply to Justin Tobler on lore

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

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(-)
Show changes to object-file.c +7 −3
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
Justin ToblerSep 23, 2026, 22:03 UTC in reply to Justin Tobler on lore

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

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(-)
Show changes to 2 files +19 −2

object-file.c, t/t1050-large.sh

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
Patrick SteinhardtSep 24, 2026, 05:58 UTC in reply to Justin Tobler on lore

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

On Wed, Sep 23, 2026 at 04:04:55PM -0500, Justin Tobler wrote:
Show 46 quoted lines
> 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
Patrick SteinhardtSep 24, 2026, 06:00 UTC in reply to Patrick Steinhardt on lore

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

On Thu, Sep 24, 2026 at 07:58:07AM +0200, Patrick Steinhardt wrote:
Show 50 quoted lines
> 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
Patrick SteinhardtSep 24, 2026, 06:01 UTC in reply to Justin Tobler on lore

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

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
Patrick SteinhardtSep 24, 2026, 06:13 UTC in reply to Justin Tobler on lore

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

On Wed, Sep 23, 2026 at 05:03:14PM -0500, Justin Tobler wrote:
Show 5 quoted lines
> 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

Back to recent threads