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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2022, 22:05 UTC
Message-ID: <xmqqtue0r978.fsf@gitster.g>
In-Reply-To: <20220118185453.33710-4-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 15 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;
> +		}
> +		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.