Re: [PATCH v5 3/3] fetch --negotiate-only: do not update submodules

From: Junio C Hamano <gitster@pobox.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v5 3/3] fetch --negotiate-only: do not update submodules
Date: Tue, 18 Jan 2022 14:05:31 -0800	[thread overview]
Message-ID: <xmqqtue0r978.fsf@gitster.g> (raw)
In-Reply-To: <20220118185453.33710-4-chooglen@google.com> (Glen Choo's message of "Tue, 18 Jan 2022 10:54:53 -0800")

Glen Choo <chooglen@google.com> writes:

> +	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;
> +		}
> +		default:
> +			die(_("--negotiate-only and --recurse-submodules cannot be used together"));
> +		}

I think that this part has the only difference since the previous
round, but I am puzzled about it.  Everything else looks as good as
the previous round was.

I did not (and I do not) mind the block for the body of this if
statement.  Even though technically a single switch() statement
makes a single statement block that does not need {} around, it is
large enough that extra {} around (which you had in the previous
round) may make it clear where the body begins and ends.

But do we really need the extra block _inside_ the switch statement?
IOW I would have expected to see this instead:

		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;
		default:
			die(_("--negotiate-only and --recurse-submodules cannot be used together"));
		}

Thanks.