Re: [PATCH 3/4] builtin: also setup gently for --help-all

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

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

>     I originally considered leaving out the changes to
>     show_usage_with_options_if_asked and relying on the parse-options API to
>     do the right thing. Unfortunately, most commands can't make it all the
>     way to parse-options when setting up gently, and trying to parse options
>     without a repo creates myriad dependency problems (like: we might read
>     config after parsing CLI options, so we have to make sure the parsed
>     options overrride config).
>     
>     Some usage.c callers, like check-ref-format, probably deserve to be
>     ported to parse-options at this point.

It is unclear what you mean by all of the above, but hopefully it
would become clear as we read the code changes.

>     I opted not to do anything too invasive with merge-recursive (like a
>     prepatory "migrate to newer usage APIs") since I think it's going the
>     way of the dodo?

I think this is fine.

> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
> index 03b5100cfa..17aa4db37a 100644
> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,
>  	if (argv[0] && ends_with(argv[0], "-subtree"))
>  		o.subtree_shift = "";
>  
> -	if (argc == 2 && !strcmp(argv[1], "-h")) {
> +	if (argc == 2 && (!strcmp(argv[1], "-h") ||
> +			  !strcmp(argv[1], "--help-all"))) {
>  		struct strbuf msg = STRBUF_INIT;
>  		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
>  		show_usage_if_asked(argc, argv, msg.buf);

;-)

> diff --git a/git.c b/git.c
> index 07a5fe39fb..40d3df1b76 100644
> --- a/git.c
> +++ b/git.c
> @@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	const char *prefix;
>  	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
>  
> -	help = argc == 2 && !strcmp(argv[1], "-h");
> +	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
>  	if (help && (run_setup & RUN_SETUP))
>  		/* demote to GENTLY to allow 'git cmd -h' outside repo */
>  		run_setup = RUN_SETUP_GENTLY;

OK.  That's obvious and straight-forward.  Behave as closely as the
case when "-h" is given, and let the lower-layer deal with the
differences between "-h" and "--help-all".

> diff --git a/parse-options.c b/parse-options.c
> index c3222cc9bb..e3ed42f709 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1464,9 +1464,14 @@ void show_usage_with_options_if_asked(int ac, const char **av,
>  				      const char * const *usagestr,
>  				      const struct option *opts)
>  {
> -	if (ac == 2 && !strcmp(av[1], "-h")) {
> -		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
> -		exit(129);
> +	if (ac == 2) {
> +		if (!strcmp(av[1], "-h")) {
> +			usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
> +			exit(129);
> +		} else if (!strcmp(av[1], "--help-all")) {
> +			usage_with_options_internal(NULL, usagestr, opts, style_full, to_out);
> +			exit(129);
> +		}
>  	}
>  }

Again, that is obvious and straight-forward.

> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> index e235ecccde..b26a03d8a0 100755
> --- a/t/t1517-outside-repo.sh
> +++ b/t/t1517-outside-repo.sh
> @@ -127,6 +127,10 @@
>  		test_expect_code 129 nongit git $cmd -h >usage &&
>  		test_grep "[Uu]sage: git $cmd " usage
>  	'
> +	test_$expect_outcome "'git $cmd --help-all' outside a repository" '
> +		test_expect_code 129 nongit git $cmd --help-all >usage &&
> +		test_grep "[Uu]sage: git $cmd " usage
> +	'
>  done
>  
>  test_expect_success 'prune does not crash with -h' '
> diff --git a/usage.c b/usage.c
> index 81913236a4..4c245ba0cb 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)
>  
>  void show_usage_if_asked(int ac, const char **av, const char *err)
>  {
> -	if (ac == 2 && !strcmp(av[1], "-h"))
> +	if (ac == 2 && (!strcmp(av[1], "-h") ||
> +			!strcmp(av[1], "--help-all")))
>  		show_usage_if_asked_helper(err);
>  }

This looks good.

I don't understand your "I originally considered leaving out ..." at
all.  We are special casing a lone "-h" here already because we know
this is where we should stop without exercising unnecessary code
because we may not even have repo!=NULL and that is the reason why
this function exists in the first place.  It is obvious to me that
we need the same special casing for "--help-all".

In other words, I think all the above changes are good.

Thanks.