Re: [PATCH 7/8] builtin/repack.c: make largest pack preferred

On Sat, Sep 11, 2021 at 12:17:37PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Fri, Sep 10 2021, Taylor Blau wrote:

quoted

+	if (geometry) {
+		struct packed_git *largest = get_largest_active_pack(geometry);
+		if (largest)
+			strvec_pushf(&cmd.args, "--preferred-pack=%s",
+				     pack_basename(largest));
+		else
+			/*
+			 * The largest pack was repacked, meaning only one pack
+			 * exists (and tautologically, it is the largest).
+			 */
+			;
+	}

Probably one of those cases where an assignment within an "if" is the
better of two evils, despite the CodingGuidelines warning against it
(but not forbidding it). The added get_largest_active_pack() could also
learn to punt on a NULL argument, in which case we wouldn't need an
assignment):

	struct packed_git *largest;
	[...]
        /* If ...[...] */
	if (geometry && (largest = get_largest_active_pack(geometry)))
		strvec_push(...);

I punted on re-phrasing the comment, because the current code invites
the reader to start reading this block, then we see that we may do
stuff, and then the comment applies to what we *didn't* do.

Of the two, I vastly prefer the form that doesn't require assignment
inside the conditional expression.

That comment could use an extra bit about how we don't handle
incremental repacks (since we don't bother looking at the size after
words in the case of `git repack -d` (which is neither all-into-one nor
geometric). Fixed both, thanks.

quoted

-		usage("read-midx [--show-objects|--checksum] <object-dir>");
+		usage("read-midx [--show-objects|--checksum|--preferred-pack] <object-dir>");

Just an aside, but I'm surprised to see this use the older
non-parse_options() usage API, which we're generally moving away
from. usage_msg_opt() is generally better.

Opportunity for future cleanup. I didn't see any point to change it in
the middle of an otherwise unrelated series.

quoted

+		git config core.multiPackIndex true &&

I remember a similar pattern from another series, does this test pass
without the config being set? I didn't check...

It does! This test was written long before 18e449f86b (midx: enable
core.multiPackIndex by default, 2020-09-25), but since 18e449f86b that
line is no longer necessary. Thanks for noticing!

Thanks,
Taylor