Re: [PATCH v3 12/20] commit-graph: verify parent list
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jun 1, 2018, 23:21 UTC
- Message-ID
- <86lgbyum8l.fsf@gmail.com>
- In-Reply-To
- <20180524162504.158394-13-dstolee@microsoft.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?
Show 6 quoted lines
> > 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)?
Show 33 quoted lines
>
> 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.
Show 6 quoted lines
> +
> + 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.
Show 8 quoted lines
> +
> + 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?
Show 25 quoted lines
> } > > 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?
Show 5 quoted lines
> + > +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