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

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

[Alex Henrie]

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

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."));
+		}
+		*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);
 	}
 	return 0;
 }
-- 
2.41.0

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

[Taylor Blau]

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

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

[Alex Henrie]

On Wed, Aug 23, 2023 at 1:37 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Aug 22, 2023 at 09:28:37PM -0600, Alex Henrie wrote:

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

You're right; I misunderstood the situation here.
--recurse-submodules="" is indeed equivalent to
--no-recurse-submodules, and that's what the advice should recommend.

On the other hand, given that the empty string does the same thing
both in a config file and on the command line, maybe it's not a
problem to allow the empty string on the command line. Personally I
think I'd still prefer to ask the user to use a more explicit syntax.
Thoughts?

-Alex

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

[Taylor Blau]

On Wed, Aug 23, 2023 at 01:53:22PM -0600, Alex Henrie wrote:
> On the other hand, given that the empty string does the same thing
> both in a config file and on the command line, maybe it's not a
> problem to allow the empty string on the command line. Personally I
> think I'd still prefer to ask the user to use a more explicit syntax.
> Thoughts?

We should be consistent, but I don't have a strong opinion.

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

Sounds right.  So there is nothing to change here, I guess.

If --recurse-submodules="" does something people do not expect to
happen, an warning might be warranted, but I somehow do not think
that is the case here.  If we are not hurting anybody by accepting
that (possibly unusual) form, I do not think we would want to add an
extra warning, either.

Thanks.