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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Michael

On 11/01/2024 08:04, Michael Lohmann wrote:
> This is done to avoid having to keep the char values in sync in
> different places and also to get compiler warnings on non-exhaustive
> switches.

I think this is a reasonable change, thanks for working on it.

> The newly introduced `revert_action`-enum aligns with the
> builtin/rebase.c `action`-enum though the name `revert_action` is chosen
> to better differentiate it from `replay_opts->action` with a different
> function. For rebase the equivalent of the latter is
> `rebase_options->type` and `rebase_options->action` corresponds to the
> `cmd` variable in revert.c.
> 
> In the rebase `action` enum there is the enumeration constant
> `ACTION_NONE` which is not particularly descriptive, since it seems to
> imply that no action should be taken. Instead it signals a start of a
> revert/cherry-pick process, so here `REVERT_ACTION_START` was chosen.

I think ACTION_NONE was intended to covey that the user did not pass one 
of the OPT_CMDMODE() options like "--continue" as there isn't a 
"--start" option. I don't have a strong opinion between "_NONE" and 
"_START".

> +enum revert_action {
> +	REVERT_ACTION_START = 0,
> +	REVERT_ACTION_CONTINUE,
> +	REVERT_ACTION_SKIP,
> +	REVERT_ACTION_ABORT,
> +	REVERT_ACTION_QUIT,
> +};

The "REVERT_" prefix is a bit unfortunate as this is used by cherry-pick 
as well but it does match the filename. As this enum is only used in 
this file I'd be quite happy to drop the "REVERT_" prefix. (I don't 
think we need to go messing with the "action" member of struct 
replay_opts to do that)

>   	/* Check for incompatible command line arguments */
> -	if (cmd) {
> -		char *this_operation;
> -		if (cmd == 'q')
> +	{
> +		char *this_operation = 0;

style note: we use "NULL" rather than "0" when initializing pointers. 
Ideally this would be a "const char *" as we assign string literals but 
that is not a new problem with this patch.

> +		switch (cmd) {
> +		case REVERT_ACTION_START:
> +			break;

I can see the attraction of using an exhaustive switch() here but as we 
don't want to do anything in the _START case it gets a bit messy with 
the extra conditional below. I wonder if we'd be better to replace "if 
(cmd) {" with "if (cmd != REVERT_ACTION_START) {". Alternatively if you 
want to stick with the switch then declaring "this_operation" at the 
beginning of the function would mean you can get rid of the new "{}" block.

> +		case REVERT_ACTION_QUIT:
>   			this_operation = "--quit";
> -		else if (cmd == 'c')
> +			break;
> +		case REVERT_ACTION_CONTINUE:
>   			this_operation = "--continue";
> -		else if (cmd == 's')
> +			break;
> +		case REVERT_ACTION_SKIP:
>   			this_operation = "--skip";
> -		else {
> -			assert(cmd == 'a'); > +			break;
> +		case REVERT_ACTION_ABORT:
>   			this_operation = "--abort";
> +			break;
>   		}
>   
> -		verify_opt_compatible(me, this_operation,
> -				"--no-commit", opts->no_commit,
> -				"--signoff", opts->signoff,
> -				"--mainline", opts->mainline,
> -				"--strategy", opts->strategy ? 1 : 0,
> -				"--strategy-option", opts->xopts.nr ? 1 : 0,
> -				"-x", opts->record_origin,
> -				"--ff", opts->allow_ff,
> -				"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
> -				"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> -				NULL);
> +		if (this_operation)

The extra indentation here is unfortunate as some of the lines are 
rather long already. In the current code it is clear that we only call 
verify_opt_compatible() when cmd is non-nul, I think it would be clearer 
to use "if (cmd != REVERT_ACTION_START)" here.

> +			verify_opt_compatible(me, this_operation,
> +					"--no-commit", opts->no_commit,
> +					"--signoff", opts->signoff,
> +					"--mainline", opts->mainline,
> +					"--strategy", opts->strategy ? 1 : 0,
> +					"--strategy-option", opts->xopts.nr ? 1 : 0,
> +					"-x", opts->record_origin,
> +					"--ff", opts->allow_ff,
> +					"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
> +					"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> +					NULL);
>   	}
 > [...]
> -	if (cmd) {
> -		opts->revs = NULL;
> -	} else {
> +	if (cmd == REVERT_ACTION_START) {

I was momentarily confused by this change but you're reversing the 
conditional. I agree that the result is an improvement.

Best Wishes

Phillip