Derrick Stolee <dstolee@xxxxxxxxxxxxx> writes:
> If the commit-graph file becomes corrupt, we need a way to verify
> its contents match the object database. In the manner of 'git fsck'
> we will implement a 'git commit-graph check' subcommand to report
> all issues with the file.
Bikeshed: should the subcommand be called 'check' or 'verify'?
>
> Add the 'check' subcommand to the 'commit-graph' builtin and its
> documentation. Add a simple test that ensures the command returns
> a zero error code.
It would be nice to have the information that the 'check' subcommand is
currently a [almost no-op] stub in the subject... but that might not
have been possible to fit.
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> Documentation/git-commit-graph.txt | 7 +++++-
> builtin/commit-graph.c | 38 ++++++++++++++++++++++++++++++
> commit-graph.c | 5 ++++
> commit-graph.h | 2 ++
> t/t5318-commit-graph.sh | 5 ++++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index 4c97b555cc..93c7841ba2 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -9,10 +9,10 @@ git-commit-graph - Write and verify Git commit graph files
> SYNOPSIS
> --------
> [verse]
> +'git commit-graph check' [--object-dir <dir>]
> 'git commit-graph read' [--object-dir <dir>]
> 'git commit-graph write' <options> [--object-dir <dir>]
I still think that [--object-dir <dir>] should be the optional parameter
to the "git" wrapper, not to the "git commit-graph" command, i.e.
'git [--object-dir=<dir>] commit-graph <command>'
But this can be done later, in a separate patch series.
>
> -
Stray change.
> DESCRIPTION
> -----------
>
> @@ -52,6 +52,11 @@ existing commit-graph file.
> Read a graph file given by the commit-graph file and output basic
> details about the graph file. Used for debugging purposes.
>
> +'check'::
> +
> +Read the commit-graph file and verify its contents against the object
> +database. Used to check for corrupted data.
> +
I wonder if we should offer to verify file without checking against the
object database (which is the costly part, I think). But this too can
be added later if needed.
>
> EXAMPLES
> --------
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 37420ae0fd..77c1a04932 100644
> --- 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 check [--object-dir <objdir>]"),
> N_("git commit-graph read [--object-dir <objdir>]"),
> N_("git commit-graph write [--object-dir <objdir>] [--append] [--stdin-packs|--stdin-commits]"),
> NULL
Isn't the case that each command would support the
[--object-dir <objdir>] parameter?
> };
>
> +static const char * const builtin_commit_graph_check_usage[] = {
> + N_("git commit-graph check [--object-dir <objdir>]"),
> + NULL
> +};
> +
Looks good to me.
> static const char * const builtin_commit_graph_read_usage[] = {
> N_("git commit-graph read [--object-dir <objdir>]"),
> NULL
> @@ -29,6 +35,36 @@ static struct opts_commit_graph {
> int append;
> } opts;
>
> +
> +static int graph_check(int argc, const char **argv)
> +{
> + struct commit_graph *graph = 0;
This is NULL, isn't it? Shouldn't it be stated as such?
> + char *graph_name;
> +
> + static struct option builtin_commit_graph_check_options[] = {
> + OPT_STRING(0, "object-dir", &opts.obj_dir,
> + N_("dir"),
> + N_("The object directory to store the graph")),
This again is not the directory to _store_ the graph; this is the
directory to _read_ graph from, or directory where the commit graph is
_stored_.
> + OPT_END(),
> + };
> +
> + argc = parse_options(argc, argv, NULL,
> + builtin_commit_graph_check_options,
> + builtin_commit_graph_check_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)
> + die("graph file %s does not exist", graph_name);
Shouldn't we quote pathname? Shouldn't this error message be marked for
translation? Shouldn't we use "commit graph file" explicitly?
> + FREE_AND_NULL(graph_name);
> +
> + return check_commit_graph(graph);
> +}
> +
> static int graph_read(int argc, const char **argv)
> {
> struct commit_graph *graph = NULL;
> @@ -160,6 +196,8 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
> PARSE_OPT_STOP_AT_NON_OPTION);
>
> if (argc > 0) {
> + if (!strcmp(argv[0], "check"))
> + return graph_check(argc, argv);
> if (!strcmp(argv[0], "read"))
> return graph_read(argc, argv);
> if (!strcmp(argv[0], "write"))
> diff --git a/commit-graph.c b/commit-graph.c
> index 3f0c142603..cd0634bba0 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -819,3 +819,8 @@ void write_commit_graph(const char *obj_dir,
> oids.alloc = 0;
> oids.nr = 0;
> }
> +
> +int check_commit_graph(struct commit_graph *g)
> +{
> + return !g;
> +}
I understand that it is just a start of implementing this feature, but
it looks a bit strange that 'read' command does more sanity checks that
the 'check' command...
> diff --git a/commit-graph.h b/commit-graph.h
> index 96cccb10f3..e8c8d99dff 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -53,4 +53,6 @@ void write_commit_graph(const char *obj_dir,
> int nr_commits,
> int append);
>
> +int check_commit_graph(struct commit_graph *g);
> +
> #endif
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 77d85aefe7..e91053271a 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -230,4 +230,9 @@ test_expect_success 'perform fast-forward merge in full repo' '
> test_cmp expect output
> '
>
> +test_expect_success 'git commit-graph check' '
> + cd "$TRASH_DIRECTORY/full" &&
> + git commit-graph check >output
> +'
There should also be negative check, that 'git commit-graph check' fails
if there is no commit-graph file, isn't it?
> +
> test_done
Best,
--
Jakub Narębski
