{"thread":{"id":"60146","subject":"[PATCH] submodule: deprecate --recurse-submodules=\"\"","startedAt":"2023-08-23T03:29:59Z","lastAt":"2023-08-23T20:27:01Z","messageCount":5,"participants":["Alex Henrie","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480943","messageId":"20230823032839.731375-1-alexhenrie24@gmail.com","threadId":"60146","inReplyTo":null,"subject":"[PATCH] submodule: deprecate --recurse-submodules=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-08-23T03:28:37Z","receivedAt":"2023-08-23T03:29:59Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The unusual syntax --recurse-submodules=\"\" (that is,\n--recurse-submodules with an empty string argument) has been an\nundocumented synonym of --recurse-submodules without an argument since\ncommit 8f0700dd33 (fetch/pull: Add the 'on-demand' value to the\n--recurse-submodules option, 2011-03-06). Deprecate that syntax to avoid\nconfusion with the submodule.recurse config option, where\nsubmodule.recurse=\"\" is equivalent to --no-recurse-submodules.\n\nThe same thing was done for --rebase-merges=\"\" in commit 33561f5170\n(rebase: deprecate --rebase-merges=\"\", 2023-03-25).\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n submodule-config.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 6a48fd12f6..8acb42744d 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -332,11 +332,17 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,\n \n \tif (unset) {\n \t\t*v = RECURSE_SUBMODULES_OFF;\n+\t} else if (!arg) {\n+\t\t*v = RECURSE_SUBMODULES_ON;\n \t} else {\n-\t\tif (arg)\n-\t\t\t*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n-\t\telse\n-\t\t\t*v = RECURSE_SUBMODULES_ON;\n+\t\tif (!*arg) {\n+\t\t\twarning(_(\"--recurse-submodules with an empty string \"\n+\t\t\t\t  \"argument is deprecated and will stop \"\n+\t\t\t\t  \"working in a future version of Git. Use \"\n+\t\t\t\t  \"--recurse-submodules without an argument \"\n+\t\t\t\t  \"instead, which does the same thing.\"));\n+\t\t}\n+\t\t*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n \t}\n \treturn 0;\n }\n-- \n2.41.0\n\n"},{"id":"480966","messageId":"ZOZf4/DYOKqQLjR+@nand.local","threadId":"60146","inReplyTo":"20230823032839.731375-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] submodule: deprecate --recurse-submodules=\"\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-23T19:37:07Z","receivedAt":"2023-08-23T19:37:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 22, 2023 at 09:28:37PM -0600, Alex Henrie wrote:\n> The unusual syntax --recurse-submodules=\"\" (that is,\n> --recurse-submodules with an empty string argument) has been an\n> undocumented synonym of --recurse-submodules without an argument since\n> commit 8f0700dd33 (fetch/pull: Add the 'on-demand' value to the\n> --recurse-submodules option, 2011-03-06). Deprecate that syntax to avoid\n> confusion with the submodule.recurse config option, where\n> submodule.recurse=\"\" is equivalent to --no-recurse-submodules.\n>\n> The same thing was done for --rebase-merges=\"\" in commit 33561f5170\n> (rebase: deprecate --rebase-merges=\"\", 2023-03-25).\n\nMakes sense, and this is certainly in the same spirit as your\n33561f5170.\n\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  submodule-config.c | 14 ++++++++++----\n>  1 file changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 6a48fd12f6..8acb42744d 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -332,11 +332,17 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,\n>\n>  \tif (unset) {\n>  \t\t*v = RECURSE_SUBMODULES_OFF;\n> +\t} else if (!arg) {\n> +\t\t*v = RECURSE_SUBMODULES_ON;\n>  \t} else {\n> -\t\tif (arg)\n> -\t\t\t*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n> -\t\telse\n> -\t\t\t*v = RECURSE_SUBMODULES_ON;\n> +\t\tif (!*arg) {\n> +\t\t\twarning(_(\"--recurse-submodules with an empty string \"\n> +\t\t\t\t  \"argument is deprecated and will stop \"\n> +\t\t\t\t  \"working in a future version of Git. Use \"\n> +\t\t\t\t  \"--recurse-submodules without an argument \"\n> +\t\t\t\t  \"instead, which does the same thing.\"));\n\nThis advice says to use `--recurse-submodules` as a non-deprecated\nsynonym for `--recurse-submodules=\"\"`, but I am not so sure that is\ncorrect advice.\n\nIn the pre-image of this patch, having arg be set to the empty string\nwould cause us to fall into the path that executes\n\n    *v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n\nwhich calls `parse_fetch_recurse()` -> `git_parse_maybe_bool()` ->\n`git_parse_maybe_bool_text()` which given the empty string will return\n0.\n\nSo here we'd be doing the equivalent of\n\n    *v = RECURSE_SUBMODULES_OFF;\n\nwhen trying to parse `--recurse-submodules=\"\"`. Should this advice\ninstead say \"[...] Use --no-recurse-submodules without an argument,\nwhich does the same thing\"?\n\nThanks,\nTaylor\n"},{"id":"480967","messageId":"CAMMLpeRam03bmO0jnsbKQB7174xv12HJzTtC-6dHFbQDMKB5gA@mail.gmail.com","threadId":"60146","inReplyTo":"ZOZf4/DYOKqQLjR+@nand.local","subject":"Re: [PATCH] submodule: deprecate --recurse-submodules=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-08-23T19:53:22Z","receivedAt":"2023-08-23T19:54:38Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Aug 23, 2023 at 1:37 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Tue, Aug 22, 2023 at 09:28:37PM -0600, Alex Henrie wrote:\n\n> > +             if (!*arg) {\n> > +                     warning(_(\"--recurse-submodules with an empty string \"\n> > +                               \"argument is deprecated and will stop \"\n> > +                               \"working in a future version of Git. Use \"\n> > +                               \"--recurse-submodules without an argument \"\n> > +                               \"instead, which does the same thing.\"));\n>\n> This advice says to use `--recurse-submodules` as a non-deprecated\n> synonym for `--recurse-submodules=\"\"`, but I am not so sure that is\n> correct advice.\n>\n> In the pre-image of this patch, having arg be set to the empty string\n> would cause us to fall into the path that executes\n>\n>     *v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n>\n> which calls `parse_fetch_recurse()` -> `git_parse_maybe_bool()` ->\n> `git_parse_maybe_bool_text()` which given the empty string will return\n> 0.\n>\n> So here we'd be doing the equivalent of\n>\n>     *v = RECURSE_SUBMODULES_OFF;\n>\n> when trying to parse `--recurse-submodules=\"\"`. Should this advice\n> instead say \"[...] Use --no-recurse-submodules without an argument,\n> which does the same thing\"?\n\nYou're right; I misunderstood the situation here.\n--recurse-submodules=\"\" is indeed equivalent to\n--no-recurse-submodules, and that's what the advice should recommend.\n\nOn the other hand, given that the empty string does the same thing\nboth in a config file and on the command line, maybe it's not a\nproblem to allow the empty string on the command line. Personally I\nthink I'd still prefer to ask the user to use a more explicit syntax.\nThoughts?\n\n-Alex\n"},{"id":"480968","messageId":"ZOZkjoknPJkhgNYf@nand.local","threadId":"60146","inReplyTo":"CAMMLpeRam03bmO0jnsbKQB7174xv12HJzTtC-6dHFbQDMKB5gA@mail.gmail.com","subject":"Re: [PATCH] submodule: deprecate --recurse-submodules=\"\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-23T19:57:02Z","receivedAt":"2023-08-23T19:57:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 23, 2023 at 01:53:22PM -0600, Alex Henrie wrote:\n> On the other hand, given that the empty string does the same thing\n> both in a config file and on the command line, maybe it's not a\n> problem to allow the empty string on the command line. Personally I\n> think I'd still prefer to ask the user to use a more explicit syntax.\n> Thoughts?\n\nWe should be consistent, but I don't have a strong opinion.\n\nThanks,\nTaylor\n"},{"id":"480972","messageId":"xmqqa5uhfqu6.fsf@gitster.g","threadId":"60146","inReplyTo":"ZOZf4/DYOKqQLjR+@nand.local","subject":"Re: [PATCH] submodule: deprecate --recurse-submodules=\"\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-23T20:26:09Z","receivedAt":"2023-08-23T20:27:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> So here we'd be doing the equivalent of\n>\n>     *v = RECURSE_SUBMODULES_OFF;\n>\n> when trying to parse `--recurse-submodules=\"\"`. Should this advice\n> instead say \"[...] Use --no-recurse-submodules without an argument,\n> which does the same thing\"?\n\nSounds right.  So there is nothing to change here, I guess.\n\nIf --recurse-submodules=\"\" does something people do not expect to\nhappen, an warning might be warranted, but I somehow do not think\nthat is the case here.  If we are not hurting anybody by accepting\nthat (possibly unusual) form, I do not think we would want to add an\nextra warning, either.\n\nThanks.\n"}]}