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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 20, 2026 at 08:28:59AM -0500, Justin Tobler wrote:
> diff --git a/object-file.c b/object-file.c
> index 210984f825..ad55c01198 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1256,6 +1256,45 @@ 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 = 0;
> +	int fd;

Hum, not quite what I meant :P I'd leave the variable explicitly
uninitialized and then set it on all exit paths. Like that, the compiler
(or at least most common compilers) would warn for all code paths where
we don't set the error code. So:

        int ret;

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

and then:

        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 =

Thanks!

Patrick