[PATCH 0/7] help: test and fix small "help -g" regression

[PATCH 0/7] help: test and fix small "help -g" regression

[Ævar Arnfjörð Bjarmason]

Fix a trivial regression in 17b3e515050 (Merge branch
'nd/command-list' into nd/complete-config-vars, 2018-05-29) where we'd
overly whitespace pad "git help -g", since the codepath moved to a
function that assumed it needed to "\n\n"-pad multiple items being
emitted, so we'd print an extra leading newline when printing one
item (as opposed to "git help -a").

In doing so I wanted to add regression tests, and to do that we first
need to make the format stable ("git help -a" will change depending on
whatever git-* you have in $PATH).

These are things I noticed when re-rolling my no-yet-picked-up series
to move Documentation/technical/* to manpages[1]. An unsubmitted
re-roll of this series depends on this one. I'll hold off on a
re-submission of it until this series has graduated.

1. https://lore.kernel.org/git/cover-v2-0.5-00000000000-20211212T194047Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (7):
  help doc: add missing "]" to "[-a|--all]"
  help.c: use puts() instead of printf{,_ln}() for consistency
  help tests: test "git" and "git help [-a|-g] spacing
  help.c: split up list_all_cmds_help() function
  help: error if [-a|-g|-c] and [-i|-m|-w] are combined
  help: add --no-[external-commands|aliases] for use with --all
  help: don't print "\n" before single-section output

 Documentation/git-help.txt | 12 ++++-
 builtin/help.c             | 34 ++++++++++++--
 help.c                     | 37 ++++++++++++----
 help.h                     |  2 +-
 t/t0012-help.sh            | 91 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 161 insertions(+), 15 deletions(-)

-- 
2.34.1.1257.g2af47340c7b

[PATCH 1/7] help doc: add missing "]" to "[-a|--all]"

[Ævar Arnfjörð Bjarmason]

Add a missing "]" to documentation added in 63eae83f8f3 (help: add "-a
--verbose" to list all commands with synopsis, 2018-05-20). This made
it seem as though "--[no-]verbose" can only be provided with "--all",
not "-a". The corresponding usage information in the C
code ("builtin_help_usage") does not have the same problem.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 44ea63cc6d3..cf1d53e9499 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all [--[no-]verbose]]
+'git help' [-a|--all] [--[no-]verbose]
 	   [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
-- 
2.34.1.1257.g2af47340c7b

[PATCH 2/7] help.c: use puts() instead of printf{,_ln}() for consistency

[Ævar Arnfjörð Bjarmason]

Change code in "help.c" that used printf_ln() without format
specifiers to use puts() instead, as other existing code in the file
does. Let's also change related code to use puts() instead of the
equivalent of calling "printf" with a "%s\n" format.

This formatting-only change will make a subsequent functional change
easier to read, as it'll be changing code that's consistently using
the same functions to do the same things.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 973e47cdc30..0ba9b866f03 100644
--- a/help.c
+++ b/help.c
@@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
 		uint32_t mask = catdesc[i].category;
 		const char *desc = catdesc[i].desc;
 
-		printf("\n%s\n", _(desc));
+		putchar('\n');
+		puts(_(desc));
 		print_command_list(cmds, mask, longest);
 	}
 	free(cmds);
@@ -317,7 +318,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
 	}
 
 	if (other_cmds->cnt) {
-		printf_ln(_("git commands available from elsewhere on your $PATH"));
+		puts(_("git commands available from elsewhere on your $PATH"));
 		putchar('\n');
 		pretty_print_cmdnames(other_cmds, colopts);
 		putchar('\n');
@@ -439,7 +440,7 @@ void list_all_cmds_help(void)
 	struct cmdname_help *aliases;
 	int i, longest;
 
-	printf_ln(_("See 'git help <command>' to read about a specific subcommand"));
+	puts(_("See 'git help <command>' to read about a specific subcommand"));
 	print_cmd_by_category(main_categories, &longest);
 
 	list_all_other_cmds(&others);
-- 
2.34.1.1257.g2af47340c7b

[PATCH 3/7] help tests: test "git" and "git help [-a|-g] spacing

[Ævar Arnfjörð Bjarmason]

There's logic in "help.c"'s "print_cmd_by_category()" to emit "help"
output with particular spacing, which doesn't make much sense when
emitting only one section with "help -g".

Let's add tests for the current spacing in preparation for a
subsequent whitespace formatting fix, and make sure that that fix
doesn't cause regressions for the "git" and "git help" output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0012-help.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 91b68c74a15..6ac293c19ed 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -138,6 +138,51 @@ test_expect_success 'git help --config-sections-for-completion' '
 	test_cmp human.munged sections
 '
 
+test_section_spacing () {
+	cat >expect &&
+	"$@" >out &&
+	grep -E "(^[^ ]|^$)" out >actual
+}
+
+test_section_spacing_trailer () {
+	test_section_spacing "$@" &&
+	test_expect_code 1 git >out &&
+	sed -n '/list available subcommands/,$p' <out >>expect
+}
+
+
+for cmd in git "git help"
+do
+	test_expect_success "'$cmd' section spacing" '
+		test_section_spacing_trailer git help <<-\EOF &&
+		usage: git [--version] [--help] [-C <path>] [-c <name>=<value>]
+
+		These are common Git commands used in various situations:
+
+		start a working area (see also: git help tutorial)
+
+		work on the current change (see also: git help everyday)
+
+		examine the history and state (see also: git help revisions)
+
+		grow, mark and tweak your common history
+
+		collaborate (see also: git help workflows)
+
+		EOF
+		test_cmp expect actual
+	'
+done
+
+test_expect_success "'git help -g' section spacing" '
+	test_section_spacing_trailer git help -g <<-\EOF &&
+
+	The Git concept guides are:
+
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.34.1.1257.g2af47340c7b

[PATCH 4/7] help.c: split up list_all_cmds_help() function

[Ævar Arnfjörð Bjarmason]

Split up the listing of commands and aliases from
list_all_cmds_help(). This will make a subsequent functional change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/help.c b/help.c
index 0ba9b866f03..ef1aa26efa6 100644
--- a/help.c
+++ b/help.c
@@ -433,15 +433,10 @@ static int get_alias(const char *var, const char *value, void *data)
 	return 0;
 }
 
-void list_all_cmds_help(void)
+static void list_all_cmds_help_external_commands(void)
 {
 	struct string_list others = STRING_LIST_INIT_DUP;
-	struct string_list alias_list = STRING_LIST_INIT_DUP;
-	struct cmdname_help *aliases;
-	int i, longest;
-
-	puts(_("See 'git help <command>' to read about a specific subcommand"));
-	print_cmd_by_category(main_categories, &longest);
+	int i;
 
 	list_all_other_cmds(&others);
 	if (others.nr)
@@ -449,6 +444,13 @@ void list_all_cmds_help(void)
 	for (i = 0; i < others.nr; i++)
 		printf("   %s\n", others.items[i].string);
 	string_list_clear(&others, 0);
+}
+
+static void list_all_cmds_help_aliases(int longest)
+{
+	struct string_list alias_list = STRING_LIST_INIT_DUP;
+	struct cmdname_help *aliases;
+	int i;
 
 	git_config(get_alias, &alias_list);
 	string_list_sort(&alias_list);
@@ -474,6 +476,17 @@ void list_all_cmds_help(void)
 	string_list_clear(&alias_list, 1);
 }
 
+void list_all_cmds_help(void)
+{
+	int longest;
+
+	puts(_("See 'git help <command>' to read about a specific subcommand"));
+	print_cmd_by_category(main_categories, &longest);
+
+	list_all_cmds_help_external_commands();
+	list_all_cmds_help_aliases(longest);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
 	int i;
-- 
2.34.1.1257.g2af47340c7b

[PATCH 5/7] help: error if [-a|-g|-c] and [-i|-m|-w] are combined

[Ævar Arnfjörð Bjarmason]

Add more sanity checking to "git help" usage by erroring out if these
man viewer options are combined with incompatible command-modes that
will never use these documentation viewers.

This continues the work started in d35d03cf93e (help: simplify by
moving to OPT_CMDMODE(), 2021-09-22) of adding more sanity checking to
"git help". Doing this allows us to clarify the "SYNOPSIS" in the
documentation, and the "git help -h" output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt |  2 +-
 builtin/help.c             | 20 ++++++++++++++++++--
 t/t0012-help.sh            | 10 ++++++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index cf1d53e9499..61d52d30f6c 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git help' [-a|--all] [--[no-]verbose]
-	   [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
+'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
 
diff --git a/builtin/help.c b/builtin/help.c
index d387131dd83..125f50f1bb0 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -75,8 +75,8 @@ static struct option builtin_help_options[] = {
 };
 
 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 [-a|--all] [--[no-]verbose]]"),
+	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
 	N_("git help [-c|--config]"),
 	NULL
@@ -581,6 +581,13 @@ static void no_extra_argc(int argc)
 			      builtin_help_usage, builtin_help_options);
 }
 
+static void no_format(void)
+{
+	if (help_format != HELP_FORMAT_NONE)
+		usage_msg_opt(_("[-a|--all] cannot be combined with [[-i|--info] [-m|--man] [-w|--web]]"),
+			      builtin_help_usage, builtin_help_options);
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
@@ -593,6 +600,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
+		no_format();
 		if (verbose) {
 			setup_pager();
 			list_all_cmds_help();
@@ -605,19 +613,27 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		break;
 	case HELP_ACTION_GUIDES:
 		no_extra_argc(argc);
+		no_format();
+
 		list_guides_help();
 		printf("%s\n", _(git_more_info_string));
 		return 0;
 	case HELP_ACTION_CONFIG_FOR_COMPLETION:
 		no_extra_argc(argc);
+		no_format();
+
 		list_config_help(SHOW_CONFIG_VARS);
 		return 0;
 	case HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION:
 		no_extra_argc(argc);
+		no_format();
+
 		list_config_help(SHOW_CONFIG_SECTIONS);
 		return 0;
 	case HELP_ACTION_CONFIG:
 		no_extra_argc(argc);
+		no_format();
+
 		setup_pager();
 		list_config_help(SHOW_CONFIG_HUMAN);
 		printf("\n%s\n", _("'git help config' for more information"));
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 6ac293c19ed..a8c603abd44 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -46,6 +46,16 @@ test_expect_success 'invalid usage' '
 	test_expect_code 129 git help --config-sections-for-completion add
 '
 
+for opt in '-a' '-g' '-c' '--config-for-completion' '--config-sections-for-completion'
+do
+	test_expect_success "invalid usage of '$opt' with [-i|-m|-w]" '
+		git help $opt &&
+		test_expect_code 129 git help $opt -i &&
+		test_expect_code 129 git help $opt -m &&
+		test_expect_code 129 git help $opt -w
+	'
+done
+
 test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-- 
2.34.1.1257.g2af47340c7b

[PATCH 6/7] help: add --no-[external-commands|aliases] for use with --all

[Ævar Arnfjörð Bjarmason]

Add the ability to only emit git's own usage information under
--all. This also allows us to extend the "test_section_spacing" tests
added in a preceding commit to test "git help --all"
output.

Previously we could not do that, as the tests might find a git-*
command in the "$PATH", which would make the output differ from one
setup to another.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt | 10 +++++++++-
 builtin/help.c             | 16 ++++++++++++++--
 help.c                     |  8 +++++---
 help.h                     |  2 +-
 t/t0012-help.sh            | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 61d52d30f6c..9307445c333 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all] [--[no-]verbose]
+'git help' [-a|--all] [--[no-]verbose] [--[no-](external-commands|aliases)]
 'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
@@ -49,6 +49,14 @@ OPTIONS
 	Prints all the available commands on the standard output. This
 	option overrides any given command or guide name.
 
+--no-external-commands::
+	When used with `--all`, exclude the listing of external "git-*"
+	commands found in the `$PATH`.
+
+--no-aliases::
+	When used with `--all`, exclude the listing of configured
+	aliases.
+
 --verbose::
 	When used with `--all` print description for all recognized
 	commands. This is the default.
diff --git a/builtin/help.c b/builtin/help.c
index 125f50f1bb0..1131b437389 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -51,9 +51,14 @@ static const char *html_path;
 static int verbose = 1;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
+static int show_external_commands = -1;
+static int show_aliases = -1;
 static struct option builtin_help_options[] = {
 	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
 		    HELP_ACTION_ALL),
+	OPT_BOOL(0, "external-commands", &show_external_commands,
+		 N_("show external commands in --all?")),
+	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all?")),
 	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
 	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"),
@@ -75,7 +80,7 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [-a|--all] [--[no-]verbose]]"),
+	N_("git help [-a|--all] [--[no-]verbose]] [--[no-](external-commands|aliases)]"),
 	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
 	N_("git help [-c|--config]"),
@@ -598,12 +603,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	if (cmd_mode != HELP_ACTION_ALL &&
+	    (show_external_commands >= 0 ||
+	     show_aliases >= 0))
+		usage_msg_opt(_("the '--no-[external-commands|aliases]' options can only be used with '--all'"),
+			      builtin_help_usage, builtin_help_options);
+
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
 		no_format();
 		if (verbose) {
 			setup_pager();
-			list_all_cmds_help();
+			list_all_cmds_help(show_external_commands,
+					   show_aliases);
 			return 0;
 		}
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
diff --git a/help.c b/help.c
index ef1aa26efa6..2296f06ad9f 100644
--- a/help.c
+++ b/help.c
@@ -476,15 +476,17 @@ static void list_all_cmds_help_aliases(int longest)
 	string_list_clear(&alias_list, 1);
 }
 
-void list_all_cmds_help(void)
+void list_all_cmds_help(int show_external_commands, int show_aliases)
 {
 	int longest;
 
 	puts(_("See 'git help <command>' to read about a specific subcommand"));
 	print_cmd_by_category(main_categories, &longest);
 
-	list_all_cmds_help_external_commands();
-	list_all_cmds_help_aliases(longest);
+	if (show_external_commands)
+		list_all_cmds_help_external_commands();
+	if (show_aliases)
+		list_all_cmds_help_aliases(longest);
 }
 
 int is_in_cmdlist(struct cmdnames *c, const char *s)
diff --git a/help.h b/help.h
index 9d383f1a0b2..971a3ad855a 100644
--- a/help.h
+++ b/help.h
@@ -20,7 +20,7 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 void list_common_cmds_help(void);
-void list_all_cmds_help(void);
+void list_all_cmds_help(int show_external_commands, int show_aliases);
 void list_guides_help(void);
 
 void list_all_main_cmds(struct string_list *list);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index a8c603abd44..c41b412e34a 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -54,6 +54,19 @@ do
 		test_expect_code 129 git help $opt -m &&
 		test_expect_code 129 git help $opt -w
 	'
+
+	if test "$opt" = "-a"
+	then
+		continue
+	fi
+
+	test_expect_success "invalid usage of '$opt' with --no-external-commands" '
+		test_expect_code 129 git help $opt --no-external-commands
+	'
+
+	test_expect_success "invalid usage of '$opt' with --no-aliases" '
+		test_expect_code 129 git help $opt --no-external-commands
+	'
 done
 
 test_expect_success "works for commands and guides by default" '
@@ -184,6 +197,30 @@ do
 	'
 done
 
+test_expect_success "'git help -a' section spacing" '
+	test_section_spacing \
+		git help -a --no-external-commands --no-aliases <<-\EOF &&
+	See '\''git help <command>'\'' to read about a specific subcommand
+
+	Main Porcelain Commands
+
+	Ancillary Commands / Manipulators
+
+	Ancillary Commands / Interrogators
+
+	Interacting with Others
+
+	Low-level Commands / Manipulators
+
+	Low-level Commands / Interrogators
+
+	Low-level Commands / Syncing Repositories
+
+	Low-level Commands / Internal Helpers
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success "'git help -g' section spacing" '
 	test_section_spacing_trailer git help -g <<-\EOF &&
 
-- 
2.34.1.1257.g2af47340c7b

[PATCH 7/7] help: don't print "\n" before single-section output

[Ævar Arnfjörð Bjarmason]

Fix a formatting regression in 1b81d8cb19d (help: use command-list.txt
for the source of guides, 2018-05-20). Adjust the output of "git help
--guides" and any other future single-section commands so that a
newline isn't inserted before the only section being printed.

This changes the output from:

    $ git help --guides

    The Git concept guides are:
    [...]

To:

    $ git help --guides
    The Git concept guides are:
    [...]

That we started printing an extra "\n" in 1b81d8cb19d wasn't intended,
but an emergent effect of moving all of the printing of "git help"
output to code that was ready to handle printing N sections.

With 1b81d8cb19d we started using the "print_cmd_by_category()"
function added earlier in the same series, or in cfb22a02ab5 (help:
use command-list.h for common command list, 2018-05-10).

Fixing this formatting nit is easy enough. Let's have all of the
output that would like to be "\n"-separated from other lines emit its
own "\n". We then adjust "print_cmd_by_category()" to only print a
"\n" to delimit the sections it's printing out.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c          | 5 ++++-
 t/t0012-help.sh | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 2296f06ad9f..30be45cea59 100644
--- a/help.c
+++ b/help.c
@@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
 		uint32_t mask = catdesc[i].category;
 		const char *desc = catdesc[i].desc;
 
-		putchar('\n');
+		if (i)
+			putchar('\n');
 		puts(_(desc));
 		print_command_list(cmds, mask, longest);
 	}
@@ -328,6 +329,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
 void list_common_cmds_help(void)
 {
 	puts(_("These are common Git commands used in various situations:"));
+	putchar('\n');
 	print_cmd_by_category(common_categories, NULL);
 }
 
@@ -481,6 +483,7 @@ void list_all_cmds_help(int show_external_commands, int show_aliases)
 	int longest;
 
 	puts(_("See 'git help <command>' to read about a specific subcommand"));
+	putchar('\n');
 	print_cmd_by_category(main_categories, &longest);
 
 	if (show_external_commands)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c41b412e34a..aae01f15229 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -223,7 +223,6 @@ test_expect_success "'git help -a' section spacing" '
 
 test_expect_success "'git help -g' section spacing" '
 	test_section_spacing_trailer git help -g <<-\EOF &&
-
 	The Git concept guides are:
 
 	EOF
-- 
2.34.1.1257.g2af47340c7b

Re: [PATCH 5/7] help: error if [-a|-g|-c] and [-i|-m|-w] are combined

[Eric Sunshine]

On Tue, Dec 28, 2021 at 10:36 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Add more sanity checking to "git help" usage by erroring out if these
> man viewer options are combined with incompatible command-modes that
> will never use these documentation viewers.
>
> This continues the work started in d35d03cf93e (help: simplify by
> moving to OPT_CMDMODE(), 2021-09-22) of adding more sanity checking to
> "git help". Doing this allows us to clarify the "SYNOPSIS" in the
> documentation, and the "git help -h" output.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/builtin/help.c b/builtin/help.c
> @@ -581,6 +581,13 @@ static void no_extra_argc(int argc)
> +static void no_format(void)
> +{
> +       if (help_format != HELP_FORMAT_NONE)
> +               usage_msg_opt(_("[-a|--all] cannot be combined with [[-i|--info] [-m|--man] [-w|--web]]"),
> +                             builtin_help_usage, builtin_help_options);
> +}

Nit: The square brackets in the message may be unnecessarily
confusing. (Indeed, what exactly do they mean in this context?) Also,
the short options may not add much value: the user who typed `-w`
knows presumably that it is shorthand for `--web`. So, one
simplification would be:

    '--all' cannot be combined with '--info', '--man', or '--web'

Re: [PATCH 6/7] help: add --no-[external-commands|aliases] for use with --all

[Eric Sunshine]

On Tue, Dec 28, 2021 at 10:36 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Add the ability to only emit git's own usage information under
> --all. This also allows us to extend the "test_section_spacing" tests
> added in a preceding commit to test "git help --all"
> output.
>
> Previously we could not do that, as the tests might find a git-*
> command in the "$PATH", which would make the output differ from one
> setup to another.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> @@ -8,7 +8,7 @@ git-help - Display help information about Git
> -'git help' [-a|--all] [--[no-]verbose]
> +'git help' [-a|--all] [--[no-]verbose] [--[no-](external-commands|aliases)]

Are these two new options mutually exclusive as this synopsis line
seems to indicate? Glancing at the code, it looks like they can be
used together. So, perhaps the synopsis should just say:

    'git help' [-a|--all] [--[no-]verbose] [--[no-]external-commands]
[--[no-]aliases]

> diff --git a/builtin/help.c b/builtin/help.c
> @@ -51,9 +51,14 @@ static const char *html_path;
>         OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
>                     HELP_ACTION_ALL),
> +       OPT_BOOL(0, "external-commands", &show_external_commands,
> +                N_("show external commands in --all?")),
> +       OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all?")),
>         OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),

Nit: I don't see question marks on any of the existing options. It's
not clear what purpose they serve here.

> @@ -75,7 +80,7 @@ static struct option builtin_help_options[] = {
>  static const char * const builtin_help_usage[] = {
> -       N_("git help [-a|--all] [--[no-]verbose]]"),
> +       N_("git help [-a|--all] [--[no-]verbose]] [--[no-](external-commands|aliases)]"),

Same observation made above about mutual exclusion.

Re: [PATCH 5/7] help: error if [-a|-g|-c] and [-i|-m|-w] are combined

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 28 2021, Eric Sunshine wrote:

> On Tue, Dec 28, 2021 at 10:36 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Add more sanity checking to "git help" usage by erroring out if these
>> man viewer options are combined with incompatible command-modes that
>> will never use these documentation viewers.
>>
>> This continues the work started in d35d03cf93e (help: simplify by
>> moving to OPT_CMDMODE(), 2021-09-22) of adding more sanity checking to
>> "git help". Doing this allows us to clarify the "SYNOPSIS" in the
>> documentation, and the "git help -h" output.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/builtin/help.c b/builtin/help.c
>> @@ -581,6 +581,13 @@ static void no_extra_argc(int argc)
>> +static void no_format(void)
>> +{
>> +       if (help_format != HELP_FORMAT_NONE)
>> +               usage_msg_opt(_("[-a|--all] cannot be combined with [[-i|--info] [-m|--man] [-w|--web]]"),
>> +                             builtin_help_usage, builtin_help_options);
>> +}
>
> Nit: The square brackets in the message may be unnecessarily
> confusing. (Indeed, what exactly do they mean in this context?) Also,
> the short options may not add much value: the user who typed `-w`
> knows presumably that it is shorthand for `--web`. So, one
> simplification would be:
>
>     '--all' cannot be combined with '--info', '--man', or '--web'

Thanks, will change it.

[PATCH v2 0/9] help: tests, parse_options() sanity, fix "help -g" regression

[Ævar Arnfjörð Bjarmason]

A late re-roll of [1] which addresses all the comments Eric Sunshine
raised, thanks for the review, and sorry the v2 took so long!

As the range-diff shows the main change is a mid-series set of changes
to sanity check more "git help <opts>" incompaitibilities. The
SYNOPSIS issues etc. Eric noted have also been fixed.

1. https://lore.kernel.org/git/cover-0.7-00000000000-20211228T153456Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (9):
  help doc: add missing "]" to "[-a|--all]"
  help.c: use puts() instead of printf{,_ln}() for consistency
  help tests: test "git" and "git help [-a|-g] spacing
  help.c: split up list_all_cmds_help() function
  help: note the option name on option incompatibility
  help: correct usage & behavior of "git help --all"
  help: error if [-a|-g|-c] and [-i|-m|-w] are combined
  help: add --no-[external-commands|aliases] for use with --all
  help: don't print "\n" before single-section output

 Documentation/git-help.txt | 15 ++++--
 builtin/help.c             | 63 +++++++++++++++++++++----
 help.c                     | 37 +++++++++++----
 help.h                     |  2 +-
 t/t0012-help.sh            | 94 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 187 insertions(+), 24 deletions(-)

Range-diff against v1:
 1:  652dae26bf4 =  1:  c4b66c36c17 help doc: add missing "]" to "[-a|--all]"
 2:  f84662469a3 =  2:  124643c4b35 help.c: use puts() instead of printf{,_ln}() for consistency
 3:  3956937cf3b !  3:  3e39116f197 help tests: test "git" and "git help [-a|-g] spacing
    @@ t/t0012-help.sh: test_expect_success 'git help --config-sections-for-completion'
     +'
     +
      test_expect_success 'generate builtin list' '
    + 	mkdir -p sub &&
      	git --list-cmds=builtins >builtins
    - '
 4:  f040dd549b4 =  4:  f9c4d5e2d28 help.c: split up list_all_cmds_help() function
 5:  12ff152bd57 <  -:  ----------- help: error if [-a|-g|-c] and [-i|-m|-w] are combined
 -:  ----------- >  5:  e5c49089106 help: note the option name on option incompatibility
 -:  ----------- >  6:  868e8a6cf83 help: correct usage & behavior of "git help --all"
 -:  ----------- >  7:  992ee6580ac help: error if [-a|-g|-c] and [-i|-m|-w] are combined
 6:  a5ef9f69530 !  8:  c81c0cbbcdb help: add --no-[external-commands|aliases] for use with --all
    @@ Documentation/git-help.txt: git-help - Display help information about Git
      --------
      [verse]
     -'git help' [-a|--all] [--[no-]verbose]
    -+'git help' [-a|--all] [--[no-]verbose] [--[no-](external-commands|aliases)]
    ++'git help' [-a|--all] [--[no-]verbose] [--[no-]external-commands] [--[no-]aliases]
      'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
      'git help' [-g|--guides]
      'git help' [-c|--config]
     @@ Documentation/git-help.txt: OPTIONS
    - 	Prints all the available commands on the standard output. This
    - 	option overrides any given command or guide name.
    + --all::
    + 	Prints all the available commands on the standard output.
      
     +--no-external-commands::
     +	When used with `--all`, exclude the listing of external "git-*"
    @@ builtin/help.c: static const char *html_path;
      	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
      		    HELP_ACTION_ALL),
     +	OPT_BOOL(0, "external-commands", &show_external_commands,
    -+		 N_("show external commands in --all?")),
    -+	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all?")),
    ++		 N_("show external commands in --all")),
    ++	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all")),
      	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
      	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"),
    @@ builtin/help.c: static struct option builtin_help_options[] = {
      
      static const char * const builtin_help_usage[] = {
     -	N_("git help [-a|--all] [--[no-]verbose]]"),
    -+	N_("git help [-a|--all] [--[no-]verbose]] [--[no-](external-commands|aliases)]"),
    ++	N_("git help [-a|--all] [--[no-]verbose]] [--[no-]external-commands] [--[no-]aliases]"),
      	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
      	N_("git help [-g|--guides]"),
      	N_("git help [-c|--config]"),
    @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
     +
      	switch (cmd_mode) {
      	case HELP_ACTION_ALL:
    - 		no_format();
    + 		opt_mode_usage(argc, "--all", help_format);
      		if (verbose) {
      			setup_pager();
     -			list_all_cmds_help();
 7:  08fd12fe7b4 =  9:  08dc693dc3e help: don't print "\n" before single-section output
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 1/9] help doc: add missing "]" to "[-a|--all]"

[Ævar Arnfjörð Bjarmason]

Add a missing "]" to documentation added in 63eae83f8f3 (help: add "-a
--verbose" to list all commands with synopsis, 2018-05-20). This made
it seem as though "--[no-]verbose" can only be provided with "--all",
not "-a". The corresponding usage information in the C
code ("builtin_help_usage") does not have the same problem.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 44ea63cc6d3..cf1d53e9499 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all [--[no-]verbose]]
+'git help' [-a|--all] [--[no-]verbose]
 	   [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency

[Ævar Arnfjörð Bjarmason]

Change code in "help.c" that used printf_ln() without format
specifiers to use puts() instead, as other existing code in the file
does. Let's also change related code to use puts() instead of the
equivalent of calling "printf" with a "%s\n" format.

This formatting-only change will make a subsequent functional change
easier to read, as it'll be changing code that's consistently using
the same functions to do the same things.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 71444906ddf..77af953826e 100644
--- a/help.c
+++ b/help.c
@@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
 		uint32_t mask = catdesc[i].category;
 		const char *desc = catdesc[i].desc;
 
-		printf("\n%s\n", _(desc));
+		putchar('\n');
+		puts(_(desc));
 		print_command_list(cmds, mask, longest);
 	}
 	free(cmds);
@@ -317,7 +318,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
 	}
 
 	if (other_cmds->cnt) {
-		printf_ln(_("git commands available from elsewhere on your $PATH"));
+		puts(_("git commands available from elsewhere on your $PATH"));
 		putchar('\n');
 		pretty_print_cmdnames(other_cmds, colopts);
 		putchar('\n');
@@ -439,7 +440,7 @@ void list_all_cmds_help(void)
 	struct cmdname_help *aliases;
 	int i, longest;
 
-	printf_ln(_("See 'git help <command>' to read about a specific subcommand"));
+	puts(_("See 'git help <command>' to read about a specific subcommand"));
 	print_cmd_by_category(main_categories, &longest);
 
 	list_all_other_cmds(&others);
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 3/9] help tests: test "git" and "git help [-a|-g] spacing

[Ævar Arnfjörð Bjarmason]

There's logic in "help.c"'s "print_cmd_by_category()" to emit "help"
output with particular spacing, which doesn't make much sense when
emitting only one section with "help -g".

Let's add tests for the current spacing in preparation for a
subsequent whitespace formatting fix, and make sure that that fix
doesn't cause regressions for the "git" and "git help" output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0012-help.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index cbd725ccac8..9ac3f5d3c4b 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -138,6 +138,51 @@ test_expect_success 'git help --config-sections-for-completion' '
 	test_cmp human.munged sections
 '
 
+test_section_spacing () {
+	cat >expect &&
+	"$@" >out &&
+	grep -E "(^[^ ]|^$)" out >actual
+}
+
+test_section_spacing_trailer () {
+	test_section_spacing "$@" &&
+	test_expect_code 1 git >out &&
+	sed -n '/list available subcommands/,$p' <out >>expect
+}
+
+
+for cmd in git "git help"
+do
+	test_expect_success "'$cmd' section spacing" '
+		test_section_spacing_trailer git help <<-\EOF &&
+		usage: git [--version] [--help] [-C <path>] [-c <name>=<value>]
+
+		These are common Git commands used in various situations:
+
+		start a working area (see also: git help tutorial)
+
+		work on the current change (see also: git help everyday)
+
+		examine the history and state (see also: git help revisions)
+
+		grow, mark and tweak your common history
+
+		collaborate (see also: git help workflows)
+
+		EOF
+		test_cmp expect actual
+	'
+done
+
+test_expect_success "'git help -g' section spacing" '
+	test_section_spacing_trailer git help -g <<-\EOF &&
+
+	The Git concept guides are:
+
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'generate builtin list' '
 	mkdir -p sub &&
 	git --list-cmds=builtins >builtins
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 4/9] help.c: split up list_all_cmds_help() function

[Ævar Arnfjörð Bjarmason]

Split up the listing of commands and aliases from
list_all_cmds_help(). This will make a subsequent functional change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/help.c b/help.c
index 77af953826e..004117347ee 100644
--- a/help.c
+++ b/help.c
@@ -433,15 +433,10 @@ static int get_alias(const char *var, const char *value, void *data)
 	return 0;
 }
 
-void list_all_cmds_help(void)
+static void list_all_cmds_help_external_commands(void)
 {
 	struct string_list others = STRING_LIST_INIT_DUP;
-	struct string_list alias_list = STRING_LIST_INIT_DUP;
-	struct cmdname_help *aliases;
-	int i, longest;
-
-	puts(_("See 'git help <command>' to read about a specific subcommand"));
-	print_cmd_by_category(main_categories, &longest);
+	int i;
 
 	list_all_other_cmds(&others);
 	if (others.nr)
@@ -449,6 +444,13 @@ void list_all_cmds_help(void)
 	for (i = 0; i < others.nr; i++)
 		printf("   %s\n", others.items[i].string);
 	string_list_clear(&others, 0);
+}
+
+static void list_all_cmds_help_aliases(int longest)
+{
+	struct string_list alias_list = STRING_LIST_INIT_DUP;
+	struct cmdname_help *aliases;
+	int i;
 
 	git_config(get_alias, &alias_list);
 	string_list_sort(&alias_list);
@@ -474,6 +476,17 @@ void list_all_cmds_help(void)
 	string_list_clear(&alias_list, 1);
 }
 
+void list_all_cmds_help(void)
+{
+	int longest;
+
+	puts(_("See 'git help <command>' to read about a specific subcommand"));
+	print_cmd_by_category(main_categories, &longest);
+
+	list_all_cmds_help_external_commands();
+	list_all_cmds_help_aliases(longest);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
 	int i;
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 5/9] help: note the option name on option incompatibility

[Ævar Arnfjörð Bjarmason]

Change the errors added in d35d03cf93e (help: simplify by moving to
OPT_CMDMODE(), 2021-09-22) to quote the offending option at the user
when invoked as e.g.:

    git help --guides garbage

Now instead of:

    fatal: this option doesn't take any other arguments

We'll emit:

    fatal: the '--guides' option doesn't take any non-option arguments

Let's also rename the function, as it will be extended to do other
checks that aren't "no extra argc" in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index d387131dd83..1c1581ef850 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -574,11 +574,12 @@ static const char *check_git_cmd(const char* cmd)
 	return cmd;
 }
 
-static void no_extra_argc(int argc)
+static void opt_mode_usage(int argc, const char *opt_mode)
 {
 	if (argc)
-		usage_msg_opt(_("this option doesn't take any other arguments"),
-			      builtin_help_usage, builtin_help_options);
+		usage_msg_optf(_("the '%s' option doesn't take any non-option arguments"),
+			       builtin_help_usage, builtin_help_options,
+			       opt_mode);
 }
 
 int cmd_help(int argc, const char **argv, const char *prefix)
@@ -604,20 +605,20 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		printf("%s\n", _(git_more_info_string));
 		break;
 	case HELP_ACTION_GUIDES:
-		no_extra_argc(argc);
+		opt_mode_usage(argc, "--guides");
 		list_guides_help();
 		printf("%s\n", _(git_more_info_string));
 		return 0;
 	case HELP_ACTION_CONFIG_FOR_COMPLETION:
-		no_extra_argc(argc);
+		opt_mode_usage(argc, "--config-for-completion");
 		list_config_help(SHOW_CONFIG_VARS);
 		return 0;
 	case HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION:
-		no_extra_argc(argc);
+		opt_mode_usage(argc, "--config-sections-for-completion");
 		list_config_help(SHOW_CONFIG_SECTIONS);
 		return 0;
 	case HELP_ACTION_CONFIG:
-		no_extra_argc(argc);
+		opt_mode_usage(argc, "--config");
 		setup_pager();
 		list_config_help(SHOW_CONFIG_HUMAN);
 		printf("\n%s\n", _("'git help config' for more information"));
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 6/9] help: correct usage & behavior of "git help --all"

[Ævar Arnfjörð Bjarmason]

Do the same for the "--all" option that I did for "--guides" in
9856ea6785c (help: correct usage & behavior of "git help --guides",
2021-09-22). I.e. we've documented it as ignoring non-option
arguments, let's have it error out instead.

As with other changes made in 62f035aee3f (Merge branch
'ab/help-config-vars', 2021-10-13) this is technically a change in
behavior, but in practice it's just a bug fix. We were ignoring this
before, but by erroring we can simplify our documentation and
synopsis, as well as avoid user confusion as they wonder what the
difference between e.g. "git help --all" and "git help --all status"
is (there wasn't any difference).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt | 5 ++---
 builtin/help.c             | 5 +++--
 t/t0012-help.sh            | 3 +++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index cf1d53e9499..d07590c8ff7 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git help' [-a|--all] [--[no-]verbose]
-	   [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
+'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
 
@@ -46,8 +46,7 @@ OPTIONS
 -------
 -a::
 --all::
-	Prints all the available commands on the standard output. This
-	option overrides any given command or guide name.
+	Prints all the available commands on the standard output.
 
 --verbose::
 	When used with `--all` print description for all recognized
diff --git a/builtin/help.c b/builtin/help.c
index 1c1581ef850..b682446bbf5 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -75,8 +75,8 @@ static struct option builtin_help_options[] = {
 };
 
 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 [-a|--all] [--[no-]verbose]]"),
+	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
 	N_("git help [-c|--config]"),
 	NULL
@@ -594,6 +594,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
+		opt_mode_usage(argc, "--all");
 		if (verbose) {
 			setup_pager();
 			list_all_cmds_help();
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 9ac3f5d3c4b..c87730aa920 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -35,6 +35,9 @@ test_expect_success 'basic help commands' '
 '
 
 test_expect_success 'invalid usage' '
+	test_expect_code 129 git help -a add &&
+	test_expect_code 129 git help --all add &&
+
 	test_expect_code 129 git help -g add &&
 	test_expect_code 129 git help -a -c &&
 
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 7/9] help: error if [-a|-g|-c] and [-i|-m|-w] are combined

[Ævar Arnfjörð Bjarmason]

Add more sanity checking to "git help" usage by erroring out if these
man viewer options are combined with incompatible command-modes that
will never use these documentation viewers.

This continues the work started in d35d03cf93e (help: simplify by
moving to OPT_CMDMODE(), 2021-09-22) of adding more sanity checking to
"git help". Doing this allows us to clarify the "SYNOPSIS" in the
documentation, and the "git help -h" output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c  | 41 +++++++++++++++++++++++++++++++++++------
 t/t0012-help.sh | 10 ++++++++++
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index b682446bbf5..1fc45adfcc7 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -574,12 +574,40 @@ static const char *check_git_cmd(const char* cmd)
 	return cmd;
 }
 
-static void opt_mode_usage(int argc, const char *opt_mode)
+static void no_help_format(const char *opt_mode, enum help_format fmt)
+{
+	const char *opt_fmt;
+
+	switch (fmt) {
+	case HELP_FORMAT_NONE:
+		return;
+	case HELP_FORMAT_MAN:
+		opt_fmt = "--man";
+		break;
+	case HELP_FORMAT_INFO:
+		opt_fmt = "--info";
+		break;
+	case HELP_FORMAT_WEB:
+		opt_fmt = "--web";
+		break;
+	default:
+		BUG("unreachable");
+	}
+
+	usage_msg_optf(_("options '%s' and '%s' cannot be used together"),
+		       builtin_help_usage, builtin_help_options, opt_mode,
+		       opt_fmt);
+}
+
+static void opt_mode_usage(int argc, const char *opt_mode,
+			   enum help_format fmt)
 {
 	if (argc)
 		usage_msg_optf(_("the '%s' option doesn't take any non-option arguments"),
 			       builtin_help_usage, builtin_help_options,
 			       opt_mode);
+
+	no_help_format(opt_mode, fmt);
 }
 
 int cmd_help(int argc, const char **argv, const char *prefix)
@@ -594,7 +622,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
-		opt_mode_usage(argc, "--all");
+		opt_mode_usage(argc, "--all", help_format);
 		if (verbose) {
 			setup_pager();
 			list_all_cmds_help();
@@ -606,20 +634,21 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		printf("%s\n", _(git_more_info_string));
 		break;
 	case HELP_ACTION_GUIDES:
-		opt_mode_usage(argc, "--guides");
+		opt_mode_usage(argc, "--guides", help_format);
 		list_guides_help();
 		printf("%s\n", _(git_more_info_string));
 		return 0;
 	case HELP_ACTION_CONFIG_FOR_COMPLETION:
-		opt_mode_usage(argc, "--config-for-completion");
+		opt_mode_usage(argc, "--config-for-completion", help_format);
 		list_config_help(SHOW_CONFIG_VARS);
 		return 0;
 	case HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION:
-		opt_mode_usage(argc, "--config-sections-for-completion");
+		opt_mode_usage(argc, "--config-sections-for-completion",
+			       help_format);
 		list_config_help(SHOW_CONFIG_SECTIONS);
 		return 0;
 	case HELP_ACTION_CONFIG:
-		opt_mode_usage(argc, "--config");
+		opt_mode_usage(argc, "--config", help_format);
 		setup_pager();
 		list_config_help(SHOW_CONFIG_HUMAN);
 		printf("\n%s\n", _("'git help config' for more information"));
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c87730aa920..f12783fd153 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -49,6 +49,16 @@ test_expect_success 'invalid usage' '
 	test_expect_code 129 git help --config-sections-for-completion add
 '
 
+for opt in '-a' '-g' '-c' '--config-for-completion' '--config-sections-for-completion'
+do
+	test_expect_success "invalid usage of '$opt' with [-i|-m|-w]" '
+		git help $opt &&
+		test_expect_code 129 git help $opt -i &&
+		test_expect_code 129 git help $opt -m &&
+		test_expect_code 129 git help $opt -w
+	'
+done
+
 test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 8/9] help: add --no-[external-commands|aliases] for use with --all

[Ævar Arnfjörð Bjarmason]

Add the ability to only emit git's own usage information under
--all. This also allows us to extend the "test_section_spacing" tests
added in a preceding commit to test "git help --all"
output.

Previously we could not do that, as the tests might find a git-*
command in the "$PATH", which would make the output differ from one
setup to another.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-help.txt | 10 +++++++++-
 builtin/help.c             | 16 ++++++++++++++--
 help.c                     |  8 +++++---
 help.h                     |  2 +-
 t/t0012-help.sh            | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index d07590c8ff7..239c68db457 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all] [--[no-]verbose]
+'git help' [-a|--all] [--[no-]verbose] [--[no-]external-commands] [--[no-]aliases]
 'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
 'git help' [-g|--guides]
 'git help' [-c|--config]
@@ -48,6 +48,14 @@ OPTIONS
 --all::
 	Prints all the available commands on the standard output.
 
+--no-external-commands::
+	When used with `--all`, exclude the listing of external "git-*"
+	commands found in the `$PATH`.
+
+--no-aliases::
+	When used with `--all`, exclude the listing of configured
+	aliases.
+
 --verbose::
 	When used with `--all` print description for all recognized
 	commands. This is the default.
diff --git a/builtin/help.c b/builtin/help.c
index 1fc45adfcc7..01eda326c31 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -51,9 +51,14 @@ static const char *html_path;
 static int verbose = 1;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
+static int show_external_commands = -1;
+static int show_aliases = -1;
 static struct option builtin_help_options[] = {
 	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
 		    HELP_ACTION_ALL),
+	OPT_BOOL(0, "external-commands", &show_external_commands,
+		 N_("show external commands in --all")),
+	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all")),
 	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
 	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"),
@@ -75,7 +80,7 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [-a|--all] [--[no-]verbose]]"),
+	N_("git help [-a|--all] [--[no-]verbose]] [--[no-]external-commands] [--[no-]aliases]"),
 	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
 	N_("git help [-c|--config]"),
@@ -620,12 +625,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	if (cmd_mode != HELP_ACTION_ALL &&
+	    (show_external_commands >= 0 ||
+	     show_aliases >= 0))
+		usage_msg_opt(_("the '--no-[external-commands|aliases]' options can only be used with '--all'"),
+			      builtin_help_usage, builtin_help_options);
+
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
 		opt_mode_usage(argc, "--all", help_format);
 		if (verbose) {
 			setup_pager();
-			list_all_cmds_help();
+			list_all_cmds_help(show_external_commands,
+					   show_aliases);
 			return 0;
 		}
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
diff --git a/help.c b/help.c
index 004117347ee..45a21e7e35c 100644
--- a/help.c
+++ b/help.c
@@ -476,15 +476,17 @@ static void list_all_cmds_help_aliases(int longest)
 	string_list_clear(&alias_list, 1);
 }
 
-void list_all_cmds_help(void)
+void list_all_cmds_help(int show_external_commands, int show_aliases)
 {
 	int longest;
 
 	puts(_("See 'git help <command>' to read about a specific subcommand"));
 	print_cmd_by_category(main_categories, &longest);
 
-	list_all_cmds_help_external_commands();
-	list_all_cmds_help_aliases(longest);
+	if (show_external_commands)
+		list_all_cmds_help_external_commands();
+	if (show_aliases)
+		list_all_cmds_help_aliases(longest);
 }
 
 int is_in_cmdlist(struct cmdnames *c, const char *s)
diff --git a/help.h b/help.h
index 9d383f1a0b2..971a3ad855a 100644
--- a/help.h
+++ b/help.h
@@ -20,7 +20,7 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 void list_common_cmds_help(void);
-void list_all_cmds_help(void);
+void list_all_cmds_help(int show_external_commands, int show_aliases);
 void list_guides_help(void);
 
 void list_all_main_cmds(struct string_list *list);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index f12783fd153..64321480c68 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -57,6 +57,19 @@ do
 		test_expect_code 129 git help $opt -m &&
 		test_expect_code 129 git help $opt -w
 	'
+
+	if test "$opt" = "-a"
+	then
+		continue
+	fi
+
+	test_expect_success "invalid usage of '$opt' with --no-external-commands" '
+		test_expect_code 129 git help $opt --no-external-commands
+	'
+
+	test_expect_success "invalid usage of '$opt' with --no-aliases" '
+		test_expect_code 129 git help $opt --no-external-commands
+	'
 done
 
 test_expect_success "works for commands and guides by default" '
@@ -187,6 +200,30 @@ do
 	'
 done
 
+test_expect_success "'git help -a' section spacing" '
+	test_section_spacing \
+		git help -a --no-external-commands --no-aliases <<-\EOF &&
+	See '\''git help <command>'\'' to read about a specific subcommand
+
+	Main Porcelain Commands
+
+	Ancillary Commands / Manipulators
+
+	Ancillary Commands / Interrogators
+
+	Interacting with Others
+
+	Low-level Commands / Manipulators
+
+	Low-level Commands / Interrogators
+
+	Low-level Commands / Syncing Repositories
+
+	Low-level Commands / Internal Helpers
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success "'git help -g' section spacing" '
 	test_section_spacing_trailer git help -g <<-\EOF &&
 
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v2 9/9] help: don't print "\n" before single-section output

[Ævar Arnfjörð Bjarmason]

Fix a formatting regression in 1b81d8cb19d (help: use command-list.txt
for the source of guides, 2018-05-20). Adjust the output of "git help
--guides" and any other future single-section commands so that a
newline isn't inserted before the only section being printed.

This changes the output from:

    $ git help --guides

    The Git concept guides are:
    [...]

To:

    $ git help --guides
    The Git concept guides are:
    [...]

That we started printing an extra "\n" in 1b81d8cb19d wasn't intended,
but an emergent effect of moving all of the printing of "git help"
output to code that was ready to handle printing N sections.

With 1b81d8cb19d we started using the "print_cmd_by_category()"
function added earlier in the same series, or in cfb22a02ab5 (help:
use command-list.h for common command list, 2018-05-10).

Fixing this formatting nit is easy enough. Let's have all of the
output that would like to be "\n"-separated from other lines emit its
own "\n". We then adjust "print_cmd_by_category()" to only print a
"\n" to delimit the sections it's printing out.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 help.c          | 5 ++++-
 t/t0012-help.sh | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 45a21e7e35c..afd3af24124 100644
--- a/help.c
+++ b/help.c
@@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
 		uint32_t mask = catdesc[i].category;
 		const char *desc = catdesc[i].desc;
 
-		putchar('\n');
+		if (i)
+			putchar('\n');
 		puts(_(desc));
 		print_command_list(cmds, mask, longest);
 	}
@@ -328,6 +329,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
 void list_common_cmds_help(void)
 {
 	puts(_("These are common Git commands used in various situations:"));
+	putchar('\n');
 	print_cmd_by_category(common_categories, NULL);
 }
 
@@ -481,6 +483,7 @@ void list_all_cmds_help(int show_external_commands, int show_aliases)
 	int longest;
 
 	puts(_("See 'git help <command>' to read about a specific subcommand"));
+	putchar('\n');
 	print_cmd_by_category(main_categories, &longest);
 
 	if (show_external_commands)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 64321480c68..6c3e1f7159d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -226,7 +226,6 @@ test_expect_success "'git help -a' section spacing" '
 
 test_expect_success "'git help -g' section spacing" '
 	test_section_spacing_trailer git help -g <<-\EOF &&
-
 	The Git concept guides are:
 
 	EOF
-- 
2.35.1.1132.ga1fe46f8690

Re: [PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -		printf("\n%s\n", _(desc));
> +		putchar('\n');
> +		puts(_(desc));

This is sort of "Meh".  Even the justification that says "we'll do
the same thing in future patches" is not really a justification, as
it is entirely fine to add more of the "line-break plus %\n" printf()
in the later steps in the same series.

>  		print_command_list(cmds, mask, longest);
>  	}
>  	free(cmds);
> @@ -317,7 +318,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
>  	}
>  
>  	if (other_cmds->cnt) {
> -		printf_ln(_("git commands available from elsewhere on your $PATH"));
> +		puts(_("git commands available from elsewhere on your $PATH"));

This *IS* an improvement, as the first parameter to printf_ln() is
supposed to be a format string, and should have been

	printf_ln("%s", _("git commands ..."));

> -	printf_ln(_("See 'git help <command>' to read about a specific subcommand"));
> +	puts(_("See 'git help <command>' to read about a specific subcommand"));

Ditto.

Re: [PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -		printf("\n%s\n", _(desc));
>> +		putchar('\n');
>> +		puts(_(desc));
>
> This is sort of "Meh".  Even the justification that says "we'll do
> the same thing in future patches" is not really a justification, as
> it is entirely fine to add more of the "line-break plus %\n" printf()
> in the later steps in the same series.

Yes, I agree that justification wouldn't make sense, "let's change this
for consistency with code that doesn't exist yet and I'm about to
introduce" is a non-starter as an argument.

But that's not what I mentioned in the commit message or why I changed
this, as noted it's for doing the same "as other existing code in the
file does".

I.e. you'll see that adjacent and related code if you run this on
master:

    git grep -W '\\n' -- help.c

Now, I fully agree that's not a *strong* reason to change this, it could
just be left in place.

I just thought post-series that skimming through those related functions
made for marginally easier reading if they all used the same pattern to
accomplish the same thing.

In any case, I don't at all feel strongly about including this change,
so I can drop it if you'd like. I just wanted to clarify why I changed
it.

>>  		print_command_list(cmds, mask, longest);
>>  	}
>>  	free(cmds);
>> @@ -317,7 +318,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
>>  	}
>>  
>>  	if (other_cmds->cnt) {
>> -		printf_ln(_("git commands available from elsewhere on your $PATH"));
>> +		puts(_("git commands available from elsewhere on your $PATH"));
>
> This *IS* an improvement, as the first parameter to printf_ln() is
> supposed to be a format string, and should have been
>
> 	printf_ln("%s", _("git commands ..."));
>
>> -	printf_ln(_("See 'git help <command>' to read about a specific subcommand"));
>> +	puts(_("See 'git help <command>' to read about a specific subcommand"));
>
> Ditto.

Re: [PATCH v2 4/9] help.c: split up list_all_cmds_help() function

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -void list_all_cmds_help(void)
> +static void list_all_cmds_help_external_commands(void)
>  {
>  	struct string_list others = STRING_LIST_INIT_DUP;
> -	struct string_list alias_list = STRING_LIST_INIT_DUP;
> -	struct cmdname_help *aliases;
> -	int i, longest;
> -
> -	puts(_("See 'git help <command>' to read about a specific subcommand"));
> -	print_cmd_by_category(main_categories, &longest);
> +	int i;
>  
>  	list_all_other_cmds(&others);
>  	if (others.nr)

Let's note that in the body of this new helper function, we still
use the printf("\n%s\n", _("group header")) pattern, not putchar('\n')
followed by puts().

> @@ -449,6 +444,13 @@ void list_all_cmds_help(void)
>  	for (i = 0; i < others.nr; i++)
>  		printf("   %s\n", others.items[i].string);
>  	string_list_clear(&others, 0);
> +}
> +
> +static void list_all_cmds_help_aliases(int longest)
> +{
> +	struct string_list alias_list = STRING_LIST_INIT_DUP;
> +	struct cmdname_help *aliases;
> +	int i;
>  
>  	git_config(get_alias, &alias_list);
>  	string_list_sort(&alias_list);

And this helper, too.

> @@ -474,6 +476,17 @@ void list_all_cmds_help(void)
>  	string_list_clear(&alias_list, 1);
>  }
>  
> +void list_all_cmds_help(void)
> +{
> +	int longest;
> +
> +	puts(_("See 'git help <command>' to read about a specific subcommand"));
> +	print_cmd_by_category(main_categories, &longest);
> +
> +	list_all_cmds_help_external_commands();
> +	list_all_cmds_help_aliases(longest);
> +}

This does make sense ;-)

Re: [PATCH v2 5/9] help: note the option name on option incompatibility

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the errors added in d35d03cf93e (help: simplify by moving to
> OPT_CMDMODE(), 2021-09-22) to quote the offending option at the user
> when invoked as e.g.:
>
>     git help --guides garbage
>
> Now instead of:
>
>     fatal: this option doesn't take any other arguments
>
> We'll emit:
>
>     fatal: the '--guides' option doesn't take any non-option arguments

Very good.

> Let's also rename the function, as it will be extended to do other
> checks that aren't "no extra argc" in a subsequent commit.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/help.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index d387131dd83..1c1581ef850 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -574,11 +574,12 @@ static const char *check_git_cmd(const char* cmd)
>  	return cmd;
>  }
>  
> -static void no_extra_argc(int argc)
> +static void opt_mode_usage(int argc, const char *opt_mode)
>  {
>  	if (argc)
> -		usage_msg_opt(_("this option doesn't take any other arguments"),
> -			      builtin_help_usage, builtin_help_options);
> +		usage_msg_optf(_("the '%s' option doesn't take any non-option arguments"),
> +			       builtin_help_usage, builtin_help_options,
> +			       opt_mode);
>  }
>  
>  int cmd_help(int argc, const char **argv, const char *prefix)
> @@ -604,20 +605,20 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  		printf("%s\n", _(git_more_info_string));
>  		break;
>  	case HELP_ACTION_GUIDES:
> -		no_extra_argc(argc);
> +		opt_mode_usage(argc, "--guides");
>  		list_guides_help();
>  		printf("%s\n", _(git_more_info_string));
>  		return 0;
>  	case HELP_ACTION_CONFIG_FOR_COMPLETION:
> -		no_extra_argc(argc);
> +		opt_mode_usage(argc, "--config-for-completion");
>  		list_config_help(SHOW_CONFIG_VARS);
>  		return 0;
>  	case HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION:
> -		no_extra_argc(argc);
> +		opt_mode_usage(argc, "--config-sections-for-completion");
>  		list_config_help(SHOW_CONFIG_SECTIONS);
>  		return 0;
>  	case HELP_ACTION_CONFIG:
> -		no_extra_argc(argc);
> +		opt_mode_usage(argc, "--config");
>  		setup_pager();
>  		list_config_help(SHOW_CONFIG_HUMAN);
>  		printf("\n%s\n", _("'git help config' for more information"));

Re: [PATCH v2 6/9] help: correct usage & behavior of "git help --all"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index cf1d53e9499..d07590c8ff7 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git help' [-a|--all] [--[no-]verbose]
> -	   [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]
> +'git help' [[-i|--info] [-m|--man] [-w|--web]] [<command>|<guide>]

Hmph, OK.  The earlier one made it appear that when -a is given to
"git help" it would also accept command or guide, which is no longer
true.

>  'git help' [-g|--guides]
>  'git help' [-c|--config]

This is not new, but don't we need to fix the mark-up of [-a|--all]
and [-g|--guides] and [-c|--config]?  It's not like "We can give 0
or more of i/-m/-w".  These are "we have to give either -a or --all
to trigger this mode".

> diff --git a/builtin/help.c b/builtin/help.c
> index 1c1581ef850..b682446bbf5 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -75,8 +75,8 @@ static struct option builtin_help_options[] = {
>  };
>  
>  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 [-a|--all] [--[no-]verbose]]"),
> +	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
>  	N_("git help [-g|--guides]"),
>  	N_("git help [-c|--config]"),
>  	NULL
> @@ -594,6 +594,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	switch (cmd_mode) {
>  	case HELP_ACTION_ALL:
> +		opt_mode_usage(argc, "--all");

OK.

>  		if (verbose) {
>  			setup_pager();
>  			list_all_cmds_help();
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 9ac3f5d3c4b..c87730aa920 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -35,6 +35,9 @@ test_expect_success 'basic help commands' '
>  '
>  
>  test_expect_success 'invalid usage' '
> +	test_expect_code 129 git help -a add &&
> +	test_expect_code 129 git help --all add &&
> +
>  	test_expect_code 129 git help -g add &&
>  	test_expect_code 129 git help -a -c &&

Re: [PATCH v2 7/9] help: error if [-a|-g|-c] and [-i|-m|-w] are combined

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add more sanity checking to "git help" usage by erroring out if these
> man viewer options are combined with incompatible command-modes that
> will never use these documentation viewers.
>
> This continues the work started in d35d03cf93e (help: simplify by
> moving to OPT_CMDMODE(), 2021-09-22) of adding more sanity checking to
> "git help". Doing this allows us to clarify the "SYNOPSIS" in the
> documentation, and the "git help -h" output.

While this is not wrong per-se, "git help --all --web" in the
future that pops the same information up in your running browser
would not be a bad thing to have.

That use of --web is certainly not in line with --man and --info,
though (at that point we'd be saying "show in browser or show in
terminal").

So I guess this is OK at least for now.

Re: [PATCH v2 8/9] help: add --no-[external-commands|aliases] for use with --all

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add the ability to only emit git's own usage information under
> --all. This also allows us to extend the "test_section_spacing" tests
> added in a preceding commit to test "git help --all"
> output.

Makes sense.

> +static int show_external_commands = -1;
> +static int show_aliases = -1;
>  static struct option builtin_help_options[] = {
>  	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
>  		    HELP_ACTION_ALL),
> +	OPT_BOOL(0, "external-commands", &show_external_commands,
> +		 N_("show external commands in --all")),
> +	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all")),
>  	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
>  	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"),
> @@ -75,7 +80,7 @@ static struct option builtin_help_options[] = {
>  };
>  
>  static const char * const builtin_help_usage[] = {
> -	N_("git help [-a|--all] [--[no-]verbose]]"),
> +	N_("git help [-a|--all] [--[no-]verbose]] [--[no-]external-commands] [--[no-]aliases]"),
>  	N_("git help [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
>  	N_("git help [-g|--guides]"),
>  	N_("git help [-c|--config]"),
> @@ -620,12 +625,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	if (cmd_mode != HELP_ACTION_ALL &&
> +	    (show_external_commands >= 0 ||
> +	     show_aliases >= 0))
> +		usage_msg_opt(_("the '--no-[external-commands|aliases]' options can only be used with '--all'"),
> +			      builtin_help_usage, builtin_help_options);

Nice attention to a small detail.

>  	switch (cmd_mode) {
>  	case HELP_ACTION_ALL:
>  		opt_mode_usage(argc, "--all", help_format);
>  		if (verbose) {
>  			setup_pager();
> -			list_all_cmds_help();
> +			list_all_cmds_help(show_external_commands,
> +					   show_aliases);
>  			return 0;
>  		}
>  		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
> diff --git a/help.c b/help.c
> index 004117347ee..45a21e7e35c 100644
> --- a/help.c
> +++ b/help.c
> @@ -476,15 +476,17 @@ static void list_all_cmds_help_aliases(int longest)
>  	string_list_clear(&alias_list, 1);
>  }
>  
> -void list_all_cmds_help(void)
> +void list_all_cmds_help(int show_external_commands, int show_aliases)
>  {
>  	int longest;
>  
>  	puts(_("See 'git help <command>' to read about a specific subcommand"));
>  	print_cmd_by_category(main_categories, &longest);
>  
> -	list_all_cmds_help_external_commands();
> -	list_all_cmds_help_aliases(longest);
> +	if (show_external_commands)
> +		list_all_cmds_help_external_commands();
> +	if (show_aliases)
> +		list_all_cmds_help_aliases(longest);
>  }

OK.  Quite straight-forward after the earlier split.

Re: [PATCH v2 9/9] help: don't print "\n" before single-section output

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
>  		uint32_t mask = catdesc[i].category;
>  		const char *desc = catdesc[i].desc;
>  
> -		putchar('\n');
> +		if (i)
> +			putchar('\n');
>  		puts(_(desc));
>  		print_command_list(cmds, mask, longest);
>  	}
> @@ -328,6 +329,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
>  void list_common_cmds_help(void)
>  {
>  	puts(_("These are common Git commands used in various situations:"));
> +	putchar('\n');
>  	print_cmd_by_category(common_categories, NULL);
>  }
>  
> @@ -481,6 +483,7 @@ void list_all_cmds_help(int show_external_commands, int show_aliases)
>  	int longest;
>  
>  	puts(_("See 'git help <command>' to read about a specific subcommand"));
> +	putchar('\n');
>  	print_cmd_by_category(main_categories, &longest);
>  
>  	if (show_external_commands)


This patch is a good example to demonstrate that some changes cannot
be reviewed without showing in the whole file what didn't get
changed.  Among three callers of print_cmd_by_category(), two of
them we see here are *not* the first to speak, and do add a blank
line before the call.  The other caller that does not appear in the
patch is what is being fixed, list_guides_help(), which has nobody
to speak before it.

The correct pattern we want to follow in these messages is that
before you say something, you add a blank line if somebody else has
spoken before you.  It might make sense to tell these print_cmd_by*
helpers if they are the first to speak by passing the pointer to a
"state" variable to indicate what has been emitted so far, instead
of making the callers responsible to physically add blank lines, as
the callers would stop knowing if nobody else has spoken before them
if we further refactor them and introduce new direct callers.

In any case, the posted patch itself looks OK.  After following
these 9 steps, I still do not see enough justification for the
earlier "use puts()" step, except for correcting the misused
printf_ln().

Thanks, will queue the whole thing.


> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 64321480c68..6c3e1f7159d 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -226,7 +226,6 @@ test_expect_success "'git help -a' section spacing" '
>  
>  test_expect_success "'git help -g' section spacing" '
>  	test_section_spacing_trailer git help -g <<-\EOF &&
> -
>  	The Git concept guides are:
>  
>  	EOF