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

On 26/09/23 03:16PM, Patrick Steinhardt wrote:

On Sun, Sep 13, 2026 at 03:26:22PM -0500, Justin Tobler wrote:

quoted

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.

quoted

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.

quoted

+	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