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

[Alex Henrie <alexhenrie24@gmail.com>]

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