On 10 May 2018 at 19:34, Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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.
> --- 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/?
> --- 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.)
> +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
