| Message ID | 20190417143044.17655-3-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Run rebase -i without forking rebase--interactive | expand |
Phillip Wood <phillip.wood123@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > commit b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22) > mistakenly marked the subverb names for translation and unnecessarily > NULL terminated the array. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/rebase.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 52114cbf0d..239a54ecfe 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > ACTION_EDIT_TODO, > ACTION_SHOW_CURRENT_PATCH, > } action = NO_ACTION; > - static const char *action_names[] = { N_("undefined"), > - N_("continue"), > - N_("skip"), > - N_("abort"), > - N_("quit"), > - N_("edit_todo"), > - N_("show_current_patch"), > - NULL }; > + static const char *action_names[] = { "undefined", > + "continue", > + "skip", > + "abort", > + "quit", > + "edit_todo", > + "show_current_patch" }; That's an improvement independent from the rest of the patches. Now we've had the C99 designated initialisers weather balloon changes for some time in our codebase, perhaps we can ensure that these entries match the intended & corresponding "enum action" constants? If we can also ensure that the array is large enough so that the trace2 call done like so trace2_cmd_mode(action_names[action]) is safe, that would be good, but that is secondary. Thanks.
On 19/04/2019 06:53, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> commit b3a5d5a80c ("trace2:data: add subverb for rebase", 2019-02-22) >> mistakenly marked the subverb names for translation and unnecessarily >> NULL terminated the array. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> builtin/rebase.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index 52114cbf0d..239a54ecfe 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> ACTION_EDIT_TODO, >> ACTION_SHOW_CURRENT_PATCH, >> } action = NO_ACTION; >> - static const char *action_names[] = { N_("undefined"), >> - N_("continue"), >> - N_("skip"), >> - N_("abort"), >> - N_("quit"), >> - N_("edit_todo"), >> - N_("show_current_patch"), >> - NULL }; >> + static const char *action_names[] = { "undefined", >> + "continue", >> + "skip", >> + "abort", >> + "quit", >> + "edit_todo", >> + "show_current_patch" }; > > That's an improvement independent from the rest of the patches. Yes I only included it as I move the definition later in the series > Now we've had the C99 designated initialisers weather balloon > changes for some time in our codebase, perhaps we can ensure that > these entries match the intended & corresponding "enum action" > constants? If we can also ensure that the array is large enough so > that the trace2 call done like so > > trace2_cmd_mode(action_names[action]) > > is safe, that would be good, but that is secondary. > > Thanks. If what's below is ok, I'll send a re-roll, I wasn't sure if it was best to die if action is larger than the array of names or just use a default. My worrying with dying is that it wont be caught by tests and will cause a problem for users who enable tracing. At least with what's below they can still rebase and hopefully report a bug about unknown action in their trace output. Best Wishes Phillip diff --git a/builtin/rebase.c b/builtin/rebase.c index 52114cbf0d..3f56be230e 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1027,14 +1027,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) ACTION_EDIT_TODO, ACTION_SHOW_CURRENT_PATCH, } action = NO_ACTION; - static const char *action_names[] = { N_("undefined"), - N_("continue"), - N_("skip"), - N_("abort"), - N_("quit"), - N_("edit_todo"), - N_("show_current_patch"), - NULL }; + static const char *action_names[] = { + [NO_ACTION] = "undefined", + [ACTION_CONTINUE] = "continue", + [ACTION_SKIP] = "skip", + [ACTION_ABORT] = "abort", + [ACTION_QUIT] = "quit", + [ACTION_EDIT_TODO] = "edit_todo", + [ACTION_SHOW_CURRENT_PATCH] = "show_current_patch" + }; const char *gpg_sign = NULL; struct string_list exec = STRING_LIST_INIT_NODUP; const char *rebase_merges = NULL; @@ -1225,8 +1226,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) trace2_cmd_mode("interactive"); else if (exec.nr) trace2_cmd_mode("interactive-exec"); - else + else if (action < ARRAY_SIZE(action_names)) trace2_cmd_mode(action_names[action]); + else + trace2_cmd_mode("unknown rebase action"); } switch (action) {
diff --git a/builtin/rebase.c b/builtin/rebase.c index 52114cbf0d..239a54ecfe 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1027,14 +1027,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) ACTION_EDIT_TODO, ACTION_SHOW_CURRENT_PATCH, } action = NO_ACTION; - static const char *action_names[] = { N_("undefined"), - N_("continue"), - N_("skip"), - N_("abort"), - N_("quit"), - N_("edit_todo"), - N_("show_current_patch"), - NULL }; + static const char *action_names[] = { "undefined", + "continue", + "skip", + "abort", + "quit", + "edit_todo", + "show_current_patch" }; const char *gpg_sign = NULL; struct string_list exec = STRING_LIST_INIT_NODUP; const char *rebase_merges = NULL;