[PATCH] usage: clarify --recurse-submodules as a boolean

[PATCH] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer]

`git switch` `git checkout`, `git reset`, and `git read-tree` allow a user to choose to
recurse into submodules. All three of these commands' short usage seems
to indicate that `--recurse-submodules` should take an argument. In
practice, though, all three of these commands parse through the same
callback path:

  option_parse_recurse_submodules_worktree_updater(...) checks for
  set/unset, or passes off to...
  parse_update_recurse_submodules_arg(...), which is a straight handoff
  to...
  parse_update_recurse(...), which only accepts true or false.

So ultimately, it can only be true or false, unlike `git push
--recurse-submodules=<enum>`. A user could provide
`--recurse-submodules=true`, but we don't typically suggest that for
boolean arguments.
(Documentation/git-(switch|checkout|reset|read-tree).txt suggests
--[no-]recurse-submodules, too.)

In fact, these three commands are the only ones that use this codepath -
so there's not any reason for it to be so meandering.  It's not possible
to stop using these as a callback entirely, though, because
option_parse_recurse_submodules_worktree_updater() modifies global state
in submodule.c.

Clarify the usage so these commands don't pretend to accept a string
argument. Also, simplify
option_parse_recurse_submodules_worktree_updater() and remove the
now-unused parse_update_recurse_submodules_arg() and
parse_update_recurse() calls.

Signed-off-by: Emily Shaffer <nasamuffin@google.com>

---
 builtin/checkout.c  |  6 +++---
 builtin/read-tree.c |  6 +++---
 builtin/reset.c     |  4 ++--
 submodule-config.c  | 20 --------------------
 submodule-config.h  |  1 -
 submodule.c         |  4 ----
 6 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 38a8cd6a96..b80ad37fc1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1587,9 +1587,9 @@ static struct option *add_common_options(struct checkout_opts *opts,
 {
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_CALLBACK_F(0, "recurse-submodules", NULL, NULL,
+			    "control recursive updating of submodules",
+			    PARSE_OPT_NOARG, option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
 		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
 		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 600d4f748f..2afb0b24a2 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -149,9 +149,9 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 			 N_("skip applying sparse checkout filter")),
 		OPT_BOOL(0, "debug-unpack", &opts.internal.debug_unpack,
 			 N_("debug unpack-trees")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_CALLBACK_F(0, "recurse-submodules", NULL, NULL,
+			    "control recursive updating of submodules",
+			    PARSE_OPT_NOARG, option_parse_recurse_submodules_worktree_updater),
 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
 		OPT_END()
 	};
diff --git a/builtin/reset.c b/builtin/reset.c
index 0ed329236c..cdf6ea6df9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -340,8 +340,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "keep", &reset_type,
 				N_("reset HEAD but keep local changes"), KEEP),
 		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "reset", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+			    NULL, "control recursive updating of submodules",
+			    PARSE_OPT_NOARG, option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
 				N_("record only the fact that removed paths will be added later")),
diff --git a/submodule-config.c b/submodule-config.c
index ecf0fcf007..9ef0bdc207 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -338,26 +338,6 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,
 	return 0;
 }
 
-static int parse_update_recurse(const char *opt, const char *arg,
-				int die_on_error)
-{
-	switch (git_parse_maybe_bool(arg)) {
-	case 1:
-		return RECURSE_SUBMODULES_ON;
-	case 0:
-		return RECURSE_SUBMODULES_OFF;
-	default:
-		if (die_on_error)
-			die("bad %s argument: %s", opt, arg);
-		return RECURSE_SUBMODULES_ERROR;
-	}
-}
-
-int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
-{
-	return parse_update_recurse(opt, arg, 1);
-}
-
 static int parse_push_recurse(const char *opt, const char *arg,
 			       int die_on_error)
 {
diff --git a/submodule-config.h b/submodule-config.h
index c2045875bb..fda6ad0162 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -55,7 +55,6 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 struct option;
 int option_fetch_parse_recurse_submodules(const struct option *opt,
 					  const char *arg, int unset);
-int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
 void repo_read_gitmodules(struct repository *repo, int skip_if_read);
 void gitmodules_config_oid(const struct object_id *commit_oid);
diff --git a/submodule.c b/submodule.c
index 94644fac0a..fe456c24c9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -236,10 +236,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 		config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 		return 0;
 	}
-	if (arg)
-		config_update_recurse_submodules =
-			parse_update_recurse_submodules_arg(opt->long_name,
-							    arg);
 	else
 		config_update_recurse_submodules = RECURSE_SUBMODULES_ON;
 
-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> `git switch` `git checkout`, `git reset`, and `git read-tree` allow a user to choose to
> recurse into submodules. All three of these commands' short usage seems
> to indicate that `--recurse-submodules` should take an argument. In
> practice, ...

Did you add 'git switch' at the last minute in so much of a hurry
that you forgot to put a comma after it, or rewrap the paragraph?
;-)

I do agree with you that "git checkout -h" and "git reset -h" that
list

	--recurse-submodules[=<checkout>]
	--recurse-submodules[=<reset>]

are being unnecessarily confusing by not saying anything about what
these placeholders are to be filled with.  

This however is a breaking change.  Even though there is no hint
that <checkout> and <reset> placeholders above take either Boolean
true or false in the documentation, they may have picked up a habit
to use the undocumented form from some random website.  I am not
sure it is safe to change the behaviour right under them, like this
patch does, and I wonder if we should do this in two steps, with its
first step doing:

 * "--[no-]recurse-submodules" from the command line gets no
   warning, as that is the way we recommend users to use the
   feature.

 * "--recurse-submodules=$true" and "--recurse-submodules=$false"
   (for various ways to spell true and false) get warning that tells
   the users that versions of Git in a year or more in the future
   will stop supporting the Boolean argument form of the option and
   instructs them to use "--[no-]recurse-submodules" instead.

We may have to also mention in the documentation that historically
the code accepted a Boolean value as an optional argument for the
option by mistake, but we are deprecating that form.

And after the second step, the code will end up looking like what
this patch shows.

Thanks.

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I do agree with you that "git checkout -h" and "git reset -h" that
> list
>
> 	--recurse-submodules[=<checkout>]
> 	--recurse-submodules[=<reset>]
>
> are being unnecessarily confusing by not saying anything about what
> these placeholders are to be filled with.  
>
> This however is a breaking change....

With your patch, the callback becomes like this:

int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
						     const char *arg, int unset)
{
	if (unset)
		config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
	else
		config_update_recurse_submodules = RECURSE_SUBMODULES_ON;
	return 0;
}

but this makes me wonder if it makes it better by turning it around
180 degrees and going in the opposite direction.

With Devil's advocate hat on, what if we declare that *any* option
that sets a boolean variable can be spelled in any of the following
ways?

    [enables "frotz" option]
    --frotz             # naturally
    --frotz=yes         # usual synonyms yes/true/1/... are accepted

    [disables "frotz" option]
    --no-frotz          # naturally
    --frotz=no          # usual synonyms no/false/0/... are accepted

It would be just the matter of updating OPT_BOOL()'s implementation.

Then the patches to builtin/checkout.c and friends would look like:

 static struct option *add_common_options(struct checkout_opts *opts,
 					 struct option *prevopts)
 {
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_BOOL(0, "recurse-submodules", &config_update_recurse_submodules,
+			N_("control recursive updating of submodules")),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),

and we no longer need the callback function.

We will not break any existing users, and then suddenly people can
now say

	--progress
        --no-progress
        --progress=yes
        --progress=no

just like --recurse-submodules=yes has silently been allowed all
these years.

Hmm?

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer]

On Fri, Apr 07, 2023 at 04:47:02PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <nasamuffin@google.com> writes:
> 
> > `git switch` `git checkout`, `git reset`, and `git read-tree` allow a user to choose to
> > recurse into submodules. All three of these commands' short usage seems
> > to indicate that `--recurse-submodules` should take an argument. In
> > practice, ...
> 
> Did you add 'git switch' at the last minute in so much of a hurry
> that you forgot to put a comma after it, or rewrap the paragraph?
> ;-)

It was 'git checkout', if you must know ;) and in such a hurry that I
also neglected to s/three/four/g. Will fix it with the reroll.

> 
> I do agree with you that "git checkout -h" and "git reset -h" that
> list
> 
> 	--recurse-submodules[=<checkout>]
> 	--recurse-submodules[=<reset>]
> 
> are being unnecessarily confusing by not saying anything about what
> these placeholders are to be filled with.  
> 
> This however is a breaking change.  Even though there is no hint
> that <checkout> and <reset> placeholders above take either Boolean
> true or false in the documentation, they may have picked up a habit
> to use the undocumented form from some random website.

Ah, yeah, I see what you mean, from my locally-built version:

  g checkout --recurse-submodules=false master
  error: option `recurse-submodules' takes no value

> I am not
> sure it is safe to change the behaviour right under them, like this
> patch does, and I wonder if we should do this in two steps, with its
> first step doing:
> 
>  * "--[no-]recurse-submodules" from the command line gets no
>    warning, as that is the way we recommend users to use the
>    feature.
> 
>  * "--recurse-submodules=$true" and "--recurse-submodules=$false"
>    (for various ways to spell true and false) get warning that tells
>    the users that versions of Git in a year or more in the future
>    will stop supporting the Boolean argument form of the option and
>    instructs them to use "--[no-]recurse-submodules" instead.
> 
> We may have to also mention in the documentation that historically
> the code accepted a Boolean value as an optional argument for the
> option by mistake, but we are deprecating that form.
> 
> And after the second step, the code will end up looking like what
> this patch shows.

I'd be happy to do so with a reroll, probably on Monday. It's true that
while these are user-facing commands which we don't guarantee backwards
compatibility for, there's not a reason to subject users to that kind of
pain unnecessarily.

Thanks for the quick response.

 - Emily

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer]

On Fri, Apr 07, 2023 at 05:03:51PM -0700, Junio C Hamano wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I do agree with you that "git checkout -h" and "git reset -h" that
> > list
> >
> > 	--recurse-submodules[=<checkout>]
> > 	--recurse-submodules[=<reset>]
> >
> > are being unnecessarily confusing by not saying anything about what
> > these placeholders are to be filled with.  
> >
> > This however is a breaking change....
> 
> With your patch, the callback becomes like this:
> 
> int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
> 						     const char *arg, int unset)
> {
> 	if (unset)
> 		config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
> 	else
> 		config_update_recurse_submodules = RECURSE_SUBMODULES_ON;
> 	return 0;
> }
> 
> but this makes me wonder if it makes it better by turning it around
> 180 degrees and going in the opposite direction.
> 
> With Devil's advocate hat on, what if we declare that *any* option
> that sets a boolean variable can be spelled in any of the following
> ways?
> 
>     [enables "frotz" option]
>     --frotz             # naturally
>     --frotz=yes         # usual synonyms yes/true/1/... are accepted
> 
>     [disables "frotz" option]
>     --no-frotz          # naturally
>     --frotz=no          # usual synonyms no/false/0/... are accepted

I don't have a strong opinion on this, sorry. :)

> 
> It would be just the matter of updating OPT_BOOL()'s implementation.
> 
> Then the patches to builtin/checkout.c and friends would look like:
> 
>  static struct option *add_common_options(struct checkout_opts *opts,
>  					 struct option *prevopts)
>  {
>  	struct option options[] = {
>  		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
> -		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
> -			    "checkout", "control recursive updating of submodules",
> -			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
> +		OPT_BOOL(0, "recurse-submodules", &config_update_recurse_submodules,
> +			N_("control recursive updating of submodules")),
>  		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
> 
> and we no longer need the callback function.

I think we do because config_update_recurse_submodules is static to
submodule.c - that is, builtin/checkout.c and friends don't have access
to set it manually with OPT_BOOL. Using the callback just to set static
state we don't naturally have access to is pretty awful, though, so I'd
be in favor of plumbing it through like other options we might be
passing to the submodule machinery.

If you do feel strongly about it, anybody else is welcome to hijack this
patch and make it so, but I doubt that I will have time to do so. Happening
to have a moment this afternoon was a bit of an accident :( so I hereby
un-lick the cookie.

> 
> We will not break any existing users, and then suddenly people can
> now say
> 
> 	--progress
>         --no-progress
>         --progress=yes
>         --progress=no
> 
> just like --recurse-submodules=yes has silently been allowed all
> these years.
> 
> Hmm?

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> I think we do because config_update_recurse_submodules is static to
> submodule.c - that is, builtin/checkout.c and friends don't have access
> to set it manually with OPT_BOOL. Using the callback just to set static
> state we don't naturally have access to is pretty awful, though, so I'd
> be in favor of plumbing it through like other options we might be
> passing to the submodule machinery.

Yes, the cleanest way to interface into that part of the submodule
machinery that wants to use a hidden static state would be to

 (1) implement a setter interface in the submodule machinery for
     that hidden static state, and

 (2) use the bog-standard OPT_BOOL() on an on-stack variable of
     cmd_checkout() and friends, and use that setter interface after
     parse_options() returns.

Then you can avoid the "pretty awful" arrangement today's code has.

Note that such a clean-up can be done independent of how an option
that yields a Boolean value can be spelled, i.e. whether we'd accept
--frotz=yes or only take --[no-]frotz.

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer]

On Fri, Apr 07, 2023 at 05:59:53PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <nasamuffin@google.com> writes:
> 
> > I think we do because config_update_recurse_submodules is static to
> > submodule.c - that is, builtin/checkout.c and friends don't have access
> > to set it manually with OPT_BOOL. Using the callback just to set static
> > state we don't naturally have access to is pretty awful, though, so I'd
> > be in favor of plumbing it through like other options we might be
> > passing to the submodule machinery.
> 
> Yes, the cleanest way to interface into that part of the submodule
> machinery that wants to use a hidden static state would be to
> 
>  (1) implement a setter interface in the submodule machinery for
>      that hidden static state, and
> 
>  (2) use the bog-standard OPT_BOOL() on an on-stack variable of
>      cmd_checkout() and friends, and use that setter interface after
>      parse_options() returns.
> 
> Then you can avoid the "pretty awful" arrangement today's code has.
> 
> Note that such a clean-up can be done independent of how an option
> that yields a Boolean value can be spelled, i.e. whether we'd accept
> --frotz=yes or only take --[no-]frotz.

Oh, totally. Yes, this sounds quite easy, I'll send a reroll today.

 - Emily

[PATCH v2] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer]

`git switch` `git checkout`, `git reset`, and `git read-tree` allow a
user to choose to recurse into submodules. All four of these commands'
short usage seems to indicate that `--recurse-submodules` should take an
argument. In practice, though, all four of these commands parse through
the same callback path:

  option_parse_recurse_submodules_worktree_updater(...) checks for
  set/unset, or passes off to...
  parse_update_recurse_submodules_arg(...), which is a straight handoff
  to...
  parse_update_recurse(...), which only accepts true or false.

So ultimately, it can only be true or false, unlike `git push
--recurse-submodules=<enum>`. A user could provide
`--recurse-submodules=true`, but we don't typically suggest that for
boolean arguments.
(Documentation/git-(switch|checkout|reset|read-tree).txt suggests
--[no-]recurse-submodules, too.)

In fact, these four commands are the only ones that use this codepath -
so there's not any reason for it to be so meandering. However, because
option_parse_recurse_submodules_worktree_updater() modifies static state
in submodule.c, we still need to get a handle to that static state
through a function call.

To preserve the behavior of --recurse-submodules=true and clarify the
usage string simultaneously, get rid of the OPT_CALLBACK_F in favor of a
simple OPT_BOOL, and call a setter in submodule.c for the static state
instead. Also, remove the now-unused
option_parse_recurse_submodules_worktree_updater(),
parse_update_recurse_submodules_arg(), and parse_update_recurse() calls.

Signed-off-by: Emily Shaffer <nasamuffin@google.com>

---

The one thing missing from this patch is tests - especially since the
need for a reroll was predicated on not breaking a specific user edge
case. But I had some trouble finding a good spot, because it seems these
commands weren't explicitly testing the --recurse-submodule flag
anywhere. I'm planning to add them sometime this week, but if someone
beats me to it, or has a suggestion of where to put them, or if Junio
wants to take this patch without them, I won't mind.

Changes since v1:

 - Added translation tag to the usage string at all 3 callsites
 - Took Junio's advice[1] and got rid of the callback interface
   entirely, replacing it with a setter to the static state in
   submodule.c.

By the way, it occurred to me to try and get a handle directly to the
static int and pass that in to OPT_BOOL instead of this overhead. I gave
it a try despite the feeling of poor layering smell, but it turns out
we're saved from having to even consider that, because
RECURSE_SUBMODULES_ON == 2. So no matter what, we will need to take an
extra step to do the conversion from bool (and be careful about setting
when it wasn't explicitly provided, because this static also takes its
value from the config before the option parse happens).

 - Emily

1: https://lore.kernel.org/git/xmqqy1n3fat2.fsf%40gitster.g
---
Range-diff against v1:
1:  c345a44e36 < -:  ---------- usage: clarify --recurse-submodules as a boolean
-:  ---------- > 1:  1ec2a064d2 usage: clarify --recurse-submodules as a boolean

 builtin/checkout.c  | 14 +++++++++++---
 builtin/read-tree.c | 10 +++++++---
 builtin/reset.c     | 10 +++++++---
 submodule-config.c  | 20 --------------------
 submodule-config.h  |  1 -
 submodule.c         | 19 +++++--------------
 submodule.h         |  6 +++---
 7 files changed, 33 insertions(+), 47 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 38a8cd6a96..d105403b0e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -80,6 +80,7 @@ struct checkout_opts {
 	int ignore_unmerged;
 	int pathspec_file_nul;
 	char *pathspec_from_file;
+	int recurse_submodules;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -1587,9 +1588,8 @@ static struct option *add_common_options(struct checkout_opts *opts,
 {
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_BOOL(0, "recurse-submodules", &opts->recurse_submodules,
+			 N_("control recursive updating of submodules")),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
 		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
 		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
@@ -1597,6 +1597,11 @@ static struct option *add_common_options(struct checkout_opts *opts,
 		OPT_END()
 	};
 	struct option *newopts = parse_options_concat(prevopts, options);
+	/*
+	 * we only want to act on --recurse-submodules if it was set explicitly,
+	 * so put it into unset third state.
+	 */
+	opts->recurse_submodules = -1;
 	free(prevopts);
 	return newopts;
 }
@@ -1677,6 +1682,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	argc = parse_options(argc, argv, prefix, options,
 			     usagestr, parseopt_flags);
 
+	if (opts->recurse_submodules >= 0)
+		set_config_update_recurse_submodules(opts->recurse_submodules);
+
 	if (opts->show_progress < 0) {
 		if (opts->quiet)
 			opts->show_progress = 0;
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 600d4f748f..4d325f7481 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -116,6 +116,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	struct unpack_trees_options opts;
 	int prefix_set = 0;
 	struct lock_file lock_file = LOCK_INIT;
+	int recurse_submodules = -1;
 	const struct option read_tree_options[] = {
 		OPT__SUPER_PREFIX(&opts.super_prefix),
 		OPT_CALLBACK_F(0, "index-output", NULL, N_("file"),
@@ -149,9 +150,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 			 N_("skip applying sparse checkout filter")),
 		OPT_BOOL(0, "debug-unpack", &opts.internal.debug_unpack,
 			 N_("debug unpack-trees")),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "checkout", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			 N_("control recursive updating of submodules")),
 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
 		OPT_END()
 	};
@@ -177,6 +177,10 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	if (opts.reset)
 		opts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;
 
+	/* only modify the value if set explicitly */
+	if (recurse_submodules >= 0)
+		set_config_update_recurse_submodules(recurse_submodules);
+
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/reset.c b/builtin/reset.c
index 0ed329236c..d67b5ab1e0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -326,6 +326,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	struct object_id oid;
 	struct pathspec pathspec;
 	int intent_to_add = 0;
+	int recurse_submodules = -1;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
 		OPT_BOOL(0, "no-refresh", &no_refresh,
@@ -339,9 +340,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				N_("reset HEAD, index and working tree"), MERGE),
 		OPT_SET_INT(0, "keep", &reset_type,
 				N_("reset HEAD but keep local changes"), KEEP),
-		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
-			    "reset", "control recursive updating of submodules",
-			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			 N_("control recursive updating of submodules")),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
 				N_("record only the fact that removed paths will be added later")),
@@ -356,6 +356,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
+	/* only modify the value if set explicitly */
+	if (recurse_submodules >= 0)
+		set_config_update_recurse_submodules(recurse_submodules);
+
 	if (pathspec_from_file) {
 		if (patch_mode)
 			die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");
diff --git a/submodule-config.c b/submodule-config.c
index ecf0fcf007..9ef0bdc207 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -338,26 +338,6 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,
 	return 0;
 }
 
-static int parse_update_recurse(const char *opt, const char *arg,
-				int die_on_error)
-{
-	switch (git_parse_maybe_bool(arg)) {
-	case 1:
-		return RECURSE_SUBMODULES_ON;
-	case 0:
-		return RECURSE_SUBMODULES_OFF;
-	default:
-		if (die_on_error)
-			die("bad %s argument: %s", opt, arg);
-		return RECURSE_SUBMODULES_ERROR;
-	}
-}
-
-int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
-{
-	return parse_update_recurse(opt, arg, 1);
-}
-
 static int parse_push_recurse(const char *opt, const char *arg,
 			       int die_on_error)
 {
diff --git a/submodule-config.h b/submodule-config.h
index c2045875bb..fda6ad0162 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -55,7 +55,6 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 struct option;
 int option_fetch_parse_recurse_submodules(const struct option *opt,
 					  const char *arg, int unset);
-int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
 void repo_read_gitmodules(struct repository *repo, int skip_if_read);
 void gitmodules_config_oid(const struct object_id *commit_oid);
diff --git a/submodule.c b/submodule.c
index 94644fac0a..8a2a6c8384 100644
--- a/submodule.c
+++ b/submodule.c
@@ -229,21 +229,12 @@ int git_default_submodule_config(const char *var, const char *value,
 	return 0;
 }
 
-int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
-						     const char *arg, int unset)
+void set_config_update_recurse_submodules(int value)
 {
-	if (unset) {
-		config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-		return 0;
-	}
-	if (arg)
-		config_update_recurse_submodules =
-			parse_update_recurse_submodules_arg(opt->long_name,
-							    arg);
-	else
-		config_update_recurse_submodules = RECURSE_SUBMODULES_ON;
-
-	return 0;
+	if (value < 0 || value > 1)
+		BUG("config_update_recurse_submodules is a boolean");
+	config_update_recurse_submodules = value ? RECURSE_SUBMODULES_ON
+						 : RECURSE_SUBMODULES_OFF;
 }
 
 /*
diff --git a/submodule.h b/submodule.h
index c55a25ca37..9532d4e073 100644
--- a/submodule.h
+++ b/submodule.h
@@ -51,9 +51,9 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *,
 					     const char *path);
 int git_default_submodule_config(const char *var, const char *value, void *cb);
 
-struct option;
-int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
-						     const char *arg, int unset);
+/* Sets static state 'config_update_recurse_submodules'. 'value' must be 0 or 1. */
+void set_config_update_recurse_submodules(int value);
+
 int is_tree_submodule_active(struct repository *repo,
 			     const struct object_id *treeish_name,
 			     const char *path);
-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> It was 'git checkout', if you must know ;) and in such a hurry that I
> also neglected to s/three/four/g. Will fix it with the reroll.

You fixed three-or-four but not the missing comma in v2, it seems.
I locally touched it up while queuing v2, but ...

>> This however is a breaking change.  Even though there is no hint
> ...
>> I am not
>> sure it is safe to change the behaviour right under them, like this
>> patch does, and I wonder if we should do this in two steps, with its
>> first step doing:
> ...
> I'd be happy to do so with a reroll, probably on Monday. It's true that
> while these are user-facing commands which we don't guarantee backwards
> compatibility for, there's not a reason to subject users to that kind of
> pain unnecessarily.

... I do not see how this part is addressed in v2.  You got too
excited by the idea of how to replace the awful abuse of parse
options callback interface with a more focused setter function in
the API and forgot to do other changes you meant to or something?

Thanks.

Re: [PATCH v2] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> `git switch` `git checkout`, `git reset`, and `git read-tree` allow a

Missing comma?

> user to choose to recurse into submodules. All four of these commands'
> short usage seems to indicate that `--recurse-submodules` should take an
> argument. In practice, though, all four of these commands parse through
> the same callback path:
>
>   option_parse_recurse_submodules_worktree_updater(...) checks for
>   set/unset, or passes off to...
>   parse_update_recurse_submodules_arg(...), which is a straight handoff
>   to...
>   parse_update_recurse(...), which only accepts true or false.

"true or false" -> "various forms of 'true' (e.g. on/1/yes) or 'false'"

> So ultimately, it can only be true or false, unlike `git push

Likewise.  Here is a minimally touched up version I queued.

Thanks.

    usage: clarify --recurse-submodules as a boolean
    
    `git switch`, `git checkout`, `git reset`, and `git read-tree` allow a
    user to choose to recurse into submodules. All four of these commands'
    short usage seems to indicate that `--recurse-submodules` should take an
    argument. In practice, though, all four of these commands parse through
    the same callback path:
    
      option_parse_recurse_submodules_worktree_updater(...) checks for
      set/unset, or passes off to...
      parse_update_recurse_submodules_arg(...), which is a straight handoff
      to...
      parse_update_recurse(...), which only accepts various ways to
      spell a Boolean
    
    So ultimately, it can only be true or false (or yes/no/on/off/etc),
    unlike `git push --recurse-submodules=<enum>`. A user could provide
    `--recurse-submodules=true`, but we don't typically suggest that for
    boolean arguments.
    
    Documentation/git-(switch|checkout|reset|read-tree).txt suggests
    --[no-]recurse-submodules, too.
    
    In fact, these four commands are the only ones that use this codepath -
    so there's not any reason for it to be so meandering. However, because
    option_parse_recurse_submodules_worktree_updater() modifies static state
    in submodule.c, we still need to get a handle to that static state
    through a function call.
    
    To preserve the behavior of --recurse-submodules=true and clarify the
    usage string simultaneously, get rid of the OPT_CALLBACK_F in favor of a
    simple OPT_BOOL, and call a setter in submodule.c for the static state
    instead. Also, remove the now-unused
    option_parse_recurse_submodules_worktree_updater(),
    parse_update_recurse_submodules_arg(), and parse_update_recurse() calls.
    
    Signed-off-by: Emily Shaffer <nasamuffin@google.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v2] usage: clarify --recurse-submodules as a boolean

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Emily Shaffer <nasamuffin@google.com> writes:
>
>> `git switch` `git checkout`, `git reset`, and `git read-tree` allow a
>
> Missing comma?
>
>> user to choose to recurse into submodules. All four of these commands'
>> short usage seems to indicate that `--recurse-submodules` should take an
>> argument. In practice, though, all four of these commands parse through
>> the same callback path:
>>
>>   option_parse_recurse_submodules_worktree_updater(...) checks for
>>   set/unset, or passes off to...
>>   parse_update_recurse_submodules_arg(...), which is a straight handoff
>>   to...
>>   parse_update_recurse(...), which only accepts true or false.
>
> "true or false" -> "various forms of 'true' (e.g. on/1/yes) or 'false'"
>
>> So ultimately, it can only be true or false, unlike `git push
>
> Likewise.  Here is a minimally touched up version I queued.
>
> Thanks.
> ...

Any updates on this topic?