Glen Choo <chooglen@xxxxxxxxxx> writes:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7bbff5a029..8b8bde8809 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -76,6 +76,7 @@ static struct transport *gtransport;
> static struct transport *gsecondary;
> static const char *submodule_prefix = "";
> static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
> static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
> static int shown_url = 0;
> static struct refspec refmap = REFSPEC_INIT_FETCH;
> @@ -167,7 +168,7 @@ static struct option builtin_fetch_options[] = {
> N_("prune remote-tracking branches no longer on remote")),
> OPT_BOOL('P', "prune-tags", &prune_tags,
> N_("prune local tags no longer on remote and clobber changed tags")),
> - OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules, N_("on-demand"),
> + OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
> N_("control recursive fetching of submodules"),
> PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
> OPT_BOOL(0, "dry-run", &dry_run,
> @@ -2014,6 +2015,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>
> argc = parse_options(argc, argv, prefix,
> builtin_fetch_options, builtin_fetch_usage, 0);
> +
> + if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
> + recurse_submodules = recurse_submodules_cli;
This made me wonder what should happen if the command line option
was given and explicitly told us to use the default, but after
following the option_fetch_parse_recurse_submodules() codeflow, I
realized that it will never return RECURSE_SUBMODULES_DEFAULT, so it
is OK. It is a bit misleading that _DEFAULT does not mean "use the
default settings" here---it merely means "this variable was left
untouched". But I suppose that it is in line with all the other
uses of RECURSE_SUBMODULES_DEFAULT, in which case it is OK for now.
> + if (negotiate_only) {
> + switch (recurse_submodules_cli) {
> + case RECURSE_SUBMODULES_OFF:
> + case RECURSE_SUBMODULES_DEFAULT: {
> + /*
> + * --negotiate-only should never recurse into
> + * submodules. Skip it by setting recurse_submodules to
> + * RECURSE_SUBMODULES_OFF.
> + */
> + recurse_submodules = RECURSE_SUBMODULES_OFF;
> + break;
> + }
It is not immediately obvious to me why we need an extra block
here. If there is no reason, let's not have it---there is no reason
to puzzle readers into wondering if anything funny is going on if
there is nothing unusual.
> + default:
> + die(_("--negotiate-only and --recurse-submodules cannot be used together"));
> + }
> + }
> +
Other than that, looking good.
Thanks.
