Re: [PATCH v3 1/9] builtin/receive-pack: properly clean up keep files

On 26/08/12 08:07AM, Patrick Steinhardt wrote:
> On Tue, Aug 11, 2026 at 12:54:07PM -0500, Justin Tobler wrote:
> > When git-receive-pack(1) stores an incoming packfile with
> > git-index-pack(1), a ".keep" file is written alongside it to hold the
> > pack in place until the references have been updated, and is removed
> > afterwards. The path used to remove it is derived via
> > `index_pack_lockfile()` from the repository's primary object directory.
> > 
> > In bdee7b3013 (builtin/receive-pack: stage incoming objects via ODB
> > transactions, 2026-07-10), git-receive-pack(1) started using the ODB
> > transaction interfaces instead of managing a temporary directory
> > directly. When starting an ODB transaction, the sources list is
> > reordered to insert the newly created transaction source first as the
> > primary to ensure writes are routed to it accordingly.
> > 
> > Prior to using ODB transactions, git-receive-pack(1) would only set the
> > temporary directory as the primary source for the child
> > git-index-pack(1) and git-unpack-objects(1) processes it spawned and the
> > parent process would set the temporary directory set as an alternate
> > only. By using ODB transactions, the ODB source list is also reordered
> > for the parent process which results in `index_pack_lockfile()` deriving
> > the ".keep" path relative to the temporary directory instead the actual
> 
> Nit: s/instead/& of/

Will fix.

> > main ODB source path. Consequently, this prevents the ".keep" file from
> > being properly removed after being migrated into the main ODB source
> > post-commit.
> 
> Hm. Are the temporary packs written into the transaction-managed tempdir
> now, or do they still end up in the main object directory?

The packfile and associated ".keep" lockfiles are both initially written
into the temporary directory managed by the ODB transaction. On
transaction commit, they are then both migrated to the main ODB.

When registering the keep tempfile, we need to record the future
post-commit location of the keep file that way it can be removed when
`odb_transaction_finalize()` is invoked. This matches the original
behavior prior to ODB transaction being introduced in
git-receive-pack(1).

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 86933d8d7e..d74b787148 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -2412,7 +2412,13 @@ static const char *unpack(int err_fd, struct shallow_info *si,
> >  		if (status)
> >  			return "index-pack fork failed";
> >  
> > -		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
> > +		/*
> > +		 * 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(transaction->source, child.out, NULL);
> >  		if (lockfile) {
> >  			pack_lockfile = register_tempfile(lockfile);
> >  			free(lockfile);
> 
> Okay. So previously, we wrote the ".keep" file into the main repository,
> whereas now we write it into the temporary object directory? Is the
> packfile itself also written in there?

Not quite, both the packfile and keep file were written to the temporary
directory and continue to do so.

Prior to bdee7b3013 (builtin/receive-pack: stage incoming objects via
ODB transactions, 2026-07-10), the ".keep" files were also being written
to the quarantine directory and migrated alongside the packfiles. The
main git-receive-pack(1) process always kept the primary ODB as the
first entry in the source list though ensuring that the "filename"
registered for keep tempfile was the final location. With ODB
transactions though, the source list order _does_ get changed and
resulted in the keep tempfile not knowing about its final location.
Consequently, it is no longer cleaned up.

> What I'm wondering is why we even need a ".keep" file at all anymore if
> we're not storing it in the main object directory. It wouldn't help us
> to avoid the race, because after committing the transaction the ".keep"
> file would remain in the temporary directory, whereas the packfile would
> have been migrated to the main object directory. So it doesn't have a
> ".keep" file at that point, and neither have references been updated to
> point to the new objects yet.

The ".keep" file does end up in the main ODB alongside the packfile when
the transaction is committed. The main problem here is that it is not
being cleaned up because the post-migration path does not match what the
registered tempfile tracks.

> So I wonder whether instead, we'd have to:
> 
>   1. Start the transaction, creating the temporary object directory.
>   
>   2. Write the packfile into the temporary object directory, but don't
>      create a ".keep" file.
> 
>   3. At commit time, first write a ".keep" file in the main object
>      directory and then migrate the packfile over.
> 
>   4. At finalization time, prune the ".keep" file from the main object
>      directory.
> 
> That would retain the current properties of the system, but as far as I
> can see this is not what we're doing here.

With this patch, this is effectly what we are doing already. The main
difference is that we are creating the ".keep" file alongside the
packfile via git-index-pack(1) and migrating both when
`odb_transaction_commit()` is invoked.

We could stop relying on git-index-pack(1) to generate the ".keep" file
and instead generate it ourselves during the commit phase as you
suggested, but I'm not sure that would really buy us anything right now.
For now, I think it would be fine to keep the changes more minimal.

I'll try to clarify the commit message a bit in the next version to
better explain what is happening.

> > diff --git a/pack.h b/pack.h
> > index 1cde92082b..68dcf08cf3 100644
> > --- a/pack.h
> > +++ b/pack.h
> > @@ -3,6 +3,7 @@
> >  
> >  #include "object.h"
> >  #include "csum-file.h"
> > +#include "odb/source.h"
> >  
> >  struct packed_git;
> >  struct pack_window;
> 
> Let's add a forward declaration instead of including this header.

Will do.

> > diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> > index 0798ddab02..400a597606 100755
> > --- a/t/t5547-push-quarantine.sh
> > +++ b/t/t5547-push-quarantine.sh
> > @@ -70,4 +70,18 @@ test_expect_success 'updating a ref from quarantine is forbidden' '
> >  	git -C update.git fsck
> >  '
> >  
> > +test_expect_success '.keep file is removed after push' '
> > +	test_when_finished rm -rf keep.git &&
> > +	git init --bare keep.git &&
> > +
> > +	git -C keep.git config set receive.unpackLimit 0 &&
> > +	test_commit foo &&
> > +	git push keep.git HEAD &&
> > +	pack="$(ls keep.git/objects/pack/pack-*.pack)" &&
> > +	keep="${pack%.pack}.keep" &&
> > +
> > +	test_path_is_file "$pack" &&
> > +	test_path_is_missing "$keep"
> > +'
> 
> This would feel a bit safer if we had a hook that verifies that we
> indeed have the ".keep" file in the right spot before committing
> everything.

I'll try to set something up in the next version. Thanks.

-Justin