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

From: Emily Shaffer <nasamuffin@google.com>
Date: Apr 8, 2023, 00:22 UTC
Message-ID: <ZDCztVHuZoCstNar@google.com>
In-Reply-To: <xmqqy1n3gryw.fsf@gitster.g>

On Fri, Apr 07, 2023 at 05:03:51PM -0700, Junio C Hamano wrote:

Show 40 quoted lines

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

Show 18 quoted lines

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

Show 13 quoted lines

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