Re: [PATCH v3 11/20] commit-graph: verify root tree OIDs

From: Jakub Narebski <jnareb@gmail.com>
Date: May 30, 2018, 22:24 UTC
Message-ID: <86zi0gvl1h.fsf@gmail.com>
In-Reply-To: <20180524162504.158394-12-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

> The 'verify' subcommand must compare the commit content parsed from the
> commit-graph and compare it against the content in the object database.

You have "compare" twice in the above sentence.

> Use lookup_commit() and parse_commit_in_graph_one() to parse the commits
> from the graph and compare against a commit that is loaded separately
> and parsed directly from the object database.

All right, that looks like a nice extension of what was done in previous patch. We want to check that cache (serialized commit graph) matches reality (object database).

>
> Add checks for the root tree OID.

All right; isn't it that now we check almost all information from commit-graph that hs match in object database (with exception of commit parents, I think).

Show 13 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c          | 17 ++++++++++++++++-
>  t/t5318-commit-graph.sh |  7 +++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 0420ebcd87..19ea369fc6 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -880,6 +880,8 @@ int verify_commit_graph(struct commit_graph *g)
>  		return verify_commit_graph_error;

NOTE: we will be checking Commit Data chunk; I think it would be good
idea to verify that size of Commit Data chunk matches (N * (H + 16) bytes)
that format gives us, so that we don't accidentally red outside of
memory if something got screwed up (like number of commits being wrong,
or file truncated).

Show 13 quoted lines

>  
>  	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 && oidcmp(&prev_oid, &cur_oid) >= 0)
> @@ -897,6 +899,11 @@ int verify_commit_graph(struct commit_graph *g)
>  
>  			cur_fanout_pos++;
>  		}
> +
> +		graph_commit = lookup_commit(&cur_oid);

So now I see why we add all commits to memory (to hash structure).

> +		if (!parse_commit_in_graph_one(g, graph_commit))
> +			graph_report("failed to parse %s from commit-graph",
> +				     oid_to_hex(&cur_oid));

All right, this verifies that commit in OID Lookup chunk has parse-able data in Commit Data chunk, isn't it?

Show 14 quoted lines

>  	}
>  
>  	while (cur_fanout_pos < 256) {
> @@ -913,16 +920,24 @@ int verify_commit_graph(struct commit_graph *g)
>  		return verify_commit_graph_error;
>  
>  	for (i = 0; i < g->num_commits; i++) {
> -		struct commit *odb_commit;
> +		struct commit *graph_commit, *odb_commit;
>  
>  		hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
>  
> +		graph_commit = lookup_commit(&cur_oid);
>  		odb_commit = (struct commit *)create_object(cur_oid.hash, alloc_commit_node());

All right, so we have commit data from graph, and commit data from the object database.

Show 12 quoted lines

>  		if (parse_commit_internal(odb_commit, 0, 0)) {
>  			graph_report("failed to parse %s from object database",
>  				     oid_to_hex(&cur_oid));
>  			continue;
>  		}
> +
> +		if (oidcmp(&get_commit_tree_in_graph_one(g, graph_commit)->object.oid,
> +			   get_commit_tree_oid(odb_commit)))
> +			graph_report("root tree OID for commit %s in commit-graph is %s != %s",
> +				     oid_to_hex(&cur_oid),
> +				     oid_to_hex(get_commit_tree_oid(graph_commit)),
> +				     oid_to_hex(get_commit_tree_oid(odb_commit)));

Maybe explicitly say that it doesn't match the value from the object database; OTOH this may be too verbose.

Show 13 quoted lines

>  	}
>  
>  	return verify_commit_graph_error;
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 996a016239..21cc8e82f3 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=`expr $GRAPH_FANOUT_OFFSET + 4 \* 255`
>  GRAPH_OID_LOOKUP_OFFSET=`expr $GRAPH_FANOUT_OFFSET + 4 \* 256`
>  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

All right, so the first is entry into record in Commit Data chunk, and the latter points into tree entry in this record -- which entry is first field:

  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
    * The first H bytes are for the OID of the root tree.

Show 11 quoted lines

>  
>  # usage: corrupt_graph_and_verify <position> <data> <string>
>  # Manipulates the commit-graph file at the position
> @@ -341,4 +343,9 @@ test_expect_success 'detect OID not in object database' '
>  		"from object database"
>  '
>  
> +test_expect_success 'detect incorrect tree OID' '
> +	corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_TREE "\01" \
> +		"root tree OID for commit"
> +'

All right.

I wonder if we can create a test for first check added (that Commit Graph data parses correctly), that is the one with the following error message:

  "failed to parse <OID> from commit-graph file".

> +
>  test_done

Regards,

-- 
Jakub Narębski