object-file: fix packfile flush during transaction commit

PATCH Superseded Justin Tobler 3 participants 18 messages Sep 13 – Sep 24

lore

Share & tools

lore

Justin Tobler

author

2026-09-13 20:26 UTC #0 lore raw

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

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

Signature · 1 line

2.55.0

Justin Tobler

author

2026-09-13 20:26 UTC #1 lore raw

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 <[email protected]>

---
 object-file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

object-file.c +7 -3

index a4cbf8b081df..0f123b79fad1 100644
@@ -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;
 }

Signature · 1 line

2.55.0

Karthik Nayak

2026-09-15 04:40 UTC #2 lore raw

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

Quoted 47 lines · Justin Tobler

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

Signed-off-by Justin Tobler <[email protected]>

---
 object-file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

object-file.c +4 -3

index a4cbf8b081df..0f123b79fad1 100644
@@ -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;

object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1296

Quoted 3 lines

object-file.c +0 -0

@@ -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?

Quoted 8 lines

object-file.c +3 -1

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

2.55.0

Justin Tobler

author

2026-09-15 08:54 UTC #3 lore raw

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

Quoted 11 lines · Karthik Nayak

Quoted 10 lines · Justin Tobler

object-file.c +1 -0

@@ -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)

Quoted 6 lines

Review static int odb_transaction_files_commit(struct odb_transaction *base) · line 1296
Quoted 2 lines
(quoted diff) +0 -0
 	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

Patrick Steinhardt

2026-09-23 13:16 UTC #4 lore raw

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

In reply to Justin Tobler

Quoted 25 lines · Justin Tobler

object-file.c +4 -1

index a4cbf8b081df..0f123b79fad1 100644
@@ -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;

object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1301

Quoted 8 lines

object-file.c +3 -0

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

Justin Tobler

author

2026-09-23 21:05 UTC #5 lore raw

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

Quoted 29 lines · Patrick Steinhardt

Quoted 28 lines · Justin Tobler

object-file.c +4 -1

index a4cbf8b081df..0f123b79fad1 100644
@@ -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);

Quoted 11 lines

object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1301
Quoted 5 lines
object-file.c +3 -0
+	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

Patrick Steinhardt

2026-09-24 05:58 UTC #6 lore raw

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

Quoted 42 lines · Justin Tobler

Quoted 29 lines · Patrick Steinhardt
Quoted 28 lines · Justin Tobler
object-file.c +4 -1
index a4cbf8b081df..0f123b79fad1 100644
@@ -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);
 
Quoted 11 lines
object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1301
Quoted 5 lines
object-file.c +3 -0
+	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.

Quoted 4 lines

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 Steinhardt

2026-09-24 06:00 UTC #7 lore raw

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

Quoted 43 lines · Patrick Steinhardt

Quoted 42 lines · Justin Tobler
Quoted 40 lines · Patrick Steinhardt
Quoted 25 lines · Justin Tobler
object-file.c +4 -1
index a4cbf8b081df..0f123b79fad1 100644
@@ -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;
object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1301
Quoted 8 lines
object-file.c +3 -0
@@ -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.

Quoted 7 lines

Quoted 4 lines
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

Justin Tobler

author

2026-09-13 20:26 UTC #8 lore raw

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

In reply to 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 <[email protected]>

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

object-file.c +2 -2

index 0f123b79fad1..210984f82532 100644
@@ -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);

t/t1050-large.sh +16 -0

index d295c265c75c..fb83c8fba619 100755
@@ -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 &&

Signature · 1 line

2.55.0

Karthik Nayak

2026-09-15 04:46 UTC #9 lore raw

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

Quoted 7 lines · 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().

Quoted 5 lines

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.

Quoted 34 lines

Signed-off-by Justin Tobler <[email protected]>

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

object-file.c +2 -2

index 0f123b79fad1..210984f82532 100644
@@ -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);

t/t1050-large.sh +0 -0

index d295c265c75c..fb83c8fba619 100755

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 94

Quoted 8 lines

t/t1050-large.sh +5 -0

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

t/t1050-large.sh test_expect_success 'add a large file or two' '

Quoted 11 lines

t/t1050-large.sh +10 -0

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

Quoted 6 lines

t/t1050-large.sh +1 -0
+
 test_expect_success 'checkout a large file' '
 	large1=$(git rev-parse :large1) &&
git update-index --add --cacheinfo 100644 $large1 another &&
--
2.55.0

Justin Tobler

author

2026-09-15 08:59 UTC #10 lore raw

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

Quoted 9 lines · Karthik Nayak

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 94
Quoted 5 lines · Justin Tobler
t/t1050-large.sh +5 -0
+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.

Quoted 5 lines

t/t1050-large.sh +5 -0

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

Quoted 8 lines

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 104
Quoted 6 lines
t/t1050-large.sh +5 -0
+
+	# 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 Steinhardt

2026-09-23 13:16 UTC #11 lore raw

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

In reply to Justin Tobler

Quoted 14 lines · Justin Tobler

object-file.c +2 -0

index 0f123b79fad1..210984f82532 100644
@@ -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;

object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1299

Quoted 8 lines

object-file.c +0 -2

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

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 92

Quoted 11 lines

t/t1050-large.sh +3 -0

index d295c265c75c..fb83c8fba619 100755
@@ -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.

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 101

Quoted 8 lines

t/t1050-large.sh +8 -0

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

author

2026-09-23 21:17 UTC #12 lore raw

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

Quoted 26 lines · Patrick Steinhardt

Quoted 14 lines · Justin Tobler

object-file.c +2 -0

index 0f123b79fad1..210984f82532 100644
@@ -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;

object-file.c static int odb_transaction_files_commit(struct odb_transaction *base) · line 1299

Quoted 8 lines

object-file.c +0 -2

@@ -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:

Quoted 6 lines

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.

Quoted 8 lines

t/t1050-large.sh +0 -0

index d295c265c75c..fb83c8fba619 100755
@@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' '
 	test $count = 1
 '

Quoted 6 lines

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 92
Quoted 3 lines
t/t1050-large.sh +3 -0
+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.

Quoted 11 lines

t/t1050-large.sh test_expect_success 'add a large file or two' ' · line 101
Quoted 8 lines
t/t1050-large.sh +8 -0
+	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.

Quoted 1 line

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

It is always welcome and appreciated! :)

Thanks,
-Justin

Justin Tobler

author

2026-09-23 22:03 UTC #13 lore raw

v2 cover object-file: fix packfile flush during transaction commit

In reply to Justin Tobler

Greetings,

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'

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

Signature · 1 line

2.55.0.424.g13c7afec21

Justin Tobler

author

2026-09-23 22:03 UTC #14 lore raw

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 <[email protected]>

---
 object-file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

object-file.c +7 -3

index a4cbf8b081..0f123b79fa 100644
@@ -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;
 }

Signature · 1 line

2.55.0.424.g13c7afec21

Patrick Steinhardt

2026-09-24 06:13 UTC #15 lore raw

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

Quoted 5 lines · 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.

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

Patrick

Justin Tobler

author

2026-09-23 22:03 UTC #16 lore raw

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

In reply to Justin Tobler

Signed-off-by Justin Tobler <[email protected]>

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

object-file.c +2 -2

index 0f123b79fa..210984f825 100644
@@ -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);

t/t1050-large.sh +17 -0

index d295c265c7..95233458b4 100755
@@ -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 &&

Signature · 1 line

2.55.0.424.g13c7afec21

Patrick Steinhardt

2026-09-24 06:01 UTC #17 lore raw

In reply to Justin Tobler

Quoted 3 lines · Justin Tobler

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

Thanks, this version looks good to me.

Patrick