Re: [PATCH v8 08/14] commit-graph: implement git commit-graph read

[Eric Sunshine]

On Sat, Apr 14, 2018 at 6:15 PM, Jakub Narebski  wrote:
> Derrick Stolee  writes:
>> + NUM_CHUNKS=$((3 + $(echo "$2" | wc -w)))
>
> I don't know if it is possible to do the above in a portable shell
> without using external 'wc' command.  Also, isn't $(( ... )) bashism?

$((...)) is POSIX and used heavily in existing Git test scripts.

Re: [PATCH v8 08/14] commit-graph: implement git commit-graph read

[Jakub Narebski]

Derrick Stolee  writes:

> From: Derrick Stolee 
> Subject: [PATCH v8 08/14] commit-graph: implement git commit-graph read

Minor nit: this is one commit message [subject] among all others that
uses "git commit-graph" instead of "git-commit-graph" in the
description.

Also, perhaps this (and similarly titled commits in this series) would
read better with quotes, that is as:

  commit-graph: implement "git commit-graph read"

Though that might be a matter of personal taste.

>
> Teach git-commit-graph to read commit graph files and summarize their 
> contents.
>
> Use the read subcommand to verify the contents of a commit graph file in the
> tests.

Better would be, in my opinion

  Use the 'read' subcommand

or

  Use the "read" subcommand

>
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |  12 +++
>  builtin/commit-graph.c |  56 
>  commit-graph.c | 137 -
>  commit-graph.h |  23 +
>  t/t5318-commit-graph.sh|  32 +--
>  5 files changed, 254 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 47996e8f89..8aad8303f5 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -9,6 +9,7 @@ git-commit-graph - Write and verify Git commit graph files
>  SYNOPSIS
>  
>  [verse]
> +'git commit-graph read' [--object-dir ]
>  'git commit-graph write'  [--object-dir ]

Why do you need this '[--object-dir ]' parameter?  Anyway, because
Git has the GIT_OBJECT_DIRECTORY environment variable support, I would
expect '--object-dir' to be parameter to the 'git' wrapper/command, like
'--git-dir' is, not to the 'git commit-graph' command, or even only its
selected individual subcommands.

>  
>  
> @@ -35,6 +36,11 @@ COMMANDS
>  Write a commit graph file based on the commits found in packfiles.
>  Includes all commits from the existing commit graph file.
>  
> +'read'::
> +
> +Read a graph file given by the commit-graph file

The above part of sentence reads very strange, as a truism.

>   and output basic
> +details about the graph file. Used for debugging purposes.

I would say that it is 'used' for testing, and is 'useful' (or 'can be
used') for debugging purposes.

> +
>  
>  EXAMPLES
>  
> @@ -45,6 +51,12 @@ EXAMPLES
>  $ git commit-graph write
>  
>  
> +* Read basic information from the commit-graph file.
> ++
> +
> +$ git commit-graph read
> +

I would personally prefer to have example output together with example
calling convention.

> +
>  
>  GIT
>  ---
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 26b6360289..efd39331d7 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -7,10 +7,16 @@
>  
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--object-dir ]"),
> + N_("git commit-graph read [--object-dir ]"),
>   N_("git commit-graph write [--object-dir ]"),
>   NULL
>  };
>  
> +static const char * const builtin_commit_graph_read_usage[] = {
> + N_("git commit-graph read [--object-dir ]"),
> + NULL
> +};
> +
>  static const char * const builtin_commit_graph_write_usage[] = {
>   N_("git commit-graph write [--object-dir ]"),
>   NULL
> @@ -20,6 +26,54 @@ static struct opts_commit_graph {
>   const char *obj_dir;
>  } opts;
>  
> +static int graph_read(int argc, const char **argv)
> +{
> + struct commit_graph *graph = NULL;
> + char *graph_name;
> +
> + static struct option builtin_commit_graph_read_options[] = {
> + OPT_STRING(0, "object-dir", _dir,
> + N_("dir"),
> + N_("The object directory to store the graph")),

Actually it is not the object directory to store the graph, but it is
the object directory to read the commit-graph file from.

> + OPT_END(),
> + };
> +
> + argc = parse_options(argc, argv, NULL,
> +  builtin_commit_graph_read_options,
> +  builtin_commit_graph_read_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);

It might be better to use single quotes around '%s'; this is absolute
pathname (if I understand it correctly), and it may contain spaces in
it.

> + FREE_AND_NULL(graph_name);
> +
> + printf("header: %08x %d %d %d %d\n",

Wouldn't it be better to print signature