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