Re: [RFC PATCH 03/12] commit-graph: check file header information

[Jakub Narebski]

Derrick Stolee  writes:

> During a run of 'git commit-graph check', list the issues with the
> header information in the commit-graph file. Some of this information
> is inferred from the loaded 'struct commit_graph'.
>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index cd0634bba0..c5e5a0f860 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -820,7 +820,34 @@ void write_commit_graph(const char *obj_dir,
>   oids.nr = 0;
>  }
>  
> +static int check_commit_graph_error;
> +#define graph_report(...) { check_commit_graph_error = 1; 
> printf(__VA_ARGS__); }

Shouldn't 'do { ... } while(0);' trick be used here, like e.g. for
trace_performance macro?

> +
>  int check_commit_graph(struct commit_graph *g)
>  {
> - return !g;
> + if (!g) {
> + graph_report(_("no commit-graph file loaded"));
> + return 1;
> + }
> +
> + check_commit_graph_error = 0;
> +

The load_commit_graph_one() function does its own checks, some of whose
are present below, and some of whose are missing.

If it is used, then why duplicate tests - you would not get here as you
would die earlier.

If it is not used, then some tests are missing.

> + if (get_be32(g->data) != GRAPH_SIGNATURE)
> + graph_report(_("commit-graph file has incorrect header"));

The load_commit_graph_one() shows more detailed information:

 "graph signature %X does not match signature %X",
  graph_signature, GRAPH_SIGNATURE)

Also, load_commit_graph_one() checks that the file is not too short, and
we actually can access whole header.

> +
> + if (*(g->data + 4) != 1)
> + graph_report(_("commit-graph file version is not 1"));

Again:

 "graph version %X does not match version %X",
  graph_version, GRAPH_VERSION

Also, here we hardcode the commit-graph file version to 1.

Accidentally, don't we offer backward compatibility, in that if git can
read commit-graph file version 2, it can also read commit-graph file
version 1?

> + if (*(g->data + 5) != GRAPH_OID_VERSION)
> + graph_report(_("commit-graph OID version is not 1 (SHA1)"));

In one part we use symbolic constant, on the other hardcoded values.  If
GRAPH_OID_VERSION changes, what then?

Also:

 "hash version %X does not match version %X",
  hash_version, GRAPH_OID_VERSION

> +
> + 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"));

All right.

> + if (g->hash_len != GRAPH_OID_LEN)
> + graph_report(_("commit-graph has incorrect hash length: %d"), 
> g->hash_len);

We could be more detailed in error report: what hash length should be,
then?

> +
> + return check_commit_graph_error;
>  }

No tests of malformed commit-graph file?

[RFC PATCH 03/12] commit-graph: check file header information

[Derrick Stolee]

During a run of 'git commit-graph check', list the issues with the
header information in the commit-graph file. Some of this information
is inferred from the loaded 'struct commit_graph'.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index cd0634bba0..c5e5a0f860 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -820,7 +820,34 @@ void write_commit_graph(const char *obj_dir,
oids.nr = 0;
 }
 
+static int check_commit_graph_error;
+#define graph_report(...) { check_commit_graph_error = 1; printf(__VA_ARGS__); 
}
+
 int check_commit_graph(struct commit_graph *g)
 {
-   return !g;
+   if (!g) {
+   graph_report(_("no commit-graph file loaded"));
+   return 1;
+   }
+
+   check_commit_graph_error = 0;
+
+   if (get_be32(g->data) != GRAPH_SIGNATURE)
+   graph_report(_("commit-graph file has incorrect header"));
+
+   if (*(g->data + 4) != 1)
+   graph_report(_("commit-graph file version is not 1"));
+   if (*(g->data + 5) != GRAPH_OID_VERSION)
+   graph_report(_("commit-graph OID version is not 1 (SHA1)"));
+
+   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"));
+   if (g->hash_len != GRAPH_OID_LEN)
+   graph_report(_("commit-graph has incorrect hash length: %d"), 
g->hash_len);
+
+   return check_commit_graph_error;
 }
-- 
2.17.0.39.g685157f7fb