Re: [PATCH v4 3/3] fetch --negotiate-only: do not update submodules
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 13, 2022, 01:16 UTC
- Message-ID
- <xmqqr19cjuzw.fsf@gitster.g>
- In-Reply-To
- <20220113004501.78822-4-chooglen@google.com>
Glen Choo <chooglen@google.com> writes:
Show 28 quoted lines
> 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.
Show 12 quoted lines
> + 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.
Show 5 quoted lines
> + default:
> + die(_("--negotiate-only and --recurse-submodules cannot be used together"));
> + }
> + }
> +Other than that, looking good.
Thanks.