Re: [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 19, 2021, 16:55 UTC
Message-ID: <YPWulAIYg+q9Bmr7@nand.local>
In-Reply-To: <patch-4.5-d776424e8c8-20210718T074936Z-avarab@gmail.com>

On Sun, Jul 18, 2021 at 09:58:08AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 32 quoted lines

> Rather than guarding all of the !argc with an additional "if" arm
> let's do an early goto to "usage". This also makes it clear that
> "save_commit_buffer" is not needed in this case.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/commit-graph.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index ff125adf2d5..16d2c517e72 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
>  			     builtin_commit_graph_options,
>  			     builtin_commit_graph_usage,
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (!argc)
> +		goto usage;
>
>  	save_commit_buffer = 0;
>
> -	if (argc > 0) {
> -		if (!strcmp(argv[0], "verify"))
> -			return graph_verify(argc, argv);
> -		if (!strcmp(argv[0], "write"))
> -			return graph_write(argc, argv);
> -	}
> +	if (!strcmp(argv[0], "verify"))
> +		return graph_verify(argc, argv);
> +	else if (argc && !strcmp(argv[0], "write"))
> +		return graph_write(argc, argv);

Unrelated to your patch here, but it may be nice to have an extra error() for unrecognized sub-commands. I actually think that this suggestion also comes from Peff originally, but it does make the "next step" a lot clearer to users who otherwise would have seen a wall of usage text.

Perhaps:

  else {
    error(_("unrecognized subcommand: %s"), argv[0]);
usage:
    usage_with_options(builtin_commit_graph_usage,
                       builtin_commit_graph_options);
  }

Thanks, Taylor