[PATCH 0/6] help: fix usage nits & bugs, completion shellscript->C

[PATCH 0/6] help: fix usage nits & bugs, completion shellscript->C

[Ævar Arnfjörð Bjarmason]

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.

This is all rather straightforward and boring, the first patch
mentions an interaction with my not-picked-up-by-Junio-yet series to
fix parse_options usage alignment at [1], but while the two strictly
speaking "semantically conflict" (in combination they'll produce
different results), the exact whitespace padding of "git help -h" is a
trivial enough issue that they can and should be considered
indepedently.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (6):
  help: correct the usage string in -h and documentation
  help: correct usage string for "git help --guides"
  help tests: add test for --config output
  help: refactor "for_human" control flow in cmd_help()
  help: correct logic error in combining --all and --config
  help / completion: make "git help" do the hard work

 Documentation/git-help.txt             |   9 ++-
 builtin/help.c                         | 102 +++++++++++++++++--------
 contrib/completion/git-completion.bash |  21 +++--
 t/t0012-help.sh                        |  45 +++++++++++
 4 files changed, 131 insertions(+), 46 deletions(-)

-- 
2.33.0.825.g2bf60429931

[PATCH 1/6] help: correct the usage string in -h and documentation

[Ævar Arnfjörð Bjarmason]

Clarify the usage string in the documentation so we group e.g. -i and
--info, and add the missing short options to the "-h" output.

The alignment of the second line is off now, but will be fixed with
another series of mine[1]. In the meantime let's just assume that fix
will make it in eventually for the purposes of this patch, if it's
misaligned for a bit it doesn't matter much.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 44fe8860b3f..568a0b606f3 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
-	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
+	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index b7eec06c3de..44ea2798cda 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"),
+	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	NULL
 };
 
-- 
2.33.0.825.g2bf60429931

[PATCH 2/6] help: correct usage string for "git help --guides"

[Ævar Arnfjörð Bjarmason]

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
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
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) and is no longer
applicable as explained above.

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 568a0b606f3..cb8e3d4da9e 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,8 +8,9 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
+'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
+'git help' [-g|--guides]
 
 DESCRIPTION
 -----------
@@ -58,8 +59,7 @@ OPTIONS
 
 -g::
 --guides::
-	Prints a list of the Git concept guides on the standard output. This
-	option overrides any given command or guide name.
+	Prints a list of the Git concept guides on the standard output.
 
 -i::
 --info::
diff --git a/builtin/help.c b/builtin/help.c
index 44ea2798cda..0f9dc31c40f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
+	N_("git help [-g|--guides]"),
 	NULL
 };
 
@@ -547,11 +548,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	int nongit;
 	enum help_format parsed_help_format;
 	const char *page;
+	int standalone = 0;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Options that take no further arguments */
+	standalone = show_config || show_guides;
+	if (standalone && argc)
+		usage_with_options(builtin_help_usage, builtin_help_options);
+
 	if (show_all) {
 		git_config(git_help_config, NULL);
 		if (verbose) {
@@ -580,11 +587,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (show_guides)
 		list_guides_help();
 
-	if (show_all || show_guides) {
+	if (show_all || standalone) {
 		printf("%s\n", _(git_more_info_string));
-		/*
-		* We're done. Ignore any remaining args
-		*/
 		return 0;
 	}
 
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 5679e29c624..6e01da614f0 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -34,6 +34,11 @@ test_expect_success 'basic help commands' '
 	git help -a >/dev/null
 '
 
+test_expect_success 'invalid usage' '
+	test_expect_code 129 git help -c git-add &&
+	test_expect_code 129 git help -g git-add
+'
+
 test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-- 
2.33.0.825.g2bf60429931

[PATCH 3/6] help tests: add test for --config output

[Ævar Arnfjörð Bjarmason]

Add a missing test for checking what the --config output added in
ac68a93fd2 (help: add --config to list all available config,
2018-05-26) looks like. We should not be emitting anything except
config variables and the brief usage information at the end here.

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

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 6e01da614f0..94d1f481c8b 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -78,6 +78,19 @@ test_expect_success 'git help -g' '
 	test_i18ngrep "^   tutorial   " help.output
 '
 
+test_expect_success 'git help -c' '
+	git help -c >help.output &&
+	cat >expect <<-\EOF &&
+
+	'"'"'git help config'"'"' for more information
+	EOF
+	grep -v -E \
+		-e "^[^.]+\.[^.]+$" \
+		-e "^[^.]+\.[^.]+\.[^.]+$" \
+		help.output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.825.g2bf60429931

[PATCH 4/6] help: refactor "for_human" control flow in cmd_help()

[Ævar Arnfjörð Bjarmason]

Instead of having two lines that call list_config_help(for_human)
let's setup the pager and print the trailer conditionally. This makes
it clearer at a glance how the two differ in behavior.

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

diff --git a/builtin/help.c b/builtin/help.c
index 0f9dc31c40f..0737b22069b 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (show_config) {
 		int for_human = show_config == 1;
 
-		if (!for_human) {
-			list_config_help(for_human);
-			return 0;
-		}
-		setup_pager();
+		if (for_human)
+			setup_pager();
 		list_config_help(for_human);
-		printf("\n%s\n", _("'git help config' for more information"));
+		if (for_human)
+			printf("\n%s\n", _("'git help config' for more information"));
+
 		return 0;
 	}
 
-- 
2.33.0.825.g2bf60429931

Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help()

[Eric Sunshine]

On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Instead of having two lines that call list_config_help(for_human)
> let's setup the pager and print the trailer conditionally. This makes
> it clearer at a glance how the two differ in behavior.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
> -               if (!for_human) {
> -                       list_config_help(for_human);
> -                       return 0;
> -               }
> -               setup_pager();
> +               if (for_human)
> +                       setup_pager();
>                 list_config_help(for_human);
> -               printf("\n%s\n", _("'git help config' for more information"));
> +               if (for_human)
> +                       printf("\n%s\n", _("'git help config' for more information"));
> +

For what it's worth, I find the original logic easier to reason about
since it gets the "simple" case out of the way early, thus I don't
have to keep as much (or any) state in mind as I'm reading the rest of
the code. However, it's highly subjective, of course; just one
person's opinion.

Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Instead of having two lines that call list_config_help(for_human)
>> let's setup the pager and print the trailer conditionally. This makes
>> it clearer at a glance how the two differ in behavior.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>> -               if (!for_human) {
>> -                       list_config_help(for_human);
>> -                       return 0;
>> -               }
>> -               setup_pager();
>> +               if (for_human)
>> +                       setup_pager();
>>                 list_config_help(for_human);
>> -               printf("\n%s\n", _("'git help config' for more information"));
>> +               if (for_human)
>> +                       printf("\n%s\n", _("'git help config' for more information"));
>> +
>
> For what it's worth, I find the original logic easier to reason about
> since it gets the "simple" case out of the way early, thus I don't
> have to keep as much (or any) state in mind as I'm reading the rest of
> the code. However, it's highly subjective, of course; just one
> person's opinion.

FWIW, it makes two of us ;-)

Quite honestly, I do not see much commonality in the code above that
targets two different audiences, so whether you handle simple one or
complex one first, a single big switch upfront that gives clearly
separate control flow to two distinct cases is easier to follow than
"as the middle step that calls list_config_help() is the same, let's
have two conditionals before and after and serve these two audiences
with a single code path that is slightly tweaked", which is the
result of this patch.

Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help()

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 08 2021, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Instead of having two lines that call list_config_help(for_human)
>>> let's setup the pager and print the trailer conditionally. This makes
>>> it clearer at a glance how the two differ in behavior.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>> -               if (!for_human) {
>>> -                       list_config_help(for_human);
>>> -                       return 0;
>>> -               }
>>> -               setup_pager();
>>> +               if (for_human)
>>> +                       setup_pager();
>>>                 list_config_help(for_human);
>>> -               printf("\n%s\n", _("'git help config' for more information"));
>>> +               if (for_human)
>>> +                       printf("\n%s\n", _("'git help config' for more information"));
>>> +
>>
>> For what it's worth, I find the original logic easier to reason about
>> since it gets the "simple" case out of the way early, thus I don't
>> have to keep as much (or any) state in mind as I'm reading the rest of
>> the code. However, it's highly subjective, of course; just one
>> person's opinion.
>
> FWIW, it makes two of us ;-)
>
> Quite honestly, I do not see much commonality in the code above that
> targets two different audiences, so whether you handle simple one or
> complex one first, a single big switch upfront that gives clearly
> separate control flow to two distinct cases is easier to follow than
> "as the middle step that calls list_config_help() is the same, let's
> have two conditionals before and after and serve these two audiences
> with a single code path that is slightly tweaked", which is the
> result of this patch.

What do you two think of the end-state of this in 6/6? I went back &
forth a bit with whether to keep this similar to pre-4/6 logic, or the
switch statement in 6/6, and then for that switch statement whether to
have the fallthrough case or not. Perhaps 6/6 with no fallthrough (but a
bit of duplication) is the best?

[PATCH 5/6] help: correct logic error in combining --all and --config

[Ævar Arnfjörð Bjarmason]

Fix a bug in the --config option that's been there ever since its
introduction in 3ac68a93fd2 (help: add --config to list all available
config, 2018-05-26). Die when --all and --config are combined,
combining them doesn't make sense.

The code for the --config option when combined with an earlier
refactoring done to support the --guide option in
65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
cause us to take the "--all" branch early and ignore the --config
option.

Let's instead list these as incompatible, both in the synopsis and
help output, and enforce it in the code itself.

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index cb8e3d4da9e..96d5f598b4b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 'git help' [-g|--guides]
+'git help' [-c|--config]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index 0737b22069b..83f71d6765e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = {
 	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
+	N_("git help [-c|--config]"),
 	NULL
 };
 
@@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	enum help_format parsed_help_format;
 	const char *page;
 	int standalone = 0;
+	int need_config = 0;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Incompatible options */
+	if (show_all + !!show_config + show_guides > 1)
+		usage_with_options(builtin_help_usage, builtin_help_options);
+
 	/* Options that take no further arguments */
 	standalone = show_config || show_guides;
 	if (standalone && argc)
 		usage_with_options(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();
@@ -571,6 +580,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		list_commands(colopts, &main_cmds, &other_cmds);
 	}
 
+	if (show_guides)
+		list_guides_help();
+
+	if (show_all || show_guides) {
+		printf("%s\n", _(git_more_info_string));
+		return 0;
+	}
+
 	if (show_config) {
 		int for_human = show_config == 1;
 
@@ -583,14 +600,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (show_guides)
-		list_guides_help();
-
-	if (show_all || standalone) {
-		printf("%s\n", _(git_more_info_string));
-		return 0;
-	}
-
 	if (!argv[0]) {
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_common_cmds_help();
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 94d1f481c8b..68e7f57470e 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -36,7 +36,10 @@ test_expect_success 'basic help commands' '
 
 test_expect_success 'invalid usage' '
 	test_expect_code 129 git help -c git-add &&
-	test_expect_code 129 git help -g git-add
+	test_expect_code 129 git help -g git-add &&
+
+	test_expect_code 129 git help -a -c &&
+	test_expect_code 129 git help -g -c
 '
 
 test_expect_success "works for commands and guides by default" '
-- 
2.33.0.825.g2bf60429931

Re: [PATCH 5/6] help: correct logic error in combining --all and --config

[Eric Sunshine]

On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Fix a bug in the --config option that's been there ever since its
> introduction in 3ac68a93fd2 (help: add --config to list all available
> config, 2018-05-26). Die when --all and --config are combined,
> combining them doesn't make sense.
>
> The code for the --config option when combined with an earlier
> refactoring done to support the --guide option in
> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
> cause us to take the "--all" branch early and ignore the --config
> option.
>
> Let's instead list these as incompatible, both in the synopsis and
> help output, and enforce it in the code itself.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/builtin/help.c b/builtin/help.c
> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)
> +       /* Incompatible options */
> +       if (show_all + !!show_config + show_guides > 1)
> +               usage_with_options(builtin_help_usage, builtin_help_options);

I personally find it highly frustrating when a program merely dumps
the usage statement without any explanation of what exactly it doesn't
like about the command-line. Printing out a simple:

    --all, --guides, --config are mutually exclusive

message would go a long way toward reducing the frustration level.

(Aside: I also find it more hostile than helpful when programs dump
the usage statement for a command-line invocation error -- even if
preceded by an explanation of the error -- since the explanation
usually gets drowned-out by the often multi-page usage text, and the
user has to go spelunking around the wall of output to try to figure
out what actually went wrong. It's much more helpful and easy to
figure out what went wrong with the invocation when only a simple
error message is printed -- without usage statement. However, that's a
separate battle, as Git already has plenty of places which dump the
usage statement in response to an invocation error.)

Re: [PATCH 5/6] help: correct logic error in combining --all and --config

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 08 2021, Eric Sunshine wrote:

> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Fix a bug in the --config option that's been there ever since its
>> introduction in 3ac68a93fd2 (help: add --config to list all available
>> config, 2018-05-26). Die when --all and --config are combined,
>> combining them doesn't make sense.
>>
>> The code for the --config option when combined with an earlier
>> refactoring done to support the --guide option in
>> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
>> cause us to take the "--all" branch early and ignore the --config
>> option.
>>
>> Let's instead list these as incompatible, both in the synopsis and
>> help output, and enforce it in the code itself.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/builtin/help.c b/builtin/help.c
>> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>> +       /* Incompatible options */
>> +       if (show_all + !!show_config + show_guides > 1)
>> +               usage_with_options(builtin_help_usage, builtin_help_options);
>
> I personally find it highly frustrating when a program merely dumps
> the usage statement without any explanation of what exactly it doesn't
> like about the command-line. Printing out a simple:
>
>     --all, --guides, --config are mutually exclusive
>
> message would go a long way toward reducing the frustration level.
>
> (Aside: I also find it more hostile than helpful when programs dump
> the usage statement for a command-line invocation error -- even if
> preceded by an explanation of the error -- since the explanation
> usually gets drowned-out by the often multi-page usage text, and the
> user has to go spelunking around the wall of output to try to figure
> out what actually went wrong. It's much more helpful and easy to
> figure out what went wrong with the invocation when only a simple
> error message is printed -- without usage statement. However, that's a
> separate battle, as Git already has plenty of places which dump the
> usage statement in response to an invocation error.)

I'll make it emit something more helpful.

More generally I've got quite a bit of parse_options() improvements
queued up locally that I've been trying to trickle in at the rate I can
get them through the list, review over at [1] would be much appreciated.

I wonder if we can do this automatically, we already have the
builtin_help_usage, we could parse that in the general case and find
that certain options are mutually exclusive per the examples there.

We'd then discover what option we parsed when usage_with_options() was
called, and automatically emit a useful message in these sorts of cases.

Of course the usage strings might be incomplete or wrong, but part of
the point would be to find those cases & a test mode to die() if a
command was called with some option combinations not suggested as
working according to its documented usage...

https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/

Re: [PATCH 5/6] help: correct logic error in combining --all and --config

[Eric Sunshine]

On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Wed, Sep 08 2021, Eric Sunshine wrote:
> > I personally find it highly frustrating when a program merely dumps
> > the usage statement without any explanation of what exactly it doesn't
> > like about the command-line. Printing out a simple:
> >
> >     --all, --guides, --config are mutually exclusive
> >
> > message would go a long way toward reducing the frustration level.
>
> I'll make it emit something more helpful.
>
> More generally I've got quite a bit of parse_options() improvements
> queued up locally that I've been trying to trickle in at the rate I can
> get them through the list, review over at [1] would be much appreciated.
> https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/

My review time is very limited these days (which is why most of my
review comments lately are superficial), but I set aside some time to
review that series for you[1]. Most of my review is subjective, but I
did identify one lurking bug (assuming I understand the code
correctly).

[1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/

> I wonder if we can do this automatically, we already have the
> builtin_help_usage, we could parse that in the general case and find
> that certain options are mutually exclusive per the examples there.
>
> We'd then discover what option we parsed when usage_with_options() was
> called, and automatically emit a useful message in these sorts of cases.
>
> Of course the usage strings might be incomplete or wrong, but part of
> the point would be to find those cases & a test mode to die() if a
> command was called with some option combinations not suggested as
> working according to its documented usage...

Perhaps, though I imagine it would have to employ some, um,
"interesting" heuristics, and be quite hit-and-miss at first, at least
until people get around to formalizing existing and new usage strings
with the specific goal of supporting that feature.

Speaking of heuristics and wishful thinking, when I read the cover
letter of your series which I just reviewed, I thought at first that
the end-goal would be to ignore whatever indentation the caller
provided following each embedded newline, and instead insert the
correct computed indentation automatically. This approach would
obviate the need for the [1/2] indentation cleanup patch. However,
doing so would require heuristics or at least manual help from the
caller to indicate the proper indentation width. I also thought
perhaps the intention of the series was to do the line-wrapping
automatically (ignoring any caller-provided embedded newlines), thus
ensuring that the lines were indented correctly _and_ fit the terminal
width properly regardless, but that's a somewhat more substantial
change.

Re: [PATCH 5/6] help: correct logic error in combining --all and --config

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 10 2021, Eric Sunshine wrote:

> On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Wed, Sep 08 2021, Eric Sunshine wrote:
>> > I personally find it highly frustrating when a program merely dumps
>> > the usage statement without any explanation of what exactly it doesn't
>> > like about the command-line. Printing out a simple:
>> >
>> >     --all, --guides, --config are mutually exclusive
>> >
>> > message would go a long way toward reducing the frustration level.
>>
>> I'll make it emit something more helpful.
>>
>> More generally I've got quite a bit of parse_options() improvements
>> queued up locally that I've been trying to trickle in at the rate I can
>> get them through the list, review over at [1] would be much appreciated.
>> https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/
>
> My review time is very limited these days (which is why most of my
> review comments lately are superficial), but I set aside some time to
> review that series for you[1]. Most of my review is subjective, but I
> did identify one lurking bug (assuming I understand the code
> correctly).

Thanks, I'll try to take a look at that in detail & re-roll soon.

> [1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/
>
>> I wonder if we can do this automatically, we already have the
>> builtin_help_usage, we could parse that in the general case and find
>> that certain options are mutually exclusive per the examples there.
>>
>> We'd then discover what option we parsed when usage_with_options() was
>> called, and automatically emit a useful message in these sorts of cases.
>>
>> Of course the usage strings might be incomplete or wrong, but part of
>> the point would be to find those cases & a test mode to die() if a
>> command was called with some option combinations not suggested as
>> working according to its documented usage...
>
> Perhaps, though I imagine it would have to employ some, um,
> "interesting" heuristics, and be quite hit-and-miss at first, at least
> until people get around to formalizing existing and new usage strings
> with the specific goal of supporting that feature.
>
> Speaking of heuristics and wishful thinking, when I read the cover
> letter of your series which I just reviewed, I thought at first that
> the end-goal would be to ignore whatever indentation the caller
> provided following each embedded newline, and instead insert the
> correct computed indentation automatically. This approach would
> obviate the need for the [1/2] indentation cleanup patch. However,
> doing so would require heuristics or at least manual help from the
> caller to indicate the proper indentation width. I also thought
> perhaps the intention of the series was to do the line-wrapping
> automatically (ignoring any caller-provided embedded newlines), thus
> ensuring that the lines were indented correctly _and_ fit the terminal
> width properly regardless, but that's a somewhat more substantial
> change.

Yeah, I considered it but decided not to mainly not for the heuristics
reason, but that any such thing means that you won't have the same
indentation in the C code. I.e. instead of:

    "git foo [--some-option]\n"
    "        [-even -more options]"

You'd have:

    "git foo [--some-option]\n"
    "[--even --more options]"

Which I think is just not as legible, but also in the general case you
get into the quagmire of how do align a thing like:

    "git [-c foo=bar] something [--option]\n"
    "[--more-options]"

Are we doing to align on the "git " or "git [-c foo=bar] something ", or
is it a "git something" where "something" is the name of a sub-command,
or is that a filename?

We control the source code so we can do it, but I thought it would be
nasty, and in any case any working solution wouldn't give you alignment
in the source code, so I dropped the idea.

[PATCH 6/6] help / completion: make "git help" do the hard work

[Ævar Arnfjörð Bjarmason]

The "help" builtin has been able to emit configuration variables since
e17ca926371 (completion: drop the hard coded list of config vars,
2018-05-26), but it hasn't produced exactly the format the completion
script wanted. Let's do that.

We got partway there in 2675ea1cc0f (completion: use 'sort -u' to
deduplicate config variable names, 2019-08-13) and
d9438873c4d (completion: deduplicate configuration sections,
2019-08-13), but after both we still needed some sorting,
de-duplicating and awk post-processing of the list.

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".

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().

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c                         | 67 ++++++++++++++++++--------
 contrib/completion/git-completion.bash | 21 ++++----
 t/t0012-help.sh                        | 24 +++++++++
 3 files changed, 81 insertions(+), 31 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 83f71d6765e..b33ef27ac79 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -38,7 +38,12 @@ static const char *html_path;
 
 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;
@@ -48,7 +53,10 @@ 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),
@@ -73,7 +81,7 @@ struct slot_expansion {
 	int found;
 };
 
-static void list_config_help(int for_human)
+static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
 		{ "advice", "*", list_config_advices },
@@ -91,6 +99,8 @@ static void list_config_help(int for_human)
 	const char **p;
 	struct slot_expansion *e;
 	struct string_list keys = STRING_LIST_INIT_DUP;
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
 	int i;
 
 	for (p = config_name_list; *p; p++) {
@@ -121,34 +131,48 @@ static void list_config_help(int for_human)
 	for (i = 0; i < keys.nr; i++) {
 		const char *var = keys.items[i].string;
 		const char *wildcard, *tag, *cut;
+		const char *dot = NULL;
+		struct strbuf sb = STRBUF_INIT;
 
-		if (for_human) {
+		switch (type) {
+		case SHOW_CONFIG_HUMAN:
 			puts(var);
 			continue;
+		case SHOW_CONFIG_SECTIONS:
+			dot = strchr(var, '.');
+			break;
+		case SHOW_CONFIG_VARS:
+			break;
+		case SHOW_CONFIG_UNSET:
+			BUG("should not get SHOW_CONFIG_UNSET here");
 		}
-
 		wildcard = strchr(var, '*');
 		tag = strchr(var, '<');
 
-		if (!wildcard && !tag) {
-			puts(var);
+		if (!dot && !wildcard && !tag) {
+			string_list_append(&keys_uniq, var);
 			continue;
 		}
 
-		if (wildcard && !tag)
+		if (dot)
+			cut = dot;
+		else if (wildcard && !tag)
 			cut = wildcard;
 		else if (!wildcard && tag)
 			cut = tag;
 		else
 			cut = wildcard < tag ? wildcard : tag;
 
-		/*
-		 * We may produce duplicates, but that's up to
-		 * git-completion.bash to handle
-		 */
-		printf("%.*s\n", (int)(cut - var), var);
+		strbuf_add(&sb, var, cut - var);
+		string_list_append(&keys_uniq, sb.buf);
+		strbuf_release(&sb);
+
 	}
 	string_list_clear(&keys, 0);
+	string_list_remove_duplicates(&keys_uniq, 0);
+	for_each_string_list_item(item, &keys_uniq)
+		puts(item->string);
+	string_list_clear(&keys_uniq, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
@@ -588,13 +612,16 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (show_config) {
-		int for_human = show_config == 1;
-
-		if (for_human)
-			setup_pager();
-		list_config_help(for_human);
-		if (for_human)
+	switch (show_config) {
+	case SHOW_CONFIG_UNSET:
+		break;
+	case SHOW_CONFIG_HUMAN:
+		setup_pager();
+		/* fallthrough */
+	case SHOW_CONFIG_VARS:
+	case SHOW_CONFIG_SECTIONS:
+		list_config_help(show_config);
+		if (show_config == SHOW_CONFIG_HUMAN)
 			printf("\n%s\n", _("'git help config' for more information"));
 
 		return 0;
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8108eda1e86..19b8a172878 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2503,7 +2503,14 @@ __git_config_vars=
 __git_compute_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_sections=
+__git_compute_config_sections ()
+{
+	test -n "$__git_config_sections" ||
+	__git_config_sections="$(git help --config-for-completion-sections)"
 }
 
 # Completes possible values of various configuration variables.
@@ -2717,16 +2724,8 @@ __git_complete_config_variable_name ()
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"
 		;;
 	*)
-		__git_compute_config_vars
-		__gitcomp "$(echo "$__git_config_vars" |
-				awk -F . '{
-					sections[$1] = 1
-				}
-				END {
-					for (s in sections)
-						print s "."
-				}
-				')" "" "$cur_"
+		__git_compute_config_sections
+		__gitcomp "$__git_config_sections" "" "$cur_" "."
 		;;
 	esac
 }
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 68e7f57470e..6d169ae3cbd 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -94,6 +94,30 @@ test_expect_success 'git help -c' '
 	test_cmp expect actual
 '
 
+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' '
+	git help -c >human &&
+	grep -E \
+	     -e "^[^.]+\.[^.]+$" \
+	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
+	     sed -e "s/\..*//" |
+	     sort -u >human.munged &&
+
+	git help --config-for-completion-sections >sections &&
+	test_cmp human.munged sections
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.825.g2bf60429931

[PATCH v2 0/5] help: fix usage nits & bugs, completion shellscript->C

[Ævar Arnfjörð Bjarmason]

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.

For v1, see : https://lore.kernel.org/git/cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com/

This should address all the feedback on v1 and more. I dropped the 4/6
refactoring change, we now also specifically say what option
combinations are bad before emitting usage info, as requested by Eric
Sunshine.

Ævar Arnfjörð Bjarmason (5):
  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 / completion: make "git help" do the hard work

 Documentation/git-help.txt             |   9 +-
 builtin/help.c                         | 110 ++++++++++++++++++-------
 contrib/completion/git-completion.bash |  21 +++--
 t/t0012-help.sh                        |  46 +++++++++++
 4 files changed, 140 insertions(+), 46 deletions(-)

Range-diff against v1:
1:  b5c79d87847 = 1:  b10bfd21f14 help: correct the usage string in -h and documentation
2:  1ebd443e43c ! 2:  039639a0dd3 help: correct usage string for "git help --guides"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    help: correct usage string for "git help --guides"
    +    help: correct usage & behavior of "git help --guides"
     
         As noted in 65f98358c0c (builtin/help.c: add --guide option,
         2013-04-02) and a133737b809 (doc: include --guide option description
    @@ Commit message
         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) and is no longer
    -    applicable as explained above.
    +    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.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/help.c: static struct option builtin_help_options[] = {
      };
      
     @@ 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 standalone = 0;
    - 
    - 	argc = parse_options(argc, argv, prefix, builtin_help_options,
      			builtin_help_usage, 0);
      	parsed_help_format = help_format;
      
     +	/* Options that take no further arguments */
    -+	standalone = show_config || show_guides;
    -+	if (standalone && argc)
    -+		usage_with_options(builtin_help_usage, builtin_help_options);
    ++	if (argc && show_guides)
    ++		usage_msg_opt(_("--guides cannot be combined with other options"),
    ++			      builtin_help_usage, builtin_help_options);
     +
      	if (show_all) {
      		git_config(git_help_config, NULL);
      		if (verbose) {
     @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
    - 	if (show_guides)
    - 		list_guides_help();
      
    --	if (show_all || show_guides) {
    -+	if (show_all || standalone) {
    + 	if (show_all || show_guides) {
      		printf("%s\n", _(git_more_info_string));
     -		/*
     -		* We're done. Ignore any remaining args
    @@ t/t0012-help.sh: test_expect_success 'basic help commands' '
      '
      
     +test_expect_success 'invalid usage' '
    -+	test_expect_code 129 git help -c git-add &&
     +	test_expect_code 129 git help -g git-add
     +'
     +
3:  d0a8045c9ed = 3:  258282095de help tests: add test for --config output
4:  e4bc7e57a6d < -:  ----------- help: refactor "for_human" control flow in cmd_help()
5:  bcc640d32a1 ! 4:  32d73d5273c 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 standalone = 0;
     +	int need_config = 0;
      
      	argc = parse_options(argc, argv, prefix, builtin_help_options,
    @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
      	parsed_help_format = help_format;
      
     +	/* Incompatible options */
    -+	if (show_all + !!show_config + show_guides > 1)
    -+		usage_with_options(builtin_help_usage, builtin_help_options);
    ++	if (show_all && show_config)
    ++		usage_msg_opt(_("--config and --all cannot be combined"),
    ++			      builtin_help_usage, builtin_help_options);
    ++
    ++	if (show_config && show_guides)
    ++		usage_msg_opt(_("--config and --guides cannot be combined"),
    ++			      builtin_help_usage, builtin_help_options);
     +
      	/* Options that take no further arguments */
    - 	standalone = show_config || show_guides;
    - 	if (standalone && argc)
    - 		usage_with_options(builtin_help_usage, builtin_help_options);
    ++	if (argc && show_config)
    ++		usage_msg_opt(_("--config cannot be combined with command or guide names"),
    ++			      builtin_help_usage, builtin_help_options);
    + 	if (argc && show_guides)
    +-		usage_msg_opt(_("--guides cannot be combined with other options"),
    ++		usage_msg_opt(_("--guides cannot be combined with command or guide names"),
    + 			      builtin_help_usage, builtin_help_options);
      
     -	if (show_all) {
     +	need_config = show_all || show_config;
    @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
     -	if (show_guides)
     -		list_guides_help();
     -
    --	if (show_all || standalone) {
    +-	if (show_all || show_guides) {
     -		printf("%s\n", _(git_more_info_string));
     -		return 0;
     -	}
    @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
     
      ## t/t0012-help.sh ##
     @@ t/t0012-help.sh: test_expect_success 'basic help commands' '
    + '
      
      test_expect_success 'invalid usage' '
    - 	test_expect_code 129 git help -c git-add &&
     -	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
6:  940061e84d1 ! 5:  e995a42cb8d help / completion: make "git help" do the hard work
    @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)
      
     -	if (show_config) {
     -		int for_human = show_config == 1;
    --
    --		if (for_human)
    --			setup_pager();
    --		list_config_help(for_human);
    --		if (for_human)
     +	switch (show_config) {
     +	case SHOW_CONFIG_UNSET:
     +		break;
    -+	case SHOW_CONFIG_HUMAN:
    -+		setup_pager();
    -+		/* fallthrough */
     +	case SHOW_CONFIG_VARS:
     +	case SHOW_CONFIG_SECTIONS:
     +		list_config_help(show_config);
    -+		if (show_config == SHOW_CONFIG_HUMAN)
    - 			printf("\n%s\n", _("'git help config' for more information"));
      
    +-		if (!for_human) {
    +-			list_config_help(for_human);
    +-			return 0;
    +-		}
    ++		return 0;
    ++	case SHOW_CONFIG_HUMAN:
    + 		setup_pager();
    +-		list_config_help(for_human);
    ++		list_config_help(show_config);
    + 		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=
-- 
2.33.0.873.g125ff7b9940

[PATCH v2 1/5] help: correct the usage string in -h and documentation

[Ævar Arnfjörð Bjarmason]

Clarify the usage string in the documentation so we group e.g. -i and
--info, and add the missing short options to the "-h" output.

The alignment of the second line is off now, but will be fixed with
another series of mine[1]. In the meantime let's just assume that fix
will make it in eventually for the purposes of this patch, if it's
misaligned for a bit it doesn't matter much.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 44fe8860b3f..568a0b606f3 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
-	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
+	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index b7eec06c3de..44ea2798cda 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"),
+	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	NULL
 };
 
-- 
2.33.0.873.g125ff7b9940

Re: [PATCH v2 1/5] help: correct the usage string in -h and documentation

[Junio C Hamano]

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

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index 44fe8860b3f..568a0b606f3 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
> -	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
> +	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]

This one is good, but ...

> diff --git a/builtin/help.c b/builtin/help.c
> index b7eec06c3de..44ea2798cda 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {
>  };
>  
>  static const char * const builtin_help_usage[] = {
> -	N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"),
> +	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
> +	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),

Aside from the addition of so-far-missing "verbose", which is
obviously a good change, I am not sure if the other change is a good
idea.

This is because *_usage[] is designed to be always shown together
with the list of options built from the option table the
parse_options() uses, and the readers will see the correspondence
between long and short options even more clear there.

    $ git help -h
    usage: git help [--all] [--guides] [--man | --web | --info] [<command>]

        -a, --all             print all available commands
        -g, --guides          print list of useful guides
        -c, --config          print all configuration variable names
        -m, --man             show man page
        -w, --web             show manual in web browser
        -i, --info            show info page
        -v, --verbose         print command description

If you look at output from 

    $ git grep -A4 -e '_usage\[' builtin/\*.c

you'll notice that many of them do not even spell out each option
and instead have a single [<options>] placeholder unless the command
has only very small number of options.  With the number of options
that "git help" takes, it might even be warranted to switch to the
more generic "git help [<option>...] [<command>]".

Not a strict veto, but just making sure if the over-cluttering of
the early lines in "help -h" output has been considered as a
possible downside before suggesting this change.

Thanks.

Re: [PATCH v2 1/5] help: correct the usage string in -h and documentation

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
>> index 44fe8860b3f..568a0b606f3 100644
>> --- a/Documentation/git-help.txt
>> +++ b/Documentation/git-help.txt
>> @@ -9,7 +9,7 @@ SYNOPSIS
>>  --------
>>  [verse]
>>  'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
>> -	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
>> +	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
>
> This one is good, but ...
>
>> diff --git a/builtin/help.c b/builtin/help.c
>> index b7eec06c3de..44ea2798cda 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {
>>  };
>>  
>>  static const char * const builtin_help_usage[] = {
>> -	N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"),
>> +	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
>> +	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
>
> Aside from the addition of so-far-missing "verbose", which is
> obviously a good change, I am not sure if the other change is a good
> idea.
>
> This is because *_usage[] is designed to be always shown together
> with the list of options built from the option table the
> parse_options() uses, and the readers will see the correspondence
> between long and short options even more clear there.
>
>     $ git help -h
>     usage: git help [--all] [--guides] [--man | --web | --info] [<command>]
>
>         -a, --all             print all available commands
>         -g, --guides          print list of useful guides
>         -c, --config          print all configuration variable names
>         -m, --man             show man page
>         -w, --web             show manual in web browser
>         -i, --info            show info page
>         -v, --verbose         print command description
>
> If you look at output from 
>
>     $ git grep -A4 -e '_usage\[' builtin/\*.c
>
> you'll notice that many of them do not even spell out each option
> and instead have a single [<options>] placeholder unless the command
> has only very small number of options.  With the number of options
> that "git help" takes, it might even be warranted to switch to the
> more generic "git help [<option>...] [<command>]".
>
> Not a strict veto, but just making sure if the over-cluttering of
> the early lines in "help -h" output has been considered as a
> possible downside before suggesting this change.

That's a valid point, but if you look at:

    for cmd in $(git --list-cmds=builtins); do echo git-$cmd: && git -P $cmd -h | sed 's/^/    /'; done 2>&1|less

I think it's fair to say it's a fairly mixed bag, some have the brevity
you describe, others are attempting to list every possible option combo
etc.

This is one of the commands that's more like than the likes of
git-branch, git-worktree, git-stash, git-index-pack etc. than git-add,
git-apply, git-mv in the sense that it's trying to exhaustively mirror
the options between the documentation and the usage string.

I do think we should pick one, but I think migrating from one style to
the other just for this one command would be an unnecessary distraction
from a change that's not about changing that style in general, let's
just stick with the pattern we find here.

I do want to make them pretty and consistent eventually, this series is
just one small step on the way there.

[PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Ævar Arnfjörð Bjarmason]

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
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
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.

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 568a0b606f3..cb8e3d4da9e 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,8 +8,9 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
+'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
+'git help' [-g|--guides]
 
 DESCRIPTION
 -----------
@@ -58,8 +59,7 @@ OPTIONS
 
 -g::
 --guides::
-	Prints a list of the Git concept guides on the standard output. This
-	option overrides any given command or guide name.
+	Prints a list of the Git concept guides on the standard output.
 
 -i::
 --info::
diff --git a/builtin/help.c b/builtin/help.c
index 44ea2798cda..51b18c291d8 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
+	N_("git help [-g|--guides]"),
 	NULL
 };
 
@@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Options that take no further arguments */
+	if (argc && show_guides)
+		usage_msg_opt(_("--guides cannot be combined with other options"),
+			      builtin_help_usage, builtin_help_options);
+
 	if (show_all) {
 		git_config(git_help_config, NULL);
 		if (verbose) {
@@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all || show_guides) {
 		printf("%s\n", _(git_more_info_string));
-		/*
-		* We're done. Ignore any remaining args
-		*/
 		return 0;
 	}
 
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 5679e29c624..c3aa016fd30 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
 	git help -a >/dev/null
 '
 
+test_expect_success 'invalid usage' '
+	test_expect_code 129 git help -g git-add
+'
+
 test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-- 
2.33.0.873.g125ff7b9940

Re: [PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Philip Oakley]

On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
> 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
> 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
> 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-help.txt |  6 +++---
>  builtin/help.c             | 11 +++++++----
>  t/t0012-help.sh            |  4 ++++
>  3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index 568a0b606f3..cb8e3d4da9e 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -8,8 +8,9 @@ git-help - Display help information about Git
>  SYNOPSIS
>  --------
>  [verse]
> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
> +'git help' [-a|--all [--[no-]verbose]]
>  	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
> +'git help' [-g|--guides]

Shouldn't we also include the [-c|--config] options here in the synopsis,
and the help_usage below?

Further, shouldn't we mention this (git help -c) on the git config man
page, e.g. "A list all available configuration variables can be
generated by `git help -c`." 

I hadn't realised the facility was even available.
>  
>  DESCRIPTION
>  -----------
> @@ -58,8 +59,7 @@ OPTIONS
>  
>  -g::
>  --guides::
> -	Prints a list of the Git concept guides on the standard output. This
> -	option overrides any given command or guide name.
> +	Prints a list of the Git concept guides on the standard output.
>  
>  -i::
>  --info::
> diff --git a/builtin/help.c b/builtin/help.c
> index 44ea2798cda..51b18c291d8 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
>  };
>  
>  static const char * const builtin_help_usage[] = {
> -	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
> +	N_("git help [-a|--all] [--[no-]verbose]]\n"
>  	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
> +	N_("git help [-g|--guides]"),
>  	NULL
>  };
>  
> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	/* Options that take no further arguments */
> +	if (argc && show_guides)
> +		usage_msg_opt(_("--guides cannot be combined with other options"),
> +			      builtin_help_usage, builtin_help_options);
> +
>  	if (show_all) {
>  		git_config(git_help_config, NULL);
>  		if (verbose) {
> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	if (show_all || show_guides) {
>  		printf("%s\n", _(git_more_info_string));
> -		/*
> -		* We're done. Ignore any remaining args
> -		*/
>  		return 0;
>  	}
>  
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 5679e29c624..c3aa016fd30 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
>  	git help -a >/dev/null
>  '
>  
> +test_expect_success 'invalid usage' '
> +	test_expect_code 129 git help -g git-add
> +'
> +
>  test_expect_success "works for commands and guides by default" '
>  	configure_help &&
>  	git help status &&

Re: [PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Philip Oakley]

On 10/09/2021 19:15, Philip Oakley wrote:
> On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
>> 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
>> 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
>> 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.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Documentation/git-help.txt |  6 +++---
>>  builtin/help.c             | 11 +++++++----
>>  t/t0012-help.sh            |  4 ++++
>>  3 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
>> index 568a0b606f3..cb8e3d4da9e 100644
>> --- a/Documentation/git-help.txt
>> +++ b/Documentation/git-help.txt
>> @@ -8,8 +8,9 @@ git-help - Display help information about Git
>>  SYNOPSIS
>>  --------
>>  [verse]
>> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
>> +'git help' [-a|--all [--[no-]verbose]]
>>  	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
>> +'git help' [-g|--guides]
> Shouldn't we also include the [-c|--config] options here in the synopsis,
> and the help_usage below?

I see this is fixed in 4/5
>
> Further, shouldn't we mention this (git help -c) on the git config man
> page, e.g. "A list all available configuration variables can be
> generated by `git help -c`." 

Still feel this one would be useful (but may be out of scope of this series)
>
> I hadn't realised the facility was even available.

Philip
>>  
>>  DESCRIPTION
>>  -----------
>> @@ -58,8 +59,7 @@ OPTIONS
>>  
>>  -g::
>>  --guides::
>> -	Prints a list of the Git concept guides on the standard output. This
>> -	option overrides any given command or guide name.
>> +	Prints a list of the Git concept guides on the standard output.
>>  
>>  -i::
>>  --info::
>> diff --git a/builtin/help.c b/builtin/help.c
>> index 44ea2798cda..51b18c291d8 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
>>  };
>>  
>>  static const char * const builtin_help_usage[] = {
>> -	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
>> +	N_("git help [-a|--all] [--[no-]verbose]]\n"
>>  	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
>> +	N_("git help [-g|--guides]"),
>>  	NULL
>>  };
>>  
>> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>  			builtin_help_usage, 0);
>>  	parsed_help_format = help_format;
>>  
>> +	/* Options that take no further arguments */
>> +	if (argc && show_guides)
>> +		usage_msg_opt(_("--guides cannot be combined with other options"),
>> +			      builtin_help_usage, builtin_help_options);
>> +
>>  	if (show_all) {
>>  		git_config(git_help_config, NULL);
>>  		if (verbose) {
>> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>  
>>  	if (show_all || show_guides) {
>>  		printf("%s\n", _(git_more_info_string));
>> -		/*
>> -		* We're done. Ignore any remaining args
>> -		*/
>>  		return 0;
>>  	}
>>  
>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>> index 5679e29c624..c3aa016fd30 100755
>> --- a/t/t0012-help.sh
>> +++ b/t/t0012-help.sh
>> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
>>  	git help -a >/dev/null
>>  '
>>  
>> +test_expect_success 'invalid usage' '
>> +	test_expect_code 129 git help -g git-add
>> +'
>> +
>>  test_expect_success "works for commands and guides by default" '
>>  	configure_help &&
>>  	git help status &&

Re: [PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 10 2021, Philip Oakley wrote:

> On 10/09/2021 19:15, Philip Oakley wrote:
>> On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
>>> 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
>>> 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
>>> 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.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>  Documentation/git-help.txt |  6 +++---
>>>  builtin/help.c             | 11 +++++++----
>>>  t/t0012-help.sh            |  4 ++++
>>>  3 files changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
>>> index 568a0b606f3..cb8e3d4da9e 100644
>>> --- a/Documentation/git-help.txt
>>> +++ b/Documentation/git-help.txt
>>> @@ -8,8 +8,9 @@ git-help - Display help information about Git
>>>  SYNOPSIS
>>>  --------
>>>  [verse]
>>> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
>>> +'git help' [-a|--all [--[no-]verbose]]
>>>  	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
>>> +'git help' [-g|--guides]
>> Shouldn't we also include the [-c|--config] options here in the synopsis,
>> and the help_usage below?
>
> I see this is fixed in 4/5

I updated the config message for the v3 to say it'll be addressed later>

>> Further, shouldn't we mention this (git help -c) on the git config man
>> page, e.g. "A list all available configuration variables can be
>> generated by `git help -c`." 
>
> Still feel this one would be useful (but may be out of scope of this series)

We already have such a mention in the documentation, it pre-dates this
series. I.e.:
    
    -c::
    --config::
            List all available configuration variables. This is a short
            summary of the list in linkgit:git-config[1].
    
The "short summary" there is quite the understatement, but that wording
was added in , 3ac68a93fd2 (help: add --config to list all available
config, 2018-05-26) so it wasn't some mistake with the option drifting
out of sync with an earlier implementation.

I think what Nguyễn meant here was "much shorte than 'git help config'".

Re: [PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Philip Oakley]

On 21/09/2021 14:49, Ævar Arnfjörð Bjarmason wrote:
>>> Further, shouldn't we mention this (git help -c) on the git config man
>>> page, e.g. "A list all available configuration variables can be
>>> generated by `git help -c`." 
>> Still feel this one would be useful (but may be out of scope of this series)
> We already have such a mention in the documentation, it pre-dates this
> series. I.e.:
>     
>     -c::
>     --config::
>             List all available configuration variables. This is a short
>             summary of the list in linkgit:git-config[1].
>     
> The "short summary" there is quite the understatement, but that wording
> was added in , 3ac68a93fd2 (help: add --config to list all available
> config, 2018-05-26) so it wasn't some mistake with the option drifting
> out of sync with an earlier implementation.
>
> I think what Nguyễn meant here was "much shorte than 'git help config'".
I was just saying the 'git help config' should point to the `git help
--config` command (extra `--`)

I then realised I ought to propose a patch, which is now in `next`
ae578de926 (doc: config, tell readers of `git help --config`, 2021-09-13),

I should have added the wider cc list.
https://lore.kernel.org/git/20210913212305.1832-1-philipoakley@iee.email/

Philip

Re: [PATCH v2 2/5] help: correct usage & behavior of "git help --guides"

[Junio C Hamano]

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

> 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
> cannot be combined with e.g. <command>.

Missing comman ',' between 'option' and 'it'.

> Change both the usage string to reflect that, and test and assert for

I couldn't immediately tell which two things "both the usage string"
is referring to.  Presumably in the doc and "help -h" output?

> 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
> 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.

Three sentences strung together with two commas ',' in between at
the end of this paragraph.  "A, let's B, it's C" -> "A. Let's B,
because it's C" or something.

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index 568a0b606f3..cb8e3d4da9e 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -8,8 +8,9 @@ git-help - Display help information about Git
>  SYNOPSIS
>  --------
>  [verse]
> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
> +'git help' [-a|--all [--[no-]verbose]]
>  	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
> +'git help' [-g|--guides]

Good.

> diff --git a/builtin/help.c b/builtin/help.c
> index 44ea2798cda..51b18c291d8 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
>  };
>  
>  static const char * const builtin_help_usage[] = {
> -	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
> +	N_("git help [-a|--all] [--[no-]verbose]]\n"
>  	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
> +	N_("git help [-g|--guides]"),

Good.  I still think -s|--long is cluttering than helping, though.

> +	/* Options that take no further arguments */
> +	if (argc && show_guides)
> +		usage_msg_opt(_("--guides cannot be combined with other options"),
> +			      builtin_help_usage, builtin_help_options);

At this point we have called parse_options() and there is no further
funky command line parsing involved, so non-zero argc does mean we
have something --guides does not know what to do.  Good.

> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	if (show_all || show_guides) {
>  		printf("%s\n", _(git_more_info_string));
> -		/*
> -		* We're done. Ignore any remaining args
> -		*/
>  		return 0;
>  	}
>  
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 5679e29c624..c3aa016fd30 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
>  	git help -a >/dev/null
>  '
>  
> +test_expect_success 'invalid usage' '
> +	test_expect_code 129 git help -g git-add

;-)  

I would have expected a bare "add" not "git-add" here, but it
is OK.  It is funny that "git help git-add" still works, but
that is a bug that is unrelated to this patch.

> +'
> +
>  test_expect_success "works for commands and guides by default" '
>  	configure_help &&
>  	git help status &&

[PATCH v2 3/5] help tests: add test for --config output

[Ævar Arnfjörð Bjarmason]

Add a missing test for checking what the --config output added in
ac68a93fd2 (help: add --config to list all available config,
2018-05-26) looks like. We should not be emitting anything except
config variables and the brief usage information at the end here.

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

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c3aa016fd30..595bf81f133 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -77,6 +77,19 @@ test_expect_success 'git help -g' '
 	test_i18ngrep "^   tutorial   " help.output
 '
 
+test_expect_success 'git help -c' '
+	git help -c >help.output &&
+	cat >expect <<-\EOF &&
+
+	'"'"'git help config'"'"' for more information
+	EOF
+	grep -v -E \
+		-e "^[^.]+\.[^.]+$" \
+		-e "^[^.]+\.[^.]+\.[^.]+$" \
+		help.output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.873.g125ff7b9940

Re: [PATCH v2 3/5] help tests: add test for --config output

[Junio C Hamano]

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

> Add a missing test for checking what the --config output added in
> ac68a93fd2 (help: add --config to list all available config,
> 2018-05-26) looks like. We should not be emitting anything except
> config variables and the brief usage information at the end here.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0012-help.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index c3aa016fd30..595bf81f133 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -77,6 +77,19 @@ test_expect_success 'git help -g' '
>  	test_i18ngrep "^   tutorial   " help.output
>  '
>  
> +test_expect_success 'git help -c' '
> +	git help -c >help.output &&
> +	cat >expect <<-\EOF &&
> +
> +	'"'"'git help config'"'"' for more information

	'\''git help config'\'' for more information

is a tad shorter.

> +	EOF
> +	grep -v -E \
> +		-e "^[^.]+\.[^.]+$" \
> +		-e "^[^.]+\.[^.]+\.[^.]+$" \

I have to question if there is much value in this test, especially
the latter pattern.  A configuration variable with three-level name
can have any byte, including a dot, in its second level, so
rejecting a name with more than three dots in it can over-filter,
depending on what new keys we'll document in the future.

> +		help.output >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'generate builtin list' '
>  	git --list-cmds=builtins >builtins
>  '

Re: [PATCH v2 3/5] help tests: add test for --config output

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Add a missing test for checking what the --config output added in
>> ac68a93fd2 (help: add --config to list all available config,
>> 2018-05-26) looks like. We should not be emitting anything except
>> config variables and the brief usage information at the end here.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t0012-help.sh | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>> index c3aa016fd30..595bf81f133 100755
>> --- a/t/t0012-help.sh
>> +++ b/t/t0012-help.sh
>> @@ -77,6 +77,19 @@ test_expect_success 'git help -g' '
>>  	test_i18ngrep "^   tutorial   " help.output
>>  '
>>  
>> +test_expect_success 'git help -c' '
>> +	git help -c >help.output &&
>> +	cat >expect <<-\EOF &&
>> +
>> +	'"'"'git help config'"'"' for more information
>
> 	'\''git help config'\'' for more information
>
> is a tad shorter.

Thanks.

>> +	EOF
>> +	grep -v -E \
>> +		-e "^[^.]+\.[^.]+$" \
>> +		-e "^[^.]+\.[^.]+\.[^.]+$" \
>
> I have to question if there is much value in this test, especially
> the latter pattern.  A configuration variable with three-level name
> can have any byte, including a dot, in its second level, so
> rejecting a name with more than three dots in it can over-filter,
> depending on what new keys we'll document in the future.

This is from what we extract from the headers in the documentation, so
it's all strings like:

    foo.<name>.bar

In cases where there's some three-level arbitrary string key. That seems
unlikely to change.

Re: [PATCH v2 3/5] help tests: add test for --config output

[Philip Oakley]

On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
> Add a missing test for checking what the --config output added in
> ac68a93fd2 (help: add --config to list all available config,

This ref should be 3ac68a93fd2
Probable hand editing mistake - it's Ok in 4/5

> 2018-05-26) looks like. We should not be emitting anything except
> config variables and the brief usage information at the end here.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0012-help.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index c3aa016fd30..595bf81f133 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -77,6 +77,19 @@ test_expect_success 'git help -g' '
>  	test_i18ngrep "^   tutorial   " help.output
>  '
>  
> +test_expect_success 'git help -c' '
> +	git help -c >help.output &&
> +	cat >expect <<-\EOF &&
> +
> +	'"'"'git help config'"'"' for more information
> +	EOF
> +	grep -v -E \
> +		-e "^[^.]+\.[^.]+$" \
> +		-e "^[^.]+\.[^.]+\.[^.]+$" \
> +		help.output >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'generate builtin list' '
>  	git --list-cmds=builtins >builtins
>  '

[PATCH v2 4/5] help: correct logic error in combining --all and --config

[Ævar Arnfjörð Bjarmason]

Fix a bug in the --config option that's been there ever since its
introduction in 3ac68a93fd2 (help: add --config to list all available
config, 2018-05-26). Die when --all and --config are combined,
combining them doesn't make sense.

The code for the --config option when combined with an earlier
refactoring done to support the --guide option in
65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
cause us to take the "--all" branch early and ignore the --config
option.

Let's instead list these as incompatible, both in the synopsis and
help output, and enforce it in the code itself.

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index cb8e3d4da9e..96d5f598b4b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 'git help' [-g|--guides]
+'git help' [-c|--config]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index 51b18c291d8..05ba2cbe380 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = {
 	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
+	N_("git help [-c|--config]"),
 	NULL
 };
 
@@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	int nongit;
 	enum help_format parsed_help_format;
 	const char *page;
+	int need_config = 0;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Incompatible options */
+	if (show_all && show_config)
+		usage_msg_opt(_("--config and --all cannot be combined"),
+			      builtin_help_usage, builtin_help_options);
+
+	if (show_config && show_guides)
+		usage_msg_opt(_("--config and --guides cannot be combined"),
+			      builtin_help_usage, builtin_help_options);
+
 	/* Options that take no further arguments */
+	if (argc && show_config)
+		usage_msg_opt(_("--config cannot be combined with command or guide names"),
+			      builtin_help_usage, builtin_help_options);
 	if (argc && show_guides)
-		usage_msg_opt(_("--guides cannot be combined with other options"),
+		usage_msg_opt(_("--guides cannot be combined with command or guide names"),
 			      builtin_help_usage, builtin_help_options);
 
-	if (show_all) {
+	need_config = show_all || show_config;
+	if (need_config)
 		git_config(git_help_config, NULL);
+
+	if (show_all) {
 		if (verbose) {
 			setup_pager();
 			list_all_cmds_help();
@@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		list_commands(colopts, &main_cmds, &other_cmds);
 	}
 
+	if (show_guides)
+		list_guides_help();
+
+	if (show_all || show_guides) {
+		printf("%s\n", _(git_more_info_string));
+		return 0;
+	}
+
 	if (show_config) {
 		int for_human = show_config == 1;
 
@@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (show_guides)
-		list_guides_help();
-
-	if (show_all || show_guides) {
-		printf("%s\n", _(git_more_info_string));
-		return 0;
-	}
-
 	if (!argv[0]) {
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_common_cmds_help();
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 595bf81f133..cbc9b64f79f 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -35,7 +35,12 @@ test_expect_success 'basic help commands' '
 '
 
 test_expect_success 'invalid usage' '
-	test_expect_code 129 git help -g git-add
+	test_expect_code 129 git help -g git-add &&
+	test_expect_code 129 git help -c git-add &&
+	test_expect_code 129 git help -g git-add &&
+
+	test_expect_code 129 git help -a -c &&
+	test_expect_code 129 git help -g -c
 '
 
 test_expect_success "works for commands and guides by default" '
-- 
2.33.0.873.g125ff7b9940

Re: [PATCH v2 4/5] help: correct logic error in combining --all and --config

[Junio C Hamano]

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

> Fix a bug in the --config option that's been there ever since its
> introduction in 3ac68a93fd2 (help: add --config to list all available
> config, 2018-05-26). Die when --all and --config are combined,
> combining them doesn't make sense.
>
> The code for the --config option when combined with an earlier
> refactoring done to support the --guide option in
> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
> cause us to take the "--all" branch early and ignore the --config
> option.
>
> Let's instead list these as incompatible, both in the synopsis and
> help output, and enforce it in the code itself.

Of course, "git help -c -a" might be a good enough UI to ask for
these variables that are usually hidden due to being deprecated, but
implementing that would be a much larger surgery (I do not think the
config_name_list[] has enough information as the process to generate
it discards the deprecated ones), so this is OK, I think.

> @@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  	int nongit;
>  	enum help_format parsed_help_format;
>  	const char *page;
> +	int need_config = 0;
>  
>  	argc = parse_options(argc, argv, prefix, builtin_help_options,
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	/* Incompatible options */
> +	if (show_all && show_config)
> +		usage_msg_opt(_("--config and --all cannot be combined"),
> +			      builtin_help_usage, builtin_help_options);
> +
> +	if (show_config && show_guides)
> +		usage_msg_opt(_("--config and --guides cannot be combined"),
> +			      builtin_help_usage, builtin_help_options);
> +
>  	/* Options that take no further arguments */
> +	if (argc && show_config)
> +		usage_msg_opt(_("--config cannot be combined with command or guide names"),
> +			      builtin_help_usage, builtin_help_options);
>  	if (argc && show_guides)
> -		usage_msg_opt(_("--guides cannot be combined with other options"),
> +		usage_msg_opt(_("--guides cannot be combined with command or guide names"),
>  			      builtin_help_usage, builtin_help_options);

This almost makes me wonder if we should be using OPT_CMDMODE and
taking advantage of its built-in "X and Y are incompatible"
detection.

> -	if (show_all) {
> +	need_config = show_all || show_config;
> +	if (need_config)
>  		git_config(git_help_config, NULL);

This change does not seem to be explained.  

We didn't handle help-config when "git help -c" was asked, but now
we do, because...?

> +	if (show_all) {
>  		if (verbose) {
>  			setup_pager();
>  			list_all_cmds_help();
> @@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  		list_commands(colopts, &main_cmds, &other_cmds);
>  	}
>  
> +	if (show_guides)
> +		list_guides_help();
> +
> +	if (show_all || show_guides) {
> +		printf("%s\n", _(git_more_info_string));
> +		return 0;
> +	}

As guides, all and config are mutually exclusive, it is unclear what
we gain by moving these two above.  The resulting code does not seem
to be more clear then before.

If anything, 

	switch (show_mode) {
	case HELP_ALL_MODE:
		... the body of the first "if" ...
		printf("%s\n", _(git_more_info_string));
		return 0;
	case HELP_GUIDES_MODE:
		list_guides_help();
		printf("%s\n", _(git_more_info_string));
		return 0;
	case HELP_CONFIG_MODE:
		... the body of the follwing "if" ...
		return 0;
	default: /* show a manual page */
		break;
	}

may make it easier to follow.

>  	if (show_config) {
>  		int for_human = show_config == 1;
>  
> @@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  		return 0;
>  	}
>  
> -	if (show_guides)
> -		list_guides_help();
> -
> -	if (show_all || show_guides) {
> -		printf("%s\n", _(git_more_info_string));
> -		return 0;
> -	}
> -
>  	if (!argv[0]) {
>  		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>  		list_common_cmds_help();
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 595bf81f133..cbc9b64f79f 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -35,7 +35,12 @@ test_expect_success 'basic help commands' '
>  '
>  
>  test_expect_success 'invalid usage' '
> -	test_expect_code 129 git help -g git-add

> +	test_expect_code 129 git help -g git-add &&
> +	test_expect_code 129 git help -c git-add &&
> +	test_expect_code 129 git help -g git-add &&

Why two "-g git-add"?

> +
> +	test_expect_code 129 git help -a -c &&
> +	test_expect_code 129 git help -g -c
>  '
>  
>  test_expect_success "works for commands and guides by default" '

[PATCH v2 5/5] help / completion: make "git help" do the hard work

[Ævar Arnfjörð Bjarmason]

The "help" builtin has been able to emit configuration variables since
e17ca926371 (completion: drop the hard coded list of config vars,
2018-05-26), but it hasn't produced exactly the format the completion
script wanted. Let's do that.

We got partway there in 2675ea1cc0f (completion: use 'sort -u' to
deduplicate config variable names, 2019-08-13) and
d9438873c4d (completion: deduplicate configuration sections,
2019-08-13), but after both we still needed some sorting,
de-duplicating and awk post-processing of the list.

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".

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().

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c                         | 67 ++++++++++++++++++--------
 contrib/completion/git-completion.bash | 21 ++++----
 t/t0012-help.sh                        | 24 +++++++++
 3 files changed, 81 insertions(+), 31 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 05ba2cbe380..9eb09d4804e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -38,7 +38,12 @@ static const char *html_path;
 
 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;
@@ -48,7 +53,10 @@ 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),
@@ -73,7 +81,7 @@ struct slot_expansion {
 	int found;
 };
 
-static void list_config_help(int for_human)
+static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
 		{ "advice", "*", list_config_advices },
@@ -91,6 +99,8 @@ static void list_config_help(int for_human)
 	const char **p;
 	struct slot_expansion *e;
 	struct string_list keys = STRING_LIST_INIT_DUP;
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
 	int i;
 
 	for (p = config_name_list; *p; p++) {
@@ -121,34 +131,48 @@ static void list_config_help(int for_human)
 	for (i = 0; i < keys.nr; i++) {
 		const char *var = keys.items[i].string;
 		const char *wildcard, *tag, *cut;
+		const char *dot = NULL;
+		struct strbuf sb = STRBUF_INIT;
 
-		if (for_human) {
+		switch (type) {
+		case SHOW_CONFIG_HUMAN:
 			puts(var);
 			continue;
+		case SHOW_CONFIG_SECTIONS:
+			dot = strchr(var, '.');
+			break;
+		case SHOW_CONFIG_VARS:
+			break;
+		case SHOW_CONFIG_UNSET:
+			BUG("should not get SHOW_CONFIG_UNSET here");
 		}
-
 		wildcard = strchr(var, '*');
 		tag = strchr(var, '<');
 
-		if (!wildcard && !tag) {
-			puts(var);
+		if (!dot && !wildcard && !tag) {
+			string_list_append(&keys_uniq, var);
 			continue;
 		}
 
-		if (wildcard && !tag)
+		if (dot)
+			cut = dot;
+		else if (wildcard && !tag)
 			cut = wildcard;
 		else if (!wildcard && tag)
 			cut = tag;
 		else
 			cut = wildcard < tag ? wildcard : tag;
 
-		/*
-		 * We may produce duplicates, but that's up to
-		 * git-completion.bash to handle
-		 */
-		printf("%.*s\n", (int)(cut - var), var);
+		strbuf_add(&sb, var, cut - var);
+		string_list_append(&keys_uniq, sb.buf);
+		strbuf_release(&sb);
+
 	}
 	string_list_clear(&keys, 0);
+	string_list_remove_duplicates(&keys_uniq, 0);
+	for_each_string_list_item(item, &keys_uniq)
+		puts(item->string);
+	string_list_clear(&keys_uniq, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
@@ -595,16 +619,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		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;
-		}
+		return 0;
+	case SHOW_CONFIG_HUMAN:
 		setup_pager();
-		list_config_help(for_human);
+		list_config_help(show_config);
 		printf("\n%s\n", _("'git help config' for more information"));
+
 		return 0;
 	}
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8108eda1e86..19b8a172878 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2503,7 +2503,14 @@ __git_config_vars=
 __git_compute_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_sections=
+__git_compute_config_sections ()
+{
+	test -n "$__git_config_sections" ||
+	__git_config_sections="$(git help --config-for-completion-sections)"
 }
 
 # Completes possible values of various configuration variables.
@@ -2717,16 +2724,8 @@ __git_complete_config_variable_name ()
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"
 		;;
 	*)
-		__git_compute_config_vars
-		__gitcomp "$(echo "$__git_config_vars" |
-				awk -F . '{
-					sections[$1] = 1
-				}
-				END {
-					for (s in sections)
-						print s "."
-				}
-				')" "" "$cur_"
+		__git_compute_config_sections
+		__gitcomp "$__git_config_sections" "" "$cur_" "."
 		;;
 	esac
 }
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index cbc9b64f79f..4405eb6dde5 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -95,6 +95,30 @@ test_expect_success 'git help -c' '
 	test_cmp expect actual
 '
 
+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' '
+	git help -c >human &&
+	grep -E \
+	     -e "^[^.]+\.[^.]+$" \
+	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
+	     sed -e "s/\..*//" |
+	     sort -u >human.munged &&
+
+	git help --config-for-completion-sections >sections &&
+	test_cmp human.munged sections
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.873.g125ff7b9940

Re: [PATCH v2 5/5] help / completion: make "git help" do the hard work

[Junio C Hamano]

This step does not seem to pass "make builtin/help.sp".

diff --git a/builtin/help.c b/builtin/help.c
index 9eb09d4804..4af4c62793 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -38,7 +38,7 @@ static const char *html_path;
 
 static int show_all = 0;
 static int show_guides = 0;
-enum show_config_type {
+static enum show_config_type {
 	SHOW_CONFIG_UNSET = 0,
 	SHOW_CONFIG_HUMAN,
 	SHOW_CONFIG_VARS,
-- 
2.33.0-549-g40c0868576

[PATCH v3 0/9] help: fix usage nits & bugs, completion shellscript->C

[Ævar Arnfjörð Bjarmason]

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

[PATCH v3 1/9] help: correct the usage string in -h and documentation

[Ævar Arnfjörð Bjarmason]

Clarify the usage string in the documentation so we group e.g. -i and
--info, and add the missing short options to the "-h" output.

The alignment of the second line is off now, but will be fixed with
another series of mine[1]. In the meantime let's just assume that fix
will make it in eventually for the purposes of this patch, if it's
misaligned for a bit it doesn't matter much.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 44fe8860b3f..568a0b606f3 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
-	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
+	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index b7eec06c3de..44ea2798cda 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"),
+	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	NULL
 };
 
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 2/9] help: correct usage & behavior of "git help --guides"

[Ævar Arnfjörð Bjarmason]

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
cannot be combined with e.g. <command>.

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 "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>
---
 Documentation/git-help.txt |  6 +++---
 builtin/help.c             | 11 +++++++----
 t/t0012-help.sh            |  4 ++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 568a0b606f3..cb8e3d4da9e 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,8 +8,9 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all [--[no-]verbose]] [-g|--guides]
+'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
+'git help' [-g|--guides]
 
 DESCRIPTION
 -----------
@@ -58,8 +59,7 @@ OPTIONS
 
 -g::
 --guides::
-	Prints a list of the Git concept guides on the standard output. This
-	option overrides any given command or guide name.
+	Prints a list of the Git concept guides on the standard output.
 
 -i::
 --info::
diff --git a/builtin/help.c b/builtin/help.c
index 44ea2798cda..51b18c291d8 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {
 };
 
 static const char * const builtin_help_usage[] = {
-	N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n"
+	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
+	N_("git help [-g|--guides]"),
 	NULL
 };
 
@@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Options that take no further arguments */
+	if (argc && show_guides)
+		usage_msg_opt(_("--guides cannot be combined with other options"),
+			      builtin_help_usage, builtin_help_options);
+
 	if (show_all) {
 		git_config(git_help_config, NULL);
 		if (verbose) {
@@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all || show_guides) {
 		printf("%s\n", _(git_more_info_string));
-		/*
-		* We're done. Ignore any remaining args
-		*/
 		return 0;
 	}
 
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 5679e29c624..0525ec3ee58 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
 	git help -a >/dev/null
 '
 
+test_expect_success 'invalid usage' '
+	test_expect_code 129 git help -g add
+'
+
 test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-- 
2.33.0.1098.gf02a64c1a2d

Re: [PATCH v3 2/9] help: correct usage & behavior of "git help --guides"

[Junio C Hamano]

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

> 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".

That is true for show_guides, but not for show_all, no?

> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	/* Options that take no further arguments */
> +	if (argc && show_guides)
> +		usage_msg_opt(_("--guides cannot be combined with other options"),
> +			      builtin_help_usage, builtin_help_options);

This is a great addition. "git help -a foo" does not seem to fail,
but shouldn't it be given the same treatment before we can remove
the "We're done" comment?

>  	if (show_all) {
>  		git_config(git_help_config, NULL);
>  		if (verbose) {
> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	if (show_all || show_guides) {
>  		printf("%s\n", _(git_more_info_string));
> -		/*
> -		* We're done. Ignore any remaining args
> -		*/
>  		return 0;
>  	}
>  
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 5679e29c624..0525ec3ee58 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '
>  	git help -a >/dev/null
>  '
>  
> +test_expect_success 'invalid usage' '
> +	test_expect_code 129 git help -g add
> +'
> +
>  test_expect_success "works for commands and guides by default" '
>  	configure_help &&
>  	git help status &&

[PATCH v3 3/9] help tests: add test for --config output

[Ævar Arnfjörð Bjarmason]

Add a missing test for checking what the --config output added in
ac68a93fd2 (help: add --config to list all available config,
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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 0525ec3ee58..63c4adb99be 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -77,6 +77,19 @@ test_expect_success 'git help -g' '
 	test_i18ngrep "^   tutorial   " help.output
 '
 
+test_expect_success 'git help -c' '
+	git help -c >help.output &&
+	cat >expect <<-\EOF &&
+
+	'\''git help config'\'' for more information
+	EOF
+	grep -v -E \
+		-e "^[^.]+\.[^.]+$" \
+		-e "^[^.]+\.[^.]+\.[^.]+$" \
+		help.output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 4/9] help: correct logic error in combining --all and --config

[Ævar Arnfjörð Bjarmason]

Fix a bug in the --config option that's been there ever since its
introduction in 3ac68a93fd2 (help: add --config to list all available
config, 2018-05-26). Die when --all and --config are combined,
combining them doesn't make sense.

The code for the --config option when combined with an earlier
refactoring done to support the --guide option in
65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
cause us to take the "--all" branch early and ignore the --config
option.

Let's instead list these as incompatible, both in the synopsis and
help output, and enforce it in the code itself.

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

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index cb8e3d4da9e..96d5f598b4b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git help' [-a|--all [--[no-]verbose]]
 	   [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]
 'git help' [-g|--guides]
+'git help' [-c|--config]
 
 DESCRIPTION
 -----------
diff --git a/builtin/help.c b/builtin/help.c
index 51b18c291d8..d0c9605dbb5 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = {
 	N_("git help [-a|--all] [--[no-]verbose]]\n"
 	   "         [[-i|--info] [-m|--man] [-w|--web]] [<command>]"),
 	N_("git help [-g|--guides]"),
+	N_("git help [-c|--config]"),
 	NULL
 };
 
@@ -553,9 +554,21 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
+	/* Incompatible options */
+	if (show_all && show_config)
+		usage_msg_opt(_("--config and --all cannot be combined"),
+			      builtin_help_usage, builtin_help_options);
+
+	if (show_config && show_guides)
+		usage_msg_opt(_("--config and --guides cannot be combined"),
+			      builtin_help_usage, builtin_help_options);
+
 	/* Options that take no further arguments */
+	if (argc && show_config)
+		usage_msg_opt(_("--config cannot be combined with command or guide names"),
+			      builtin_help_usage, builtin_help_options);
 	if (argc && show_guides)
-		usage_msg_opt(_("--guides cannot be combined with other options"),
+		usage_msg_opt(_("--guides cannot be combined with command or guide names"),
 			      builtin_help_usage, builtin_help_options);
 
 	if (show_all) {
@@ -570,6 +583,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		list_commands(colopts, &main_cmds, &other_cmds);
 	}
 
+	if (show_guides)
+		list_guides_help();
+
+	if (show_all || show_guides) {
+		printf("%s\n", _(git_more_info_string));
+		return 0;
+	}
+
 	if (show_config) {
 		int for_human = show_config == 1;
 
@@ -583,14 +604,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (show_guides)
-		list_guides_help();
-
-	if (show_all || show_guides) {
-		printf("%s\n", _(git_more_info_string));
-		return 0;
-	}
-
 	if (!argv[0]) {
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_common_cmds_help();
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 63c4adb99be..b4ed6229ed8 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -35,7 +35,8 @@ test_expect_success 'basic help commands' '
 '
 
 test_expect_success 'invalid usage' '
-	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" '
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 5/9] help: correct logic error in combining --all and --guides

[Ævar Arnfjörð Bjarmason]

The --all and --guides commands could be combined, which wouldn't have
any impact on the output except for:

    git help --all --guides --no-verbose

Listing the guide alongside that output was clearly not intended, so
let's error out here. See 002b726a400 (builtin/help.c: add
list_common_guides_help() function, 2013-04-02) for the initial
implementation.

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

diff --git a/builtin/help.c b/builtin/help.c
index d0c9605dbb5..30f160a4669 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -559,6 +559,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("--config and --all cannot be combined"),
 			      builtin_help_usage, builtin_help_options);
 
+	if (show_all && show_guides)
+		usage_msg_opt(_("--config and --guides cannot be combined"),
+			      builtin_help_usage, builtin_help_options);
+
 	if (show_config && show_guides)
 		usage_msg_opt(_("--config and --guides cannot be combined"),
 			      builtin_help_usage, builtin_help_options);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index b4ed6229ed8..69e385d3b66 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -36,7 +36,12 @@ test_expect_success 'basic help commands' '
 
 test_expect_success 'invalid usage' '
 	test_expect_code 129 git help -g add &&
-	test_expect_code 129 git help -a -c
+	test_expect_code 129 git help -a -c &&
+
+	test_expect_code 129 git help -g add &&
+	test_expect_code 129 git help -a -g &&
+
+	test_expect_code 129 git help -g -c
 '
 
 test_expect_success "works for commands and guides by default" '
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 6/9] help: simplify by moving to OPT_CMDMODE()

[Ævar Arnfjörð Bjarmason]

As preceding commits have incrementally established all of the --all,
--guides, --config and hidden --config-for-completion options are
mutually exclusive. So let's use OPT_CMDMODE() to parse the
command-line instead, and take advantage of its conflicting options
detection.

This is the first command with a hidden CMDMODE, so let's introduce a
OPT_CMDMODE_F() macro to go along with OPT_CMDMODE().

I think this makes the usage information that we emit slightly worse,
e.g. before we'd emit:

    $ git help --all --config
    fatal: --config and --all cannot be combined

    usage: git help [-a|--all] [--[no-]verbose]]
             [[-i|--info] [-m|--man] [-w|--web]] [<command>]
       or: git help [-g|--guides]
       or: git help [-c|--config]
    [...]
    $

And now:

    $ git help --all --config
    error: option `config' is incompatible with --all
    $

But improving that is a general topic for parse-options.c improvement,
i.e. we should probably emit the full usage in that case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c  | 81 ++++++++++++++++++++++---------------------------
 parse-options.h |  6 ++--
 2 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 30f160a4669..6a022d9803e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -34,27 +34,36 @@ enum help_format {
 	HELP_FORMAT_WEB
 };
 
-static const char *html_path;
+static enum help_action {
+	HELP_ACTION_ALL = 1,
+	HELP_ACTION_GUIDES,
+	HELP_ACTION_CONFIG,
+	HELP_ACTION_CONFIG_FOR_COMPLETION,
+} cmd_mode;
 
-static int show_all = 0;
-static int show_guides = 0;
-static int show_config;
+static const char *html_path;
 static int verbose = 1;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
 static struct option builtin_help_options[] = {
-	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
+	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
+		    HELP_ACTION_ALL),
 	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('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),
 	OPT_SET_INT('i', "info", &help_format, N_("show info page"),
 			HELP_FORMAT_INFO),
 	OPT__VERBOSE(&verbose, N_("print command description")),
+
+	OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
+		    HELP_ACTION_GUIDES),
+	OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
+		    HELP_ACTION_CONFIG),
+	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
+		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+
 	OPT_END(),
 };
 
@@ -544,6 +553,13 @@ static const char *check_git_cmd(const char* cmd)
 	return cmd;
 }
 
+static void no_extra_argc(int argc)
+{
+	if (argc)
+		usage_msg_opt(_("this option doesn't take any other arguments"),
+			      builtin_help_usage, builtin_help_options);
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
@@ -554,28 +570,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	/* Incompatible options */
-	if (show_all && show_config)
-		usage_msg_opt(_("--config and --all cannot be combined"),
-			      builtin_help_usage, builtin_help_options);
-
-	if (show_all && show_guides)
-		usage_msg_opt(_("--config and --guides cannot be combined"),
-			      builtin_help_usage, builtin_help_options);
-
-	if (show_config && show_guides)
-		usage_msg_opt(_("--config and --guides cannot be combined"),
-			      builtin_help_usage, builtin_help_options);
-
-	/* Options that take no further arguments */
-	if (argc && show_config)
-		usage_msg_opt(_("--config cannot be combined with command or guide names"),
-			      builtin_help_usage, builtin_help_options);
-	if (argc && show_guides)
-		usage_msg_opt(_("--guides cannot be combined with command or guide names"),
-			      builtin_help_usage, builtin_help_options);
-
-	if (show_all) {
+	switch (cmd_mode) {
+	case HELP_ACTION_ALL:
 		git_config(git_help_config, NULL);
 		if (verbose) {
 			setup_pager();
@@ -585,25 +581,20 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		load_command_list("git-", &main_cmds, &other_cmds);
 		list_commands(colopts, &main_cmds, &other_cmds);
-	}
-
-	if (show_guides)
+		printf("%s\n", _(git_more_info_string));
+		break;
+	case HELP_ACTION_GUIDES:
+		no_extra_argc(argc);
 		list_guides_help();
-
-	if (show_all || show_guides) {
 		printf("%s\n", _(git_more_info_string));
 		return 0;
-	}
-
-	if (show_config) {
-		int for_human = show_config == 1;
-
-		if (!for_human) {
-			list_config_help(for_human);
-			return 0;
-		}
+	case HELP_ACTION_CONFIG_FOR_COMPLETION:
+		list_config_help(0);
+		return 0;
+	case HELP_ACTION_CONFIG:
+		no_extra_argc(argc);
 		setup_pager();
-		list_config_help(for_human);
+		list_config_help(1);
 		printf("\n%s\n", _("'git help config' for more information"));
 		return 0;
 	}
diff --git a/parse-options.h b/parse-options.h
index a845a9d9527..0e9271dde5c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -169,8 +169,10 @@ struct option {
 #define OPT_BOOL(s, l, v, h)        OPT_BOOL_F(s, l, v, h, 0)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
-#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
-				      (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
+#define OPT_CMDMODE_F(s, l, v, h, i, f)  { OPTION_SET_INT, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), NULL, (i) }
+#define OPT_CMDMODE(s, l, v, h, i)  OPT_CMDMODE_F(s, l, v, h, i, 0)
+
 #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
-- 
2.33.0.1098.gf02a64c1a2d

Re: [PATCH v3 6/9] help: simplify by moving to OPT_CMDMODE()

[Junio C Hamano]

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

> +static void no_extra_argc(int argc)
> +{
> +	if (argc)
> +		usage_msg_opt(_("this option doesn't take any other arguments"),
> +			      builtin_help_usage, builtin_help_options);
> +}

The mention of "this option" smells like a loss of information.  I
might expect the --verbose option might take the verbosity level and
ask for

    $ git help --guides --verbose 99

and the above message may give me a false impression that "this
option" refers to "--verbose" that does not take any value.

As long as --all/--guides/--config are understood as "special" and
different from other options, this may be OK, especially given that
the user will get an error message much earlier if you give two or
more of them at the same time, so such an "which option do you
mean?" confusion may not happen very often.

>  int cmd_help(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit;
> @@ -554,28 +570,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	switch (cmd_mode) {
> +	case HELP_ACTION_ALL:
>  		git_config(git_help_config, NULL);
>  		if (verbose) {
>  			setup_pager();
> @@ -585,25 +581,20 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>  		load_command_list("git-", &main_cmds, &other_cmds);
>  		list_commands(colopts, &main_cmds, &other_cmds);
> +		printf("%s\n", _(git_more_info_string));
> +		break;
> +	case HELP_ACTION_GUIDES:
> +		no_extra_argc(argc);
>  		list_guides_help();
>  		printf("%s\n", _(git_more_info_string));
>  		return 0;
> +	case HELP_ACTION_CONFIG_FOR_COMPLETION:
> +		list_config_help(0);
> +		return 0;
> +	case HELP_ACTION_CONFIG:
> +		no_extra_argc(argc);
>  		setup_pager();
> +		list_config_help(1);
>  		printf("\n%s\n", _("'git help config' for more information"));
>  		return 0;
>  	}

Re: [PATCH v3 6/9] help: simplify by moving to OPT_CMDMODE()

[Junio C Hamano]

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

> As preceding commits have incrementally established all of the --all,
> --guides, --config and hidden --config-for-completion options are
> mutually exclusive. So let's use OPT_CMDMODE() to parse the
> command-line instead, and take advantage of its conflicting options
> detection.

Sounds quite logical.

> I think this makes the usage information that we emit slightly worse,
> e.g. before we'd emit:
>
>     $ git help --all --config
>     fatal: --config and --all cannot be combined
>
>     usage: git help [-a|--all] [--[no-]verbose]]
>              [[-i|--info] [-m|--man] [-w|--web]] [<command>]
>        or: git help [-g|--guides]
>        or: git help [-c|--config]
>     [...]
>     $
>
> And now:
>
>     $ git help --all --config
>     error: option `config' is incompatible with --all
>     $

We often hear that our error messages for command line options are
unhelpful by being too broad, don't we?  I think the new one would
be much better by not scrolling away the most important part.  Those
who want to know about options other than --all and --config can
issue "git help -h" after seeing that single line error, but not
everybody has to do so.

> But improving that is a general topic for parse-options.c improvement,
> i.e. we should probably emit the full usage in that case.

Yes, absolutely.

[PATCH v3 7/9] help tests: test --config-for-completion option & output

[Ævar Arnfjörð Bjarmason]

Add a regression test for the --config-for-completion option, this was
tested for indirectly with the test added in 7a09a8f093e (completion:
add tests for 'git config' completion, 2019-08-13), but let's do it
directly here as well.

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

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 69e385d3b66..25bbaf0d586 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -41,7 +41,8 @@ test_expect_success 'invalid usage' '
 	test_expect_code 129 git help -g add &&
 	test_expect_code 129 git help -a -g &&
 
-	test_expect_code 129 git help -g -c
+	test_expect_code 129 git help -g -c &&
+	test_expect_code 0 git help --config-for-completion add
 '
 
 test_expect_success "works for commands and guides by default" '
@@ -96,6 +97,20 @@ test_expect_success 'git help -c' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git help --config-for-completion' '
+	git help -c >human &&
+	grep -E \
+	     -e "^[^.]+\.[^.]+$" \
+	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
+	     sed -e "s/\*.*//" -e "s/<.*//" |
+	     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 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 8/9] help / completion: make "git help" do the hard work

[Ævar Arnfjörð Bjarmason]

The "help" builtin has been able to emit configuration variables since
e17ca926371 (completion: drop the hard coded list of config vars,
2018-05-26), but it hasn't produced exactly the format the completion
script wanted. Let's do that.

We got partway there in 2675ea1cc0f (completion: use 'sort -u' to
deduplicate config variable names, 2019-08-13) and
d9438873c4d (completion: deduplicate configuration sections,
2019-08-13), but after both we still needed some sorting,
de-duplicating and awk post-processing of the list.

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". The "--config-for-completion" output is the same as before after
being passed through "sort -u".

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).

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                         | 54 +++++++++++++++++++-------
 contrib/completion/git-completion.bash | 21 +++++-----
 t/t0012-help.sh                        | 17 ++++++--
 3 files changed, 65 insertions(+), 27 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6a022d9803e..9a255a9aee6 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -34,11 +34,18 @@ enum help_format {
 	HELP_FORMAT_WEB
 };
 
+enum show_config_type {
+	SHOW_CONFIG_HUMAN,
+	SHOW_CONFIG_VARS,
+	SHOW_CONFIG_SECTIONS,
+};
+
 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;
@@ -63,6 +70,8 @@ static struct option builtin_help_options[] = {
 		    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(),
 };
@@ -82,7 +91,7 @@ struct slot_expansion {
 	int found;
 };
 
-static void list_config_help(int for_human)
+static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
 		{ "advice", "*", list_config_advices },
@@ -100,6 +109,8 @@ static void list_config_help(int for_human)
 	const char **p;
 	struct slot_expansion *e;
 	struct string_list keys = STRING_LIST_INIT_DUP;
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
 	int i;
 
 	for (p = config_name_list; *p; p++) {
@@ -130,34 +141,46 @@ static void list_config_help(int for_human)
 	for (i = 0; i < keys.nr; i++) {
 		const char *var = keys.items[i].string;
 		const char *wildcard, *tag, *cut;
+		const char *dot = NULL;
+		struct strbuf sb = STRBUF_INIT;
 
-		if (for_human) {
+		switch (type) {
+		case SHOW_CONFIG_HUMAN:
 			puts(var);
 			continue;
+		case SHOW_CONFIG_SECTIONS:
+			dot = strchr(var, '.');
+			break;
+		case SHOW_CONFIG_VARS:
+			break;
 		}
-
 		wildcard = strchr(var, '*');
 		tag = strchr(var, '<');
 
-		if (!wildcard && !tag) {
-			puts(var);
+		if (!dot && !wildcard && !tag) {
+			string_list_append(&keys_uniq, var);
 			continue;
 		}
 
-		if (wildcard && !tag)
+		if (dot)
+			cut = dot;
+		else if (wildcard && !tag)
 			cut = wildcard;
 		else if (!wildcard && tag)
 			cut = tag;
 		else
 			cut = wildcard < tag ? wildcard : tag;
 
-		/*
-		 * We may produce duplicates, but that's up to
-		 * git-completion.bash to handle
-		 */
-		printf("%.*s\n", (int)(cut - var), var);
+		strbuf_add(&sb, var, cut - var);
+		string_list_append(&keys_uniq, sb.buf);
+		strbuf_release(&sb);
+
 	}
 	string_list_clear(&keys, 0);
+	string_list_remove_duplicates(&keys_uniq, 0);
+	for_each_string_list_item(item, &keys_uniq)
+		puts(item->string);
+	string_list_clear(&keys_uniq, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
@@ -589,12 +612,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		printf("%s\n", _(git_more_info_string));
 		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 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(1);
+		list_config_help(SHOW_CONFIG_HUMAN);
 		printf("\n%s\n", _("'git help config' for more information"));
 		return 0;
 	}
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8108eda1e86..b9f63701930 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2503,7 +2503,14 @@ __git_config_vars=
 __git_compute_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)"
+}
+
+__git_config_sections=
+__git_compute_config_sections ()
+{
+	test -n "$__git_config_sections" ||
+	__git_config_sections="$(git help --config-sections-for-completion)"
 }
 
 # Completes possible values of various configuration variables.
@@ -2717,16 +2724,8 @@ __git_complete_config_variable_name ()
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"
 		;;
 	*)
-		__git_compute_config_vars
-		__gitcomp "$(echo "$__git_config_vars" |
-				awk -F . '{
-					sections[$1] = 1
-				}
-				END {
-					for (s in sections)
-						print s "."
-				}
-				')" "" "$cur_"
+		__git_compute_config_sections
+		__gitcomp "$__git_config_sections" "" "$cur_" "."
 		;;
 	esac
 }
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 25bbaf0d586..60d713021f9 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -42,7 +42,8 @@ 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 "works for commands and guides by default" '
@@ -106,11 +107,21 @@ 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 "^[^.]+\.[^.]+$" \
+	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
+	     sed -e "s/\..*//" |
+	     sort -u >human.munged &&
+
+	git help --config-sections-for-completion >sections &&
+	test_cmp human.munged sections
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH v3 9/9] help: move column config discovery to help.c library

[Ævar Arnfjörð Bjarmason]

When a git_config() call was added in dbfae689690 (help: reuse
print_columns() for help -a, 2012-04-13) to read the column config
we'd always use the resulting "colopts" variable.

Then in 63eae83f8f3 (help: add "-a --verbose" to list all commands
with synopsis, 2018-05-20) we started only using the "colopts" config
under "--all" if "--no-verbose" was also given, but the "git_config()"
call was not moved inside the "verbose" branch of the code.

This change effectively does that, we'll only call list_commands()
under "--all --no-verbose", so let's have it look up the config it
needs. See 26c7d067832 (help -a: improve and make --verbose default, 2018-09-29) for another case in help.c where we look up config.

The get_colopts() function is named for consistency with the existing
get_alias() function added in 26c7d067832.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/help.c |  7 +------
 help.c         | 16 ++++++++++++++--
 help.h         |  2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 9a255a9aee6..0a40d8cf094 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -7,7 +7,6 @@
 #include "exec-cmd.h"
 #include "parse-options.h"
 #include "run-command.h"
-#include "column.h"
 #include "config-list.h"
 #include "help.h"
 #include "alias.h"
@@ -50,7 +49,6 @@ static enum help_action {
 
 static const char *html_path;
 static int verbose = 1;
-static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
 static struct option builtin_help_options[] = {
@@ -384,8 +382,6 @@ static int add_man_viewer_info(const char *var, const char *value)
 
 static int git_help_config(const char *var, const char *value, void *cb)
 {
-	if (starts_with(var, "column."))
-		return git_column_config(var, value, "help", &colopts);
 	if (!strcmp(var, "help.format")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -595,7 +591,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	switch (cmd_mode) {
 	case HELP_ACTION_ALL:
-		git_config(git_help_config, NULL);
 		if (verbose) {
 			setup_pager();
 			list_all_cmds_help();
@@ -603,7 +598,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		}
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		load_command_list("git-", &main_cmds, &other_cmds);
-		list_commands(colopts, &main_cmds, &other_cmds);
+		list_commands(&main_cmds, &other_cmds);
 		printf("%s\n", _(git_more_info_string));
 		break;
 	case HELP_ACTION_GUIDES:
diff --git a/help.c b/help.c
index be2fa642415..973e47cdc30 100644
--- a/help.c
+++ b/help.c
@@ -293,9 +293,21 @@ void load_command_list(const char *prefix,
 	exclude_cmds(other_cmds, main_cmds);
 }
 
-void list_commands(unsigned int colopts,
-		   struct cmdnames *main_cmds, struct cmdnames *other_cmds)
+static int get_colopts(const char *var, const char *value, void *data)
 {
+	unsigned int *colopts = data;
+
+	if (starts_with(var, "column."))
+		return git_column_config(var, value, "help", colopts);
+
+	return 0;
+}
+
+void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
+{
+	unsigned int colopts = 0;
+	git_config(get_colopts, &colopts);
+
 	if (main_cmds->cnt) {
 		const char *exec_path = git_exec_path();
 		printf_ln(_("available git commands in '%s'"), exec_path);
diff --git a/help.h b/help.h
index 5871e93ba2d..9d383f1a0b2 100644
--- a/help.h
+++ b/help.h
@@ -37,7 +37,7 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len);
 /* Here we require that excludes is a sorted list. */
 void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
 int is_in_cmdlist(struct cmdnames *cmds, const char *name);
-void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
+void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds);
 void get_version_info(struct strbuf *buf, int show_build_options);
 
 /*
-- 
2.33.0.1098.gf02a64c1a2d