Junio C Hamano <gitster@xxxxxxxxx> writes:
> Glen Choo <chooglen@xxxxxxxxxx> 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.
Yes, that makes sense. This was why I added it initially.
> 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.
Ah, I misunderstood. I'll blame the fact that I'm recovering from
COVID ;) I'll remove the block from the "case".
