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

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

Michael Lohmann <mi.al.lohmann@gmail.com> writes:

>  	struct option base_options[] = {
> -		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> -		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> -		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> -		OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
> +		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
> +		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
> +		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
> +		OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
>  		OPT_CLEANUP(&cleanup_arg),
>  		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
>  		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),

I actually do not terribly mind reusing the single letter option
(e.g. 'x' in "-x") and the command mode, as it is descriptive
enough.  The argument that an enum allows compilers warn about
non-exhausitve switches is valid if we have such a switch.

> +	switch (cmd) {
> +	case ACTION_START:
> +		goto skip_opt_compatibility_verification;
> +	case ACTION_QUIT:
> +		this_operation = "--quit";
> +		break;
> +	case ACTION_CONTINUE:
> +		this_operation = "--continue";
> +		break;
> +	case ACTION_SKIP:
> +		this_operation = "--skip";
> +		break;
> +	case ACTION_ABORT:
> +		this_operation = "--abort";
> +		break;
>  	}
>  
> +	verify_opt_compatible(me, this_operation,

And indeed the if/elseif cascade in the original is easier to ensure
its exhaustiveness by turning it into a switch.

HOWEVER.

If I were writing this patch, I would rather do it like so:

	this_operation = NULL;
	switch (cmd) {
	case 'q':
		this_operation = '--quit";
                break;
	...
	case 'a':
		this_operation = '--abort";
                break;
	default: /* everything else is compatible with others */
		break;
	}
	if (this_operation)
		verify_opt_compatible(me, this_operation, ...);

Use of enum is optional; I just didn't like too much churning to
illustrate a single idea (here, the single idea being "switch is
more appropriate then if/else cascade in this case"), and I think
this is easier to read than with enums [*].

	Side note: After all the single letter option names are
	meant to be mnemonic.  "case 'q'" is just as descriptive as
	"case ACTION_QUIT" in the context of this switch statement.

HTH.