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

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> 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@microsoft.com>
> ---
>  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