Derrick Stolee <dstolee@xxxxxxxxxxxxx> writes:
> While running 'git commit-graph check', verify that the object IDs
> are listed in lexicographic order and that the fanout table correctly
> navigates into that list of object IDs.
All right. I think we can also sanity check the fanout table (for
example that it has 256 elements), see below.
>
> In anticipation of checking the commits in the commit-graph file
> against the object database, parse the commits from that file in
> advance. We perform this parse now to ensure the object cache contains
> only commits from this commit-graph file.
I guess this part could be a separate commit (a separate patch), because
it is not connected to the earlier part.
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> commit-graph.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
No tests that it detects broken commit-graph file (e.g. one that is
truncated)?
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 6d0d303a7a..6e3c08cd5c 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -835,6 +835,9 @@ static int check_commit_graph_error;
>
> int check_commit_graph(struct commit_graph *g)
> {
> + uint32_t i, cur_fanout_pos = 0;
> + struct object_id prev_oid, cur_oid;
> +
> if (!g) {
> graph_report(_("no commit-graph file loaded"));
> return 1;
> @@ -859,5 +862,36 @@ int check_commit_graph(struct commit_graph *g)
> if (g->hash_len != GRAPH_OID_LEN)
> graph_report(_("commit-graph has incorrect hash length: %d"), g->hash_len);
>
> + for (i = 0; i < g->num_commits; i++) {
> + struct commit *graph_commit;
> +
> + hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
> +
> + if (i > 0 && oidcmp(&prev_oid, &cur_oid) >= 0)
> + graph_report(_("commit-graph has incorrect oid order: %s then %s"),
Good. Reporting what problem is; we could have also reported the
position at which there is this problem.
> +
> + oid_to_hex(&prev_oid),
> + oid_to_hex(&cur_oid));
> + oidcpy(&prev_oid, &cur_oid);
> +
> + while (cur_oid.hash[0] > cur_fanout_pos) {
> + uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
> + if (i != fanout_value)
> + graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
Good. Reporting details of the problem.
> + cur_fanout_pos, fanout_value, i);
> +
> + cur_fanout_pos++;
> + }
One thing you don't check here is that fanout is closed, that is all the
rest of fanout data up to 256th element (if they are any) all points at
the same position past the last element of OID Lookup chunk.
> +
> + graph_commit = lookup_commit(&cur_oid);
> +
> + if (!parse_commit_in_graph_one(g, graph_commit))
> + graph_report(_("failed to parse %s from commit-graph"), oid_to_hex(&cur_oid));
Doesn't whis check Commit Data (CDAT) chunk, and therefore should better
be in a separate commit?
> +
> + if (graph_commit->graph_pos != i)
> + graph_report(_("graph_pos for commit %s is %u != %u"), oid_to_hex(&cur_oid),
> + graph_commit->graph_pos, i);
Hmmm... it seems to me that the above does not check that commit-graph
file is correct, but that the parsing code is correct.
> + }
> +
> return check_commit_graph_error;
> }
