On Mon, Mar 26, 2018 at 07:04:54PM +0200, Duy Nguyen wrote: > >> +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). */ > > 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. > > 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
