[PATCH 4/4] object-file: create ODB transaction temporary directory eagerly

[Justin Tobler <jltobler@gmail.com>]

By default, ODB transactions for the "files" backend create their
temporary object directory lazily once the first loose object is about
to be written, and only if batched fsync is enabled. Transactions
configured for git-receive-pack(1) are the exception as the directory is
needed to quarantine incoming objects up front. Lazily creating the
temporary directory in this manner is awkward for a couple of reasons:

  - Writing loose objects has to know about the transaction internals to
    trigger the lazy creation of the temporary object directory.

  - Whether objects are staged in a temporary object directory, and thus
    whether they become visible at once on commit, depends on the
    configured fsync method.

Create the temporary object directory eagerly for every transaction when
it is started instead. This also ensures that no object is written to
the main object database before the transaction is committed.

Note that committing a transaction migrates every written object into
the primary object directory even when batched fsync is disabled. This
adds some minor overhead to ODB transaction callers that previously only
used a temporary directory if batched fsync was enabled. The following
benchmark uses the default fsync configuration and adds 10000 files of 4
KiB each to a freshly initialized repository via git-add(1):

  Benchmark 1: git add . (rev = HEAD~)
    Time (mean ± σ):     895.0 ms ±  23.8 ms    [User: 545.3 ms, System: 340.7 ms]
    Range (min … max):   866.7 ms … 928.7 ms    20 runs

  Benchmark 2: git add . (rev = HEAD)
    Time (mean ± σ):     956.3 ms ±  26.1 ms    [User: 541.4 ms, System: 405.2 ms]
    Range (min … max):   923.9 ms … 993.6 ms    20 runs

  Summary
    git add . (rev = HEAD~) ran
      1.07 ± 0.04 times faster than git add . (rev = HEAD)

Typically large numbers of loose objects are not expected to be written
into an ODB transaction so the overhead here should be marginal.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c         | 56 +++++++++----------------------------------
 object-file.h         |  1 -
 odb/source-loose.c    |  6 -----
 t/t0004-unwritable.sh | 19 +++++++--------
 4 files changed, 19 insertions(+), 63 deletions(-)

diff --git a/object-file.c b/object-file.c
index da189111bd..a9fb42705c 100644
--- a/object-file.c
+++ b/object-file.c
@@ -489,7 +489,6 @@ struct odb_transaction_files {
 
 	struct tmp_objdir *objdir;
 	struct transaction_packfile packfile;
-	const char *prefix;
 
 	struct strvec pack_names;
 	struct tempfile **pack_lockfiles;
@@ -497,27 +496,6 @@ struct odb_transaction_files {
 	size_t pack_lockfiles_alloc;
 };
 
-int odb_transaction_files_prepare(struct odb_transaction *base)
-{
-	struct odb_transaction_files *transaction =
-		container_of_or_null(base, struct odb_transaction_files, base);
-
-	/*
-	 * We lazily create the temporary object directory
-	 * the first time an object might be added, since
-	 * callers may not know whether any objects will be
-	 * added at the time they call odb_transaction_files_begin.
-	 */
-	if (!transaction || transaction->objdir)
-		return 0;
-
-	transaction->objdir = tmp_objdir_create(base->source->odb->repo, transaction->prefix, 0);
-	if (!transaction->objdir)
-		return error(_("unable to create temporary object directory"));
-
-	return 0;
-}
-
 void odb_transaction_files_fsync(struct odb_transaction *base,
 				 int fd, const char *filename)
 {
@@ -1548,13 +1526,10 @@ static int odb_transaction_files_env(struct odb_transaction *base,
 {
 	struct odb_transaction_files *transaction =
 		container_of(base, struct odb_transaction_files, base);
-	int ret;
 
-	ret = odb_transaction_files_prepare(&transaction->base);
-	if (!ret)
-		strvec_pushv(env, tmp_objdir_env(transaction->objdir));
+	strvec_pushv(env, tmp_objdir_env(transaction->objdir));
 
-	return ret;
+	return 0;
 }
 
 int odb_transaction_files_begin(struct odb_source *source,
@@ -1562,6 +1537,8 @@ int odb_transaction_files_begin(struct odb_source *source,
 				enum odb_transaction_flags flags)
 {
 	struct odb_transaction_files *transaction;
+	const char *prefix;
+	int ret;
 
 	transaction = xcalloc(1, sizeof(*transaction));
 	transaction->base.source = source;
@@ -1573,24 +1550,13 @@ int odb_transaction_files_begin(struct odb_source *source,
 	transaction->flags = flags;
 	strvec_init(&transaction->pack_names);
 
-	transaction->prefix = "transaction";
-	if (flags & ODB_TRANSACTION_RECEIVE) {
-		/*
-		 * ODB transactions for git-receive-pack(1) eagerly create a
-		 * temporary directory and use a different temporary directory
-		 * prefix.
-		 *
-		 * NEEDSWORK: This transaction flag is only used by the "files"
-		 * backend to special case temporary directory set up and
-		 * handling. Ideally transaction users should not have to care
-		 * though. To avoid this, we could eagerly create the temporary
-		 * directory and use the same prefix name for all transactions.
-		 */
-		transaction->prefix = "incoming";
-		if (odb_transaction_files_prepare(&transaction->base)) {
-			free(transaction);
-			return -1;
-		}
+	prefix = flags & ODB_TRANSACTION_RECEIVE ? "incoming" : "transaction";
+	transaction->objdir = tmp_objdir_create(source->odb->repo, prefix, 0);
+	if (!transaction->objdir) {
+		ret = error_errno(_("unable to create temporary object directory in %s"),
+				  source->path);
+		free(transaction);
+		return ret;
 	}
 
 	*out = &transaction->base;
diff --git a/object-file.h b/object-file.h
index f44758c4f8..9ad79bbde0 100644
--- a/object-file.h
+++ b/object-file.h
@@ -177,7 +177,6 @@ int odb_transaction_files_begin(struct odb_source *source,
 				struct odb_transaction **out,
 				enum odb_transaction_flags flags);
 
-int odb_transaction_files_prepare(struct odb_transaction *base);
 void odb_transaction_files_fsync(struct odb_transaction *base,
 				 int fd, const char *filename);
 
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 50cb388c35..9330ae94b5 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -769,9 +769,6 @@ static int write_loose_object(struct odb_source_loose *loose,
 	static struct strbuf tmp_file = STRBUF_INIT;
 	static struct strbuf filename = STRBUF_INIT;
 
-	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
-		odb_transaction_files_prepare(loose->base.odb->transaction);
-
 	odb_loose_path(loose, &filename, oid);
 
 	fd = start_loose_object_common(loose, &tmp_file, filename.buf, flags,
@@ -860,9 +857,6 @@ static int odb_source_loose_write_object_stream(struct odb_source *source,
 	char hdr[MAX_HEADER_LEN];
 	int hdrlen;
 
-	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
-		odb_transaction_files_prepare(loose->base.odb->transaction);
-
 	/* Since oid is not determined, save tmp file to odb path. */
 	strbuf_addf(&filename, "%s/", loose->base.path);
 	hdrlen = format_object_header(hdr, sizeof(hdr), in_stream->type, in_stream->size);
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 3bdafbae0f..7292672a66 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -24,8 +24,8 @@ test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable reposi
 test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree'
 test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' '
 	cat >expect <<-\EOF &&
-	error: insufficient permission for adding an object to repository database .git/objects
-	fatal: git-write-tree: error building trees
+	error: unable to create temporary object directory in .git/objects: Permission denied
+	fatal: failed to start ODB transaction
 	EOF
 	test_cmp expect out.write-tree
 '
@@ -39,8 +39,8 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository
 test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit'
 test_expect_success COMMIT_OUT 'commit output on unwritable repository' '
 	cat >expect <<-\EOF &&
-	error: insufficient permission for adding an object to repository database .git/objects
-	error: Error building trees
+	error: unable to create temporary object directory in .git/objects: Permission denied
+	fatal: failed to start ODB transaction
 	EOF
 	test_cmp expect out.commit
 '
@@ -55,9 +55,8 @@ test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repo
 test_lazy_prereq UPDATE_INDEX_OUT 'test -e "$TRASH_DIRECTORY"/out.update-index'
 test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' '
 	cat >expect <<-\EOF &&
-	error: insufficient permission for adding an object to repository database .git/objects
-	error: file: failed to insert into database
-	fatal: Unable to process path file
+	error: unable to create temporary object directory in .git/objects: Permission denied
+	fatal: failed to start ODB transaction
 	EOF
 	test_cmp expect out.update-index
 '
@@ -72,10 +71,8 @@ test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '
 test_lazy_prereq ADD_OUT 'test -e "$TRASH_DIRECTORY"/out.add'
 test_expect_success ADD_OUT 'add output on unwritable repository' '
 	cat >expect <<-\EOF &&
-	error: insufficient permission for adding an object to repository database .git/objects
-	error: file: failed to insert into database
-	error: unable to index file '\''file'\''
-	fatal: updating files failed
+	error: unable to create temporary object directory in .git/objects: Permission denied
+	fatal: failed to start ODB transaction
 	EOF
 	test_cmp expect out.add
 '
-- 
2.56.0.173.gf57c9a7388