Re: [PATCH v2 02/12] commit-graph: verify file header information

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> writes:

> During a run of 'git commit-graph verify', list the issues with the
> header information in the commit-graph file. Some of this information
> is inferred from the loaded 'struct commit_graph'. Some header
> information is checked as part of load_commit_graph_one().
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b25aaed128..d2db20e49a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -818,7 +818,37 @@ void write_commit_graph(const char *obj_dir,
>  	oids.nr = 0;
>  }
>  
> +static int verify_commit_graph_error;
> +
> +static void graph_report(const char *fmt, ...)
> +{
> +	va_list ap;
> +	struct strbuf sb = STRBUF_INIT;
> +	verify_commit_graph_error = 1;
> +
> +	va_start(ap, fmt);
> +	strbuf_vaddf(&sb, fmt, ap);
> +
> +	fprintf(stderr, "%s\n", sb.buf);
> +	strbuf_release(&sb);
> +	va_end(ap);
> +}
> +
>  int verify_commit_graph(struct commit_graph *g)
>  {
> -	return !g;
> +	if (!g) {
> +		graph_report("no commit-graph file loaded");
> +		return 1;
> +	}

I won't be repeating what Martin said, but I agree with it.  Well, that
or make it a separate patch.

> +
> +	verify_commit_graph_error = 0;
> +

A quick reminder for myself.  The load_commit_graph_one() that is used
to fill the commit_graph parameter alreaady verifies that:
 - file is not too small, i.e. smaller than GRAPH_MIN_SIZE
 - it has correct signature
 - it has correct graph version
 - it has correct hash version
 - chunks [offsets] all fit within file
 - that OID Fanout, OID Lookup, Commit Data and Large Edges chunks are
   not repeated, though not that all required chunks are present

> +	if (!g->chunk_oid_fanout)
> +		graph_report("commit-graph is missing the OID Fanout chunk");
> +	if (!g->chunk_oid_lookup)
> +		graph_report("commit-graph is missing the OID Lookup chunk");
> +	if (!g->chunk_commit_data)
> +		graph_report("commit-graph is missing the Commit Data chunk");

This one checks that all chunks that needs to be present are present.
Nice.

There are a few more things that we can check about CHUNK LOOKUP part.
For example we would detect if file was truncated because the offset of
some chunk would be pointing outside the file... unless the truncation
falls within the last chunk.  We don't check that terminating label
(chunk "\0\0\0\0" offset) is outside file, I think.

We also don't check that positions of subsequent chunks are sorted
(increasing offsets), so that each chunk length is positive.


I also wonder if we shouldn't at least _warn_ about unknown chunks.

> +
> +	return verify_commit_graph_error;
>  }

Nice trick to be able to check as much as possible without segfaulting,
while still returning correct error result.


Testing newly intruduced functionality would be hard, unless relying on
hand-crafted files, or on some helper to produce invalid commit-graph
files.

-- 
Jakub Narębski