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

> 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/

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

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

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

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

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

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