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

[Justin Tobler <jltobler@gmail.com>]

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 V2:
  - In `write_pack_lockfile()` the error is now explicitly assigned to
    `ret`.
  - Removed an added newline that was not needed.

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              | 126 +++++++++++++++++++++++++++++--------
 pack-write.c               |  64 ++++++++++++++-----
 pack.h                     |  10 +++
 t/t5547-push-quarantine.sh |  30 ++++++++-
 4 files changed, 183 insertions(+), 47 deletions(-)

Range-diff against v2:
1:  b9197cbf3b = 1:  b9197cbf3b pack-write: make index-pack output comment hash agnostic
2:  e27679ac16 = 2:  e27679ac16 pack-write: lift and expose index-pack report parsing
3:  9236493740 ! 3:  e7f72aa53d object-file: explicitly write ".keep" lockfile
    @@ object-file.c: int read_loose_object(struct repository *repo,
     +{
     +	struct strbuf lock_path = STRBUF_INIT;
     +	struct strbuf contents = STRBUF_INIT;
    -+	int ret = -1;
    ++	int ret = 0;
     +	int fd;
     +
     +	strbuf_addf(&lock_path, "%s/pack/pack-%s.keep",
    @@ 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) {
    -+		error_errno(_("unable to create '%s'"), lock_path.buf);
    ++		ret = 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);
    ++		ret = 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);
    ++		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);
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      			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);
     +
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
     +
      		odb_source_prepare(transaction->quarantine,
      				   ODB_PREPARE_FLUSH_CACHES);
    -+
      	}
      
     -	return 0;
4:  c1a175e2b9 ! 4:  046ff4741e object-file: write ".keep" lockfiles during transaction commit
    @@ object-file.c: static int write_pack_lockfile(struct odb_source *source, const c
      	}
      
     +	*out = register_tempfile(lock_path.buf);
    - 	ret = 0;
    - 
    ++
      out:
    + 	strbuf_release(&lock_path);
    + 	strbuf_release(&contents);
     @@ object-file.c: static int write_pack_lockfile(struct odb_source *source, const char *pack_name,
      	return ret;
      }
    @@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transactio
      
      		odb_source_prepare(transaction->quarantine,
      				   ODB_PREPARE_FLUSH_CACHES);
    --
    - 	}
    - 
    +@@ object-file.c: static int odb_transaction_files_write_pack(struct odb_transaction *base,
      	ret = 0;
      
      out:

base-commit: f57c9a73884400380bddd172acc88b569834f909
-- 
2.56.0.173.gf57c9a7388