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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 12, 2026, 06:07 UTC
Message-ID: <anwNonpw5SZuHADv@pks.im>
In-Reply-To: <20260811175415.2044235-2-jltobler@gmail.com>

On Tue, Aug 11, 2026 at 12:54:07PM -0500, Justin Tobler wrote:

Show 20 quoted lines

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

Show 19 quoted lines

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

Show 12 quoted lines

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

Show 21 quoted lines

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

Thanks!

Patrick