| Message ID | patch-v2-2.5-039639a0dd3-20210910T112545Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | help: fix usage nits & bugs, completion shellscript->C | expand |
On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote: > As noted in 65f98358c0c (builtin/help.c: add --guide option, > 2013-04-02) and a133737b809 (doc: include --guide option description > for "git help", 2013-04-02) which introduced the --guide option it > cannot be combined with e.g. <command>. > > Change both the usage string to reflect that, and test and assert for > this behavior in the command itself. Now that we assert this in code > we don't need to exhaustively describe the previous confusing behavior > in the documentation either, instead of silently ignoring the provided > argument we'll now error out. > > The comment being removed was added in 15f7d494380 (builtin/help.c: > split "-a" processing into two, 2013-04-02). The "Ignore any remaining > args" part of it is now no longer applicable as explained above, let's > just remove it entirely, it's rather obvious that if we're returning > we're done. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > Documentation/git-help.txt | 6 +++--- > builtin/help.c | 11 +++++++---- > t/t0012-help.sh | 4 ++++ > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt > index 568a0b606f3..cb8e3d4da9e 100644 > --- a/Documentation/git-help.txt > +++ b/Documentation/git-help.txt > @@ -8,8 +8,9 @@ git-help - Display help information about Git > SYNOPSIS > -------- > [verse] > -'git help' [-a|--all [--[no-]verbose]] [-g|--guides] > +'git help' [-a|--all [--[no-]verbose]] > [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] > +'git help' [-g|--guides] Shouldn't we also include the [-c|--config] options here in the synopsis, and the help_usage below? Further, shouldn't we mention this (git help -c) on the git config man page, e.g. "A list all available configuration variables can be generated by `git help -c`." I hadn't realised the facility was even available. > > DESCRIPTION > ----------- > @@ -58,8 +59,7 @@ OPTIONS > > -g:: > --guides:: > - Prints a list of the Git concept guides on the standard output. This > - option overrides any given command or guide name. > + Prints a list of the Git concept guides on the standard output. > > -i:: > --info:: > diff --git a/builtin/help.c b/builtin/help.c > index 44ea2798cda..51b18c291d8 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = { > }; > > static const char * const builtin_help_usage[] = { > - N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" > + N_("git help [-a|--all] [--[no-]verbose]]\n" > " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), > + N_("git help [-g|--guides]"), > NULL > }; > > @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix) > builtin_help_usage, 0); > parsed_help_format = help_format; > > + /* Options that take no further arguments */ > + if (argc && show_guides) > + usage_msg_opt(_("--guides cannot be combined with other options"), > + builtin_help_usage, builtin_help_options); > + > if (show_all) { > git_config(git_help_config, NULL); > if (verbose) { > @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) > > if (show_all || show_guides) { > printf("%s\n", _(git_more_info_string)); > - /* > - * We're done. Ignore any remaining args > - */ > return 0; > } > > diff --git a/t/t0012-help.sh b/t/t0012-help.sh > index 5679e29c624..c3aa016fd30 100755 > --- a/t/t0012-help.sh > +++ b/t/t0012-help.sh > @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' ' > git help -a >/dev/null > ' > > +test_expect_success 'invalid usage' ' > + test_expect_code 129 git help -g git-add > +' > + > test_expect_success "works for commands and guides by default" ' > configure_help && > git help status &&
On 10/09/2021 19:15, Philip Oakley wrote: > On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote: >> As noted in 65f98358c0c (builtin/help.c: add --guide option, >> 2013-04-02) and a133737b809 (doc: include --guide option description >> for "git help", 2013-04-02) which introduced the --guide option it >> cannot be combined with e.g. <command>. >> >> Change both the usage string to reflect that, and test and assert for >> this behavior in the command itself. Now that we assert this in code >> we don't need to exhaustively describe the previous confusing behavior >> in the documentation either, instead of silently ignoring the provided >> argument we'll now error out. >> >> The comment being removed was added in 15f7d494380 (builtin/help.c: >> split "-a" processing into two, 2013-04-02). The "Ignore any remaining >> args" part of it is now no longer applicable as explained above, let's >> just remove it entirely, it's rather obvious that if we're returning >> we're done. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> Documentation/git-help.txt | 6 +++--- >> builtin/help.c | 11 +++++++---- >> t/t0012-help.sh | 4 ++++ >> 3 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt >> index 568a0b606f3..cb8e3d4da9e 100644 >> --- a/Documentation/git-help.txt >> +++ b/Documentation/git-help.txt >> @@ -8,8 +8,9 @@ git-help - Display help information about Git >> SYNOPSIS >> -------- >> [verse] >> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides] >> +'git help' [-a|--all [--[no-]verbose]] >> [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] >> +'git help' [-g|--guides] > Shouldn't we also include the [-c|--config] options here in the synopsis, > and the help_usage below? I see this is fixed in 4/5 > > Further, shouldn't we mention this (git help -c) on the git config man > page, e.g. "A list all available configuration variables can be > generated by `git help -c`." Still feel this one would be useful (but may be out of scope of this series) > > I hadn't realised the facility was even available. Philip >> >> DESCRIPTION >> ----------- >> @@ -58,8 +59,7 @@ OPTIONS >> >> -g:: >> --guides:: >> - Prints a list of the Git concept guides on the standard output. This >> - option overrides any given command or guide name. >> + Prints a list of the Git concept guides on the standard output. >> >> -i:: >> --info:: >> diff --git a/builtin/help.c b/builtin/help.c >> index 44ea2798cda..51b18c291d8 100644 >> --- a/builtin/help.c >> +++ b/builtin/help.c >> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = { >> }; >> >> static const char * const builtin_help_usage[] = { >> - N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" >> + N_("git help [-a|--all] [--[no-]verbose]]\n" >> " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), >> + N_("git help [-g|--guides]"), >> NULL >> }; >> >> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix) >> builtin_help_usage, 0); >> parsed_help_format = help_format; >> >> + /* Options that take no further arguments */ >> + if (argc && show_guides) >> + usage_msg_opt(_("--guides cannot be combined with other options"), >> + builtin_help_usage, builtin_help_options); >> + >> if (show_all) { >> git_config(git_help_config, NULL); >> if (verbose) { >> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) >> >> if (show_all || show_guides) { >> printf("%s\n", _(git_more_info_string)); >> - /* >> - * We're done. Ignore any remaining args >> - */ >> return 0; >> } >> >> diff --git a/t/t0012-help.sh b/t/t0012-help.sh >> index 5679e29c624..c3aa016fd30 100755 >> --- a/t/t0012-help.sh >> +++ b/t/t0012-help.sh >> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' ' >> git help -a >/dev/null >> ' >> >> +test_expect_success 'invalid usage' ' >> + test_expect_code 129 git help -g git-add >> +' >> + >> test_expect_success "works for commands and guides by default" ' >> configure_help && >> git help status &&
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > As noted in 65f98358c0c (builtin/help.c: add --guide option, > 2013-04-02) and a133737b809 (doc: include --guide option description > for "git help", 2013-04-02) which introduced the --guide option it > cannot be combined with e.g. <command>. Missing comman ',' between 'option' and 'it'. > Change both the usage string to reflect that, and test and assert for I couldn't immediately tell which two things "both the usage string" is referring to. Presumably in the doc and "help -h" output? > this behavior in the command itself. Now that we assert this in code > we don't need to exhaustively describe the previous confusing behavior > in the documentation either, instead of silently ignoring the provided > argument we'll now error out. > > The comment being removed was added in 15f7d494380 (builtin/help.c: > split "-a" processing into two, 2013-04-02). The "Ignore any remaining > args" part of it is now no longer applicable as explained above, let's > just remove it entirely, it's rather obvious that if we're returning > we're done. Three sentences strung together with two commas ',' in between at the end of this paragraph. "A, let's B, it's C" -> "A. Let's B, because it's C" or something. > diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt > index 568a0b606f3..cb8e3d4da9e 100644 > --- a/Documentation/git-help.txt > +++ b/Documentation/git-help.txt > @@ -8,8 +8,9 @@ git-help - Display help information about Git > SYNOPSIS > -------- > [verse] > -'git help' [-a|--all [--[no-]verbose]] [-g|--guides] > +'git help' [-a|--all [--[no-]verbose]] > [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] > +'git help' [-g|--guides] Good. > diff --git a/builtin/help.c b/builtin/help.c > index 44ea2798cda..51b18c291d8 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = { > }; > > static const char * const builtin_help_usage[] = { > - N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" > + N_("git help [-a|--all] [--[no-]verbose]]\n" > " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), > + N_("git help [-g|--guides]"), Good. I still think -s|--long is cluttering than helping, though. > + /* Options that take no further arguments */ > + if (argc && show_guides) > + usage_msg_opt(_("--guides cannot be combined with other options"), > + builtin_help_usage, builtin_help_options); At this point we have called parse_options() and there is no further funky command line parsing involved, so non-zero argc does mean we have something --guides does not know what to do. Good. > @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) > > if (show_all || show_guides) { > printf("%s\n", _(git_more_info_string)); > - /* > - * We're done. Ignore any remaining args > - */ > return 0; > } > > diff --git a/t/t0012-help.sh b/t/t0012-help.sh > index 5679e29c624..c3aa016fd30 100755 > --- a/t/t0012-help.sh > +++ b/t/t0012-help.sh > @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' ' > git help -a >/dev/null > ' > > +test_expect_success 'invalid usage' ' > + test_expect_code 129 git help -g git-add ;-) I would have expected a bare "add" not "git-add" here, but it is OK. It is funny that "git help git-add" still works, but that is a bug that is unrelated to this patch. > +' > + > test_expect_success "works for commands and guides by default" ' > configure_help && > git help status &&
On Fri, Sep 10 2021, Philip Oakley wrote: > On 10/09/2021 19:15, Philip Oakley wrote: >> On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote: >>> As noted in 65f98358c0c (builtin/help.c: add --guide option, >>> 2013-04-02) and a133737b809 (doc: include --guide option description >>> for "git help", 2013-04-02) which introduced the --guide option it >>> cannot be combined with e.g. <command>. >>> >>> Change both the usage string to reflect that, and test and assert for >>> this behavior in the command itself. Now that we assert this in code >>> we don't need to exhaustively describe the previous confusing behavior >>> in the documentation either, instead of silently ignoring the provided >>> argument we'll now error out. >>> >>> The comment being removed was added in 15f7d494380 (builtin/help.c: >>> split "-a" processing into two, 2013-04-02). The "Ignore any remaining >>> args" part of it is now no longer applicable as explained above, let's >>> just remove it entirely, it's rather obvious that if we're returning >>> we're done. >>> >>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >>> --- >>> Documentation/git-help.txt | 6 +++--- >>> builtin/help.c | 11 +++++++---- >>> t/t0012-help.sh | 4 ++++ >>> 3 files changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt >>> index 568a0b606f3..cb8e3d4da9e 100644 >>> --- a/Documentation/git-help.txt >>> +++ b/Documentation/git-help.txt >>> @@ -8,8 +8,9 @@ git-help - Display help information about Git >>> SYNOPSIS >>> -------- >>> [verse] >>> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides] >>> +'git help' [-a|--all [--[no-]verbose]] >>> [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] >>> +'git help' [-g|--guides] >> Shouldn't we also include the [-c|--config] options here in the synopsis, >> and the help_usage below? > > I see this is fixed in 4/5 I updated the config message for the v3 to say it'll be addressed later> >> Further, shouldn't we mention this (git help -c) on the git config man >> page, e.g. "A list all available configuration variables can be >> generated by `git help -c`." > > Still feel this one would be useful (but may be out of scope of this series) We already have such a mention in the documentation, it pre-dates this series. I.e.: -c:: --config:: List all available configuration variables. This is a short summary of the list in linkgit:git-config[1]. The "short summary" there is quite the understatement, but that wording was added in , 3ac68a93fd2 (help: add --config to list all available config, 2018-05-26) so it wasn't some mistake with the option drifting out of sync with an earlier implementation. I think what Nguyễn meant here was "much shorte than 'git help config'".
On 21/09/2021 14:49, Ævar Arnfjörð Bjarmason wrote: >>> Further, shouldn't we mention this (git help -c) on the git config man >>> page, e.g. "A list all available configuration variables can be >>> generated by `git help -c`." >> Still feel this one would be useful (but may be out of scope of this series) > We already have such a mention in the documentation, it pre-dates this > series. I.e.: > > -c:: > --config:: > List all available configuration variables. This is a short > summary of the list in linkgit:git-config[1]. > > The "short summary" there is quite the understatement, but that wording > was added in , 3ac68a93fd2 (help: add --config to list all available > config, 2018-05-26) so it wasn't some mistake with the option drifting > out of sync with an earlier implementation. > > I think what Nguyễn meant here was "much shorte than 'git help config'". I was just saying the 'git help config' should point to the `git help --config` command (extra `--`) I then realised I ought to propose a patch, which is now in `next` ae578de926 (doc: config, tell readers of `git help --config`, 2021-09-13), I should have added the wider cc list. https://lore.kernel.org/git/20210913212305.1832-1-philipoakley@iee.email/ Philip
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index 568a0b606f3..cb8e3d4da9e 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -8,8 +8,9 @@ git-help - Display help information about Git SYNOPSIS -------- [verse] -'git help' [-a|--all [--[no-]verbose]] [-g|--guides] +'git help' [-a|--all [--[no-]verbose]] [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] +'git help' [-g|--guides] DESCRIPTION ----------- @@ -58,8 +59,7 @@ OPTIONS -g:: --guides:: - Prints a list of the Git concept guides on the standard output. This - option overrides any given command or guide name. + Prints a list of the Git concept guides on the standard output. -i:: --info:: diff --git a/builtin/help.c b/builtin/help.c index 44ea2798cda..51b18c291d8 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = { }; static const char * const builtin_help_usage[] = { - N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" + N_("git help [-a|--all] [--[no-]verbose]]\n" " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), + N_("git help [-g|--guides]"), NULL }; @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix) builtin_help_usage, 0); parsed_help_format = help_format; + /* Options that take no further arguments */ + if (argc && show_guides) + usage_msg_opt(_("--guides cannot be combined with other options"), + builtin_help_usage, builtin_help_options); + if (show_all) { git_config(git_help_config, NULL); if (verbose) { @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all || show_guides) { printf("%s\n", _(git_more_info_string)); - /* - * We're done. Ignore any remaining args - */ return 0; } diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 5679e29c624..c3aa016fd30 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' ' git help -a >/dev/null ' +test_expect_success 'invalid usage' ' + test_expect_code 129 git help -g git-add +' + test_expect_success "works for commands and guides by default" ' configure_help && git help status &&
As noted in 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) and a133737b809 (doc: include --guide option description for "git help", 2013-04-02) which introduced the --guide option it cannot be combined with e.g. <command>. Change both the usage string to reflect that, and test and assert for this behavior in the command itself. Now that we assert this in code we don't need to exhaustively describe the previous confusing behavior in the documentation either, instead of silently ignoring the provided argument we'll now error out. The comment being removed was added in 15f7d494380 (builtin/help.c: split "-a" processing into two, 2013-04-02). The "Ignore any remaining args" part of it is now no longer applicable as explained above, let's just remove it entirely, it's rather obvious that if we're returning we're done. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-help.txt | 6 +++--- builtin/help.c | 11 +++++++---- t/t0012-help.sh | 4 ++++ 3 files changed, 14 insertions(+), 7 deletions(-)