git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Taylor Blau <me@ttaylorr.com>
Date
Jul 20, 2021, 18:14 UTC
Message-ID
<YPcSemODaPJ8rkOr@nand.local>
In-Reply-To
<patch-4.6-6e9bd877866-20210720T113707Z-avarab@gmail.com>
On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 48 quoted lines
> 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
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 19 of 40 in “commit-graph: usage fixes”
  1. 0/5 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 18, 2021
  2. 2/5 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 18, 2021
  3. Andrei RybakJul 18, 2021
  4. Taylor BlauJul 19, 2021
  5. 1/5 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 18, 2021
  6. Taylor BlauJul 19, 2021
  7. 3/5 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 18, 2021
  8. Taylor BlauJul 19, 2021
  9. Ævar Arnfjörð BjarmasonJul 20, 2021
  10. 4/5 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 18, 2021
  11. Taylor BlauJul 19, 2021
  12. 5/5 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 18, 2021
  13. Taylor BlauJul 19, 2021
  14. 0/6 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 20, 2021
  15. 1/6 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 20, 2021
  16. 2/6 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 20, 2021
  17. 3/6 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 20, 2021
  18. 4/6 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Jul 20, 2021
  19. Taylor BlauJul 20, 2021
  20. 6/6 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 20, 2021
  21. SZEDER GáborJul 20, 2021
  22. SZEDER GáborJul 20, 2021
  23. Taylor BlauJul 20, 2021
  24. Ævar Arnfjörð BjarmasonJul 20, 2021
  25. Taylor BlauJul 20, 2021
  26. Ævar Arnfjörð BjarmasonJul 21, 2021
  27. Jeff KingJul 21, 2021
  28. Junio C HamanoJul 21, 2021
  29. 5/6 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 20, 2021
  30. Taylor BlauJul 20, 2021
  31. 0/7 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Aug 23, 2021
  32. 1/7 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Aug 23, 2021
  33. 2/7 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Aug 23, 2021
  34. 3/7 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Aug 23, 2021
  35. 4/7 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Aug 23, 2021
  36. 5/7 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Aug 23, 2021
  37. 6/7 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Aug 23, 2021
  38. 7/7 commit-graph: show "unexpected subcommand" errorÆvar Arnfjörð Bjarmason, Aug 23, 2021
  39. Taylor BlauAug 30, 2021
  40. Junio C HamanoAug 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.