git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Nov 19, 2018, 18:44 UTC
Message-ID
<20181119184438.GA21528@sigill.intra.peff.net>
In-Reply-To
<20181119162137.GA10621@sigill.intra.peff.net>
On Mon, Nov 19, 2018 at 11:21:38AM -0500, Jeff King wrote:
Show 38 quoted lines
> > +       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
Previous: Jeff KingNext: Ben Peart
Message 4 of 18 in “Git Test Coverage Report (v2.20.0-rc0)”
  1. Derrick StoleeNov 19, 2018
  2. Derrick StoleeNov 19, 2018
  3. Jeff KingNov 19, 2018
  4. Jeff KingNov 19, 2018
  5. Ben PeartNov 19, 2018
  6. Derrick StoleeNov 19, 2018
  7. Jeff KingNov 20, 2018
  8. Derrick StoleeNov 20, 2018
  9. Jeff KingNov 20, 2018
  10. Ævar Arnfjörð BjarmasonNov 19, 2018
  11. Jonathan NiederNov 19, 2018
  12. Ævar Arnfjörð BjarmasonNov 19, 2018
  13. Derrick StoleeNov 19, 2018
  14. Ævar Arnfjörð BjarmasonNov 19, 2018
  15. Derrick StoleeNov 19, 2018
  16. Derrick StoleeNov 19, 2018
  17. Ævar Arnfjörð BjarmasonNov 19, 2018
  18. Johannes SchindelinNov 20, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.