[PATCH 0/3] object-file: fix ".keep" lockfile ownership in ODB transactions

[PATCH 0/3] object-file: fix ".keep" lockfile ownership in ODB transactions

[Justin Tobler]

Greetings,

This series addresses a bug report[1] regarding a git-receive-pack(1)
process deleting ".keep" lockfiles of with the same name but from
another concurrent git-receive-pack(1) process receiving an identical
packfile. Deleting the ".keep" lockfile for another process leaves its
newly written packfile vulnerable to deletion by another concurrent
housekeeping process.

The root of the problem here is that the temporary ".keep" lockfiles are
generated and registered in the object quarantine prior to being written
in the primary ODB. This means that even if the ODB transaction never
commits and exits early, the tempfile atexit(3) handler still runs and
tries to delete the lockfile at its expected final location. This
blindly deletes any file matching regardless of whether is was created
by the process.

The first two patches are some preparatory changes so that
`odb_transaction_files_write_pack()` is able to start explicitly
creating the lockfiles without relying on git-index-pack(1). Patch three
migrates lockfile generation to happen during ODB transaction commit to
ensure the lockfile is actually able to be written before registering it
as a tempfile.

Thanks,
-Justin

[1]: <932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com>

Justin Tobler (3):
  pack-write: lift and expose index-pack report parsing
  object-file: explicitly write ".keep" lockfile
  object-file: write ".keep" lockfiles during transaction commit

 object-file.c              | 117 +++++++++++++++++++++++++++++--------
 pack-write.c               |  69 ++++++++++++++++------
 pack.h                     |  10 ++++
 t/t5547-push-quarantine.sh |  29 ++++++++-
 4 files changed, 181 insertions(+), 44 deletions(-)


base-commit: f57c9a73884400380bddd172acc88b569834f909
-- 
2.55.0.424.g13c7afec21

[PATCH 1/3] pack-write: lift and expose index-pack report parsing

[Justin Tobler]

Once git-index-pack(1) finishes writing a packfile, it reports the
hexadecimal name of that packfile on stdout prefixed with either "pack"
or "keep" depending on whether a ".keep" file was also created alongside
it. Callers that are required to manage the lifecycle of ".keep" files
parse this report via `index_pack_lockfile()`, which provides the path
to the temporary ".keep" file.

In a subsequent commit, the "files" ODB transaction begins staging
packfiles in a transaction without eagerly creating a ".keep" file and
must instead read git-index-pack(1) "pack" output to get the packfile
name. Extract the generic parsing logic for the git-index-pack(1) output
into `read_index_pack_report()` in preparation accordingly.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 pack-write.c | 69 +++++++++++++++++++++++++++++++++++++++-------------
 pack.h       | 10 ++++++++
 2 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index 85674e4b72..5f5bb894a5 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -469,33 +469,68 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(struct odb_source *source, int ip_out,
-			  int *is_well_formed)
+int read_index_pack_report(struct repository *repo, int ip_out,
+			   struct strbuf *pack_name, int *is_keep)
 {
+	const int len = repo->hash_algo->hexsz + 6;
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = source->odb->repo->hash_algo->hexsz + 6;
+	const char *name;
+	int keep;
 
 	/*
-	 * The first thing we expect from index-pack's output
-	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
-	 * %40s is the newly created pack SHA1 name.  In the "keep"
-	 * case, we need it to remove the corresponding .keep file
-	 * later on.  If we don't get that then tough luck with it.
+	 * The first thing we expect from index-pack's output is
+	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
+	 * name of the packfile it has just written. The line is therefore five
+	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
+	 * "keep" prefix tells us that index-pack has also created the
+	 * corresponding ".keep" file.
 	 */
-	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
-		const char *name;
+	if (read_in_full(ip_out, packname, len) != len || packname[len-1] != '\n')
+		return -1;
+
+	packname[len-1] = 0;
+	if (skip_prefix(packname, "pack\t", &name))
+		keep = 0;
+	else if (skip_prefix(packname, "keep\t", &name))
+		keep = 1;
+	else
+		return -1;
+
+	strbuf_addstr(pack_name, name);
+	if (is_keep)
+		*is_keep = keep;
+
+	return 0;
+}
+
+char *index_pack_lockfile(struct odb_source *source, int ip_out,
+			  int *is_well_formed)
+{
+	struct strbuf pack_name = STRBUF_INIT;
+	char *lockfile = NULL;
+	int is_keep = 0;
 
+	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
 		if (is_well_formed)
-			*is_well_formed = 1;
-		packname[len-1] = 0;
-		if (skip_prefix(packname, "keep\t", &name))
-			return xstrfmt("%s/pack/pack-%s.keep",
-				       source->path, name);
+			*is_well_formed = 0;
 		return NULL;
 	}
+
 	if (is_well_formed)
-		*is_well_formed = 0;
-	return NULL;
+		*is_well_formed = 1;
+
+	/*
+	 * If there is no ".keep" file, then there is no lockfile to return.
+	 */
+	if (!is_keep) {
+		strbuf_release(&pack_name);
+		return NULL;
+	}
+
+	lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);
+
+	strbuf_release(&pack_name);
+	return lockfile;
 }
 
 /*
diff --git a/pack.h b/pack.h
index ada506b5c5..ddf2d52934 100644
--- a/pack.h
+++ b/pack.h
@@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
+
+/*
+ * Read the report that git-index-pack(1) writes to its standard output after
+ * the packfile is written. The hexadecimal name of the packfile is appended to
+ * `pack_name` and, if `is_keep` is non-NULL, set it to indicate whether a
+ * ".keep" file was written. Return 0 on success and a negative error code on
+ * failure.
+ */
+int read_index_pack_report(struct repository *repo, int ip_out,
+			   struct strbuf *pack_name, int *is_keep);
 char *index_pack_lockfile(struct odb_source *source, int fd,
 			  int *is_well_formed);
 
-- 
2.55.0.424.g13c7afec21

[PATCH 2/3] object-file: explicitly write ".keep" lockfile

[Justin Tobler]

The `odb_transaction_write_pack()` interface for the "files" backend
relies on git-index-pack(1) to write the specified packfile into an ODB
transaction as a packfile. To prevent a concurrent housekeeping
operation from deleting the newly written packfile prior to the
references being updated, a ".keep" lockfile is also generated by
git-index-pack(1) via the `--keep` option. Consequently, the ".keep"
lockfile is written into the ODB transaction alongside the packfile and
later migrated together to the primary ODB during transaction commit.

In a subsequent commit, writing the lockfile is deferred until
transaction commit and thus cannot rely on git-index-pack(1) to write
it. In preparation for this, update `odb_transaction_files_write_pack()`
to explicitly generate the same ".keep" lockfile itself.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 99 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 20 deletions(-)

diff --git a/object-file.c b/object-file.c
index 210984f825..ebe46d35de 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1256,6 +1256,43 @@ int read_loose_object(struct repository *repo,
 	return ret;
 }
 
+static int write_pack_lock(struct odb_source *source, const char *pack_name,
+			   const char *keep_msg)
+{
+	struct strbuf lock_path = STRBUF_INIT;
+	struct strbuf contents = STRBUF_INIT;
+	int ret = -1;
+	int fd;
+
+	strbuf_addf(&lock_path, "%s/pack/pack-%s.keep",
+		    source->path, pack_name);
+	strbuf_addf(&contents, "%s\n", keep_msg);
+
+	fd = safe_create_file_with_leading_directories(source->odb->repo,
+						       lock_path.buf);
+	if (fd < 0)
+		goto out;
+
+	if (write_in_full(fd, contents.buf, contents.len) < 0) {
+		close(fd);
+		unlink(lock_path.buf);
+		goto out;
+	}
+
+	if (close(fd)) {
+		unlink(lock_path.buf);
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	strbuf_release(&lock_path);
+	strbuf_release(&contents);
+
+	return ret;
+}
+
 static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
@@ -1389,18 +1426,17 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 	} else {
+		struct strbuf final_path = STRBUF_INIT;
+		struct strbuf pack_name = STRBUF_INIT;
+		struct strbuf keep_msg = STRBUF_INIT;
 		char hostname[HOST_NAME_MAX + 1];
-		char *lockfile;
+		int ret;
 
 		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
 		push_header_arg(&child.args, &hdr);
 
 		if (xgethostname(hostname, sizeof(hostname)))
 			xsnprintf(hostname, sizeof(hostname), "localhost");
-		strvec_pushf(&child.args,
-			     "--keep=receive-pack %"PRIuMAX" on %s",
-			     (uintmax_t)getpid(),
-			     hostname);
 
 		if (!opts->quiet && err_fd)
 			strvec_push(&child.args, "--show-resolving-progress");
@@ -1424,31 +1460,54 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 
-		/*
-		 * The lockfile filepath is expected to be the final location of
-		 * the ".keep" file after being migrated to the main ODB source.
-		 * This ensures the lockfile can be found and removed later
-		 * after the ODB transaction has been committed.
-		 */
-		lockfile = index_pack_lockfile(base->source, child.out, NULL);
-		if (lockfile) {
-			ALLOC_GROW(transaction->pack_lockfiles,
-				   transaction->pack_lockfiles_nr + 1,
-				   transaction->pack_lockfiles_alloc);
-			transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
-				register_tempfile(lockfile);
-			free(lockfile);
-		}
+		ret = read_index_pack_report(repo, child.out, &pack_name, NULL);
+
 		close(child.out);
 
 		status = finish_command(&child);
 		if (status) {
+			strbuf_release(&pack_name);
 			strbuf_addstr(err_msg, "index-pack abnormal exit");
 			return -1;
 		}
 
+		if (ret) {
+			strbuf_release(&pack_name);
+			strbuf_addstr(err_msg, "index-pack failed to report pack name");
+			return -1;
+		}
+
+		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
+			    (uintmax_t)getpid(), hostname);
+
+		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
+			strbuf_release(&pack_name);
+			strbuf_release(&keep_msg);
+			strbuf_addstr(err_msg, "lockfile write failed");
+			return -1;
+		}
+
+		ALLOC_GROW(transaction->pack_lockfiles,
+			   transaction->pack_lockfiles_nr + 1,
+			   transaction->pack_lockfiles_alloc);
+
+		/*
+		 * The lockfile filepath is expected to be the final location of
+		 * the ".keep" file after being migrated to the main ODB source.
+		 * This ensures the lockfile can be found and removed later
+		 * after the ODB transaction has been committed.
+		 */
+		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
+			    base->source->path, pack_name.buf);
+		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
+			register_tempfile(final_path.buf);
+
 		odb_source_prepare(transaction->quarantine,
 				   ODB_PREPARE_FLUSH_CACHES);
+
+		strbuf_release(&final_path);
+		strbuf_release(&pack_name);
+		strbuf_release(&keep_msg);
 	}
 
 	return 0;
-- 
2.55.0.424.g13c7afec21

[PATCH 3/3] object-file: write ".keep" lockfiles during transaction commit

[Justin Tobler]

The "files" ODB backend writes a ".keep" lockfile into the transaction
quarantine directory via `odb_transaction_files_write_pack()`. When the
ODB transaction is committed, the ".keep" lockfile is migrated to the
primary ODB alongside its packfile. Writing the temporary lockfile in
this manner has a couple of downsides:

  - Failing to migrate the lockfile during transaction commit due to a
    concurrent process already acquiring the same lock still allows the
    rest of the transaction to be migrated into the primary ODB. This
    dirties the primary ODB with new objects even though no references
    were updated and the operation failed.

  - Even more concerning, the ".keep" tempfile atexit(3) handler tries
    to remove any lockfile of the same name regardless of whether it was
    created by the same process. If the ".keep" lockfile of the same
    name for a concurrent process gets deleted prematurely, its packfile
    becomes at risk of being deleted by concurrent housekeeping.

Stop creating/registering the temporary lockfiles when writing the
packfile to the transaction and instead do so when committing the
transaction. This ensures the lockfile is properly acquired prior to
migrating objects and that only the owning process can delete its own
lockfiles.

Reported-by: Qin ShiCheng <qeesung@live.com>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c              | 76 +++++++++++++++++++++-----------------
 t/t5547-push-quarantine.sh | 29 +++++++++++++--
 2 files changed, 69 insertions(+), 36 deletions(-)

diff --git a/object-file.c b/object-file.c
index ebe46d35de..39e6299be1 100644
--- a/object-file.c
+++ b/object-file.c
@@ -492,6 +492,7 @@ struct odb_transaction_files {
 	struct transaction_packfile packfile;
 	const char *prefix;
 
+	struct strvec pack_names;
 	struct tempfile **pack_lockfiles;
 	size_t pack_lockfiles_nr;
 	size_t pack_lockfiles_alloc;
@@ -1257,7 +1258,7 @@ int read_loose_object(struct repository *repo,
 }
 
 static int write_pack_lock(struct odb_source *source, const char *pack_name,
-			   const char *keep_msg)
+			   const char *keep_msg, struct tempfile **out)
 {
 	struct strbuf lock_path = STRBUF_INIT;
 	struct strbuf contents = STRBUF_INIT;
@@ -1284,6 +1285,7 @@ static int write_pack_lock(struct odb_source *source, const char *pack_name,
 		goto out;
 	}
 
+	*out = register_tempfile(lock_path.buf);
 	ret = 0;
 
 out:
@@ -1293,6 +1295,40 @@ static int write_pack_lock(struct odb_source *source, const char *pack_name,
 	return ret;
 }
 
+static int write_pack_locks(struct odb_transaction_files *transaction)
+{
+	struct strbuf keep_msg = STRBUF_INIT;
+	char hostname[HOST_NAME_MAX + 1];
+	int ret = 0;
+
+	if (!transaction->pack_names.nr)
+		return 0;
+
+	if (xgethostname(hostname, sizeof(hostname)))
+		xsnprintf(hostname, sizeof(hostname), "localhost");
+	strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
+		    (uintmax_t)getpid(), hostname);
+
+	for (size_t i = 0; i < transaction->pack_names.nr; i++) {
+		struct tempfile *lock;
+
+		ret = write_pack_lock(transaction->base.source,
+				      transaction->pack_names.v[i],
+				      keep_msg.buf, &lock);
+		if (ret < 0)
+			break;
+
+		ALLOC_GROW(transaction->pack_lockfiles,
+			   transaction->pack_lockfiles_nr + 1,
+			   transaction->pack_lockfiles_alloc);
+		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = lock;
+	}
+
+	strbuf_release(&keep_msg);
+
+	return ret;
+}
+
 static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
@@ -1321,6 +1357,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 		delete_tempfile(&temp);
 		strbuf_release(&temp_path);
 
+		if (write_pack_locks(transaction))
+			return error(_("unable to lock packfiles"));
+
 		/*
 		 * Make the object files visible in the primary ODB after their data is
 		 * fully durable.
@@ -1426,18 +1465,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 	} else {
-		struct strbuf final_path = STRBUF_INIT;
 		struct strbuf pack_name = STRBUF_INIT;
-		struct strbuf keep_msg = STRBUF_INIT;
-		char hostname[HOST_NAME_MAX + 1];
 		int ret;
 
 		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
 		push_header_arg(&child.args, &hdr);
 
-		if (xgethostname(hostname, sizeof(hostname)))
-			xsnprintf(hostname, sizeof(hostname), "localhost");
-
 		if (!opts->quiet && err_fd)
 			strvec_push(&child.args, "--show-resolving-progress");
 		if (err_fd)
@@ -1477,37 +1510,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 
-		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
-			    (uintmax_t)getpid(), hostname);
-
-		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
-			strbuf_release(&pack_name);
-			strbuf_release(&keep_msg);
-			strbuf_addstr(err_msg, "lockfile write failed");
-			return -1;
-		}
-
-		ALLOC_GROW(transaction->pack_lockfiles,
-			   transaction->pack_lockfiles_nr + 1,
-			   transaction->pack_lockfiles_alloc);
-
-		/*
-		 * The lockfile filepath is expected to be the final location of
-		 * the ".keep" file after being migrated to the main ODB source.
-		 * This ensures the lockfile can be found and removed later
-		 * after the ODB transaction has been committed.
-		 */
-		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
-			    base->source->path, pack_name.buf);
-		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
-			register_tempfile(final_path.buf);
+		strvec_push(&transaction->pack_names, pack_name.buf);
 
 		odb_source_prepare(transaction->quarantine,
 				   ODB_PREPARE_FLUSH_CACHES);
 
-		strbuf_release(&final_path);
 		strbuf_release(&pack_name);
-		strbuf_release(&keep_msg);
 	}
 
 	return 0;
@@ -1523,6 +1531,7 @@ static int odb_transaction_files_finalize(struct odb_transaction *base)
 		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
 
 	free(transaction->pack_lockfiles);
+	strvec_clear(&transaction->pack_names);
 
 	return ret;
 }
@@ -1555,6 +1564,7 @@ int odb_transaction_files_begin(struct odb_source *source,
 	transaction->base.write_pack = odb_transaction_files_write_pack;
 	transaction->base.env = odb_transaction_files_env;
 	transaction->flags = flags;
+	strvec_init(&transaction->pack_names);
 
 	transaction->prefix = "bulk-fsync";
 	if (flags & ODB_TRANSACTION_RECEIVE) {
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1b7097179e..cf349eee2c 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -77,10 +77,9 @@ test_expect_success '.keep file is removed after push' '
 	git -C keep.git config set receive.unpackLimit 0 &&
 
 	# While incoming objects are still quarantined, validate that the
-	# ".keep" lockfile is present in the quarantine directory.
+	# ".keep" lockfile is not present in the quarantine directory.
 	test_hook -C keep.git pre-receive <<-\EOF &&
-	keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" &&
-	test -f "$keep"
+	test ! -e "$GIT_QUARANTINE_PATH"/pack/pack-*.keep
 	EOF
 
 	# After quarantined objects are migrated, validate that the ".keep"
@@ -101,4 +100,28 @@ test_expect_success '.keep file is removed after push' '
 	test_path_is_missing "$keep"
 '
 
+test_expect_success '.keep file created outside of process is not removed' '
+	test_when_finished rm -rf keep.git &&
+	git init --bare keep.git &&
+
+	git -C keep.git config set receive.unpackLimit 0 &&
+
+	# Create a ".keep" lockfile matching the packfile to simulate a
+	# concurrent git-receive-pack(1) that was already written its lockfile.
+	test_hook -C keep.git pre-receive <<-\EOF &&
+	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
+	pack="${pack##*/}" &&
+	echo "baz" >"objects/pack/${pack%.pack}.keep"
+	EOF
+
+	test_commit bar &&
+	test_must_fail git push keep.git HEAD &&
+
+	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
+	# left in its place.
+	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
+	test_path_is_file "$keep" &&
+	test_grep "baz" "$keep"
+'
+
 test_done
-- 
2.55.0.424.g13c7afec21

Re: [PATCH 1/3] pack-write: lift and expose index-pack report parsing

[Patrick Steinhardt]

On Thu, Oct 15, 2026 at 09:28:59PM -0500, Justin Tobler wrote:
> Once git-index-pack(1) finishes writing a packfile, it reports the
> hexadecimal name of that packfile on stdout prefixed with either "pack"
> or "keep" depending on whether a ".keep" file was also created alongside
> it. Callers that are required to manage the lifecycle of ".keep" files
> parse this report via `index_pack_lockfile()`, which provides the path
> to the temporary ".keep" file.
> 
> In a subsequent commit, the "files" ODB transaction begins staging
> packfiles in a transaction without eagerly creating a ".keep" file and
> must instead read git-index-pack(1) "pack" output to get the packfile
> name. Extract the generic parsing logic for the git-index-pack(1) output
> into `read_index_pack_report()` in preparation accordingly.

So if I understand correctly, the intent is to eventually stop creating
the ".keep" file in the quarantine directory? I assume that instead,
you'll then manually create that file in the final location before
moving around the pack itself.

If so that makes sense -- it's kind of pointless to create ".keep" files
in quarantines anyway, nobody but the current process should be touching
it anyway.

> diff --git a/pack-write.c b/pack-write.c
> index 85674e4b72..5f5bb894a5 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -469,33 +469,68 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
>  	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
>  }
>  
> -char *index_pack_lockfile(struct odb_source *source, int ip_out,
> -			  int *is_well_formed)
> +int read_index_pack_report(struct repository *repo, int ip_out,
> +			   struct strbuf *pack_name, int *is_keep)
>  {
> +	const int len = repo->hash_algo->hexsz + 6;
>  	char packname[GIT_MAX_HEXSZ + 6];
> -	const int len = source->odb->repo->hash_algo->hexsz + 6;
> +	const char *name;
> +	int keep;

Suggestion, please feel free to ignore:

    int is_keep_ignored;

    if (!is_keep)
        is_keep = &is_keep_ignored;

Then you can unconditionally use `is_keep` further down, which I think
is a bit less awkward.

>  	/*
> -	 * The first thing we expect from index-pack's output
> -	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
> -	 * %40s is the newly created pack SHA1 name.  In the "keep"
> -	 * case, we need it to remove the corresponding .keep file
> -	 * later on.  If we don't get that then tough luck with it.
> +	 * The first thing we expect from index-pack's output is
> +	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
> +	 * name of the packfile it has just written. The line is therefore five
> +	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
> +	 * "keep" prefix tells us that index-pack has also created the
> +	 * corresponding ".keep" file.
>  	 */

Okay. I originally thought you only reflow this comment, but you fix it
to no longer assume that packfile hashes use SHA1. I would have either
pointed this out in the commit message or done this as a preparatory
commit.

> -	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
> -		const char *name;
> +	if (read_in_full(ip_out, packname, len) != len || packname[len-1] != '\n')
> +		return -1;
> +
> +	packname[len-1] = 0;

Missing spaces around `-`.

> +	if (skip_prefix(packname, "pack\t", &name))
> +		keep = 0;
> +	else if (skip_prefix(packname, "keep\t", &name))
> +		keep = 1;
> +	else
> +		return -1;
> +
> +	strbuf_addstr(pack_name, name);
> +	if (is_keep)
> +		*is_keep = keep;
> +
> +	return 0;
> +}
> +
> +char *index_pack_lockfile(struct odb_source *source, int ip_out,
> +			  int *is_well_formed)
> +{
> +	struct strbuf pack_name = STRBUF_INIT;
> +	char *lockfile = NULL;
> +	int is_keep = 0;
>  
> +	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
>  		if (is_well_formed)
> -			*is_well_formed = 1;
> -		packname[len-1] = 0;
> -		if (skip_prefix(packname, "keep\t", &name))
> -			return xstrfmt("%s/pack/pack-%s.keep",
> -				       source->path, name);
> +			*is_well_formed = 0;
>  		return NULL;
>  	}
> +
>  	if (is_well_formed)
> -		*is_well_formed = 0;
> -	return NULL;
> +		*is_well_formed = 1;
> +
> +	/*
> +	 * If there is no ".keep" file, then there is no lockfile to return.
> +	 */
> +	if (!is_keep) {
> +		strbuf_release(&pack_name);
> +		return NULL;
> +	}

Can we maybe have a single exit path responsible for releasing
`pack_name`?

    /* If there is no ".keep" file, then there is no lockfile to return. */
    if (is_keep)
        lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);

    strbuf_release(&pack_name);
    return lockfile;

> diff --git a/pack.h b/pack.h
> index ada506b5c5..ddf2d52934 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
>  void fixup_pack_header_footer(const struct git_hash_algo *, int,
>  			      unsigned char *, const char *, uint32_t,
>  			      unsigned char *, off_t);
> +
> +/*
> + * Read the report that git-index-pack(1) writes to its standard output after
> + * the packfile is written. The hexadecimal name of the packfile is appended to
> + * `pack_name` and, if `is_keep` is non-NULL, set it to indicate whether a

s/set it to/it is set to/

> + * ".keep" file was written. Return 0 on success and a negative error code on
> + * failure.
> + */
> +int read_index_pack_report(struct repository *repo, int ip_out,
> +			   struct strbuf *pack_name, int *is_keep);
>  char *index_pack_lockfile(struct odb_source *source, int fd,
>  			  int *is_well_formed);

Patrick

Re: [PATCH 2/3] object-file: explicitly write ".keep" lockfile

[Patrick Steinhardt]

On Thu, Oct 15, 2026 at 09:29:00PM -0500, Justin Tobler wrote:
> diff --git a/object-file.c b/object-file.c
> index 210984f825..ebe46d35de 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1256,6 +1256,43 @@ int read_loose_object(struct repository *repo,
>  	return ret;
>  }
>  
> +static int write_pack_lock(struct odb_source *source, const char *pack_name,
> +			   const char *keep_msg)

Nit: I'd have called this `write_pack_keep()`, but I see that we refer
to the files as "lock" in `index_pack_lockfile(), too. Should we align
those two functions and call this `write_pack_lockfile()`?

> +{
> +	struct strbuf lock_path = STRBUF_INIT;
> +	struct strbuf contents = STRBUF_INIT;
> +	int ret = -1;
> +	int fd;
> +
> +	strbuf_addf(&lock_path, "%s/pack/pack-%s.keep",
> +		    source->path, pack_name);
> +	strbuf_addf(&contents, "%s\n", keep_msg);
> +
> +	fd = safe_create_file_with_leading_directories(source->odb->repo,
> +						       lock_path.buf);

The command has just written its packfile, so can't we assume that
leading directories have already been created?

> +	if (fd < 0)
> +		goto out;
> +
> +	if (write_in_full(fd, contents.buf, contents.len) < 0) {
> +		close(fd);
> +		unlink(lock_path.buf);
> +		goto out;
> +	}
> +
> +	if (close(fd)) {

`if (close(fd) < 0)`, just to be consistent.

> @@ -1389,18 +1426,17 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  			return -1;
>  		}
>  	} else {
> +		struct strbuf final_path = STRBUF_INIT;
> +		struct strbuf pack_name = STRBUF_INIT;
> +		struct strbuf keep_msg = STRBUF_INIT;
>  		char hostname[HOST_NAME_MAX + 1];
> -		char *lockfile;
> +		int ret;
>  
>  		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
>  		push_header_arg(&child.args, &hdr);
>  
>  		if (xgethostname(hostname, sizeof(hostname)))
>  			xsnprintf(hostname, sizeof(hostname), "localhost");
> -		strvec_pushf(&child.args,
> -			     "--keep=receive-pack %"PRIuMAX" on %s",
> -			     (uintmax_t)getpid(),
> -			     hostname);

Okay. So instead of asking git-index-pack(1) to write the ".keep" file,
we now...

> @@ -1424,31 +1460,54 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  			return -1;
>  		}
>  
> -		/*
> -		 * The lockfile filepath is expected to be the final location of
> -		 * the ".keep" file after being migrated to the main ODB source.
> -		 * This ensures the lockfile can be found and removed later
> -		 * after the ODB transaction has been committed.
> -		 */
> -		lockfile = index_pack_lockfile(base->source, child.out, NULL);
> -		if (lockfile) {
> -			ALLOC_GROW(transaction->pack_lockfiles,
> -				   transaction->pack_lockfiles_nr + 1,
> -				   transaction->pack_lockfiles_alloc);
> -			transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
> -				register_tempfile(lockfile);
> -			free(lockfile);
> -		}
> +		ret = read_index_pack_report(repo, child.out, &pack_name, NULL);
> +

... read its stdout to figure out where the packfile was written to
and...

>  		status = finish_command(&child);
>  		if (status) {
> +			strbuf_release(&pack_name);
>  			strbuf_addstr(err_msg, "index-pack abnormal exit");
>  			return -1;
>  		}
>  
> +		if (ret) {
> +			strbuf_release(&pack_name);
> +			strbuf_addstr(err_msg, "index-pack failed to report pack name");
> +			return -1;
> +		}
> +
> +		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
> +			    (uintmax_t)getpid(), hostname);
> +
> +		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
> +			strbuf_release(&pack_name);
> +			strbuf_release(&keep_msg);
> +			strbuf_addstr(err_msg, "lockfile write failed");
> +			return -1;
> +		}

... then write the lockfile ourselves.

By the way, the error handling here starts to become unwieldy. Can we
maybe have a common exit path that releases all resources for us?

> +		ALLOC_GROW(transaction->pack_lockfiles,
> +			   transaction->pack_lockfiles_nr + 1,
> +			   transaction->pack_lockfiles_alloc);
> +
> +		/*
> +		 * The lockfile filepath is expected to be the final location of
> +		 * the ".keep" file after being migrated to the main ODB source.
> +		 * This ensures the lockfile can be found and removed later
> +		 * after the ODB transaction has been committed.
> +		 */
> +		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
> +			    base->source->path, pack_name.buf);
> +		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
> +			register_tempfile(final_path.buf);

So here we retain the status quo, which is to record the lockfile in the
final location even though it doesn't exist there yet. And that can lead
to us pruning a pack lockfile from a different process.

Patrick

Re: [PATCH 3/3] object-file: write ".keep" lockfiles during transaction commit

[Patrick Steinhardt]

On Thu, Oct 15, 2026 at 09:29:01PM -0500, Justin Tobler wrote:
> The "files" ODB backend writes a ".keep" lockfile into the transaction
> quarantine directory via `odb_transaction_files_write_pack()`. When the
> ODB transaction is committed, the ".keep" lockfile is migrated to the
> primary ODB alongside its packfile. Writing the temporary lockfile in
> this manner has a couple of downsides:
> 
>   - Failing to migrate the lockfile during transaction commit due to a
>     concurrent process already acquiring the same lock still allows the
>     rest of the transaction to be migrated into the primary ODB. This
>     dirties the primary ODB with new objects even though no references
>     were updated and the operation failed.

I'm not fully sure I understand this downside. Are you saying that we
fail to migrate the lockfile, then continue migrating everything else
only to then fail the whole transaction?

... skipping ahead a bit...

Oh, I get it now. You propose to first acquire all lockfiles before
migrating anything so that we cannot conflict with a concurrent process
that aims to claim the same locks.

That will of course mean that we will fail the transaction for any
concurrent process that happens to write the same packfiles. But based
on what you claim that was already the case anyway, but now we notice
early and thus won't commit parts of the transaction. Makes sense.

>   - Even more concerning, the ".keep" tempfile atexit(3) handler tries
>     to remove any lockfile of the same name regardless of whether it was
>     created by the same process. If the ".keep" lockfile of the same
>     name for a concurrent process gets deleted prematurely, its packfile
>     becomes at risk of being deleted by concurrent housekeeping.

And this is the other issue that you've already been hinting at.

> diff --git a/object-file.c b/object-file.c
> index ebe46d35de..39e6299be1 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1321,6 +1357,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  		delete_tempfile(&temp);
>  		strbuf_release(&temp_path);
>  
> +		if (write_pack_locks(transaction))
> +			return error(_("unable to lock packfiles"));

Okay, we'll now write lockfiles before migrating any of the data. So if
any of the files already happens to exist, we'll bail out here. It would
have been great to maybe also provide a proper failure message via errno
for example, but okay.

> @@ -1477,37 +1510,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  			return -1;
>  		}
>  
> -		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
> -			    (uintmax_t)getpid(), hostname);
> -
> -		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
> -			strbuf_release(&pack_name);
> -			strbuf_release(&keep_msg);
> -			strbuf_addstr(err_msg, "lockfile write failed");
> -			return -1;
> -		}

And here we stop writing lockfiles when writing the pack.

> diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> index 1b7097179e..cf349eee2c 100755
> --- a/t/t5547-push-quarantine.sh
> +++ b/t/t5547-push-quarantine.sh
> @@ -77,10 +77,9 @@ test_expect_success '.keep file is removed after push' '
>  	git -C keep.git config set receive.unpackLimit 0 &&
>  
>  	# While incoming objects are still quarantined, validate that the
> -	# ".keep" lockfile is present in the quarantine directory.
> +	# ".keep" lockfile is not present in the quarantine directory.
>  	test_hook -C keep.git pre-receive <<-\EOF &&
> -	keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" &&
> -	test -f "$keep"
> +	test ! -e "$GIT_QUARANTINE_PATH"/pack/pack-*.keep
>  	EOF
>  
>  	# After quarantined objects are migrated, validate that the ".keep"
> @@ -101,4 +100,28 @@ test_expect_success '.keep file is removed after push' '
>  	test_path_is_missing "$keep"
>  '
>  
> +test_expect_success '.keep file created outside of process is not removed' '
> +	test_when_finished rm -rf keep.git &&
> +	git init --bare keep.git &&
> +
> +	git -C keep.git config set receive.unpackLimit 0 &&
> +
> +	# Create a ".keep" lockfile matching the packfile to simulate a
> +	# concurrent git-receive-pack(1) that was already written its lockfile.
> +	test_hook -C keep.git pre-receive <<-\EOF &&
> +	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
> +	pack="${pack##*/}" &&
> +	echo "baz" >"objects/pack/${pack%.pack}.keep"
> +	EOF
> +
> +	test_commit bar &&
> +	test_must_fail git push keep.git HEAD &&
> +
> +	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
> +	# left in its place.
> +	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
> +	test_path_is_file "$keep" &&
> +	test_grep "baz" "$keep"

Also, there should be no corresponding ".pack" file, right? Do we want
to assert that, too?

Thanks!

Patrick

Re: [PATCH 1/3] pack-write: lift and expose index-pack report parsing

[Justin Tobler]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> On Thu, Oct 15, 2026 at 09:28:59PM -0500, Justin Tobler wrote:
> > Once git-index-pack(1) finishes writing a packfile, it reports the
> > hexadecimal name of that packfile on stdout prefixed with either "pack"
> > or "keep" depending on whether a ".keep" file was also created alongside
> > it. Callers that are required to manage the lifecycle of ".keep" files
> > parse this report via `index_pack_lockfile()`, which provides the path
> > to the temporary ".keep" file.
> > 
> > In a subsequent commit, the "files" ODB transaction begins staging
> > packfiles in a transaction without eagerly creating a ".keep" file and
> > must instead read git-index-pack(1) "pack" output to get the packfile
> > name. Extract the generic parsing logic for the git-index-pack(1) output
> > into `read_index_pack_report()` in preparation accordingly.
> 
> So if I understand correctly, the intent is to eventually stop creating
> the ".keep" file in the quarantine directory? I assume that instead,
> you'll then manually create that file in the final location before
> moving around the pack itself.

That is correct. Instead of migrating the ".keep" file in the quarantine
with all the other object files, the intent is for the ".keep" file to
be manually created during transaction commit first. I'll update the
commit message here a bit to allude to this a bit more directly.

> If so that makes sense -- it's kind of pointless to create ".keep" files
> in quarantines anyway, nobody but the current process should be touching
> it anyway.

Yup, creating the lockfile in the quarantine doesn't really tell us
anything. It also makes it a bit more complicated during transaction
commit to detect whether there is already a lockfile for the same pack
that exists.

> > diff --git a/pack-write.c b/pack-write.c
> > index 85674e4b72..5f5bb894a5 100644
> > --- a/pack-write.c
> > +++ b/pack-write.c
> > @@ -469,33 +469,68 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
> >  	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
> >  }
> >  
> > -char *index_pack_lockfile(struct odb_source *source, int ip_out,
> > -			  int *is_well_formed)
> > +int read_index_pack_report(struct repository *repo, int ip_out,
> > +			   struct strbuf *pack_name, int *is_keep)
> >  {
> > +	const int len = repo->hash_algo->hexsz + 6;
> >  	char packname[GIT_MAX_HEXSZ + 6];
> > -	const int len = source->odb->repo->hash_algo->hexsz + 6;
> > +	const char *name;
> > +	int keep;
> 
> Suggestion, please feel free to ignore:
> 
>     int is_keep_ignored;
> 
>     if (!is_keep)
>         is_keep = &is_keep_ignored;
> 
> Then you can unconditionally use `is_keep` further down, which I think
> is a bit less awkward.

Good suggestion. Will use in the next version.

> >  	/*
> > -	 * The first thing we expect from index-pack's output
> > -	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
> > -	 * %40s is the newly created pack SHA1 name.  In the "keep"
> > -	 * case, we need it to remove the corresponding .keep file
> > -	 * later on.  If we don't get that then tough luck with it.
> > +	 * The first thing we expect from index-pack's output is
> > +	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
> > +	 * name of the packfile it has just written. The line is therefore five
> > +	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
> > +	 * "keep" prefix tells us that index-pack has also created the
> > +	 * corresponding ".keep" file.
> >  	 */
> 
> Okay. I originally thought you only reflow this comment, but you fix it
> to no longer assume that packfile hashes use SHA1. I would have either
> pointed this out in the commit message or done this as a preparatory
> commit.

Will split out the code comment change into a preparatory commit.

> > -	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
> > -		const char *name;
> > +	if (read_in_full(ip_out, packname, len) != len || packname[len-1] != '\n')
> > +		return -1;
> > +
> > +	packname[len-1] = 0;
> 
> Missing spaces around `-`.

Will fix.

> > +	if (skip_prefix(packname, "pack\t", &name))
> > +		keep = 0;
> > +	else if (skip_prefix(packname, "keep\t", &name))
> > +		keep = 1;
> > +	else
> > +		return -1;
> > +
> > +	strbuf_addstr(pack_name, name);
> > +	if (is_keep)
> > +		*is_keep = keep;
> > +
> > +	return 0;
> > +}
> > +
> > +char *index_pack_lockfile(struct odb_source *source, int ip_out,
> > +			  int *is_well_formed)
> > +{
> > +	struct strbuf pack_name = STRBUF_INIT;
> > +	char *lockfile = NULL;
> > +	int is_keep = 0;
> >  
> > +	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
> >  		if (is_well_formed)
> > -			*is_well_formed = 1;
> > -		packname[len-1] = 0;
> > -		if (skip_prefix(packname, "keep\t", &name))
> > -			return xstrfmt("%s/pack/pack-%s.keep",
> > -				       source->path, name);
> > +			*is_well_formed = 0;
> >  		return NULL;
> >  	}
> > +
> >  	if (is_well_formed)
> > -		*is_well_formed = 0;
> > -	return NULL;
> > +		*is_well_formed = 1;
> > +
> > +	/*
> > +	 * If there is no ".keep" file, then there is no lockfile to return.
> > +	 */
> > +	if (!is_keep) {
> > +		strbuf_release(&pack_name);
> > +		return NULL;
> > +	}
> 
> Can we maybe have a single exit path responsible for releasing
> `pack_name`?
> 
>     /* If there is no ".keep" file, then there is no lockfile to return. */
>     if (is_keep)
>         lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);
> 
>     strbuf_release(&pack_name);
>     return lockfile;

That is indeed much better. Will use in the next version. Thanks.

> > diff --git a/pack.h b/pack.h
> > index ada506b5c5..ddf2d52934 100644
> > --- a/pack.h
> > +++ b/pack.h
> > @@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
> >  void fixup_pack_header_footer(const struct git_hash_algo *, int,
> >  			      unsigned char *, const char *, uint32_t,
> >  			      unsigned char *, off_t);
> > +
> > +/*
> > + * Read the report that git-index-pack(1) writes to its standard output after
> > + * the packfile is written. The hexadecimal name of the packfile is appended to
> > + * `pack_name` and, if `is_keep` is non-NULL, set it to indicate whether a
> 
> s/set it to/it is set to/

Thanks for the careful eyes.

-Justin

Re: [PATCH 2/3] object-file: explicitly write ".keep" lockfile

[Justin Tobler]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> On Thu, Oct 15, 2026 at 09:29:00PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index 210984f825..ebe46d35de 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1256,6 +1256,43 @@ int read_loose_object(struct repository *repo,
> >  	return ret;
> >  }
> >  
> > +static int write_pack_lock(struct odb_source *source, const char *pack_name,
> > +			   const char *keep_msg)
> 
> Nit: I'd have called this `write_pack_keep()`, but I see that we refer
> to the files as "lock" in `index_pack_lockfile(), too. Should we align
> those two functions and call this `write_pack_lockfile()`?

Ya, that's probably a bit better. Will rename to `write_pack_lockfile()`
in the next version.

> > +{
> > +	struct strbuf lock_path = STRBUF_INIT;
> > +	struct strbuf contents = STRBUF_INIT;
> > +	int ret = -1;
> > +	int fd;
> > +
> > +	strbuf_addf(&lock_path, "%s/pack/pack-%s.keep",
> > +		    source->path, pack_name);
> > +	strbuf_addf(&contents, "%s\n", keep_msg);
> > +
> > +	fd = safe_create_file_with_leading_directories(source->odb->repo,
> > +						       lock_path.buf);
> 
> The command has just written its packfile, so can't we assume that
> leading directories have already been created?

Yes, with how the function is used currently we can make that
assumption. The lockfile is always being written into the quarantine
directory after the packfile is written.

I would like to keep this function a bit more generic though because it
gets reused in the subsequent commit to write the lockfile directly into
the primary ODB during transaction commit and there I don't think we can
assume that the leading directories have already been committed. Using
`safe_create_file_with_leading_directories()` doesn't really cost us
anything either because it always tries to immediately create the file
first before creating the leading directories. Ultimately, this is just
a bit more defensive and I was hoping to reduce the changes overall.

I'll leave it as-is here unless others feel strongly it should be moved
to the last patch.

> > +	if (fd < 0)
> > +		goto out;
> > +
> > +	if (write_in_full(fd, contents.buf, contents.len) < 0) {
> > +		close(fd);
> > +		unlink(lock_path.buf);
> > +		goto out;
> > +	}
> > +
> > +	if (close(fd)) {
> 
> `if (close(fd) < 0)`, just to be consistent.

Will update in next version.

[snip]
> >  		status = finish_command(&child);
> >  		if (status) {
> > +			strbuf_release(&pack_name);
> >  			strbuf_addstr(err_msg, "index-pack abnormal exit");
> >  			return -1;
> >  		}
> >  
> > +		if (ret) {
> > +			strbuf_release(&pack_name);
> > +			strbuf_addstr(err_msg, "index-pack failed to report pack name");
> > +			return -1;
> > +		}
> > +
> > +		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
> > +			    (uintmax_t)getpid(), hostname);
> > +
> > +		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
> > +			strbuf_release(&pack_name);
> > +			strbuf_release(&keep_msg);
> > +			strbuf_addstr(err_msg, "lockfile write failed");
> > +			return -1;
> > +		}
> 
> ... then write the lockfile ourselves.
> 
> By the way, the error handling here starts to become unwieldy. Can we
> maybe have a common exit path that releases all resources for us?

That's fair. I'll clean this up a bit in the next version. Thanks.

> > +		ALLOC_GROW(transaction->pack_lockfiles,
> > +			   transaction->pack_lockfiles_nr + 1,
> > +			   transaction->pack_lockfiles_alloc);
> > +
> > +		/*
> > +		 * The lockfile filepath is expected to be the final location of
> > +		 * the ".keep" file after being migrated to the main ODB source.
> > +		 * This ensures the lockfile can be found and removed later
> > +		 * after the ODB transaction has been committed.
> > +		 */
> > +		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
> > +			    base->source->path, pack_name.buf);
> > +		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
> > +			register_tempfile(final_path.buf);
> 
> So here we retain the status quo, which is to record the lockfile in the
> final location even though it doesn't exist there yet. And that can lead
> to us pruning a pack lockfile from a different process.

Yup, that is exactly right. :)

-Justin

Re: [PATCH 3/3] object-file: write ".keep" lockfiles during transaction commit

[Justin Tobler]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> On Thu, Oct 15, 2026 at 09:29:01PM -0500, Justin Tobler wrote:
> > The "files" ODB backend writes a ".keep" lockfile into the transaction
> > quarantine directory via `odb_transaction_files_write_pack()`. When the
> > ODB transaction is committed, the ".keep" lockfile is migrated to the
> > primary ODB alongside its packfile. Writing the temporary lockfile in
> > this manner has a couple of downsides:
> > 
> >   - Failing to migrate the lockfile during transaction commit due to a
> >     concurrent process already acquiring the same lock still allows the
> >     rest of the transaction to be migrated into the primary ODB. This
> >     dirties the primary ODB with new objects even though no references
> >     were updated and the operation failed.
> 
> I'm not fully sure I understand this downside. Are you saying that we
> fail to migrate the lockfile, then continue migrating everything else
> only to then fail the whole transaction?
> 
> ... skipping ahead a bit...
> 
> Oh, I get it now. You propose to first acquire all lockfiles before
> migrating anything so that we cannot conflict with a concurrent process
> that aims to claim the same locks.
> 
> That will of course mean that we will fail the transaction for any
> concurrent process that happens to write the same packfiles. But based
> on what you claim that was already the case anyway, but now we notice
> early and thus won't commit parts of the transaction. Makes sense.

That is correct. Previously when migrating the quarantined objects, if a
matching ".keep" file was already present in the primary ODB,
`check_collision()` would detect the preexisting ".keep" lockfile and
bubble up the error to `tmp_objdir_migrate()`, but still let the
migration continue. This ultimately causes `odb_transaction_commit()` to
also return an error.

For git-receive-pack(1), an error during ODB transaction commit already
aborts the operation. The main change here is that by manually creating the
lockfiles during transaction commit, we can avoid the migration step
entirely if the lockfiles are not able to be created first.

I'll try rewording the commit message here to be a bit more clear about
this.

[snip]
> > diff --git a/object-file.c b/object-file.c
> > index ebe46d35de..39e6299be1 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1321,6 +1357,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> >  		delete_tempfile(&temp);
> >  		strbuf_release(&temp_path);
> >  
> > +		if (write_pack_locks(transaction))
> > +			return error(_("unable to lock packfiles"));
> 
> Okay, we'll now write lockfiles before migrating any of the data. So if
> any of the files already happens to exist, we'll bail out here. It would
> have been great to maybe also provide a proper failure message via errno
> for example, but okay.

Ya, good suggestion. I think I'll update the `write_pack_lockfile()`
helper in patch 2 to provide this additional context. There I can also
print the specific lockfile causing the issue.

[snip]
> > @@ -101,4 +100,28 @@ test_expect_success '.keep file is removed after push' '
> >  	test_path_is_missing "$keep"
> >  '
> >  
> > +test_expect_success '.keep file created outside of process is not removed' '
> > +	test_when_finished rm -rf keep.git &&
> > +	git init --bare keep.git &&
> > +
> > +	git -C keep.git config set receive.unpackLimit 0 &&
> > +
> > +	# Create a ".keep" lockfile matching the packfile to simulate a
> > +	# concurrent git-receive-pack(1) that was already written its lockfile.
> > +	test_hook -C keep.git pre-receive <<-\EOF &&
> > +	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
> > +	pack="${pack##*/}" &&
> > +	echo "baz" >"objects/pack/${pack%.pack}.keep"
> > +	EOF
> > +
> > +	test_commit bar &&
> > +	test_must_fail git push keep.git HEAD &&
> > +
> > +	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
> > +	# left in its place.
> > +	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
> > +	test_path_is_file "$keep" &&
> > +	test_grep "baz" "$keep"
> 
> Also, there should be no corresponding ".pack" file, right? Do we want
> to assert that, too?

That is correct. I'll add an assertion in the next version to make sure
the packfile is not migrated too.

Thanks for the review,
-Justin

[PATCH v2 0/4] object-file: fix ".keep" lockfile ownership in ODB transactions

[Justin Tobler]

Greetings,

This series addresses a bug report[1] regarding a git-receive-pack(1)
process deleting ".keep" lockfiles of with the same name but from
another concurrent git-receive-pack(1) process receiving an identical
packfile. Deleting the ".keep" lockfile for another process leaves its
newly written packfile vulnerable to deletion by another concurrent
housekeeping process.

The root of the problem here is that the temporary ".keep" lockfiles are
generated and registered in the object quarantine prior to being written
in the primary ODB. This means that even if the ODB transaction never
commits and exits early, the tempfile atexit(3) handler still runs and
tries to delete the lockfile at its expected final location. This
blindly deletes any file matching regardless of whether is was created
by the process.

The first three patches are some preparatory changes so that
`odb_transaction_files_write_pack()` is able to start explicitly
creating the lockfiles without relying on git-index-pack(1). Patch four
migrates lockfile generation to happen during ODB transaction commit to
ensure the lockfile is actually able to be written before registering it
as a tempfile.

Changes since V1:
  - Split out comment change into separate commit as suggested.
  - Improved error handling and logic flow in a couple of functions.
  - Renamed `write_pack_lock()` to `write_pack_lockfile()`.
  - Added better error messages and errno reporting to
    `write_pack_lockfile()` helper.
  - Adapted tests case to assert that packfile is not migrated.
  - Clarified some commit messages and fixed some typos.

Thanks,
-Justin

[1]: <932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com>

Justin Tobler (4):
  pack-write: make index-pack output comment hash agnostic
  pack-write: lift and expose index-pack report parsing
  object-file: explicitly write ".keep" lockfile
  object-file: write ".keep" lockfiles during transaction commit

 object-file.c              | 127 +++++++++++++++++++++++++++++--------
 pack-write.c               |  64 ++++++++++++++-----
 pack.h                     |  10 +++
 t/t5547-push-quarantine.sh |  30 ++++++++-
 4 files changed, 184 insertions(+), 47 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  b9197cbf3b pack-write: make index-pack output comment hash agnostic
1:  62521f3220 ! 2:  e27679ac16 pack-write: lift and expose index-pack report parsing
    @@ Commit message
         parse this report via `index_pack_lockfile()`, which provides the path
         to the temporary ".keep" file.
     
    -    In a subsequent commit, the "files" ODB transaction begins staging
    -    packfiles in a transaction without eagerly creating a ".keep" file and
    -    must instead read git-index-pack(1) "pack" output to get the packfile
    -    name. Extract the generic parsing logic for the git-index-pack(1) output
    -    into `read_index_pack_report()` in preparation accordingly.
    +    In a subsequent commit, the "files" ODB transaction begins creating the
    +    pack lockfiles during transaction commit instead of eagerly staging them
    +    in the transaction quarantine directory with the packfile. Extract the
    +    generic parsing logic for the git-index-pack(1) output into
    +    `read_index_pack_report()` in preparation for recording the names of
    +    newly written packfiles.
     
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
    @@ pack-write.c: void fixup_pack_header_footer(const struct git_hash_algo *hash_alg
     +	const int len = repo->hash_algo->hexsz + 6;
      	char packname[GIT_MAX_HEXSZ + 6];
     -	const int len = source->odb->repo->hash_algo->hexsz + 6;
    ++	int is_keep_ignored;
     +	const char *name;
    -+	int keep;
    ++
    ++	if (!is_keep)
    ++		is_keep = &is_keep_ignored;
      
      	/*
    --	 * The first thing we expect from index-pack's output
    --	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
    --	 * %40s is the newly created pack SHA1 name.  In the "keep"
    --	 * case, we need it to remove the corresponding .keep file
    --	 * later on.  If we don't get that then tough luck with it.
    -+	 * The first thing we expect from index-pack's output is
    -+	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
    -+	 * name of the packfile it has just written. The line is therefore five
    -+	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
    -+	 * "keep" prefix tells us that index-pack has also created the
    -+	 * corresponding ".keep" file.
    + 	 * The first thing we expect from index-pack's output is
    +@@ pack-write.c: char *index_pack_lockfile(struct odb_source *source, int ip_out,
    + 	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
    + 	 * "keep" prefix tells us that index-pack has also created the
    + 	 * corresponding ".keep" file.
    +-	 *
    +-	 * In the "keep" case, we need it to remove the corresponding ".keep"
    +-	 * file later on. If we don't get that then tough luck with it.
      	 */
     -	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
     -		const char *name;
    -+	if (read_in_full(ip_out, packname, len) != len || packname[len-1] != '\n')
    ++	if (read_in_full(ip_out, packname, len) != len || packname[len - 1] != '\n')
     +		return -1;
     +
    -+	packname[len-1] = 0;
    ++	packname[len - 1] = 0;
     +	if (skip_prefix(packname, "pack\t", &name))
    -+		keep = 0;
    ++		*is_keep = 0;
     +	else if (skip_prefix(packname, "keep\t", &name))
    -+		keep = 1;
    ++		*is_keep = 1;
     +	else
     +		return -1;
     +
     +	strbuf_addstr(pack_name, name);
    -+	if (is_keep)
    -+		*is_keep = keep;
     +
     +	return 0;
     +}
    -+
    + 
     +char *index_pack_lockfile(struct odb_source *source, int ip_out,
     +			  int *is_well_formed)
     +{
     +	struct strbuf pack_name = STRBUF_INIT;
     +	char *lockfile = NULL;
     +	int is_keep = 0;
    - 
    ++
     +	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
      		if (is_well_formed)
     -			*is_well_formed = 1;
    @@ pack-write.c: void fixup_pack_header_footer(const struct git_hash_algo *hash_alg
     -	return NULL;
     +		*is_well_formed = 1;
     +
    -+	/*
    -+	 * If there is no ".keep" file, then there is no lockfile to return.
    -+	 */
    -+	if (!is_keep) {
    -+		strbuf_release(&pack_name);
    -+		return NULL;
    -+	}
    -+
    -+	lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);
    ++	/* If there is no ".keep" file, then there is no lockfile to return. */
    ++	if (is_keep)
    ++		lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);
     +
     +	strbuf_release(&pack_name);
     +	return lockfile;
    @@ pack.h: off_t write_pack_header(struct hashfile *f, uint32_t);
     +/*
     + * Read the report that git-index-pack(1) writes to its standard output after
     + * the packfile is written. The hexadecimal name of the packfile is appended to
    -+ * `pack_name` and, if `is_keep` is non-NULL, set it to indicate whether a
    ++ * `pack_name` and, if `is_keep` is non-NULL, it is set to indicate whether a
     + * ".keep" file was written. Return 0 on success and a negative error code on
     + * failure.
     + */
2:  beebbc65d8 ! 3:  9236493740 object-file: explicitly write ".keep" lockfile
    @@ object-file.c: int read_loose_object(struct repository *repo,
      	return ret;
      }
      
    -+static int write_pack_lock(struct odb_source *source, const char *pack_name,
    -+			   const char *keep_msg)
    ++static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
    ++			       const char *keep_msg)
     +{
     +	struct strbuf lock_path = STRBUF_INIT;
     +	struct strbuf contents = STRBUF_INIT;
    @@ object-file.c: int read_loose_object(struct repository *repo,
     +
     +	fd = safe_create_file_with_leading_directories(source->odb->repo,
     +						       lock_path.buf);
    -+	if (fd < 0)
    ++	if (fd < 0) {
    ++		error_errno(_("unable to create '%s'"), lock_path.buf);
     +		goto out;
    ++	}
     +
     +	if (write_in_full(fd, contents.buf, contents.len) < 0) {
    ++		error_errno(_("unable to write '%s'"), lock_path.buf);
     +		close(fd);
     +		unlink(lock_path.buf);
     +		goto out;
     +	}
     +
    -+	if (close(fd)) {
    ++	if (close(fd) < 0) {
    ++		error_errno(_("unable to close '%s'"), lock_path.buf);
     +		unlink(lock_path.buf);
     +		goto out;
     +	}
    @@ object-file.c: int read_loose_object(struct repository *repo,
      {
      	struct odb_transaction_files *transaction =
     @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
    - 			return -1;
    + 		container_of(base, struct odb_transaction_files, base);
    + 	struct repository *repo = base->source->odb->repo;
    + 	struct child_process child = CHILD_PROCESS_INIT;
    ++	struct strbuf final_path = STRBUF_INIT;
    ++	struct strbuf pack_name = STRBUF_INIT;
    ++	struct strbuf keep_msg = STRBUF_INIT;
    + 	struct pack_header hdr;
    + 	const char *hdr_err;
    + 	int err_fd = opts->err_fd;
    ++	int ret = -1;
    + 	int status;
    + 
    + 	hdr_err = parse_pack_header(&hdr, pack_fd);
    +@@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
      		}
      	} else {
    -+		struct strbuf final_path = STRBUF_INIT;
    -+		struct strbuf pack_name = STRBUF_INIT;
    -+		struct strbuf keep_msg = STRBUF_INIT;
      		char hostname[HOST_NAME_MAX + 1];
     -		char *lockfile;
    -+		int ret;
      
      		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
      		push_header_arg(&child.args, &hdr);
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      
      		status = finish_command(&child);
      		if (status) {
    -+			strbuf_release(&pack_name);
      			strbuf_addstr(err_msg, "index-pack abnormal exit");
    - 			return -1;
    - 		}
    - 
    -+		if (ret) {
    -+			strbuf_release(&pack_name);
    -+			strbuf_addstr(err_msg, "index-pack failed to report pack name");
    -+			return -1;
    +-			return -1;
    ++			goto out;
     +		}
     +
    ++		if (ret) {
    ++			strbuf_addstr(err_msg, "index-pack failed to report pack name");
    ++			goto out;
    + 		}
    + 
     +		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
     +			    (uintmax_t)getpid(), hostname);
     +
    -+		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
    -+			strbuf_release(&pack_name);
    -+			strbuf_release(&keep_msg);
    ++		if (write_pack_lockfile(transaction->quarantine, pack_name.buf,
    ++					keep_msg.buf)) {
     +			strbuf_addstr(err_msg, "lockfile write failed");
    -+			return -1;
    ++			goto out;
     +		}
     +
     +		ALLOC_GROW(transaction->pack_lockfiles,
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      		odb_source_prepare(transaction->quarantine,
      				   ODB_PREPARE_FLUSH_CACHES);
     +
    -+		strbuf_release(&final_path);
    -+		strbuf_release(&pack_name);
    -+		strbuf_release(&keep_msg);
      	}
      
    - 	return 0;
    +-	return 0;
    ++	ret = 0;
    ++
    ++out:
    ++	strbuf_release(&final_path);
    ++	strbuf_release(&pack_name);
    ++	strbuf_release(&keep_msg);
    ++
    ++	return ret;
    + }
    + 
    + static int odb_transaction_files_finalize(struct odb_transaction *base)
3:  735b922865 ! 4:  c1a175e2b9 object-file: write ".keep" lockfiles during transaction commit
    @@ Commit message
         primary ODB alongside its packfile. Writing the temporary lockfile in
         this manner has a couple of downsides:
     
    -      - Failing to migrate the lockfile during transaction commit due to a
    -        concurrent process already acquiring the same lock still allows the
    -        rest of the transaction to be migrated into the primary ODB. This
    -        dirties the primary ODB with new objects even though no references
    -        were updated and the operation failed.
    +      - Migrating quarantined objects via `tmp_objdir_migrate()` doesn't
    +        immediately stop if a ".keep" lockfile already exists for the same
    +        packfile in the primary ODB and instead allows all the remaining
    +        objects to be migrated. So even though `odb_transaction_commit()`
    +        ultimately returns an error here, the objects are needlessly written
    +        to the primary ODB anyways.
     
           - Even more concerning, the ".keep" tempfile atexit(3) handler tries
             to remove any lockfile of the same name regardless of whether it was
    @@ object-file.c: struct odb_transaction_files {
     @@ object-file.c: int read_loose_object(struct repository *repo,
      }
      
    - static int write_pack_lock(struct odb_source *source, const char *pack_name,
    --			   const char *keep_msg)
    -+			   const char *keep_msg, struct tempfile **out)
    + static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
    +-			       const char *keep_msg)
    ++			       const char *keep_msg, struct tempfile **out)
      {
      	struct strbuf lock_path = STRBUF_INIT;
      	struct strbuf contents = STRBUF_INIT;
    -@@ object-file.c: static int write_pack_lock(struct odb_source *source, const char *pack_name,
    +@@ object-file.c: static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
      		goto out;
      	}
      
    @@ object-file.c: static int write_pack_lock(struct odb_source *source, const char
      	ret = 0;
      
      out:
    -@@ object-file.c: static int write_pack_lock(struct odb_source *source, const char *pack_name,
    +@@ object-file.c: static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
      	return ret;
      }
      
    -+static int write_pack_locks(struct odb_transaction_files *transaction)
    ++static int write_pack_lockfiles(struct odb_transaction_files *transaction)
     +{
     +	struct strbuf keep_msg = STRBUF_INIT;
     +	char hostname[HOST_NAME_MAX + 1];
    @@ object-file.c: static int write_pack_lock(struct odb_source *source, const char
     +	for (size_t i = 0; i < transaction->pack_names.nr; i++) {
     +		struct tempfile *lock;
     +
    -+		ret = write_pack_lock(transaction->base.source,
    -+				      transaction->pack_names.v[i],
    -+				      keep_msg.buf, &lock);
    ++		ret = write_pack_lockfile(transaction->base.source,
    ++					  transaction->pack_names.v[i],
    ++					  keep_msg.buf, &lock);
     +		if (ret < 0)
     +			break;
     +
    @@ object-file.c: static int odb_transaction_files_commit(struct odb_transaction *b
      		delete_tempfile(&temp);
      		strbuf_release(&temp_path);
      
    -+		if (write_pack_locks(transaction))
    ++		if (write_pack_lockfiles(transaction))
     +			return error(_("unable to lock packfiles"));
     +
      		/*
      		 * Make the object files visible in the primary ODB after their data is
      		 * fully durable.
    +@@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
    + 		container_of(base, struct odb_transaction_files, base);
    + 	struct repository *repo = base->source->odb->repo;
    + 	struct child_process child = CHILD_PROCESS_INIT;
    +-	struct strbuf final_path = STRBUF_INIT;
    + 	struct strbuf pack_name = STRBUF_INIT;
    +-	struct strbuf keep_msg = STRBUF_INIT;
    + 	struct pack_header hdr;
    + 	const char *hdr_err;
    + 	int err_fd = opts->err_fd;
     @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
      			return -1;
      		}
      	} else {
    --		struct strbuf final_path = STRBUF_INIT;
    - 		struct strbuf pack_name = STRBUF_INIT;
    --		struct strbuf keep_msg = STRBUF_INIT;
     -		char hostname[HOST_NAME_MAX + 1];
    - 		int ret;
    - 
    +-
      		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
      		push_header_arg(&child.args, &hdr);
      
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      			strvec_push(&child.args, "--show-resolving-progress");
      		if (err_fd)
     @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
    - 			return -1;
    + 			goto out;
      		}
      
     -		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
     -			    (uintmax_t)getpid(), hostname);
     -
    --		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
    --			strbuf_release(&pack_name);
    --			strbuf_release(&keep_msg);
    +-		if (write_pack_lockfile(transaction->quarantine, pack_name.buf,
    +-					keep_msg.buf)) {
     -			strbuf_addstr(err_msg, "lockfile write failed");
    --			return -1;
    +-			goto out;
     -		}
     -
     -		ALLOC_GROW(transaction->pack_lockfiles,
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      
      		odb_source_prepare(transaction->quarantine,
      				   ODB_PREPARE_FLUSH_CACHES);
    - 
    --		strbuf_release(&final_path);
    - 		strbuf_release(&pack_name);
    --		strbuf_release(&keep_msg);
    +-
      	}
      
    - 	return 0;
    + 	ret = 0;
    + 
    + out:
    +-	strbuf_release(&final_path);
    + 	strbuf_release(&pack_name);
    +-	strbuf_release(&keep_msg);
    + 
    + 	return ret;
    + }
     @@ object-file.c: static int odb_transaction_files_finalize(struct odb_transaction *base)
      		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
      
    @@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after pus
     +	git -C keep.git config set receive.unpackLimit 0 &&
     +
     +	# Create a ".keep" lockfile matching the packfile to simulate a
    -+	# concurrent git-receive-pack(1) that was already written its lockfile.
    ++	# concurrent git-receive-pack(1) that has already written its lockfile.
     +	test_hook -C keep.git pre-receive <<-\EOF &&
     +	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
     +	pack="${pack##*/}" &&
    @@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after pus
     +	# left in its place.
     +	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
     +	test_path_is_file "$keep" &&
    -+	test_grep "baz" "$keep"
    ++	test_grep "baz" "$keep" &&
    ++	test_path_is_missing "${keep%.keep}.pack"
     +'
     +
      test_done

base-commit: f57c9a73884400380bddd172acc88b569834f909
-- 
2.56.0.173.gf57c9a7388

[PATCH v2 1/4] pack-write: make index-pack output comment hash agnostic

[Justin Tobler]

The code comment in `index_pack_lockfile()` explains the expected format
of output from git-index-pack(1). As written it explicitly mentions
SHA1, but both the git-index-pack(1) output and the parsing are not
specific to SHA1 and also support SHA256.

Update the comment explanation to be hash agnostic accordingly.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 pack-write.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index 85674e4b72..b1311c9923 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -476,11 +476,15 @@ char *index_pack_lockfile(struct odb_source *source, int ip_out,
 	const int len = source->odb->repo->hash_algo->hexsz + 6;
 
 	/*
-	 * The first thing we expect from index-pack's output
-	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
-	 * %40s is the newly created pack SHA1 name.  In the "keep"
-	 * case, we need it to remove the corresponding .keep file
-	 * later on.  If we don't get that then tough luck with it.
+	 * The first thing we expect from index-pack's output is
+	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
+	 * name of the packfile it has just written. The line is therefore five
+	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
+	 * "keep" prefix tells us that index-pack has also created the
+	 * corresponding ".keep" file.
+	 *
+	 * In the "keep" case, we need it to remove the corresponding ".keep"
+	 * file later on. If we don't get that then tough luck with it.
 	 */
 	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
 		const char *name;
-- 
2.56.0.173.gf57c9a7388

[PATCH v2 2/4] pack-write: lift and expose index-pack report parsing

[Justin Tobler]

Once git-index-pack(1) finishes writing a packfile, it reports the
hexadecimal name of that packfile on stdout prefixed with either "pack"
or "keep" depending on whether a ".keep" file was also created alongside
it. Callers that are required to manage the lifecycle of ".keep" files
parse this report via `index_pack_lockfile()`, which provides the path
to the temporary ".keep" file.

In a subsequent commit, the "files" ODB transaction begins creating the
pack lockfiles during transaction commit instead of eagerly staging them
in the transaction quarantine directory with the packfile. Extract the
generic parsing logic for the git-index-pack(1) output into
`read_index_pack_report()` in preparation for recording the names of
newly written packfiles.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 pack-write.c | 56 ++++++++++++++++++++++++++++++++++++++--------------
 pack.h       | 10 ++++++++++
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index b1311c9923..2ed33448ba 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -469,11 +469,16 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(struct odb_source *source, int ip_out,
-			  int *is_well_formed)
+int read_index_pack_report(struct repository *repo, int ip_out,
+			   struct strbuf *pack_name, int *is_keep)
 {
+	const int len = repo->hash_algo->hexsz + 6;
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = source->odb->repo->hash_algo->hexsz + 6;
+	int is_keep_ignored;
+	const char *name;
+
+	if (!is_keep)
+		is_keep = &is_keep_ignored;
 
 	/*
 	 * The first thing we expect from index-pack's output is
@@ -482,24 +487,45 @@ char *index_pack_lockfile(struct odb_source *source, int ip_out,
 	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
 	 * "keep" prefix tells us that index-pack has also created the
 	 * corresponding ".keep" file.
-	 *
-	 * In the "keep" case, we need it to remove the corresponding ".keep"
-	 * file later on. If we don't get that then tough luck with it.
 	 */
-	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
-		const char *name;
+	if (read_in_full(ip_out, packname, len) != len || packname[len - 1] != '\n')
+		return -1;
+
+	packname[len - 1] = 0;
+	if (skip_prefix(packname, "pack\t", &name))
+		*is_keep = 0;
+	else if (skip_prefix(packname, "keep\t", &name))
+		*is_keep = 1;
+	else
+		return -1;
+
+	strbuf_addstr(pack_name, name);
+
+	return 0;
+}
 
+char *index_pack_lockfile(struct odb_source *source, int ip_out,
+			  int *is_well_formed)
+{
+	struct strbuf pack_name = STRBUF_INIT;
+	char *lockfile = NULL;
+	int is_keep = 0;
+
+	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
 		if (is_well_formed)
-			*is_well_formed = 1;
-		packname[len-1] = 0;
-		if (skip_prefix(packname, "keep\t", &name))
-			return xstrfmt("%s/pack/pack-%s.keep",
-				       source->path, name);
+			*is_well_formed = 0;
 		return NULL;
 	}
+
 	if (is_well_formed)
-		*is_well_formed = 0;
-	return NULL;
+		*is_well_formed = 1;
+
+	/* If there is no ".keep" file, then there is no lockfile to return. */
+	if (is_keep)
+		lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);
+
+	strbuf_release(&pack_name);
+	return lockfile;
 }
 
 /*
diff --git a/pack.h b/pack.h
index ada506b5c5..2f2ce97c11 100644
--- a/pack.h
+++ b/pack.h
@@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
+
+/*
+ * Read the report that git-index-pack(1) writes to its standard output after
+ * the packfile is written. The hexadecimal name of the packfile is appended to
+ * `pack_name` and, if `is_keep` is non-NULL, it is set to indicate whether a
+ * ".keep" file was written. Return 0 on success and a negative error code on
+ * failure.
+ */
+int read_index_pack_report(struct repository *repo, int ip_out,
+			   struct strbuf *pack_name, int *is_keep);
 char *index_pack_lockfile(struct odb_source *source, int fd,
 			  int *is_well_formed);
 
-- 
2.56.0.173.gf57c9a7388

[PATCH v2 3/4] object-file: explicitly write ".keep" lockfile

[Justin Tobler]

The `odb_transaction_write_pack()` interface for the "files" backend
relies on git-index-pack(1) to write the specified packfile into an ODB
transaction as a packfile. To prevent a concurrent housekeeping
operation from deleting the newly written packfile prior to the
references being updated, a ".keep" lockfile is also generated by
git-index-pack(1) via the `--keep` option. Consequently, the ".keep"
lockfile is written into the ODB transaction alongside the packfile and
later migrated together to the primary ODB during transaction commit.

In a subsequent commit, writing the lockfile is deferred until
transaction commit and thus cannot rely on git-index-pack(1) to write
it. In preparation for this, update `odb_transaction_files_write_pack()`
to explicitly generate the same ".keep" lockfile itself.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c | 108 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 86 insertions(+), 22 deletions(-)

diff --git a/object-file.c b/object-file.c
index 210984f825..07a61c610a 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1256,6 +1256,47 @@ int read_loose_object(struct repository *repo,
 	return ret;
 }
 
+static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
+			       const char *keep_msg)
+{
+	struct strbuf lock_path = STRBUF_INIT;
+	struct strbuf contents = STRBUF_INIT;
+	int ret = -1;
+	int fd;
+
+	strbuf_addf(&lock_path, "%s/pack/pack-%s.keep",
+		    source->path, pack_name);
+	strbuf_addf(&contents, "%s\n", keep_msg);
+
+	fd = safe_create_file_with_leading_directories(source->odb->repo,
+						       lock_path.buf);
+	if (fd < 0) {
+		error_errno(_("unable to create '%s'"), lock_path.buf);
+		goto out;
+	}
+
+	if (write_in_full(fd, contents.buf, contents.len) < 0) {
+		error_errno(_("unable to write '%s'"), lock_path.buf);
+		close(fd);
+		unlink(lock_path.buf);
+		goto out;
+	}
+
+	if (close(fd) < 0) {
+		error_errno(_("unable to close '%s'"), lock_path.buf);
+		unlink(lock_path.buf);
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	strbuf_release(&lock_path);
+	strbuf_release(&contents);
+
+	return ret;
+}
+
 static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
@@ -1348,9 +1389,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 		container_of(base, struct odb_transaction_files, base);
 	struct repository *repo = base->source->odb->repo;
 	struct child_process child = CHILD_PROCESS_INIT;
+	struct strbuf final_path = STRBUF_INIT;
+	struct strbuf pack_name = STRBUF_INIT;
+	struct strbuf keep_msg = STRBUF_INIT;
 	struct pack_header hdr;
 	const char *hdr_err;
 	int err_fd = opts->err_fd;
+	int ret = -1;
 	int status;
 
 	hdr_err = parse_pack_header(&hdr, pack_fd);
@@ -1390,17 +1435,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 		}
 	} else {
 		char hostname[HOST_NAME_MAX + 1];
-		char *lockfile;
 
 		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
 		push_header_arg(&child.args, &hdr);
 
 		if (xgethostname(hostname, sizeof(hostname)))
 			xsnprintf(hostname, sizeof(hostname), "localhost");
-		strvec_pushf(&child.args,
-			     "--keep=receive-pack %"PRIuMAX" on %s",
-			     (uintmax_t)getpid(),
-			     hostname);
 
 		if (!opts->quiet && err_fd)
 			strvec_push(&child.args, "--show-resolving-progress");
@@ -1424,34 +1464,58 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 
-		/*
-		 * The lockfile filepath is expected to be the final location of
-		 * the ".keep" file after being migrated to the main ODB source.
-		 * This ensures the lockfile can be found and removed later
-		 * after the ODB transaction has been committed.
-		 */
-		lockfile = index_pack_lockfile(base->source, child.out, NULL);
-		if (lockfile) {
-			ALLOC_GROW(transaction->pack_lockfiles,
-				   transaction->pack_lockfiles_nr + 1,
-				   transaction->pack_lockfiles_alloc);
-			transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
-				register_tempfile(lockfile);
-			free(lockfile);
-		}
+		ret = read_index_pack_report(repo, child.out, &pack_name, NULL);
+
 		close(child.out);
 
 		status = finish_command(&child);
 		if (status) {
 			strbuf_addstr(err_msg, "index-pack abnormal exit");
-			return -1;
+			goto out;
+		}
+
+		if (ret) {
+			strbuf_addstr(err_msg, "index-pack failed to report pack name");
+			goto out;
 		}
 
+		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
+			    (uintmax_t)getpid(), hostname);
+
+		if (write_pack_lockfile(transaction->quarantine, pack_name.buf,
+					keep_msg.buf)) {
+			strbuf_addstr(err_msg, "lockfile write failed");
+			goto out;
+		}
+
+		ALLOC_GROW(transaction->pack_lockfiles,
+			   transaction->pack_lockfiles_nr + 1,
+			   transaction->pack_lockfiles_alloc);
+
+		/*
+		 * The lockfile filepath is expected to be the final location of
+		 * the ".keep" file after being migrated to the main ODB source.
+		 * This ensures the lockfile can be found and removed later
+		 * after the ODB transaction has been committed.
+		 */
+		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
+			    base->source->path, pack_name.buf);
+		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
+			register_tempfile(final_path.buf);
+
 		odb_source_prepare(transaction->quarantine,
 				   ODB_PREPARE_FLUSH_CACHES);
+
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	strbuf_release(&final_path);
+	strbuf_release(&pack_name);
+	strbuf_release(&keep_msg);
+
+	return ret;
 }
 
 static int odb_transaction_files_finalize(struct odb_transaction *base)
-- 
2.56.0.173.gf57c9a7388

[PATCH v2 4/4] object-file: write ".keep" lockfiles during transaction commit

[Justin Tobler]

The "files" ODB backend writes a ".keep" lockfile into the transaction
quarantine directory via `odb_transaction_files_write_pack()`. When the
ODB transaction is committed, the ".keep" lockfile is migrated to the
primary ODB alongside its packfile. Writing the temporary lockfile in
this manner has a couple of downsides:

  - Migrating quarantined objects via `tmp_objdir_migrate()` doesn't
    immediately stop if a ".keep" lockfile already exists for the same
    packfile in the primary ODB and instead allows all the remaining
    objects to be migrated. So even though `odb_transaction_commit()`
    ultimately returns an error here, the objects are needlessly written
    to the primary ODB anyways.

  - Even more concerning, the ".keep" tempfile atexit(3) handler tries
    to remove any lockfile of the same name regardless of whether it was
    created by the same process. If the ".keep" lockfile of the same
    name for a concurrent process gets deleted prematurely, its packfile
    becomes at risk of being deleted by concurrent housekeeping.

Stop creating/registering the temporary lockfiles when writing the
packfile to the transaction and instead do so when committing the
transaction. This ensures the lockfile is properly acquired prior to
migrating objects and that only the owning process can delete its own
lockfiles.

Reported-by: Qin ShiCheng <qeesung@live.com>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 object-file.c              | 77 +++++++++++++++++++++-----------------
 t/t5547-push-quarantine.sh | 30 +++++++++++++--
 2 files changed, 70 insertions(+), 37 deletions(-)

diff --git a/object-file.c b/object-file.c
index 07a61c610a..b7465adf04 100644
--- a/object-file.c
+++ b/object-file.c
@@ -492,6 +492,7 @@ struct odb_transaction_files {
 	struct transaction_packfile packfile;
 	const char *prefix;
 
+	struct strvec pack_names;
 	struct tempfile **pack_lockfiles;
 	size_t pack_lockfiles_nr;
 	size_t pack_lockfiles_alloc;
@@ -1257,7 +1258,7 @@ int read_loose_object(struct repository *repo,
 }
 
 static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
-			       const char *keep_msg)
+			       const char *keep_msg, struct tempfile **out)
 {
 	struct strbuf lock_path = STRBUF_INIT;
 	struct strbuf contents = STRBUF_INIT;
@@ -1288,6 +1289,7 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
 		goto out;
 	}
 
+	*out = register_tempfile(lock_path.buf);
 	ret = 0;
 
 out:
@@ -1297,6 +1299,40 @@ static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
 	return ret;
 }
 
+static int write_pack_lockfiles(struct odb_transaction_files *transaction)
+{
+	struct strbuf keep_msg = STRBUF_INIT;
+	char hostname[HOST_NAME_MAX + 1];
+	int ret = 0;
+
+	if (!transaction->pack_names.nr)
+		return 0;
+
+	if (xgethostname(hostname, sizeof(hostname)))
+		xsnprintf(hostname, sizeof(hostname), "localhost");
+	strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
+		    (uintmax_t)getpid(), hostname);
+
+	for (size_t i = 0; i < transaction->pack_names.nr; i++) {
+		struct tempfile *lock;
+
+		ret = write_pack_lockfile(transaction->base.source,
+					  transaction->pack_names.v[i],
+					  keep_msg.buf, &lock);
+		if (ret < 0)
+			break;
+
+		ALLOC_GROW(transaction->pack_lockfiles,
+			   transaction->pack_lockfiles_nr + 1,
+			   transaction->pack_lockfiles_alloc);
+		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = lock;
+	}
+
+	strbuf_release(&keep_msg);
+
+	return ret;
+}
+
 static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
@@ -1325,6 +1361,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 		delete_tempfile(&temp);
 		strbuf_release(&temp_path);
 
+		if (write_pack_lockfiles(transaction))
+			return error(_("unable to lock packfiles"));
+
 		/*
 		 * Make the object files visible in the primary ODB after their data is
 		 * fully durable.
@@ -1389,9 +1428,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 		container_of(base, struct odb_transaction_files, base);
 	struct repository *repo = base->source->odb->repo;
 	struct child_process child = CHILD_PROCESS_INIT;
-	struct strbuf final_path = STRBUF_INIT;
 	struct strbuf pack_name = STRBUF_INIT;
-	struct strbuf keep_msg = STRBUF_INIT;
 	struct pack_header hdr;
 	const char *hdr_err;
 	int err_fd = opts->err_fd;
@@ -1434,14 +1471,9 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			return -1;
 		}
 	} else {
-		char hostname[HOST_NAME_MAX + 1];
-
 		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
 		push_header_arg(&child.args, &hdr);
 
-		if (xgethostname(hostname, sizeof(hostname)))
-			xsnprintf(hostname, sizeof(hostname), "localhost");
-
 		if (!opts->quiet && err_fd)
 			strvec_push(&child.args, "--show-resolving-progress");
 		if (err_fd)
@@ -1479,41 +1511,16 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
 			goto out;
 		}
 
-		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
-			    (uintmax_t)getpid(), hostname);
-
-		if (write_pack_lockfile(transaction->quarantine, pack_name.buf,
-					keep_msg.buf)) {
-			strbuf_addstr(err_msg, "lockfile write failed");
-			goto out;
-		}
-
-		ALLOC_GROW(transaction->pack_lockfiles,
-			   transaction->pack_lockfiles_nr + 1,
-			   transaction->pack_lockfiles_alloc);
-
-		/*
-		 * The lockfile filepath is expected to be the final location of
-		 * the ".keep" file after being migrated to the main ODB source.
-		 * This ensures the lockfile can be found and removed later
-		 * after the ODB transaction has been committed.
-		 */
-		strbuf_addf(&final_path, "%s/pack/pack-%s.keep",
-			    base->source->path, pack_name.buf);
-		transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
-			register_tempfile(final_path.buf);
+		strvec_push(&transaction->pack_names, pack_name.buf);
 
 		odb_source_prepare(transaction->quarantine,
 				   ODB_PREPARE_FLUSH_CACHES);
-
 	}
 
 	ret = 0;
 
 out:
-	strbuf_release(&final_path);
 	strbuf_release(&pack_name);
-	strbuf_release(&keep_msg);
 
 	return ret;
 }
@@ -1528,6 +1535,7 @@ static int odb_transaction_files_finalize(struct odb_transaction *base)
 		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
 
 	free(transaction->pack_lockfiles);
+	strvec_clear(&transaction->pack_names);
 
 	return ret;
 }
@@ -1560,6 +1568,7 @@ int odb_transaction_files_begin(struct odb_source *source,
 	transaction->base.write_pack = odb_transaction_files_write_pack;
 	transaction->base.env = odb_transaction_files_env;
 	transaction->flags = flags;
+	strvec_init(&transaction->pack_names);
 
 	transaction->prefix = "bulk-fsync";
 	if (flags & ODB_TRANSACTION_RECEIVE) {
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1b7097179e..6c14dfc42b 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -77,10 +77,9 @@ test_expect_success '.keep file is removed after push' '
 	git -C keep.git config set receive.unpackLimit 0 &&
 
 	# While incoming objects are still quarantined, validate that the
-	# ".keep" lockfile is present in the quarantine directory.
+	# ".keep" lockfile is not present in the quarantine directory.
 	test_hook -C keep.git pre-receive <<-\EOF &&
-	keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" &&
-	test -f "$keep"
+	test ! -e "$GIT_QUARANTINE_PATH"/pack/pack-*.keep
 	EOF
 
 	# After quarantined objects are migrated, validate that the ".keep"
@@ -101,4 +100,29 @@ test_expect_success '.keep file is removed after push' '
 	test_path_is_missing "$keep"
 '
 
+test_expect_success '.keep file created outside of process is not removed' '
+	test_when_finished rm -rf keep.git &&
+	git init --bare keep.git &&
+
+	git -C keep.git config set receive.unpackLimit 0 &&
+
+	# Create a ".keep" lockfile matching the packfile to simulate a
+	# concurrent git-receive-pack(1) that has already written its lockfile.
+	test_hook -C keep.git pre-receive <<-\EOF &&
+	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
+	pack="${pack##*/}" &&
+	echo "baz" >"objects/pack/${pack%.pack}.keep"
+	EOF
+
+	test_commit bar &&
+	test_must_fail git push keep.git HEAD &&
+
+	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
+	# left in its place.
+	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
+	test_path_is_file "$keep" &&
+	test_grep "baz" "$keep" &&
+	test_path_is_missing "${keep%.keep}.pack"
+'
+
 test_done
-- 
2.56.0.173.gf57c9a7388