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

[Emily Shaffer <nasamuffin@google.com>]

On Fri, Apr 07, 2023 at 05:03:51PM -0700, Junio C Hamano wrote:
> 
> 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

I don't have a strong opinion on this, sorry. :)

> 
> 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.

I think we do because config_update_recurse_submodules is static to
submodule.c - that is, builtin/checkout.c and friends don't have access
to set it manually with OPT_BOOL. Using the callback just to set static
state we don't naturally have access to is pretty awful, though, so I'd
be in favor of plumbing it through like other options we might be
passing to the submodule machinery.

If you do feel strongly about it, anybody else is welcome to hijack this
patch and make it so, but I doubt that I will have time to do so. Happening
to have a moment this afternoon was a bit of an accident :( so I hereby
un-lick the cookie.

> 
> 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?