Re: [PATCH 3/3] object-file: write ".keep" lockfiles during transaction commit

[Justin Tobler <jltobler@gmail.com>]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> On Thu, Oct 15, 2026 at 09:29:01PM -0500, Justin Tobler wrote:
> > The "files" ODB backend writes a ".keep" lockfile into the transaction
> > quarantine directory via `odb_transaction_files_write_pack()`. When the
> > ODB transaction is committed, the ".keep" lockfile is migrated to the
> > primary ODB alongside its packfile. Writing the temporary lockfile in
> > this manner has a couple of downsides:
> > 
> >   - Failing to migrate the lockfile during transaction commit due to a
> >     concurrent process already acquiring the same lock still allows the
> >     rest of the transaction to be migrated into the primary ODB. This
> >     dirties the primary ODB with new objects even though no references
> >     were updated and the operation failed.
> 
> I'm not fully sure I understand this downside. Are you saying that we
> fail to migrate the lockfile, then continue migrating everything else
> only to then fail the whole transaction?
> 
> ... skipping ahead a bit...
> 
> Oh, I get it now. You propose to first acquire all lockfiles before
> migrating anything so that we cannot conflict with a concurrent process
> that aims to claim the same locks.
> 
> That will of course mean that we will fail the transaction for any
> concurrent process that happens to write the same packfiles. But based
> on what you claim that was already the case anyway, but now we notice
> early and thus won't commit parts of the transaction. Makes sense.

That is correct. Previously when migrating the quarantined objects, if a
matching ".keep" file was already present in the primary ODB,
`check_collision()` would detect the preexisting ".keep" lockfile and
bubble up the error to `tmp_objdir_migrate()`, but still let the
migration continue. This ultimately causes `odb_transaction_commit()` to
also return an error.

For git-receive-pack(1), an error during ODB transaction commit already
aborts the operation. The main change here is that by manually creating the
lockfiles during transaction commit, we can avoid the migration step
entirely if the lockfiles are not able to be created first.

I'll try rewording the commit message here to be a bit more clear about
this.

[snip]
> > diff --git a/object-file.c b/object-file.c
> > index ebe46d35de..39e6299be1 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1321,6 +1357,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> >  		delete_tempfile(&temp);
> >  		strbuf_release(&temp_path);
> >  
> > +		if (write_pack_locks(transaction))
> > +			return error(_("unable to lock packfiles"));
> 
> Okay, we'll now write lockfiles before migrating any of the data. So if
> any of the files already happens to exist, we'll bail out here. It would
> have been great to maybe also provide a proper failure message via errno
> for example, but okay.

Ya, good suggestion. I think I'll update the `write_pack_lockfile()`
helper in patch 2 to provide this additional context. There I can also
print the specific lockfile causing the issue.

[snip]
> > @@ -101,4 +100,28 @@ test_expect_success '.keep file is removed after push' '
> >  	test_path_is_missing "$keep"
> >  '
> >  
> > +test_expect_success '.keep file created outside of process is not removed' '
> > +	test_when_finished rm -rf keep.git &&
> > +	git init --bare keep.git &&
> > +
> > +	git -C keep.git config set receive.unpackLimit 0 &&
> > +
> > +	# Create a ".keep" lockfile matching the packfile to simulate a
> > +	# concurrent git-receive-pack(1) that was already written its lockfile.
> > +	test_hook -C keep.git pre-receive <<-\EOF &&
> > +	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
> > +	pack="${pack##*/}" &&
> > +	echo "baz" >"objects/pack/${pack%.pack}.keep"
> > +	EOF
> > +
> > +	test_commit bar &&
> > +	test_must_fail git push keep.git HEAD &&
> > +
> > +	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
> > +	# left in its place.
> > +	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
> > +	test_path_is_file "$keep" &&
> > +	test_grep "baz" "$keep"
> 
> Also, there should be no corresponding ".pack" file, right? Do we want
> to assert that, too?

That is correct. I'll add an assertion in the next version to make sure
the packfile is not migrated too.

Thanks for the review,
-Justin