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
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 <redacted>
---
Documentation/git-help.txt | 2 +-
builtin/help.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 14 +++++++++-----
t/t0012-help.sh | 5 +++++
3 files changed, 17 insertions(+), 8 deletions(-)
@@ -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::
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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)intnongit;enumhelp_formatparsed_help_format;constchar*page;+intstandalone=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){
@@ -34,6 +34,11 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-cgit-add&&+test_expect_code129githelp-ggit-add+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
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 <redacted>
---
builtin/help.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)if(show_config){intfor_human=show_config==1;-if(!for_human){-list_config_help(for_human);-return0;-}-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"));+return0;}
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 <redacted>
---
Documentation/git-help.txt | 1 +
builtin/help.c | 27 ++++++++++++++++++---------
t/t0012-help.sh | 5 ++++-
3 files changed, 23 insertions(+), 10 deletions(-)
@@ -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)enumhelp_formatparsed_help_format;constchar*page;intstandalone=0;+intneed_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();
@@ -36,7 +36,10 @@ test_expect_success 'basic help commands' ' test_expect_success'invalid usage''test_expect_code129githelp-cgit-add&&-test_expect_code129githelp-ggit-add+test_expect_code129githelp-ggit-add&&++test_expect_code129githelp-a-c&&+test_expect_code129githelp-g-c' test_expect_success"works for commands and guides by default"'
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 <redacted>
---
builtin/help.c | 67 ++++++++++++++++++--------
contrib/completion/git-completion.bash | 21 ++++----
t/t0012-help.sh | 24 +++++++++
3 files changed, 81 insertions(+), 31 deletions(-)
@@ -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),
@@ -121,34 +131,48 @@ static void list_config_help(int for_human)for(i=0;i<keys.nr;i++){constchar*var=keys.items[i].string;constchar*wildcard,*tag,*cut;+constchar*dot=NULL;+structstrbufsb=STRBUF_INIT;-if(for_human){+switch(type){+caseSHOW_CONFIG_HUMAN:puts(var);continue;+caseSHOW_CONFIG_SECTIONS:+dot=strchr(var,'.');+break;+caseSHOW_CONFIG_VARS:+break;+caseSHOW_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;+elseif(wildcard&&!tag)cut=wildcard;elseif(!wildcard&&tag)cut=tag;elsecut=wildcard<tag?wildcard:tag;-/*-*Wemayproduceduplicates,butthat'supto-*git-completion.bashtohandle-*/-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);}staticenumhelp_formatparse_help_format(constchar*format)
@@ -588,13 +612,16 @@ int cmd_help(int argc, const char **argv, const char *prefix)return0;}-if(show_config){-intfor_human=show_config==1;--if(for_human)-setup_pager();-list_config_help(for_human);-if(for_human)+switch(show_config){+caseSHOW_CONFIG_UNSET:+break;+caseSHOW_CONFIG_HUMAN:+setup_pager();+/* fallthrough */+caseSHOW_CONFIG_VARS:+caseSHOW_CONFIG_SECTIONS:+list_config_help(show_config);+if(show_config==SHOW_CONFIG_HUMAN)printf("\n%s\n",_("'git help config' for more information"));return0;
@@ -2503,7 +2503,14 @@ __git_config_vars= __git_compute_config_vars(){test-n"$__git_config_vars"||-__git_config_vars="$(githelp--config-for-completion|sort-u)"+__git_config_vars="$(githelp--config-for-completion-vars)"+}++__git_config_sections=+__git_compute_config_sections()+{+test-n"$__git_config_sections"||+__git_config_sections="$(githelp--config-for-completion-sections)"}# Completes possible values of various configuration variables.
From: Eric Sunshine <hidden> Date: 2021-09-08 16:39:18
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
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.)
From: Eric Sunshine <hidden> Date: 2021-09-08 16:42:00
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
@@ -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.
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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 <redacted>
---
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/
From: Eric Sunshine <hidden> Date: 2021-09-10 08:09:27
On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Wed, Sep 08 2021, Eric Sunshine wrote:
quoted
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.
On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
On Wed, Sep 08 2021, Eric Sunshine wrote:
quoted
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.
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.
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 [off-list ref]
## 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 [off-list ref]
@@ 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
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 <redacted>
---
Documentation/git-help.txt | 2 +-
builtin/help.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 11 +++++++----
t/t0012-help.sh | 4 ++++
3 files changed, 14 insertions(+), 7 deletions(-)
@@ -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::
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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){
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-ggit-add+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
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 <redacted>
---
Documentation/git-help.txt | 1 +
builtin/help.c | 37 +++++++++++++++++++++++++++----------
t/t0012-help.sh | 7 ++++++-
3 files changed, 34 insertions(+), 11 deletions(-)
@@ -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)intnongit;enumhelp_formatparsed_help_format;constchar*page;+intneed_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();
@@ -35,7 +35,12 @@ test_expect_success 'basic help commands' '' test_expect_success'invalid usage''-test_expect_code129githelp-ggit-add+test_expect_code129githelp-ggit-add&&+test_expect_code129githelp-cgit-add&&+test_expect_code129githelp-ggit-add&&++test_expect_code129githelp-a-c&&+test_expect_code129githelp-g-c' test_expect_success"works for commands and guides by default"'
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 <redacted>
---
builtin/help.c | 67 ++++++++++++++++++--------
contrib/completion/git-completion.bash | 21 ++++----
t/t0012-help.sh | 24 +++++++++
3 files changed, 81 insertions(+), 31 deletions(-)
@@ -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),
@@ -121,34 +131,48 @@ static void list_config_help(int for_human)for(i=0;i<keys.nr;i++){constchar*var=keys.items[i].string;constchar*wildcard,*tag,*cut;+constchar*dot=NULL;+structstrbufsb=STRBUF_INIT;-if(for_human){+switch(type){+caseSHOW_CONFIG_HUMAN:puts(var);continue;+caseSHOW_CONFIG_SECTIONS:+dot=strchr(var,'.');+break;+caseSHOW_CONFIG_VARS:+break;+caseSHOW_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;+elseif(wildcard&&!tag)cut=wildcard;elseif(!wildcard&&tag)cut=tag;elsecut=wildcard<tag?wildcard:tag;-/*-*Wemayproduceduplicates,butthat'supto-*git-completion.bashtohandle-*/-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);}staticenumhelp_formatparse_help_format(constchar*format)
@@ -595,16 +619,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)return0;}-if(show_config){-intfor_human=show_config==1;+switch(show_config){+caseSHOW_CONFIG_UNSET:+break;+caseSHOW_CONFIG_VARS:+caseSHOW_CONFIG_SECTIONS:+list_config_help(show_config);-if(!for_human){-list_config_help(for_human);-return0;-}+return0;+caseSHOW_CONFIG_HUMAN:setup_pager();-list_config_help(for_human);+list_config_help(show_config);printf("\n%s\n",_("'git help config' for more information"));+return0;}
@@ -2503,7 +2503,14 @@ __git_config_vars= __git_compute_config_vars(){test-n"$__git_config_vars"||-__git_config_vars="$(githelp--config-for-completion|sort-u)"+__git_config_vars="$(githelp--config-for-completion-vars)"+}++__git_config_sections=+__git_compute_config_sections()+{+test-n"$__git_config_sections"||+__git_config_sections="$(githelp--config-for-completion-sections)"}# Completes possible values of various configuration variables.
From: Philip Oakley <hidden> Date: 2021-09-10 18:15:51
On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
quoted hunk
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 11 +++++++----
t/t0012-help.sh | 4 ++++
3 files changed, 14 insertions(+), 7 deletions(-)
@@ -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.
quoted hunk
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::
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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){
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-ggit-add+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&
From: Philip Oakley <hidden> Date: 2021-09-10 18:29:42
On 10/09/2021 19:15, Philip Oakley wrote:
On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
quoted
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 11 +++++++----
t/t0012-help.sh | 4 ++++
3 files changed, 14 insertions(+), 7 deletions(-)
@@ -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
quoted
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::
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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){
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-ggit-add+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&
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.
From: Junio C Hamano <hidden> Date: 2021-09-11 01:22:54
Ævar Arnfjörð Bjarmason [off-list ref] 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.
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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.
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-ggit-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 &&
From: Junio C Hamano <hidden> Date: 2021-09-11 01:33:10
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
@@ -77,6 +77,19 @@ test_expect_success 'git help -g' 'test_i18ngrep"^ tutorial "help.output'+test_expect_success'git help -c''+githelp-c>help.output&&+cat>expect<<-\EOF&&++'"'"'git help config'"'"'formoreinformation
'\''git help config'\'' for more information
is a tad shorter.
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.
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
@@ -77,6 +77,19 @@ test_expect_success 'git help -g' 'test_i18ngrep"^ tutorial "help.output'+test_expect_success'git help -c''+githelp-c>help.output&&+cat>expect<<-\EOF&&++'"'"'git help config'"'"'formoreinformation
'\''git help config'\'' for more information
is a tad shorter.
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.
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.
From: Philip Oakley <hidden> Date: 2021-09-13 19:21:35
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
quoted hunk
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:
quoted
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 11 +++++++----
t/t0012-help.sh | 4 ++++
3 files changed, 14 insertions(+), 7 deletions(-)
@@ -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>
quoted
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'".
From: Philip Oakley <hidden> Date: 2021-09-21 14:20:10
On 21/09/2021 14:49, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
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
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 [off-list ref]
@@ 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 [off-list ref]
## 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 [off-list ref]
## 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
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 <redacted>
---
Documentation/git-help.txt | 2 +-
builtin/help.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
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 <redacted>
---
Documentation/git-help.txt | 6 +++---
builtin/help.c | 11 +++++++----
t/t0012-help.sh | 4 ++++
3 files changed, 14 insertions(+), 7 deletions(-)
@@ -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::
@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {};staticconstchar*constbuiltin_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){
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-gadd+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&
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 <redacted>
---
t/t0012-help.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
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 <redacted>
---
Documentation/git-help.txt | 1 +
builtin/help.c | 31 ++++++++++++++++++++++---------
t/t0012-help.sh | 3 ++-
3 files changed, 25 insertions(+), 10 deletions(-)
@@ -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){
@@ -35,7 +35,8 @@ test_expect_success 'basic help commands' '' test_expect_success'invalid usage''-test_expect_code129githelp-gadd+test_expect_code129githelp-gadd&&+test_expect_code129githelp-a-c' test_expect_success"works for commands and guides by default"'
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 <redacted>
---
builtin/help.c | 4 ++++
t/t0012-help.sh | 7 ++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
@@ -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);
@@ -36,7 +36,12 @@ test_expect_success 'basic help commands' ' test_expect_success'invalid usage''test_expect_code129githelp-gadd&&-test_expect_code129githelp-a-c+test_expect_code129githelp-a-c&&++test_expect_code129githelp-gadd&&+test_expect_code129githelp-a-g&&++test_expect_code129githelp-g-c' test_expect_success"works for commands and guides by default"'
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 <redacted>
---
builtin/help.c | 81 ++++++++++++++++++++++---------------------------
parse-options.h | 6 ++--
2 files changed, 40 insertions(+), 47 deletions(-)
@@ -34,27 +34,36 @@ enum help_format {HELP_FORMAT_WEB};-staticconstchar*html_path;+staticenumhelp_action{+HELP_ACTION_ALL=1,+HELP_ACTION_GUIDES,+HELP_ACTION_CONFIG,+HELP_ACTION_CONFIG_FOR_COMPLETION,+}cmd_mode;-staticintshow_all=0;-staticintshow_guides=0;-staticintshow_config;+staticconstchar*html_path;staticintverbose=1;staticunsignedintcolopts;staticenumhelp_formathelp_format=HELP_FORMAT_NONE;staticintexclude_guides;staticstructoptionbuiltin_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)returncmd;}+staticvoidno_extra_argc(intargc)+{+if(argc)+usage_msg_opt(_("this option doesn't take any other arguments"),+builtin_help_usage,builtin_help_options);+}+intcmd_help(intargc,constchar**argv,constchar*prefix){intnongit;
@@ -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){+caseHELP_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;+caseHELP_ACTION_GUIDES:+no_extra_argc(argc);list_guides_help();--if(show_all||show_guides){printf("%s\n",_(git_more_info_string));return0;-}--if(show_config){-intfor_human=show_config==1;--if(!for_human){-list_config_help(for_human);-return0;-}+caseHELP_ACTION_CONFIG_FOR_COMPLETION:+list_config_help(0);+return0;+caseHELP_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"));return0;}
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 <redacted>
---
t/t0012-help.sh | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
@@ -41,7 +41,8 @@ test_expect_success 'invalid usage' 'test_expect_code129githelp-gadd&&test_expect_code129githelp-a-g&&-test_expect_code129githelp-g-c+test_expect_code129githelp-g-c&&+test_expect_code0githelp--config-for-completionadd' test_expect_success"works for commands and guides by default"'
@@ -96,6 +97,20 @@ test_expect_success 'git help -c' 'test_cmpexpectactual'+test_expect_success'git help --config-for-completion''+githelp-c>human&&+grep-E\+-e"^[^.]+\.[^.]+$"\+-e"^[^.]+\.[^.]+\.[^.]+$"human|+sed-e"s/\*.*//"-e"s/<.*//"|+sort-u>human.munged&&++githelp--config-for-completion>vars&&+sort-u<vars>vars.new&&+mvvars.newvars&&+test_cmphuman.mungedvars+'+ test_expect_success'generate builtin list''git--list-cmds=builtins>builtins'
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 <redacted>
---
builtin/help.c | 7 +------
help.c | 16 ++++++++++++++--
help.h | 2 +-
3 files changed, 16 insertions(+), 9 deletions(-)
@@ -37,7 +37,7 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len);/* Here we require that excludes is a sorted list. */voidexclude_cmds(structcmdnames*cmds,structcmdnames*excludes);intis_in_cmdlist(structcmdnames*cmds,constchar*name);-voidlist_commands(unsignedintcolopts,structcmdnames*main_cmds,structcmdnames*other_cmds);+voidlist_commands(structcmdnames*main_cmds,structcmdnames*other_cmds);voidget_version_info(structstrbuf*buf,intshow_build_options);/*
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 <redacted>
---
builtin/help.c | 54 +++++++++++++++++++-------
contrib/completion/git-completion.bash | 21 +++++-----
t/t0012-help.sh | 17 ++++++--
3 files changed, 65 insertions(+), 27 deletions(-)
@@ -589,12 +612,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)printf("%s\n",_(git_more_info_string));return0;caseHELP_ACTION_CONFIG_FOR_COMPLETION:-list_config_help(0);+no_extra_argc(argc);+list_config_help(SHOW_CONFIG_VARS);+return0;+caseHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION:+no_extra_argc(argc);+list_config_help(SHOW_CONFIG_SECTIONS);return0;caseHELP_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"));return0;}
@@ -2503,7 +2503,14 @@ __git_config_vars= __git_compute_config_vars(){test-n"$__git_config_vars"||-__git_config_vars="$(githelp--config-for-completion|sort-u)"+__git_config_vars="$(githelp--config-for-completion)"+}++__git_config_sections=+__git_compute_config_sections()+{+test-n"$__git_config_sections"||+__git_config_sections="$(githelp--config-sections-for-completion)"}# Completes possible values of various configuration variables.
@@ -42,7 +42,8 @@ test_expect_success 'invalid usage' 'test_expect_code129githelp-a-g&&test_expect_code129githelp-g-c&&-test_expect_code0githelp--config-for-completionadd+test_expect_code129githelp--config-for-completionadd&&+test_expect_code129githelp--config-sections-for-completionadd' 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&&githelp--config-for-completion>vars&&-sort-u<vars>vars.new&&-mvvars.newvars&&test_cmphuman.mungedvars'+test_expect_success'git help --config-sections-for-completion''+githelp-c>human&&+grep-E\+-e"^[^.]+\.[^.]+$"\+-e"^[^.]+\.[^.]+\.[^.]+$"human|+sed-e"s/\..*//"|+sort-u>human.munged&&++githelp--config-sections-for-completion>sections&&+test_cmphuman.mungedsections+'+ test_expect_success'generate builtin list''git--list-cmds=builtins>builtins'
From: Junio C Hamano <hidden> Date: 2021-09-23 18:05:23
Ævar Arnfjörð Bjarmason [off-list ref] 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.
From: Junio C Hamano <hidden> Date: 2021-09-23 18:05:30
Ævar Arnfjörð Bjarmason [off-list ref] 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?
quoted hunk
@@ -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?
quoted hunk
if (show_all) {
git_config(git_help_config, NULL);
if (verbose) {
@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' 'githelp-a>/dev/null'+test_expect_success'invalid usage''+test_expect_code129githelp-gadd+'+ test_expect_success"works for commands and guides by default"'configure_help&&githelpstatus&&