On 11/20/2018 6:34 AM, Jeff King wrote:
On Mon, Nov 19, 2018 at 10:40:53AM -0500, Derrick Stolee wrote:
Since depth is never incremented, we are not covering this block. Is it
possible to test?
This should be covered by the fix in:
https://public-inbox.org/git/20181120095053.gc22...@sigill.intra.peff.net/
because now entries at the top-level are depth "1". The "depth++" line
is still never executed in our test suite. I'm not sure how much that
matters.
Thanks! I'll go take a look at your patch.
delta-islands.c
c8d521faf7 53) memcpy(b, old, size);
This memcpy happens when the 'old' island_bitmap is passed to
island_bitmap_new(), but...
c8d521faf7 187) b->refcount--;
c8d521faf7 188) b = kh_value(island_marks, pos) = island_bitmap_new(b);
This block has the only non-NULL caller to island_bitmap_new().
This is another case where it triggers a lot for a reasonably-sized
repo, but it's hard to construct a small case. This code implements a
copy-on-write of the bitmap, which means the same objects have to be
accessible from two different paths through the reachability graph, each
with different island marks. And then a test would I guess make sure
that the correct subsets of objects never become deltas, which gets
complicated.
And I think that's a pattern with the delta-island code. What we really
care about most is that if we throw a real fork-network repository at
it, it produces faster clones with fewer un-reusable deltas. So I think
a much more interesting approach here would be perf tests. But:
- we'd want to count those as coverage, and that likely makes your
coverage tests prohibitively expensive
- it requires a specialized repo to demonstrate, which most people
aren't going to have handy
Do you have regularly-running tests that check this in your
infrastructure? As long as someone would notice if this code starts
failing, that would be enough.
c8d521faf7 212) obj = ((struct tag *)obj)->tagged;
c8d521faf7 213) if (obj) {
c8d521faf7 214) parse_object(the_repository, &obj->oid);
c8d521faf7 215) marks = create_or_get_island_marks(obj);
c8d521faf7 216) island_bitmap_set(marks, island_counter);
It appears that this block would happen if we placed a tag in the delta
island.
Yep. Again, exercised by real repositories.
I'm not sure how far we want to go in the blind pursuit of coverage.
Certainly we could add a tag to the repo in t5320, and this code would
get executed. But verifying that it's doing the right thing is much
harder (and is more easily done with a perf test).
Blind coverage goals are definitely not worth the effort. My goal here
was to re-check all of the new code (since last release) that is not
covered, because it's easier to hide a bug there.
c8d521faf7 397) strbuf_addch(&island_name, '-');
This block is inside the following patch:
[...]
Yeah, this triggers if your regex has more than one capture group (and
likewise, we almost certainly don't run the "you have too many groups"
warning).
Did you know that regexes are notoriously under-tested [1]? When looking
at this code, I didn't even know regexes were involved (but I didn't
look enough at the context).
[1] https://dl.acm.org/citation.cfm?id=3236072
c8d521faf7 433) continue;
c8d521faf7 436) list[dst] = list[src];
These blocks are inside the following nested loop in deduplicate_islands():
+ for (ref = 0; ref + 1 < island_count; ref++) {
+ for (src = ref + 1, dst = src; src < island_count; src++) {
+ if (list[ref]->hash == list[src]->hash)
+ continue;
+
+ if (src != dst)
+ list[dst] = list[src];
+
+ dst++;
+ }
+ island_count = dst;
+ }
This means that our "deduplication" logic is never actually doing anything
meaningful.
Sorry, I don't even remember what this code is trying to do. The island
code is 5+ years old, and just recently ported to upstream Git by
Christian. And that's perhaps part of my laziness in the above tests; it
would be a significant effort to re-figure out all these corner cases.
It's a big part of why I hadn't been sending the patches upstream
myself.
Sure. Hopefully pointing out these blocks gives us more motivation to
manually inspect them and avoid silly bugs.
Thanks,
-Stolee
