Re: [PATCH 1/6] odb: don't remove a ".keep" we never installed

From: Justin Tobler <jltobler@gmail.com>
Date: Sep 15, 2026, 18:25 UTC
Message-ID: <aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54>
In-Reply-To: <932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com>

On 26/09/14 11:31AM, Qin ShiCheng via GitGitGadget wrote:

Show 6 quoted lines

>From: Qin ShiCheng <qeesung@live.com>
>
>receive-pack runs index-pack with "--keep" over the quarantine, which
>writes a "pack-XXX.keep" there. The path we register as a tempfile is
>a different one: where that ".keep" will land once the quarantine is
>migrated into the main object database.

Yup, when the ".keep" file gets registered as a tempfile, it needs to know where it will eventually be located post-migration. That way it can be deleted after the references have been updated or if the process exits early. This is a bit awkward, but its a result of use relying on git-index-pack(1) to create the ".keep" file for us and it gets written to the quaratine directory.

Show 7 quoted lines

>Nothing of ours is at that path yet, and something else may be. Two
>pushes of identical content produce identical thin packs, index-pack
>names a pack after its contents, and so both want the same ".keep" in
>the main object database. If the other push still holds it, that file
>is what keeps its pack from being repacked away, and we remove it at
>exit regardless -- even when pre-receive rejected our push and nothing
>was migrated at all.

Interesting, for a pair of identical concurrent pushes, if one exits early it could end of deleting the other processes packfile out from under it. Really the process should probably only delete a ".keep" file that itself created.

Something worth noting, if there are two concurrent identical pushes, both will generate the same ".keep", but the keep message contained will differ. In such cases, when the quarantined files are migrated to the ODB, the ".keep" file that gets migrated first "wins" and the other push will fail because the competing ".keep" fails the collision check and consequently the push fails. I mention this because the current behavior for how Git handles concurrent identical pushes is to reject one of them. So if a process encounters an already existing ".keep" file in the main ODB, it may be sufficient to abort early anyways.

Show 5 quoted lines

>Register the path right before the migration instead, and once the
>migration has returned, read the files back. index-pack wrote the
>message we handed it; a file that says something else was not written
>for us, so let go of it without removing it. tempfile gains
>unregister_tempfile() for that.

Right, registering the temporary ".keep" files doesn't really need to happen prior to the ODB transaction commit anyways. In fact, we could go a step further and stop using git-index-pack(1) to prematurely create ".keep" files altogether in favor of letting the commit phase of the ODB transaction create it explicitly. This has a couple of benefits:

	- It avoids the already awkward tracking of ".keep" files in ODB
	  transaction pre-commit.
	- It would also make fixing the issue in question a bit easier
	  by allowing us to simply try to create the ".keep" file and if
	  it already exists, unregister the tempfile and abort early.

Completely unrelated to this bug as part of another series I'm working on locally, I've already have some patches that start creating ".keep" files explicitly during the ODB commit phase in the "files" backend. I would be happy to pick these patches out and send them upstream with some small adjustments to also fix the issue here in your first patch. Just let me know what you would perfer. :)

Thanks, -Justin