[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