[PATCH v3 0/9] help: fix usage nits & bugs, completion shellscript->C
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 21, 2021, 22:40 UTC
- Message-ID
- <cover-v3-0.9-00000000000-20210921T223223Z-avarab@gmail.com>
- In-Reply-To
- <cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com>
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@gmail.com/
Æ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@gmail.com>
@@ 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@gmail.com>
## 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@gmail.com>
## 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