Re: [PATCH] submodule: deprecate --recurse-submodules=""

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 22, 2023 at 09:28:37PM -0600, Alex Henrie wrote:
> The unusual syntax --recurse-submodules="" (that is,
> --recurse-submodules with an empty string argument) has been an
> undocumented synonym of --recurse-submodules without an argument since
> commit 8f0700dd33 (fetch/pull: Add the 'on-demand' value to the
> --recurse-submodules option, 2011-03-06). Deprecate that syntax to avoid
> confusion with the submodule.recurse config option, where
> submodule.recurse="" is equivalent to --no-recurse-submodules.
>
> The same thing was done for --rebase-merges="" in commit 33561f5170
> (rebase: deprecate --rebase-merges="", 2023-03-25).

Makes sense, and this is certainly in the same spirit as your
33561f5170.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  submodule-config.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 6a48fd12f6..8acb42744d 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -332,11 +332,17 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,
>
>  	if (unset) {
>  		*v = RECURSE_SUBMODULES_OFF;
> +	} else if (!arg) {
> +		*v = RECURSE_SUBMODULES_ON;
>  	} else {
> -		if (arg)
> -			*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);
> -		else
> -			*v = RECURSE_SUBMODULES_ON;
> +		if (!*arg) {
> +			warning(_("--recurse-submodules with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--recurse-submodules without an argument "
> +				  "instead, which does the same thing."));

This advice says to use `--recurse-submodules` as a non-deprecated
synonym for `--recurse-submodules=""`, but I am not so sure that is
correct advice.

In the pre-image of this patch, having arg be set to the empty string
would cause us to fall into the path that executes

    *v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);

which calls `parse_fetch_recurse()` -> `git_parse_maybe_bool()` ->
`git_parse_maybe_bool_text()` which given the empty string will return
0.

So here we'd be doing the equivalent of

    *v = RECURSE_SUBMODULES_OFF;

when trying to parse `--recurse-submodules=""`. Should this advice
instead say "[...] Use --no-recurse-submodules without an argument,
which does the same thing"?

Thanks,
Taylor