| Message ID | patch-4.6-6e9bd877866-20210720T113707Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: usage fixes | expand |
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
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]); + usage_with_options(builtin_multi_pack_index_usage, + builtin_multi_pack_index_options); }
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(-)