Re: [PATCH v3 12/20] commit-graph: verify parent list

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> 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@microsoft.com>
> ---
>  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