From: Patrick Steinhardt Date: Fri, 14 Aug 2026 07:46:32 GMT Subject: Re: [PATCH v3 1/9] builtin/receive-pack: properly clean up keep files Message-ID: In-Reply-To: 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