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

> 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.