Re: [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Refactor the "goto usage" pattern added in
> cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
> command, 2021-03-30) to maintain the same brevity, but doesn't run
> afoul of the recommendation in CodingGuidelines about braces:
>
>     When there are multiple arms to a conditional and some of them
>     require braces, enclose even a single line block in braces for
>     consistency[...]
>
> Let's also change "argv == 0" to juts "!argv", per:
>
>     Do not explicitly compare an integral value with constant 0 or
>     '\0', or a pointer value with constant NULL[...]
>
> I'm changing this because in a subsequent commit I'll make
> builtin/commit-graph.c use the same pattern, having the two similarly
> structured commands match aids readability.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/multi-pack-index.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 5d3ea445fdb..2952388a8eb 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
>  	if (!opts.object_dir)
>  		opts.object_dir = get_object_directory();
>
> -	if (argc == 0)
> +	if (!argc)
>  		goto usage;
>
>  	if (!strcmp(argv[0], "repack"))
> @@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
>  		return cmd_multi_pack_index_verify(argc, argv);
>  	else if (!strcmp(argv[0], "expire"))
>  		return cmd_multi_pack_index_expire(argc, argv);
> -	else {
> +
>  usage:
> -		error(_("unrecognized subcommand: %s"), argv[0]);
> -		usage_with_options(builtin_multi_pack_index_usage,
> -				   builtin_multi_pack_index_options);
> -	}
> +	error(_("unrecognized subcommand: %s"), argv[0]);

Not the fault of this patch, but since we jump to this from the "if
(!argc)" conditional, reading "argv[0]" is UB. Some compilers will print
out:

    error: unrecognized subcommand: (null)

which at least doesn't segfault, but is still ugly. I don't mind calling
it ugly since I was the one to introduce this behavior (by accident, of
course).

I sent a patch [1] yesterday to squash this, but we should consider
combining the two (or dropping this patch from the series and then
coming back to it later on).

If you want to combine forces, here's what I think that could look like
(without your s-o-b, which you'll have to reattach).

[1]: https://lore.kernel.org/git/8c0bb3e0dc121bd68f7014000fbb60b28750a0fe.1626715096.git.me@ttaylorr.com/

--- >8 ---

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Subject: [PATCH] multi-pack-index: refactor "goto usage" pattern

Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) to maintain the same brevity, but doesn't run
afoul of the recommendation in CodingGuidelines about braces:

    When there are multiple arms to a conditional and some of them
    require braces, enclose even a single line block in braces for
    consistency[...]

Let's also change "argv == 0" to juts "!argv", per:

    Do not explicitly compare an integral value with constant 0 or
    '\0', or a pointer value with constant NULL[...]

I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.

While here, fix an issue since where cd57bc41bb
(builtin/multi-pack-index.c: display usage on unrecognized command,
2021-03-30) we sometimes jump to 'usage' without any arguments.

Many compilers will save us from a segfault here, but the end result is
ugly, since it mentions an unrecognized subcommand when we didn't even
pass one, and (on GCC) includes "(null)" in its output.

Move the "usage" label down past the error about unrecognized
subcommands so that it is only triggered when it should be. While we're
at it, bulk up our test coverage in this area, too.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c  | 11 +++++------
 t/t5319-multi-pack-index.sh |  6 ++++++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fd..e3a684c842 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();

-	if (argc == 0)
+	if (!argc)
 		goto usage;

 	if (!strcmp(argv[0], "repack"))
@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
 		return cmd_multi_pack_index_verify(argc, argv);
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
-	else {
-usage:
+	else
 		error(_("unrecognized subcommand: %s"), argv[0]);
-		usage_with_options(builtin_multi_pack_index_usage,
-				   builtin_multi_pack_index_options);
-	}
+usage:
+	usage_with_options(builtin_multi_pack_index_usage,
+			   builtin_multi_pack_index_options);
 }
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 5641d158df..fcde6dcded 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -824,4 +824,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
 	)
 '

+
+test_expect_success 'usage shown without sub-command' '
+	test_expect_code 129 git multi-pack-index 2>err &&
+	! test_i18ngrep "unrecognized subcommand" err
+'
+
 test_done
--
2.31.1.163.ga65ce7f831