Re: Git Test Coverage Report (Thursday, June 6 2019)

[Derrick Stolee <stolee@gmail.com>]

On 6/6/2019 9:19 PM, Derrick Stolee wrote:
> commit-graph.c
> a53af50b 346) if (!oideq(&oids[n], &cur_g->oid) ||
> a53af50b 347)     !hasheq(oids[n].hash, g->chunk_base_graphs + g->hash_len * n)) {
> a53af50b 348) warning(_("commit-graph chain does not match"));
> a53af50b 349) return 0;

I'm calling myself out that this warning() should be hit by the "verify" checks, as
this is a critical behavior check.

> cd556367 353) cur_g = cur_g->base_graph;

Here is an example of a line where I definitely hit this block when I add a die()
statement.

I think I figured out the reason the coverage report was reporting coverage
incorrectly sometimes: I was accidentally running the test suite in parallel.
This causes lots of issues with how gcov tracks coverage as there are lots of
race conditions. Running without parallelism takes a LOT longer, so I'll need
to use my own machine instead of a hosted build.

> cd556367 396) warning(_("invalid commit-graph chain: line '%s' not a hash"),

I'm lukewarm on how important it is to test this, but I'll give it a try.

> 4ece4fc1 945) if (find_commit_in_graph(parent->item,
> 4ece4fc1 948) edge_value = pos;

This case is triggered by an octopus merge with a not-first parent being
in the base graph instead of in the tip file. I'll add a test that covers
this situation, as it could be prone to errors.

> 00a8cb54 1523)     (max_commits && num_commits > max_commits))) {

This is a reminder that I forgot to add tests for the '--max-commits' argument.

> c6b73769 1540) ctx->num_commit_graphs_after = 1;
> c6b73769 1541) ctx->new_base_graph = NULL;

This is another subtle case that I should test: we are writing a commit-graph
with the --split option and we think we should write a tip graph, but the
current file is the old (non-split) commit-graph file _and_ it is in an alternate.
This prevents us from trying to move the commit-graph in the alternate, and we
must write a single commit-graph file in our object directory.

> 0e2ec504 1617) duplicates++;
> 0e2ec504 1620) ctx->commits.list[last_distinct + 1] = ctx->commits.list[i];
> 0e2ec504 1628) ctx->num_extra_edges += num_parents - 2;

This code in deduplicate_commits() is doing somewhat important work: it takes
the combined commit lists from multiple levels of the commit-graph and then
de-duplicates the lists. However, if we are writing the commit-graphs properly
then they will never have duplicates at this point. I don't think it is valid
to have duplicates, either, as then a commit will have ambiguous graph position.

Perhaps deduplicate_commits() should remove the duplicate logic and replace it
with a BUG() or die() statement upon finding a duplicate. The sorting and
checking for octopus merges is still important.

Thanks,
-Stolee