| Message ID | 20230823032839.731375-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: deprecate --recurse-submodules="" | expand |
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
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
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
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.
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; }
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(-)