On Tue, Jun 20, 2023 at 07:01:15PM +0000, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee <redacted>
When installing a packfile, we place the .pack file before the .idx
file. The intention is that Git scans for .idx files in the pack
directory and then loads the .pack files from that list.
However, when we delete packfiles, we do not do this in the reverse
order as we should. The unlink_pack_path() method deletes the .pack
followed by the .idx.
This creates a window where the process could be interrupted between
the .pack deletion and the .idx deletion, leaving the repository in a
state that looks strange, but isn't actually too problematic if we
assume the pack was safe to delete. The .idx without a .pack will cause
some overhead, but will not interrupt other Git processes.
I think that this specific case wouldn't cause a problem since we were
about to delete that pack anyway, but it may be worth emphasizing that
having a missing pack with an extant .idx file is not always guaranteed
to be safe.
This ordering was introduced into the 'git repack' builtin by
a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), though
we must be careful to track history through the code move in 8434e85d5f9
(repack: refactor pack deletion for future use, 2019-06-10) to see that.
Thanks for calling this out, I agree that the "regression" dates back to
a1bbc6c0176, since we can see that in git-repack.sh, we do:
# Remove the "old-" files
for name in $names
do
rm -f "$PACKDIR/old-pack-$name.idx"
rm -f "$PACKDIR/old-pack-$name.pack"
done
...removing the .idx first, which is the right thing to do here.
quoted hunk ↗ jump to hunk
diff --git a/packfile.c b/packfile.c
index fd083c86e00..6b591ddcccf 100644
--- a/packfile.c
+++ b/packfile.c
@@ -381,7 +381,7 @@ void close_object_store(struct raw_object_store *o)
void unlink_pack_path(const char *pack_name, int force_delete)
{
- static const char *exts[] = {".pack", ".idx", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"};
+ static const char *exts[] = {".idx", ".pack", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"};
int i;
struct strbuf buf = STRBUF_INIT;
size_t plen;
Looks good, thanks.
Thanks,
Taylor