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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH 7/8] builtin/repack.c: make largest pack preferred
Date: Sat, 11 Sep 2021 12:17:37 +0200	[thread overview]
Message-ID: <875yv79zot.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <a790ee5ac6c03d6832599e77f84c352f577d6287.1631331139.git.me@ttaylorr.com>

On Fri, Sep 10 2021, Taylor Blau wrote:

> +	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.

> -		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.

> +		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...