Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`

From: Jeff King <peff@peff.net>
Date: May 5, 2023, 21:26 UTC
Message-ID: <20230505212631.GB3321533@coredump.intra.peff.net>
In-Reply-To: <ZFLSDS89y8P2+g/2@nand.local>

On Wed, May 03, 2023 at 05:28:45PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> > OK, I understand the use case you're trying to support, and your
> > approach mostly makes sense. But there are two things I was surprised by
> > in the implementation:
> >
> >   1. Does this need to be tied to cruft packs? The same logic would
> >      apply to "repack -A" which turns objects loose (of course you
> >      probably don't want to do that in the long term for performance
> >      reasons, but it's conceptually the same thing; see below).
> 
> I agree that you wouldn't want to do it for performance reasons, but I'm
> comfortable with the asymmetry here, since this is `pack.extraCruftTips`
> (emphasis on "cruft"), so it's not clear that it has to be related to
> "repack -A".
> 
> Happy to change things up if you feel strongly, though.

I don't feel strongly. I certainly have no intent to run "git repack -A" with extra tips specified. Mostly I just thought that it would be simpler to just apply it everywhere, both conceptually to the user and within the implementation.

But since the cruft-pack implementation diverges quite a bit from the regular "-A" handling, I guess it makes the code more complex rather than less. The asymmetry feels like a wart to me, but I guess in the long run we'd hope that the "turn unreachable loose" strategy is deprecated anyway.

-Peff