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

quoted

quoted

+unsigned long oe_get_size_slow(struct packing_data *pack,
+                            const struct object_entry *e)

[...]

quoted

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).
   */

quoted

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.

quoted

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