Re: [PATCH v3 03/10] builtin/maintenance: introduce "geometric-repack" task

From: Jeff King <peff@peff.net>
Date: Oct 25, 2025, 19:15 UTC
Message-ID: <20251025191550.GA279793@coredump.intra.peff.net>
In-Reply-To: <20251024-pks-maintenance-geometric-strategy-v3-3-9b5b3bdb4387@pks.im>

On Fri, Oct 24, 2025 at 08:57:16AM +0200, Patrick Steinhardt wrote:

Show 6 quoted lines

> +		# Repacking should now cause a no-op geometric repack because
> +		# no packfiles need to be combined.
> +		ls -l .git/objects/pack >before &&
> +		run_and_verify_geometric_pack 1 &&
> +		ls -l .git/objects/pack >after &&
> +		test_cmp before after &&

I got a CI failure from this test like this:

   + diff -u before after
   --- before 2025-10-25 17:51:59.985025237 +0000
   +++ after  2025-10-25 17:52:00.304026445 +0000
   @@ -1,5 +1,5 @@
    total 16
   --rw-rw-r-- 1 builder builder 1252 Oct 25 17:51 multi-pack-index
   +-rw-rw-r-- 1 builder builder 1252 Oct 25 17:52 multi-pack-index
    -r--r--r-- 1 builder builder 1156 Oct 25 17:51 pack-68c20c4590a622a21395b4480621d55494112a83.idx
    -r--r--r-- 1 builder builder  226 Oct 25 17:51 pack-68c20c4590a622a21395b4480621d55494112a83.pack
    -r--r--r-- 1 builder builder   64 Oct 25 17:51 pack-68c20c4590a622a21395b4480621d55494112a83.rev

I'm not sure if this is a bug or a race condition in the test. If "no-op" means "do not generate a new pack, but do generate a new midx" then it's a race condition (the regenerated midx might move across the minute boundary). If it means "do not even generate a new midx", then there is a bug. ;)

You can generate the race at will like this:

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0d76693fee..2b5141196f 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -501,6 +501,7 @@ test_expect_success 'geometric repacking task' '
 		# Repacking should now cause a no-op geometric repack because
 		# no packfiles need to be combined.
 		ls -l .git/objects/pack >before &&
+		sleep 60 &&
 		run_and_verify_geometric_pack 1 &&
 		ls -l .git/objects/pack >after &&
 		test_cmp before after &&

though if we are going to be picky about timestamps, it probably makes
sense to use a higher resolution. Sadly I don't think there's a portable
way to do that with "ls", and "stat" is probably likewise something we
can't assume. I'd turn to perl, but I know you've been trying to avoid
depending on it. You can hack around it with:

  test-tool chmtime -v +0 .git/objects/pack/*

for this case, I'd think.

-Peff