On Mon, Nov 19, 2018 at 11:21:38AM -0500, Jeff King wrote:
> > + if (use_delta_islands) {
> > + const char *p;
> > + unsigned depth = 0;
> > + struct object_entry *ent;
> > +
> > + for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))
> > + depth++;
> > +
> > + ent = packlist_find(&to_pack, obj->oid.hash, NULL);
> > + if (ent && depth > ent->tree_depth)
> > + ent->tree_depth = depth;
> > + }
> > }
> >
> > And that 'ent->tree_depth = depth;' line is replaced with the
> > oe_set_tree_depth() call in the report.
> >
> > Since depth is never incremented, we are not covering this block. Is it
> > possible to test?
>
> Looks like t5320 only has single-level trees. We probably just need to
> add a deeper tree. A more interesting case is when an object really is
> found at multiple depths, but constructing a case that cared about that
> would be quite complicated.
>
> That said, there is much bigger problem with this code, which is that
> 108f530385 (pack-objects: move tree_depth into 'struct packing_data',
> 2018-08-16) is totally broken. It works on the trivial repository in the
> test, but try this (especially under valgrind or ASan) on a real
> repository:
>
> git repack -adi
>
> which will crash immediately. It doesn't correctly maintain the
> invariant that if tree_depth is not NULL, it is the same size as
> the main object array.
>
> I'll see if I can come up with a fix.
Just a quick update to prevent anyone else looking at it: I have a fix
for this (and another related issue with that commit).
There's an edge case in that depth computation that I think is
unhandled, as well. I _think_ it doesn't trigger very often, but I'm
running some experiments to verify it. That's S-L-O-W, so I probably
won't have results until tomorrow.
-Peff
