Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> But your way of seeing it also makes sense to me. I think I just find
> the "START" name jarring because we do not use that word elsewhere to
> describe the action.

Thanks.  I forgot to say that I share the same feeling, both about
"NONE could mean no-op" (but then seriously why would anybody sane
want that?) and "START is not how we spell these things".  I can see
how DEFAULT could make sense, but if somebody picked DEFAULT between
two sensible choices NONE and DEFAULT here, especially if they claim
that they started this enum to mimick what is done in another place,
and after they were told that the other place they are imitating
follows the convention of using NONE for "nothing specified, so use
default", I would have to say that they are trying to be different
for the sake of being different, which is not a good sign.  I'd want
our contributors to be original where being original matters more.

>> +{
>> +	switch (cmd) {
>> +	case ACTION_CONTINUE: return "--continue";
>> +	case ACTION_SKIP: return "--skip";
>> +	case ACTION_ABORT: return "--abort";
>> +	case ACTION_QUIT: return "--quit";
>> +	case ACTION_START: BUG("no commandline flag for ACTION_START");
>> +	}
>> +}
>
> I find this perfectly readable, and is likely the way I'd write it in a
> personal project. But in this project I find we tend to stick to more
> conventional formatting, like:
>
>   switch (cmd) {
>   case ACTION_CONTINUE:
> 	return "--continue";
>   case ACTION_SKIP:
> 	return "--skip";
>   ...and so on...

Same.  I try to do the latter while working on this project, but I
do admit I use the former in small one-page tools I write outside
the context of this project.

>> +	if (cmd != ACTION_START)
>
> Likewise here I'd probably leave this as "if (cmd)".

I 100% agree with the suggestion to explicitly define something to
be 0 when we are going to use it for its Boolean value.  So an
alternative would be to treat all ACTION_* enum values the same, and
not define the first one explicitly to 0.  

Especially in the context of a patch that wants to turn if/elseif
cascades to switch, I would suspect that the latter, as switch/case
does not special case the falsehood among other possible values of
integer type, might be easier to maintain in the longer term.

Thanks.