Re: [PATCH v7 00/13] nd/pack-objects-pack-struct updates

From: Jeff King <peff@peff.net>
Date: Mar 27, 2018, 16:53 UTC
Message-ID: <20180327165311.GA15102@sigill.intra.peff.net>
In-Reply-To: <CACsJy8Br0+PGauEYLUarOFrq4bPbnE5u95XemzFyyn_riBZ_xA@mail.gmail.com>

On Mon, Mar 26, 2018 at 07:04:54PM +0200, Duy Nguyen wrote:

Show 9 quoted lines

> >> +unsigned long oe_get_size_slow(struct packing_data *pack,
> >> +                            const struct object_entry *e)
> [...]
> > But short of that, it's probably worth a comment explaining what's going
> > on.
> 
> I thought the elaboration on "size" in the big comment block in front
> of struct object_entry was enough. I was wrong. Will add something
> here.

It may be my fault for reading the interdiff, which didn't include that comment. I was literally just thinking something like:

  /*
   * Return the size of the object without doing any delta
   * reconstruction (so non-deltas are true object sizes, but
   * deltas return the size of the delta data).
   */

Show 8 quoted lines

> > I see there's a one-off test for GIT_TEST_FULL_IN_PACK_ARRAY, which I
> > think is a good idea, since it makes sure the code is exercised in a
> > normal test suite run. Should we do the same for GIT_TEST_OE_SIZE_BITS?
> 
> I think the problem with OE_SIZE_BITS is it has many different code
> paths (like reused deltas) which is hard to make sure it runs. But yes
> I think I could construct a pack that executes both code paths in
> oe_get_size_slow(). Will do in a reroll.

OK. If it's too painful to construct a good example, don't worry about it. It sounds like we're unlikely to get full coverage anyway.

Show 7 quoted lines

> > I haven't done an in-depth read of each patch yet; this was just what
> > jumped out at me from reading the interdiff.
> 
> I would really appreciate it if you could find some time to do it. The
> bugs I found in this round proved that I had no idea what's really
> going on in pack-objects. Sure I know the big picture but that's far
> from enough to do changes like this.

I didn't get to it today, but I'll try to give it a careful read. There are quite a few corners of pack-objects I don't know well, but I think at this point I may be the most expert of remaining people. Scary. :)

-Peff