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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 16, 2026 at 04:37:15PM -0500, Justin Tobler wrote:
> 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);

Right, `safe_create_file_with_leading_directories()` guarantees that
open(3p) was the last function that was called when returning, so we
know that `errno` would be properly set.

> +		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;
> +	}

Nit: now that all error paths call `error_errno()` I would have
preferred to keep `ret` uninitialized and then assign the return value
of `error_errno()` to it. But that's a minor detail only that does not
warrant a reroll.

> @@ -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);
> +

Nit: empty newline before curly brace.

>  	}
>  
> -	return 0;
> +	ret = 0;
> +
> +out:
> +	strbuf_release(&final_path);
> +	strbuf_release(&pack_name);
> +	strbuf_release(&keep_msg);
> +
> +	return ret;
>  }

This reads a lot better now with the unified exit path, thanks!

Patrick