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

[Patrick Steinhardt <ps@pks.im>]

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