| Message ID | patch-5.6-bcc640d32a1-20210908T151949Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | help: fix usage nits & bugs, completion shellscript->C | expand |
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Fix a bug in the --config option that's been there ever since its > introduction in 3ac68a93fd2 (help: add --config to list all available > config, 2018-05-26). Die when --all and --config are combined, > combining them doesn't make sense. > > The code for the --config option when combined with an earlier > refactoring done to support the --guide option in > 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would > cause us to take the "--all" branch early and ignore the --config > option. > > Let's instead list these as incompatible, both in the synopsis and > help output, and enforce it in the code itself. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/builtin/help.c b/builtin/help.c > @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix) > + /* Incompatible options */ > + if (show_all + !!show_config + show_guides > 1) > + usage_with_options(builtin_help_usage, builtin_help_options); I personally find it highly frustrating when a program merely dumps the usage statement without any explanation of what exactly it doesn't like about the command-line. Printing out a simple: --all, --guides, --config are mutually exclusive message would go a long way toward reducing the frustration level. (Aside: I also find it more hostile than helpful when programs dump the usage statement for a command-line invocation error -- even if preceded by an explanation of the error -- since the explanation usually gets drowned-out by the often multi-page usage text, and the user has to go spelunking around the wall of output to try to figure out what actually went wrong. It's much more helpful and easy to figure out what went wrong with the invocation when only a simple error message is printed -- without usage statement. However, that's a separate battle, as Git already has plenty of places which dump the usage statement in response to an invocation error.)
On Wed, Sep 08 2021, Eric Sunshine wrote: > On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> Fix a bug in the --config option that's been there ever since its >> introduction in 3ac68a93fd2 (help: add --config to list all available >> config, 2018-05-26). Die when --all and --config are combined, >> combining them doesn't make sense. >> >> The code for the --config option when combined with an earlier >> refactoring done to support the --guide option in >> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would >> cause us to take the "--all" branch early and ignore the --config >> option. >> >> Let's instead list these as incompatible, both in the synopsis and >> help output, and enforce it in the code itself. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> diff --git a/builtin/help.c b/builtin/help.c >> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix) >> + /* Incompatible options */ >> + if (show_all + !!show_config + show_guides > 1) >> + usage_with_options(builtin_help_usage, builtin_help_options); > > I personally find it highly frustrating when a program merely dumps > the usage statement without any explanation of what exactly it doesn't > like about the command-line. Printing out a simple: > > --all, --guides, --config are mutually exclusive > > message would go a long way toward reducing the frustration level. > > (Aside: I also find it more hostile than helpful when programs dump > the usage statement for a command-line invocation error -- even if > preceded by an explanation of the error -- since the explanation > usually gets drowned-out by the often multi-page usage text, and the > user has to go spelunking around the wall of output to try to figure > out what actually went wrong. It's much more helpful and easy to > figure out what went wrong with the invocation when only a simple > error message is printed -- without usage statement. However, that's a > separate battle, as Git already has plenty of places which dump the > usage statement in response to an invocation error.) I'll make it emit something more helpful. More generally I've got quite a bit of parse_options() improvements queued up locally that I've been trying to trickle in at the rate I can get them through the list, review over at [1] would be much appreciated. I wonder if we can do this automatically, we already have the builtin_help_usage, we could parse that in the general case and find that certain options are mutually exclusive per the examples there. We'd then discover what option we parsed when usage_with_options() was called, and automatically emit a useful message in these sorts of cases. Of course the usage strings might be incomplete or wrong, but part of the point would be to find those cases & a test mode to die() if a command was called with some option combinations not suggested as working according to its documented usage... https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/
On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Wed, Sep 08 2021, Eric Sunshine wrote: > > I personally find it highly frustrating when a program merely dumps > > the usage statement without any explanation of what exactly it doesn't > > like about the command-line. Printing out a simple: > > > > --all, --guides, --config are mutually exclusive > > > > message would go a long way toward reducing the frustration level. > > I'll make it emit something more helpful. > > More generally I've got quite a bit of parse_options() improvements > queued up locally that I've been trying to trickle in at the rate I can > get them through the list, review over at [1] would be much appreciated. > https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/ My review time is very limited these days (which is why most of my review comments lately are superficial), but I set aside some time to review that series for you[1]. Most of my review is subjective, but I did identify one lurking bug (assuming I understand the code correctly). [1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/ > I wonder if we can do this automatically, we already have the > builtin_help_usage, we could parse that in the general case and find > that certain options are mutually exclusive per the examples there. > > We'd then discover what option we parsed when usage_with_options() was > called, and automatically emit a useful message in these sorts of cases. > > Of course the usage strings might be incomplete or wrong, but part of > the point would be to find those cases & a test mode to die() if a > command was called with some option combinations not suggested as > working according to its documented usage... Perhaps, though I imagine it would have to employ some, um, "interesting" heuristics, and be quite hit-and-miss at first, at least until people get around to formalizing existing and new usage strings with the specific goal of supporting that feature. Speaking of heuristics and wishful thinking, when I read the cover letter of your series which I just reviewed, I thought at first that the end-goal would be to ignore whatever indentation the caller provided following each embedded newline, and instead insert the correct computed indentation automatically. This approach would obviate the need for the [1/2] indentation cleanup patch. However, doing so would require heuristics or at least manual help from the caller to indicate the proper indentation width. I also thought perhaps the intention of the series was to do the line-wrapping automatically (ignoring any caller-provided embedded newlines), thus ensuring that the lines were indented correctly _and_ fit the terminal width properly regardless, but that's a somewhat more substantial change.
On Fri, Sep 10 2021, Eric Sunshine wrote: > On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> On Wed, Sep 08 2021, Eric Sunshine wrote: >> > I personally find it highly frustrating when a program merely dumps >> > the usage statement without any explanation of what exactly it doesn't >> > like about the command-line. Printing out a simple: >> > >> > --all, --guides, --config are mutually exclusive >> > >> > message would go a long way toward reducing the frustration level. >> >> I'll make it emit something more helpful. >> >> More generally I've got quite a bit of parse_options() improvements >> queued up locally that I've been trying to trickle in at the rate I can >> get them through the list, review over at [1] would be much appreciated. >> https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/ > > My review time is very limited these days (which is why most of my > review comments lately are superficial), but I set aside some time to > review that series for you[1]. Most of my review is subjective, but I > did identify one lurking bug (assuming I understand the code > correctly). Thanks, I'll try to take a look at that in detail & re-roll soon. > [1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/ > >> I wonder if we can do this automatically, we already have the >> builtin_help_usage, we could parse that in the general case and find >> that certain options are mutually exclusive per the examples there. >> >> We'd then discover what option we parsed when usage_with_options() was >> called, and automatically emit a useful message in these sorts of cases. >> >> Of course the usage strings might be incomplete or wrong, but part of >> the point would be to find those cases & a test mode to die() if a >> command was called with some option combinations not suggested as >> working according to its documented usage... > > Perhaps, though I imagine it would have to employ some, um, > "interesting" heuristics, and be quite hit-and-miss at first, at least > until people get around to formalizing existing and new usage strings > with the specific goal of supporting that feature. > > Speaking of heuristics and wishful thinking, when I read the cover > letter of your series which I just reviewed, I thought at first that > the end-goal would be to ignore whatever indentation the caller > provided following each embedded newline, and instead insert the > correct computed indentation automatically. This approach would > obviate the need for the [1/2] indentation cleanup patch. However, > doing so would require heuristics or at least manual help from the > caller to indicate the proper indentation width. I also thought > perhaps the intention of the series was to do the line-wrapping > automatically (ignoring any caller-provided embedded newlines), thus > ensuring that the lines were indented correctly _and_ fit the terminal > width properly regardless, but that's a somewhat more substantial > change. Yeah, I considered it but decided not to mainly not for the heuristics reason, but that any such thing means that you won't have the same indentation in the C code. I.e. instead of: "git foo [--some-option]\n" " [-even -more options]" You'd have: "git foo [--some-option]\n" "[--even --more options]" Which I think is just not as legible, but also in the general case you get into the quagmire of how do align a thing like: "git [-c foo=bar] something [--option]\n" "[--more-options]" Are we doing to align on the "git " or "git [-c foo=bar] something ", or is it a "git something" where "something" is the name of a sub-command, or is that a filename? We control the source code so we can do it, but I thought it would be nasty, and in any case any working solution wouldn't give you alignment in the source code, so I dropped the idea.
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index cb8e3d4da9e..96d5f598b4b 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -11,6 +11,7 @@ SYNOPSIS 'git help' [-a|--all [--[no-]verbose]] [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] 'git help' [-g|--guides] +'git help' [-c|--config] DESCRIPTION ----------- diff --git a/builtin/help.c b/builtin/help.c index 0737b22069b..83f71d6765e 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = { N_("git help [-a|--all] [--[no-]verbose]]\n" " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), N_("git help [-g|--guides]"), + N_("git help [-c|--config]"), NULL }; @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix) enum help_format parsed_help_format; const char *page; int standalone = 0; + int need_config = 0; argc = parse_options(argc, argv, prefix, builtin_help_options, builtin_help_usage, 0); parsed_help_format = help_format; + /* Incompatible options */ + if (show_all + !!show_config + show_guides > 1) + usage_with_options(builtin_help_usage, builtin_help_options); + /* Options that take no further arguments */ standalone = show_config || show_guides; if (standalone && argc) usage_with_options(builtin_help_usage, builtin_help_options); - if (show_all) { + need_config = show_all || show_config; + if (need_config) git_config(git_help_config, NULL); + + if (show_all) { if (verbose) { setup_pager(); list_all_cmds_help(); @@ -571,6 +580,14 @@ int cmd_help(int argc, const char **argv, const char *prefix) list_commands(colopts, &main_cmds, &other_cmds); } + if (show_guides) + list_guides_help(); + + if (show_all || show_guides) { + printf("%s\n", _(git_more_info_string)); + return 0; + } + if (show_config) { int for_human = show_config == 1; @@ -583,14 +600,6 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } - if (show_guides) - list_guides_help(); - - if (show_all || standalone) { - printf("%s\n", _(git_more_info_string)); - return 0; - } - if (!argv[0]) { printf(_("usage: %s%s"), _(git_usage_string), "\n\n"); list_common_cmds_help(); diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 94d1f481c8b..68e7f57470e 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -36,7 +36,10 @@ test_expect_success 'basic help commands' ' test_expect_success 'invalid usage' ' test_expect_code 129 git help -c git-add && - test_expect_code 129 git help -g git-add + test_expect_code 129 git help -g git-add && + + test_expect_code 129 git help -a -c && + test_expect_code 129 git help -g -c ' test_expect_success "works for commands and guides by default" '
Fix a bug in the --config option that's been there ever since its introduction in 3ac68a93fd2 (help: add --config to list all available config, 2018-05-26). Die when --all and --config are combined, combining them doesn't make sense. The code for the --config option when combined with an earlier refactoring done to support the --guide option in 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would cause us to take the "--all" branch early and ignore the --config option. Let's instead list these as incompatible, both in the synopsis and help output, and enforce it in the code itself. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-help.txt | 1 + builtin/help.c | 27 ++++++++++++++++++--------- t/t0012-help.sh | 5 ++++- 3 files changed, 23 insertions(+), 10 deletions(-)