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

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 7, 2019, 14:43 UTC
Message-ID: <29379827-541e-fe75-f46a-661eb727fc1d@gmail.com>
In-Reply-To: <396091fc-5572-19a5-4f18-61c258590dd5@gmail.com>

On 6/6/2019 9:19 PM, Derrick Stolee wrote:

Show 5 quoted lines

> 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