Re: Git Test Coverage Report (v2.20.0-rc0)

From: Jeff King <peff@peff.net>
To: Derrick Stolee <stolee@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	"Stefan Beller" <sbeller@google.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Ben Peart" <benpeart@microsoft.com>
Subject: Re: Git Test Coverage Report (v2.20.0-rc0)
Date: Mon, 19 Nov 2018 13:44:39 -0500	[thread overview]
Message-ID: <20181119184438.GA21528@sigill.intra.peff.net> (raw)
In-Reply-To: <20181119162137.GA10621@sigill.intra.peff.net>

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