| Message ID | ZDCWrl4GhgYKYFYG@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usage: clarify --recurse-submodules as a boolean | expand |
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.
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?
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
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?
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.
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
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.
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;
`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(-)