Re: [PATCH 01/12] commit-graph: add 'verify' subcommand

From: Martin Ågren <martin.agren@gmail.com>
Date: May 10, 2018, 18:15 UTC
Message-ID: <CAN0heSrg7W86UFKjCXv-NC1+tYr42B-aUksRhWv7Oug1PG8g8Q@mail.gmail.com>
In-Reply-To: <20180510173345.40577-2-dstolee@microsoft.com>

On 10 May 2018 at 19:34, Derrick Stolee <dstolee@microsoft.com> 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.

Show 11 quoted lines

> '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.

> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt

> +'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/?

Show 9 quoted lines

> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -7,11 +7,17 @@
>
>  static char const * const builtin_commit_graph_usage[] = {
>         N_("git commit-graph [--object-dir <objdir>]"),
> +       N_("git commit-graph verify [--object-dir <objdir>]"),
>         N_("git commit-graph read [--object-dir <objdir>]"),
>         N_("git commit-graph write [--object-dir <objdir>] [--append] [--stdin-packs|--stdin-commits]"),

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.)

Show 25 quoted lines

> +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