diff mbox series

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

Message ID patch-v3-6.9-b52269eeab9-20210921T223223Z-avarab@gmail.com (mailing list archive)
State Accepted
Commit d35d03cf93ef0dba3e975c78fce73db91d52ba42
Headers show
Series help: fix usage nits & bugs, completion shellscript->C | expand

Commit Message

Ævar Arnfjörð Bjarmason Sept. 21, 2021, 10:40 p.m. UTC
As preceding commits have incrementally established all of the --all,
--guides, --config and hidden --config-for-completion options are
mutually exclusive. So let's use OPT_CMDMODE() to parse the
command-line instead, and take advantage of its conflicting options
detection.

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

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

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

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

And now:

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

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

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

Comments

Junio C Hamano Sept. 23, 2021, 6:03 p.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

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

    $ git help --guides --verbose 99

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

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

>  int cmd_help(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit;
> @@ -554,28 +570,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  			builtin_help_usage, 0);
>  	parsed_help_format = help_format;
>  
> +	switch (cmd_mode) {
> +	case HELP_ACTION_ALL:
>  		git_config(git_help_config, NULL);
>  		if (verbose) {
>  			setup_pager();
> @@ -585,25 +581,20 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>  		load_command_list("git-", &main_cmds, &other_cmds);
>  		list_commands(colopts, &main_cmds, &other_cmds);
> +		printf("%s\n", _(git_more_info_string));
> +		break;
> +	case HELP_ACTION_GUIDES:
> +		no_extra_argc(argc);
>  		list_guides_help();
>  		printf("%s\n", _(git_more_info_string));
>  		return 0;
> +	case HELP_ACTION_CONFIG_FOR_COMPLETION:
> +		list_config_help(0);
> +		return 0;
> +	case HELP_ACTION_CONFIG:
> +		no_extra_argc(argc);
>  		setup_pager();
> +		list_config_help(1);
>  		printf("\n%s\n", _("'git help config' for more information"));
>  		return 0;
>  	}
Junio C Hamano Sept. 23, 2021, 6:05 p.m. UTC | #2
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

Sounds quite logical.

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

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

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

Yes, absolutely.
diff mbox series

Patch

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