Re: [PATCH v1 1/2] sequencer: don't abbreviate a command if it doesn't have a short form
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 30, 2020, 17:50 UTC
- Message-ID
- <xmqqeet991fj.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200330124236.6716-2-alban.gruin@gmail.com>
Alban Gruin <alban.gruin@gmail.com> writes:
Show 7 quoted lines
> static char command_to_char(const enum todo_command command)
> {
> - if (command < TODO_COMMENT && todo_command_info[command].c)
> + if (command < TODO_COMMENT)
> return todo_command_info[command].c;
> return comment_line_char;
> }This is not a new issue, and it may not even be an issue at all, but it is curious that command_to_string() barfs with "unknown command" when fed an int outside enum todo_command or TODO_COMMENT iteslf, while this returns comment_line_char. Makes a reader wonder if both of them should be dying the same way.
Show 17 quoted lines
> @@ -4963,6 +4963,8 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
> max = num;
>
> for (item = todo_list->items, i = 0; i < max; i++, item++) {
> + char cmd;
> +
> /* if the item is not a command write it and continue */
> if (item->command >= TODO_COMMENT) {
> strbuf_addf(buf, "%.*s\n", item->arg_len,
> @@ -4971,8 +4973,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
> }
>
> /* add command to the buffer */
> - if (flags & TODO_LIST_ABBREVIATE_CMDS)
> - strbuf_addch(buf, command_to_char(item->command));
> + cmd = command_to_char(item->command);
> + if (flags & TODO_LIST_ABBREVIATE_CMDS && cmd)Even though the precedence rule may not require it, for readability's sake, it would be easier to see the association if this is written with an extra set of parentheses, i.e.
if ((flags & TODO_LIST_ABBREVIATE_CMDS) && cmd)
> + strbuf_addch(buf, cmd); > else > strbuf_addstr(buf, command_to_string(item->command));
The logic is quite clear. If there is an abbreviation and the user prefers to see it, we use it, but otherwise we'll give the full spelling.
We are sure we will never get TODO_COMMENT here in item->command at this point (the loop would have already continued after adding it to the buffer), so it does not affect us that command_to_string() would die. For that matter, if we made command_to_char() die, just like command_to_string() would, nobody will get hurt and the resulting code would become saner. But obviously it is outside the scope of this fix (#leftoverbits).
Thanks.