| Message ID | patch-v2-4.5-32d73d5273c-20210910T112545Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | help: fix usage nits & bugs, completion shellscript->C | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Fix a bug in the --config option that's been there ever since its > introduction in 3ac68a93fd2 (help: add --config to list all available > config, 2018-05-26). Die when --all and --config are combined, > combining them doesn't make sense. > > The code for the --config option when combined with an earlier > refactoring done to support the --guide option in > 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would > cause us to take the "--all" branch early and ignore the --config > option. > > Let's instead list these as incompatible, both in the synopsis and > help output, and enforce it in the code itself. Of course, "git help -c -a" might be a good enough UI to ask for these variables that are usually hidden due to being deprecated, but implementing that would be a much larger surgery (I do not think the config_name_list[] has enough information as the process to generate it discards the deprecated ones), so this is OK, I think. > @@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix) > int nongit; > enum help_format parsed_help_format; > const char *page; > + int need_config = 0; > > argc = parse_options(argc, argv, prefix, builtin_help_options, > builtin_help_usage, 0); > parsed_help_format = help_format; > > + /* Incompatible options */ > + if (show_all && show_config) > + usage_msg_opt(_("--config and --all cannot be combined"), > + builtin_help_usage, builtin_help_options); > + > + if (show_config && show_guides) > + usage_msg_opt(_("--config and --guides cannot be combined"), > + builtin_help_usage, builtin_help_options); > + > /* Options that take no further arguments */ > + if (argc && show_config) > + usage_msg_opt(_("--config cannot be combined with command or guide names"), > + builtin_help_usage, builtin_help_options); > if (argc && show_guides) > - usage_msg_opt(_("--guides cannot be combined with other options"), > + usage_msg_opt(_("--guides cannot be combined with command or guide names"), > builtin_help_usage, builtin_help_options); This almost makes me wonder if we should be using OPT_CMDMODE and taking advantage of its built-in "X and Y are incompatible" detection. > - if (show_all) { > + need_config = show_all || show_config; > + if (need_config) > git_config(git_help_config, NULL); This change does not seem to be explained. We didn't handle help-config when "git help -c" was asked, but now we do, because...? > + if (show_all) { > if (verbose) { > setup_pager(); > list_all_cmds_help(); > @@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix) > list_commands(colopts, &main_cmds, &other_cmds); > } > > + if (show_guides) > + list_guides_help(); > + > + if (show_all || show_guides) { > + printf("%s\n", _(git_more_info_string)); > + return 0; > + } As guides, all and config are mutually exclusive, it is unclear what we gain by moving these two above. The resulting code does not seem to be more clear then before. If anything, switch (show_mode) { case HELP_ALL_MODE: ... the body of the first "if" ... printf("%s\n", _(git_more_info_string)); return 0; case HELP_GUIDES_MODE: list_guides_help(); printf("%s\n", _(git_more_info_string)); return 0; case HELP_CONFIG_MODE: ... the body of the follwing "if" ... return 0; default: /* show a manual page */ break; } may make it easier to follow. > if (show_config) { > int for_human = show_config == 1; > > @@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) > return 0; > } > > - if (show_guides) > - list_guides_help(); > - > - if (show_all || show_guides) { > - printf("%s\n", _(git_more_info_string)); > - return 0; > - } > - > if (!argv[0]) { > printf(_("usage: %s%s"), _(git_usage_string), "\n\n"); > list_common_cmds_help(); > diff --git a/t/t0012-help.sh b/t/t0012-help.sh > index 595bf81f133..cbc9b64f79f 100755 > --- a/t/t0012-help.sh > +++ b/t/t0012-help.sh > @@ -35,7 +35,12 @@ test_expect_success 'basic help commands' ' > ' > > test_expect_success 'invalid usage' ' > - test_expect_code 129 git help -g git-add > + test_expect_code 129 git help -g git-add && > + test_expect_code 129 git help -c git-add && > + test_expect_code 129 git help -g git-add && Why two "-g git-add"? > + > + test_expect_code 129 git help -a -c && > + test_expect_code 129 git help -g -c > ' > > test_expect_success "works for commands and guides by default" '
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index cb8e3d4da9e..96d5f598b4b 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -11,6 +11,7 @@ SYNOPSIS 'git help' [-a|--all [--[no-]verbose]] [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] 'git help' [-g|--guides] +'git help' [-c|--config] DESCRIPTION ----------- diff --git a/builtin/help.c b/builtin/help.c index 51b18c291d8..05ba2cbe380 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = { N_("git help [-a|--all] [--[no-]verbose]]\n" " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), N_("git help [-g|--guides]"), + N_("git help [-c|--config]"), NULL }; @@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix) int nongit; enum help_format parsed_help_format; const char *page; + int need_config = 0; argc = parse_options(argc, argv, prefix, builtin_help_options, builtin_help_usage, 0); parsed_help_format = help_format; + /* Incompatible options */ + if (show_all && show_config) + usage_msg_opt(_("--config and --all cannot be combined"), + builtin_help_usage, builtin_help_options); + + if (show_config && show_guides) + usage_msg_opt(_("--config and --guides cannot be combined"), + builtin_help_usage, builtin_help_options); + /* Options that take no further arguments */ + if (argc && show_config) + usage_msg_opt(_("--config cannot be combined with command or guide names"), + builtin_help_usage, builtin_help_options); if (argc && show_guides) - usage_msg_opt(_("--guides cannot be combined with other options"), + usage_msg_opt(_("--guides cannot be combined with command or guide names"), builtin_help_usage, builtin_help_options); - if (show_all) { + need_config = show_all || show_config; + if (need_config) git_config(git_help_config, NULL); + + if (show_all) { if (verbose) { setup_pager(); list_all_cmds_help(); @@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix) list_commands(colopts, &main_cmds, &other_cmds); } + if (show_guides) + list_guides_help(); + + if (show_all || show_guides) { + printf("%s\n", _(git_more_info_string)); + return 0; + } + if (show_config) { int for_human = show_config == 1; @@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } - if (show_guides) - list_guides_help(); - - if (show_all || show_guides) { - printf("%s\n", _(git_more_info_string)); - return 0; - } - if (!argv[0]) { printf(_("usage: %s%s"), _(git_usage_string), "\n\n"); list_common_cmds_help(); diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 595bf81f133..cbc9b64f79f 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -35,7 +35,12 @@ test_expect_success 'basic help commands' ' ' test_expect_success 'invalid usage' ' - test_expect_code 129 git help -g git-add + test_expect_code 129 git help -g git-add && + test_expect_code 129 git help -c git-add && + test_expect_code 129 git help -g git-add && + + test_expect_code 129 git help -a -c && + test_expect_code 129 git help -g -c ' test_expect_success "works for commands and guides by default" '
Fix a bug in the --config option that's been there ever since its introduction in 3ac68a93fd2 (help: add --config to list all available config, 2018-05-26). Die when --all and --config are combined, combining them doesn't make sense. The code for the --config option when combined with an earlier refactoring done to support the --guide option in 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would cause us to take the "--all" branch early and ignore the --config option. Let's instead list these as incompatible, both in the synopsis and help output, and enforce it in the code itself. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-help.txt | 1 + builtin/help.c | 37 +++++++++++++++++++++++++++---------- t/t0012-help.sh | 7 ++++++- 3 files changed, 34 insertions(+), 11 deletions(-)