Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary

From: Jeff King <peff@peff.net>
Date: Aug 25, 2020, 17:42 UTC
Message-ID: <20200825174218.GF1414394@coredump.intra.peff.net>
In-Reply-To: <20200825173425.GA16844@syl.lan>

On Tue, Aug 25, 2020 at 01:34:25PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> > I.e., drop step 2 above, and make step 3 just clear_midx_file(). Which
> > is roughly what the code does now, isn't it? Or is there some reason
> > that "expire" is more interesting than just clearing?
> 
> It's not clear to me whether you're talking about my patch, or what a
> more full integration with 'git repack' looks like.
> 
> If you are talking about my patch, I disagree: checking that the MIDX
> doesn't know about a pack we're dropping *is* useful even without
> all-in-one, because of '.keep' packs (as demonstrated by the new test in
> my patch).

I hadn't considered .keep. So all-in-one may still involve selectively deleting some packs. It makes sense, then, for repack to consider whether the midx is actually redundant or not rather than just always clearing it (i.e., what your patch does).

In general I consider .keep packs kind of an awful feature that introduces a lot of confusion and works against other features like bitmaps. But I guess that they're the only thing that allows you to have a gigantic Windows-like repo where you never fully pack it, but just keep acquiring a string of big packs. Which is the exact case that the midx is most useful for. So they're definitely worth considering and supporting here.

> To me, this patch seems like an incremental step in the direction that
> we ultimately want to be going, but it's hard to untangle whether the
> ensuing discussion is targeted at my patch, or the ultimate goal.

I wasn't sure of the answer to that until we untangled more. I.e., it wasn't clear to me if your incremental step was even in the right direction if we weren't in fact ever selectively deleting packs in git-repack. (And now it sounds like we aren't via "git repack -d", which was my original question, but we are via .keep).

-Peff