Re: [PATCH 04/12] commit-graph: verify fanout and lookup table
From: Martin Ågren <hidden>
Date: 2018-05-10 18:29:22
On 10 May 2018 at 19:34, Derrick Stolee [off-list ref] wrote:
quoted hunk ↗ jump to hunk
While running 'git commit-graph verify', verify that the object IDs are listed in lexicographic order and that the fanout table correctly navigates into that list of object IDs. Signed-off-by: Derrick Stolee <redacted> --- commit-graph.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)diff --git a/commit-graph.c b/commit-graph.c index ce11af1d20..b4c146c423 100644 --- a/commit-graph.c +++ b/commit-graph.c@@ -839,6 +839,9 @@ static int verify_commit_graph_error; int verify_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;@@ -853,5 +856,35 @@ int verify_commit_graph(struct commit_graph *g) if (!g->chunk_commit_data) graph_report(_("commit-graph is missing the Commit Data chunk")); + for (i = 0; i < g->num_commits; i++) { + 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"),
Minor: I think our style would prefer s/i > 0/i/. I suppose the second check should be s/>=/>/, but it's not like it should matter. ;-) I wonder if this is a message that would virtually never make sense to a user, but more to a developer. Leave it untranslated to make sure any bug reports to the list are readable to us?
+ + oid_to_hex(&prev_oid), + oid_to_hex(&cur_oid));
Hmm, these two lines do not actually achieve anything?
+ 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"),
+ cur_fanout_pos, fanout_value, i);Same though re `_()`, even more so because of the more technical notation.
+
+ cur_fanout_pos++;
+ }
+ }
+
+ while (cur_fanout_pos < 256) {
+ uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
+
+ if (g->num_commits != fanout_value)
+ graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
+ cur_fanout_pos, fanout_value, i);Same here. Or maybe these should just give a translated user-readable basic idea of what is wrong and skip the details? Martin