This series fixes various bugs & edge cases in the "git help" command, and improves and splits the internal-only "--completion-for-config" option into two, and as a result can get rid of an awk/sort -u pipeline in the bash completion. Since the v2 I: * Read & tried to address all the feedback in one way or another * We now use OPT_CMDMODE() in builtin/help.c, that's indeed much nicer * I kept the preceding non-OPT_CMDMODE() steps pretty much as-is, since we can't use OPT_CMDMODE() until we've explained ad changed --all, --guides and --config to all be mutually exclusive. * The "vars" completion helper is now called --completion-for-config, which as 8/9 explains is done for backwards compatibility. * There's a new post-cleanup 9/9 which moves the "colopts" code into the help.c library where it's used, this was in response to Junio's comment asking about why I'd moved a git_config() call. That older code was buggy, but now our git_config() usage makes more sense. 1. https://lore.kernel.org/git/cover-v2-0.5-00000000000-20210910T112545Z-avarab@xxxxxxxxx/ Ævar Arnfjörð Bjarmason (9): help: correct the usage string in -h and documentation help: correct usage & behavior of "git help --guides" help tests: add test for --config output help: correct logic error in combining --all and --config help: correct logic error in combining --all and --guides help: simplify by moving to OPT_CMDMODE() help tests: test --config-for-completion option & output help / completion: make "git help" do the hard work help: move column config discovery to help.c library Documentation/git-help.txt | 9 +- builtin/help.c | 131 ++++++++++++++++--------- contrib/completion/git-completion.bash | 21 ++-- help.c | 16 ++- help.h | 2 +- parse-options.h | 6 +- t/t0012-help.sh | 49 +++++++++ 7 files changed, 166 insertions(+), 68 deletions(-) Range-diff against v2: 1: b10bfd21f14 = 1: 5341ddbe23e help: correct the usage string in -h and documentation 2: 039639a0dd3 ! 2: e24ab59bc94 help: correct usage & behavior of "git help --guides" @@ Commit message As noted in 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) and a133737b809 (doc: include --guide option description - for "git help", 2013-04-02) which introduced the --guide option it + for "git help", 2013-04-02) which introduced the --guide option, it cannot be combined with e.g. <command>. - Change both the usage string to reflect that, and test and assert for - this behavior in the command itself. Now that we assert this in code - we don't need to exhaustively describe the previous confusing behavior - in the documentation either, instead of silently ignoring the provided + Change the command and the "SYNOPSIS" section to reflect that desired + behavior. Now that we assert this in code we don't need to + exhaustively describe the previous confusing behavior in the + documentation either, instead of silently ignoring the provided argument we'll now error out. - The comment being removed was added in 15f7d494380 (builtin/help.c: - split "-a" processing into two, 2013-04-02). The "Ignore any remaining - args" part of it is now no longer applicable as explained above, let's - just remove it entirely, it's rather obvious that if we're returning - we're done. + The "We're done. Ignore any remaining args" comment added in + 15f7d494380 (builtin/help.c: split "-a" processing into two, + 2013-04-02) can now be removed, it's obvious that we're asserting the + behavior with the check of "argc". + + The "--config" option is still missing from the synopsis, it will be + added in a subsequent commit where we'll fix bugs in its + implementation. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ t/t0012-help.sh: 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 add +' + test_expect_success "works for commands and guides by default" ' 3: 258282095de ! 3: 6a8965e1b5b help tests: add test for --config output @@ Commit message 2018-05-26) looks like. We should not be emitting anything except config variables and the brief usage information at the end here. + The second test regexp here might not match three-level variables in + general, as their second level could contain ".", but in this case + we're always emitting what we extract from the documentation, so it's + all strings like: + + foo.<name>.bar + + If we did introduce something like variable example content here we'd + like this to break, since we'd then be likely to break the + git-completion.bash. + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## t/t0012-help.sh ## @@ t/t0012-help.sh: test_expect_success 'git help -g' ' + git help -c >help.output && + cat >expect <<-\EOF && + -+ '"'"'git help config'"'"' for more information ++ '\''git help config'\'' for more information + EOF + grep -v -E \ + -e "^[^.]+\.[^.]+$" \ 4: 32d73d5273c ! 4: d5df231954a help: correct logic error in combining --all and --config @@ builtin/help.c: static const char * const builtin_help_usage[] = { }; @@ builtin/help.c: 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; @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix) + 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(); + if (show_all) { @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix) list_commands(colopts, &main_cmds, &other_cmds); } @@ t/t0012-help.sh: 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_code 129 git help -g add ++ test_expect_code 129 git help -g add && ++ test_expect_code 129 git help -a -c ' test_expect_success "works for commands and guides by default" ' -: ----------- > 5: bf3ac71f256 help: correct logic error in combining --all and --guides -: ----------- > 6: b52269eeab9 help: simplify by moving to OPT_CMDMODE() -: ----------- > 7: cc031c8d339 help tests: test --config-for-completion option & output 5: e995a42cb8d ! 8: 836e19f8612 help / completion: make "git help" do the hard work @@ Commit message We can instead simply do the relevant parsing ourselves (we were doing most of it already), and call string_list_remove_duplicates() after already sorting the list, so the caller doesn't need to invoke "sort - -u". + -u". The "--config-for-completion" output is the same as before after + being passed through "sort -u". - This changes the output of the section list to emit lines like "alias" - instead of "alias.". The dot suffix is better done as an argument to - __gitcomp(). + Then add a new "--config-sections-for-completion" option. Under that + output we'll emit config sections like "alias" (instead of "alias." in + the --config-for-completion output). - This means that we'll have the list_config_help() function do a bit - more work, let's switch its "for_human" to passing a full - "show_config", but as an enum type so we can have the compiler check - what values we're expecting to get. + We need to be careful to leave the "--config-for-completion" option + compatible with users git, but are still running a shell with an older + git-completion.bash. If we e.g. changed the option name they'd see + messages about git-completion.bash being unable to find the + "--config-for-completion" option. + + Such backwards compatibility isn't something we should bend over + backwards for, it's only helping users who: + + * Upgrade git + * Are in an old shell + * The git-completion.bash in that shell hasn't cached the old + "--config-for-completion" output already. + + But since it's easy in this case to retain compatibility, let's do it, + the older versions of git-completion.bash won't care that the input + they get doesn't change after a "sort -u". + + While we're at it let's make "--config-for-completion" die if there's + anything left over in "argc", and do the same in the new + "--config-sections-for-completion" option. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/help.c ## -@@ builtin/help.c: static const char *html_path; +@@ builtin/help.c: enum help_format { + HELP_FORMAT_WEB + }; - static int show_all = 0; - static int show_guides = 0; --static int show_config; +enum show_config_type { -+ SHOW_CONFIG_UNSET = 0, + SHOW_CONFIG_HUMAN, + SHOW_CONFIG_VARS, + SHOW_CONFIG_SECTIONS, -+} show_config; - static int verbose = 1; - static unsigned int colopts; - static enum help_format help_format = HELP_FORMAT_NONE; ++}; ++ + static enum help_action { + HELP_ACTION_ALL = 1, + HELP_ACTION_GUIDES, + HELP_ACTION_CONFIG, + HELP_ACTION_CONFIG_FOR_COMPLETION, ++ HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, + } cmd_mode; + + static const char *html_path; @@ builtin/help.c: static struct option builtin_help_options[] = { - OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")), - OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")), - OPT_BOOL('c', "config", &show_config, N_("print all configuration variable names")), -- OPT_SET_INT_F(0, "config-for-completion", &show_config, "", 2, PARSE_OPT_HIDDEN), -+ OPT_SET_INT_F(0, "config-for-completion-vars", &show_config, "", -+ SHOW_CONFIG_VARS, PARSE_OPT_HIDDEN), -+ OPT_SET_INT_F(0, "config-for-completion-sections", &show_config, "", -+ SHOW_CONFIG_SECTIONS, PARSE_OPT_HIDDEN), - OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN), - OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"), - HELP_FORMAT_WEB), + HELP_ACTION_CONFIG), + OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "", + HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN), ++ OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "", ++ HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN), + + OPT_END(), + }; @@ builtin/help.c: struct slot_expansion { int found; }; @@ builtin/help.c: static void list_config_help(int for_human) + break; + case SHOW_CONFIG_VARS: + break; -+ case SHOW_CONFIG_UNSET: -+ BUG("should not get SHOW_CONFIG_UNSET here"); } - wildcard = strchr(var, '*'); @@ builtin/help.c: static void list_config_help(int for_human) static enum help_format parse_help_format(const char *format) @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix) + printf("%s\n", _(git_more_info_string)); return 0; - } - -- if (show_config) { -- int for_human = show_config == 1; -+ switch (show_config) { -+ case SHOW_CONFIG_UNSET: -+ break; -+ case SHOW_CONFIG_VARS: -+ case SHOW_CONFIG_SECTIONS: -+ list_config_help(show_config); - -- if (!for_human) { -- list_config_help(for_human); -- return 0; -- } + case HELP_ACTION_CONFIG_FOR_COMPLETION: +- list_config_help(0); ++ no_extra_argc(argc); ++ list_config_help(SHOW_CONFIG_VARS); + return 0; -+ case SHOW_CONFIG_HUMAN: ++ case HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION: ++ no_extra_argc(argc); ++ list_config_help(SHOW_CONFIG_SECTIONS); + return 0; + case HELP_ACTION_CONFIG: + no_extra_argc(argc); setup_pager(); -- list_config_help(for_human); -+ list_config_help(show_config); +- list_config_help(1); ++ list_config_help(SHOW_CONFIG_HUMAN); printf("\n%s\n", _("'git help config' for more information")); -+ return 0; } - ## contrib/completion/git-completion.bash ## @@ contrib/completion/git-completion.bash: __git_config_vars= @@ contrib/completion/git-completion.bash: __git_config_vars= { test -n "$__git_config_vars" || - __git_config_vars="$(git help --config-for-completion | sort -u)" -+ __git_config_vars="$(git help --config-for-completion-vars)" ++ __git_config_vars="$(git help --config-for-completion)" +} + +__git_config_sections= +__git_compute_config_sections () +{ + test -n "$__git_config_sections" || -+ __git_config_sections="$(git help --config-for-completion-sections)" ++ __git_config_sections="$(git help --config-sections-for-completion)" } # Completes possible values of various configuration variables. @@ contrib/completion/git-completion.bash: __git_complete_config_variable_name () } ## t/t0012-help.sh ## -@@ t/t0012-help.sh: test_expect_success 'git help -c' ' - test_cmp expect actual +@@ t/t0012-help.sh: test_expect_success 'invalid usage' ' + test_expect_code 129 git help -a -g && + + test_expect_code 129 git help -g -c && +- test_expect_code 0 git help --config-for-completion add ++ test_expect_code 129 git help --config-for-completion add && ++ test_expect_code 129 git help --config-sections-for-completion add ' -+test_expect_success 'git help --config-for-completion-vars' ' -+ git help -c >human && -+ grep -E \ -+ -e "^[^.]+\.[^.]+$" \ -+ -e "^[^.]+\.[^.]+\.[^.]+$" human | -+ sed -e "s/\*.*//" -e "s/<.*//" | -+ sort -u >human.munged && -+ -+ git help --config-for-completion-vars >vars && -+ test_cmp human.munged vars -+' -+ -+test_expect_success 'git help --config-for-completion-sections' ' + test_expect_success "works for commands and guides by default" ' +@@ t/t0012-help.sh: test_expect_success 'git help --config-for-completion' ' + sort -u >human.munged && + + git help --config-for-completion >vars && +- sort -u <vars >vars.new && +- mv vars.new vars && + test_cmp human.munged vars + ' + ++test_expect_success 'git help --config-sections-for-completion' ' + git help -c >human && + grep -E \ + -e "^[^.]+\.[^.]+$" \ @@ t/t0012-help.sh: test_expect_success 'git help -c' ' + sed -e "s/\..*//" | + sort -u >human.munged && + -+ git help --config-for-completion-sections >sections && ++ git help --config-sections-for-completion >sections && + test_cmp human.munged sections +' + -: ----------- > 9: 29ee7cf375b help: move column config discovery to help.c library -- 2.33.0.1098.gf02a64c1a2d
