Re: [PATCH] submodule: deprecate --recurse-submodules=""
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 23, 2023, 19:37 UTC
- Message-ID
- <ZOZf4/DYOKqQLjR+@nand.local>
- In-Reply-To
- <20230823032839.731375-1-alexhenrie24@gmail.com>
On Tue, Aug 22, 2023 at 09:28:37PM -0600, Alex Henrie wrote:
Show 10 quoted lines
> 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.
Show 26 quoted lines
> 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