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.
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 `ACTION_START` was chosen.
Co-authored-by: Wanja Henze <wanja.hentze@xxxxxxxxxx>
Signed-off-by: Michael Lohmann <mi.al.lohmann@xxxxxxxxx>
---
On 11. Jan 2024, at 20:37, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> > 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".
>
> I agree with you why NONE is called as such. If "revert" does not
> take "--start" (I do not remember offhand), I would think it would
> be better to follow suit.
My point was that yes, it might be in sync with what the user passes in
as arguments, but when I followed the code and saw lots of references to
ACTION_NONE I was puzzled, since my intuition of that name was that
_no action_ should be taken (which did not make sense to me).
So the (provocative) question is: Do we want to keep the variable name
in sync with some input parameters, or rather with the real action that
should be taken?
(Depending on the outcome of this discussion I would also prepare a
patch renaming it in builtin/rebase.c)
What do you think about this version which keeps the
`if (cmd != ACTION_START)` in favour of the `goto` and instead of the
constant if/else checks for the `verify_opt_compatible` (with the
`assert` at the last one) here is one version with a
`get_cmd_flag`-function (I am not that happy with the name...) that has
a `switch` and it has a runtime error handling with `BUG`.
I think it is the most concise of the options so far.
Ciao
Michael
builtin/revert.c | 65 +++++++++++++++++++++++++++---------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..891aa1d720 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -20,6 +20,14 @@
* Copyright (c) 2005 Junio C Hamano
*/
+enum action {
+ ACTION_START = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+};
+
static const char * const revert_usage[] = {
N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."),
N_("git revert (--continue | --skip | --abort | --quit)"),
@@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {
NULL
};
+static char* get_cmd_optionname(enum action cmd)
+{
+ 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");
+ }
+}
+
static const char *action_name(const struct replay_opts *opts)
{
return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
@@ -85,12 +104,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
- int cmd = 0;
+ enum action cmd = ACTION_START;
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")),
@@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
}
/* Check for incompatible command line arguments */
- if (cmd) {
- char *this_operation;
- if (cmd == 'q')
- this_operation = "--quit";
- else if (cmd == 'c')
- this_operation = "--continue";
- else if (cmd == 's')
- this_operation = "--skip";
- else {
- assert(cmd == 'a');
- this_operation = "--abort";
- }
-
- verify_opt_compatible(me, this_operation,
+ if (cmd != ACTION_START)
+ verify_opt_compatible(me, get_cmd_optionname(cmd),
"--no-commit", opts->no_commit,
"--signoff", opts->signoff,
"--mainline", opts->mainline,
@@ -168,7 +175,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
NULL);
- }
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
@@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--edit", opts->edit > 0,
NULL);
- if (cmd) {
- opts->revs = NULL;
- } else {
+ if (cmd == ACTION_START) {
struct setup_revision_opt s_r_opt;
opts->revs = xmalloc(sizeof(*opts->revs));
repo_init_revisions(the_repository, opts->revs, NULL);
@@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
+ } else {
+ opts->revs = NULL;
}
if (argc > 1)
@@ -210,19 +216,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
free(options);
- if (cmd == 'q') {
+ switch (cmd) {
+ case ACTION_QUIT: {
int ret = sequencer_remove_state(opts);
if (!ret)
remove_branch_state(the_repository, 0);
return ret;
}
- if (cmd == 'c')
+ case ACTION_CONTINUE:
return sequencer_continue(the_repository, opts);
- if (cmd == 'a')
+ case ACTION_ABORT:
return sequencer_rollback(the_repository, opts);
- if (cmd == 's')
+ case ACTION_SKIP:
return sequencer_skip(the_repository, opts);
- return sequencer_pick_revisions(the_repository, opts);
+ case ACTION_START:
+ return sequencer_pick_revisions(the_repository, opts);
+ }
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
2.39.3 (Apple Git-145)
