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

From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.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:35:48 -0400	[thread overview]
Message-ID: <YTza5MVTcBgOFXRS@nand.local> (raw)
In-Reply-To: <875yv79zot.fsf@evledraar.gmail.com>

On Sat, Sep 11, 2021 at 12:17:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> 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.

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.

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

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