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

On Thu, Aug 13, 2026 at 04:45:16PM -0500, Justin Tobler wrote:
> 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.

Thanks for the explanation, this helped a lot!

Patrick