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

[Patrick Steinhardt <ps@pks.im>]

On Sat, Oct 25, 2025 at 03:15:50PM -0400, Jeff King wrote:
> On Fri, Oct 24, 2025 at 08:57:16AM +0200, Patrick Steinhardt wrote:
> 
> > +		# 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.

Interesting! I would say that this is an issue in git-repack(1) itself:
if the geometric repack didn't lead to any new packs, and if all of the
packs are already covered by a MIDX, then we still rather pointlessly
regenerate the MIDX even though it won't cover anything new.

I wonder whether we want a patch like the below one? Problem though is
that we'd also have to check whether any of the other options have
changed, otherwise we for example wouldn't generate bitmaps.

In any case though, I feel like this is a bit out of scope for this
patch series. Other strategies that write a MIDX behave the same, so
this is something we can fix later on.

Patrick

diff --git a/repack-midx.c b/repack-midx.c
index 6f6202c5bcc..efa47bb55b5 100644
--- a/repack-midx.c
+++ b/repack-midx.c
@@ -285,6 +285,35 @@ static void remove_redundant_bitmaps(struct string_list *include,
 	strbuf_release(&path);
 }
 
+static bool midx_needs_repack(const struct repack_write_midx_opts *opts,
+			      const struct string_list *include)
+{
+	struct strset set = STRSET_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	bool needs_repack;
+
+	if (opts->existing->midx_packs.nr != include->nr)
+		return true;
+
+	for (size_t i = 0; i < opts->existing->midx_packs.nr; i++) {
+		const char *item = opts->existing->midx_packs.items[i].string;
+
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item);
+		strbuf_strip_suffix(&buf, ".pack");
+		strbuf_addstr(&buf, ".idx");
+
+		strset_add(&set, buf.buf);
+	}
+
+	needs_repack = false;
+	for (size_t i = 0; i < include->nr && !needs_repack; i++)
+		needs_repack = !strset_contains(&set, include->items[i].string);
+
+	strset_clear(&set);
+	return needs_repack;
+}
+
 int write_midx_included_packs(struct repack_write_midx_opts *opts)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -295,7 +324,7 @@ int write_midx_included_packs(struct repack_write_midx_opts *opts)
 	int ret = 0;
 
 	midx_included_packs(&include, opts);
-	if (!include.nr)
+	if (!include.nr || !midx_needs_repack(opts, &include))
 		goto done;
 
 	cmd.in = -1;