Re: [RFC PATCH 05/12] commit-graph: check fanout and lookup table
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Apr 20, 2018, 07:27 UTC
- Message-ID
- <864lk6fics.fsf@gmail.com>
- In-Reply-To
- <20180417181028.198397-6-dstolee@microsoft.com>
Derrick Stolee <dstolee@microsoft.com> 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.
Show 5 quoted lines
> > 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.
Show 5 quoted lines
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > commit-graph.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+)
No tests that it detects broken commit-graph file (e.g. one that is truncated)?
Show 26 quoted lines
>
> 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.
Show 9 quoted lines
> +
> + 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.
Show 5 quoted lines
> +
> + 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; > }