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
