Re: [PATCH] usage: clarify --recurse-submodules as a boolean

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

Junio C Hamano <gitster@pobox.com> writes:

> I do agree with you that "git checkout -h" and "git reset -h" that
> list
>
> 	--recurse-submodules[=<checkout>]
> 	--recurse-submodules[=<reset>]
>
> are being unnecessarily confusing by not saying anything about what
> these placeholders are to be filled with.  
>
> This however is a breaking change....

With your patch, the callback becomes like this:

int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
						     const char *arg, int unset)
{
	if (unset)
		config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
	else
		config_update_recurse_submodules = RECURSE_SUBMODULES_ON;
	return 0;
}

but this makes me wonder if it makes it better by turning it around
180 degrees and going in the opposite direction.

With Devil's advocate hat on, what if we declare that *any* option
that sets a boolean variable can be spelled in any of the following
ways?

    [enables "frotz" option]
    --frotz             # naturally
    --frotz=yes         # usual synonyms yes/true/1/... are accepted

    [disables "frotz" option]
    --no-frotz          # naturally
    --frotz=no          # usual synonyms no/false/0/... are accepted

It would be just the matter of updating OPT_BOOL()'s implementation.

Then the patches to builtin/checkout.c and friends would look like:

 static struct option *add_common_options(struct checkout_opts *opts,
 					 struct option *prevopts)
 {
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_BOOL(0, "recurse-submodules", &config_update_recurse_submodules,
+			N_("control recursive updating of submodules")),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),

and we no longer need the callback function.

We will not break any existing users, and then suddenly people can
now say

	--progress
        --no-progress
        --progress=yes
        --progress=no

just like --recurse-submodules=yes has silently been allowed all
these years.

Hmm?