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

[Justin Tobler <jltobler@gmail.com>]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> 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()`?

Ya, that's probably a bit better. Will rename to `write_pack_lockfile()`
in the next version.

> > +{
> > +	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?

Yes, with how the function is used currently we can make that
assumption. The lockfile is always being written into the quarantine
directory after the packfile is written.

I would like to keep this function a bit more generic though because it
gets reused in the subsequent commit to write the lockfile directly into
the primary ODB during transaction commit and there I don't think we can
assume that the leading directories have already been committed. Using
`safe_create_file_with_leading_directories()` doesn't really cost us
anything either because it always tries to immediately create the file
first before creating the leading directories. Ultimately, this is just
a bit more defensive and I was hoping to reduce the changes overall.

I'll leave it as-is here unless others feel strongly it should be moved
to the last patch.

> > +	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.

Will update in next version.

[snip]
> >  		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?

That's fair. I'll clean this up a bit in the next version. Thanks.

> > +		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.

Yup, that is exactly right. :)

-Justin