Derrick Stolee <dstolee@xxxxxxxxxxxxx> writes:
> The commit-graph file stores parents in a two-column portion of the
> commit data chunk. If there is only one parent, then the second column
> stores 0xFFFFFFFF to indicate no second parent.
All right, it is certainly nice to have this information, but isn't it
something that one caan find in Documentation/technical/commit-graph-format.txt?
>
> The 'verify' subcommand checks the parent list for the commit loaded
> from the commit-graph and the one parsed from the object database. Test
> these checks for corrupt parents, too many parents, and wrong parents.
>
> The octopus merge will be tested in a later commit.
Does this mean that after this commit but before the next one the
'verify' subcommand would have false negatives for octopus merges
(falsely indicating that commit-graph is invalid)?
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> commit-graph.c | 25 +++++++++++++++++++++++++
> t/t5318-commit-graph.sh | 18 ++++++++++++++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 19ea369fc6..fff22dc0c3 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -921,6 +921,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;
>
> hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
>
> @@ -938,6 +939,30 @@ int verify_commit_graph(struct commit_graph *g)
> oid_to_hex(&cur_oid),
> oid_to_hex(get_commit_tree_oid(graph_commit)),
> oid_to_hex(get_commit_tree_oid(odb_commit)));
> +
> + graph_parents = graph_commit->parents;
> + odb_parents = odb_commit->parents;
> +
> + while (graph_parents) {
> + if (odb_parents == NULL) {
> + graph_report("commit-graph parent list for commit %s is too long",
> + oid_to_hex(&cur_oid));
> + break;
> + }
All right, so this would catch the situation where there are more
parents for a commit in commit-graph than they are in the object
database version.
> +
> + if (oidcmp(&graph_parents->item->object.oid, &odb_parents->item->object.oid))
> + graph_report("commit-graph parent for %s is %s != %s",
> + oid_to_hex(&cur_oid),
> + oid_to_hex(&graph_parents->item->object.oid),
> + oid_to_hex(&odb_parents->item->object.oid));
All right, so this would catch the situation where parents do not match
between commit-graph and the object database.
> +
> + graph_parents = graph_parents->next;
> + odb_parents = odb_parents->next;
> + }
> +
> + if (odb_parents != NULL)
> + graph_report("commit-graph parent list for commit %s terminates early",
> + oid_to_hex(&cur_oid));
So this would catch the situation where there are more parents for a
commit in the object database than they are in the commit-graph. Does
this handle octopus merges automatically, or is it left for the future
work/commit?
> }
>
> return verify_commit_graph_error;
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 21cc8e82f3..12f0d7f54d 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8`
> GRAPH_BYTE_OID_LOOKUP_MISSING=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4 + 10`
> GRAPH_COMMIT_DATA_OFFSET=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* $NUM_COMMITS`
> 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`
>
> # usage: corrupt_graph_and_verify <position> <data> <string>
> # Manipulates the commit-graph file at the position
> @@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' '
> "root tree OID for commit"
> '
>
> +test_expect_success 'detect incorrect parent int-id' '
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_PARENT "\01" \
> + "invalid parent"
> +'
So this would actually be caught by code introduced earlier, and not by
the one introduced in this commit -- but logically this test belongs
here, ian't it?
> +
> +test_expect_success 'detect extra parent int-id' '
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_EXTRA_PARENT "\00" \
> + "is too long"
> +'
O.K., so the commit has one parent and we have corrupted it to read as
if it had more than one (and commit-graph would then have more parents
than reality, that is the object database).
Sidenote: I think we can use regexp for checking if the error message
matches, isn't it?
> +
> +test_expect_success 'detect incorrect tree OID' '
Errr... what? The name of this test seems very wrong...
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_WRONG_PARENT "\01" \
> + "commit-graph parent for"
> +'
So here you modify the prent list in commit graph so that the commit
number points fits within commit-graph; it would of course make the
commit-graph and the object database version of parents do not match.
Good.
> +
> test_done
