[v1,02/12] rebase: don't translate trace strings
diff mbox series

Message ID 20190417143044.17655-3-phillip.wood123@gmail.com
State New
Headers show
Series
  • Run rebase -i without forking rebase--interactive
Related show

Commit Message

Phillip Wood April 17, 2019, 2:30 p.m. UTC
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(-)

Comments

Junio C Hamano April 19, 2019, 5:53 a.m. UTC | #1
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.
Phillip Wood April 25, 2019, 5:47 p.m. UTC | #2
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) {

Patch
diff mbox series

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;