Re: [RFC PATCH 02/12] commit-graph: add 'check' subcommand

[Jakub Narebski]

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", _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"))
> +   

[RFC PATCH 02/12] commit-graph: add 'check' subcommand

[Derrick Stolee]

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", _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