From 5e106cb22b6b0ecb2a7c45cf7a82fbfeb4ee2960 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:25 -0500 Subject: [PATCH 1/4] pack-write: make index-pack output comment hash agnostic 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 Signed-off-by: Junio C Hamano --- 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\n" or "keep\t\n", where 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; From 21bb64feefa3ca4a5982075781421fc4f98d92b6 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:26 -0500 Subject: [PATCH 2/4] pack-write: lift and expose index-pack report parsing 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 Signed-off-by: Junio C Hamano --- 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); From dfb7f49776848c90ee825be20afa1944b424c971 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:27 -0500 Subject: [PATCH 3/4] object-file: explicitly write ".keep" lockfile 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 Signed-off-by: Junio C Hamano --- object-file.c | 107 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 85 insertions(+), 22 deletions(-) diff --git a/object-file.c b/object-file.c index 210984f825..74a31760bc 100644 --- a/object-file.c +++ b/object-file.c @@ -1256,6 +1256,47 @@ out: 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; + 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) { + ret = error_errno(_("unable to create '%s'"), lock_path.buf); + goto out; + } + + if (write_in_full(fd, contents.buf, contents.len) < 0) { + ret = error_errno(_("unable to write '%s'"), lock_path.buf); + close(fd); + unlink(lock_path.buf); + goto out; + } + + if (close(fd) < 0) { + ret = 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,57 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, return -1; } + 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"); + 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. */ - 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); - } - close(child.out); - - status = finish_command(&child); - if (status) { - strbuf_addstr(err_msg, "index-pack abnormal exit"); - return -1; - } + 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) From b07099fff9401316b0e55c43f8dde46ddf0e7423 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 21 Oct 2026 13:25:28 -0500 Subject: [PATCH 4/4] object-file: write ".keep" lockfiles during transaction commit 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 Signed-off-by: Justin Tobler Signed-off-by: Junio C Hamano --- object-file.c | 76 +++++++++++++++++++++----------------- t/t5547-push-quarantine.sh | 30 +++++++++++++-- 2 files changed, 70 insertions(+), 36 deletions(-) diff --git a/object-file.c b/object-file.c index 74a31760bc..e67c0865a2 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 @@ 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; @@ -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 @@ out: 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,29 +1511,7 @@ 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); @@ -1510,9 +1520,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, ret = 0; out: - strbuf_release(&final_path); strbuf_release(&pack_name); - strbuf_release(&keep_msg); return ret; } @@ -1527,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; } @@ -1559,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