Re: [RFC PATCH 02/12] commit-graph: add 'check' subcommand
Derrick Stolee 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 > --- > 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 ] > 'git commit-graph read' [--object-dir ] > 'git commit-graph write' [--object-dir ] I still think that [--object-dir ] should be the optional parameter to the "git" wrapper, not to the "git commit-graph" command, i.e. 'git [--object-dir=] commit-graph ' 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 ]"), > + N_("git commit-graph check [--object-dir ]"), > N_("git commit-graph read [--object-dir ]"), > N_("git commit-graph write [--object-dir ] [--append] > [--stdin-packs|--stdin-commits]"), > NULL Isn't the case that each command would support the [--object-dir ] parameter? > }; > > +static const char * const builtin_commit_graph_check_usage[] = { > + N_("git commit-graph check [--object-dir ]"), > + NULL > +}; > + Looks good to me. > static const char * const builtin_commit_graph_read_usage[] = { > N_("git commit-graph read [--object-dir ]"), > 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); >
[RFC PATCH 02/12] commit-graph: add 'check' subcommand
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. 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. Signed-off-by: Derrick Stolee --- 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 ] 'git commit-graph read' [--object-dir ] 'git commit-graph write' [--object-dir ] - 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. + 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 ]"), + N_("git commit-graph check [--object-dir ]"), N_("git commit-graph read [--object-dir ]"), N_("git commit-graph write [--object-dir ] [--append] [--stdin-packs|--stdin-commits]"), NULL }; +static const char * const builtin_commit_graph_check_usage[] = { + N_("git commit-graph check [--object-dir ]"), + NULL +}; + static const char * const builtin_commit_graph_read_usage[] = { N_("git commit-graph read [--object-dir ]"), 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; + 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")), + 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); + 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; +} 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 +' + test_done -- 2.17.0.39.g685157f7fb