Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse

From: Jeff King <peff@peff.net>
Date: Oct 13, 2019, 07:38 UTC
Message-ID: <20191013073851.GA7001@sigill.intra.peff.net>
In-Reply-To: <xmqqsgnyg76d.fsf@gitster-ct.c.googlers.com>

On Sat, Oct 12, 2019 at 09:04:58AM +0900, Junio C Hamano wrote:

Show 12 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The current code does so by creating a new entry in the reused_chunks
> > array. In the worst case that can grow to have the same number of
> > entries as we have objects. So this code was an attempt to pad the
> > header of a shrunken entry to keep it the same size. I don't remember
> > all the problems we ran into with that, but in the end we found that it
> > didn't actually help much, because in practice we don't end up with a
> > lot of chunks anyway.
> 
> Hmm, I am kind of surprised that the decoding side allowed such a
> padding.

IIRC, the "padding" is just a sequence of 0-length-plus-continuation-bit varint bytes. And for some reason it worked for the size but not for the delta offset value. So the decoder wasn't aware of it, but simply hadn't explicitly enforced that there weren't pointless bytes.

But yeah, it seems like a pretty hacky thing to rely on. I don't think we ever even ran that code in production, and the if(0) was just leftover experimental cruft.

> > I think the original code may simply have been buggy and nobody noticed.
> > Here's what I wrote when this line was added in our fork:
> [...]
> Impressed by the careful thinking here.

It's unfortunate that the reasoning there wasn't part of the earlier submission. I'm not sure how to reconcile that. The patches as originally written can't be applied now (they were munged over the years during merges with upstream). And some of them are just "oops, fix this dumb bug" trash that we wouldn't want to take anyway.

So I think at best they're something for somebody to manually look at and try to incorporate into the commit messages of the revised patch series. But I didn't give them to Christian to work with because it's hard to even figure out which patches are still relevant. I wish I had a better tooling there. I've been playing with something that looks at a diff and then tries to blame each of the touched lines. Which is sort of like the "-L" line-log, I guess, but for a very non-contiguous set of lines.

-Peff