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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

This change does not seem to be explained.  

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

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

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

If anything, 

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

may make it easier to follow.

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

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

Why two "-g git-add"?

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