On 10 May 2018 at 19:34, Derrick Stolee [off-list ref] wrote:

In case the commit-graph file becomes corrupt, we need a way to
verify its contents match the object database. In the manner of

s/verify its/verify that its/ might read better.

'git fsck' we will implement a 'git commit-graph verify' subcommand
to report all issues with the file.

Add the 'verify' subcommand to the 'commit-graph' builtin and its
documentation. The subcommand is currently a no-op except for
loading the commit-graph into memory, which may trigger run-time
errors that would be caught by normal use. Add a simple test that
ensures the command returns a zero error code.

If no commit-graph file exists, this is an acceptable state. Do
not report any errors.

This all makes sense to me.

+'verify'::
+
+Read the commit-graph file and verify its contents against the object
+database. Used to verify for corrupted data.

s/verify for/check for/?

Minor nit: In the man-page, you added verify after read, which makes
more sense I think (r < v < w).

(I also note that the man-page synopsis doesn't give the no-subcommand
usage.)

+static int graph_verify(int argc, const char **argv)
+{
+       struct commit_graph *graph = 0;
+       char *graph_name;
+
+       static struct option builtin_commit_graph_verify_options[] = {
+               OPT_STRING(0, "object-dir", &opts.obj_dir,
+                          N_("dir"),
+                          N_("The object directory to store the graph")),
+               OPT_END(),
+       };
+
+       argc = parse_options(argc, argv, NULL,
+                            builtin_commit_graph_verify_options,
+                            builtin_commit_graph_verify_usage, 0);
+
+       if (!opts.obj_dir)
+               opts.obj_dir = get_object_directory();
+
+       graph_name = get_commit_graph_filename(opts.obj_dir);
+       graph = load_commit_graph_one(graph_name);
+
+       if (!graph)
+               return 0;
+       FREE_AND_NULL(graph_name);

Maybe the FREE_AND_NULL could go immediately after the call to
`load_commit_graph_one()`. It makes it more obvious that you're done
with the name, and -- perhaps more importantly -- means that throwing a
leak-checker at this won't complain if we take the early return.

+
+       return verify_commit_graph(graph);

A leak-checker would still complain about leaking `graph`. I think it
would be ok to just UNLEAK it before calling `verify_commit_graph()`.
This is IMHO close enough to returning from `cmd_commit_graph()` to make
UNLEAK an acceptable, or even the correct, solution.

I realize that `graph_read()` is doing something similar to this patch
already, so what you have here is certainly the most consistent code.

Martin