Re: [PATCH v3 13/20] commit-graph: verify generation number
flat view
From: Jakub Narebski <hidden>
Date: 2018-06-02 12:24:03
Derrick Stolee [off-list ref] writes:
While iterating through the commit parents, perform the generation number calculation and compare against the value stored in the commit-graph.
All right, that's good. What about commit-graph files that have GENERATION_NUMBER_ZERO for all its commits (because we verify single commit-graph file only, there wouldn't be GENERATION_NUMBER_ZERO mixed with non-zero generation numbers)? Unless we can assume that no commit-graph file in the wild would have GENERATION_NUMBER_ZERO.
The tests demonstrate that having a different set of parents affects the generation number calculation, and this value propagates to descendants. Hence, we drop the single-line condition on the output.
I don't understand what part of changes this paragraph of the commit message refers to. Anyway, changing parents may not lead to changed generation numbers; take for example commit with single parent, which we change to other commit with the same generation number.
Signed-off-by: Derrick Stolee <redacted> --- commit-graph.c | 18 ++++++++++++++++++ t/t5318-commit-graph.sh | 6 ++++++
Sidenote: I have just realized that it may be better to put validation-related tests into different test file.
quoted hunk
2 files changed, 24 insertions(+)diff --git a/commit-graph.c b/commit-graph.c index fff22dc0c3..ead92460c1 100644 --- a/commit-graph.c +++ b/commit-graph.c@@ -922,6 +922,7 @@ int verify_commit_graph(struct commit_graph *g) for (i = 0; i < g->num_commits; i++) { struct commit *graph_commit, *odb_commit; struct commit_list *graph_parents, *odb_parents; + uint32_t max_generation = 0; hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);@@ -956,6 +957,9 @@ int verify_commit_graph(struct commit_graph *g) oid_to_hex(&graph_parents->item->object.oid), oid_to_hex(&odb_parents->item->object.oid)); + if (graph_parents->item->generation > max_generation) + max_generation = graph_parents->item->generation; +
All right, that calculates the maximum of generation number of commit parents.
quoted hunk
graph_parents = graph_parents->next; odb_parents = odb_parents->next; }@@ -963,6 +967,20 @@ int verify_commit_graph(struct commit_graph *g) if (odb_parents != NULL) graph_report("commit-graph parent list for commit %s terminates early", oid_to_hex(&cur_oid)); + + /* + * If one of our parents has generation GENERATION_NUMBER_MAX, then + * our generation is also GENERATION_NUMBER_MAX. Decrement to avoid + * extra logic in the following condition. + */
Nice trick.
+ if (max_generation == GENERATION_NUMBER_MAX) + max_generation--;
What about GENERATION_NUMBER_ZERO?
quoted hunk
+ + if (graph_commit->generation != max_generation + 1) + graph_report("commit-graph generation for commit %s is %u != %u", + oid_to_hex(&cur_oid), + graph_commit->generation, + max_generation + 1);
I think we should also check that generation numbers do not exceed GENERATION_NUMBER_MAX... unless it is already taken care of?
quoted hunk
} return verify_commit_graph_error;diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 12f0d7f54d..673b0d37d5 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET GRAPH_BYTE_COMMIT_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN` GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4` GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3` +GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8` # usage: corrupt_graph_and_verify <position> <data> <string> # Manipulates the commit-graph file at the position@@ -366,4 +367,9 @@ test_expect_success 'detect incorrect tree OID' ' "commit-graph parent for" ' +test_expect_success 'detect incorrect generation number' ' + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_GENERATION "\01" \
I assume that you have checked that it actually corrupts generation number (without affecting commit date).
+ "generation"
A very minor nitpick: Not "generation for commit"?
quoted hunk
+' + test_done