[v1,1/2] sequencer: don't abbreviate a command if it doesn't have a short form
diff mbox series

Message ID 20200330124236.6716-2-alban.gruin@gmail.com
State New
Headers show
Series
  • rebase --merge: fix fast forwarding when `rebase.abbreviateCommands' is set
Related show

Commit Message

Alban Gruin March 30, 2020, 12:42 p.m. UTC
When the sequencer is requested to abbreviate commands, it will replace
those that does not have a short form (eg. `noop') by a comment mark.
`noop' serves no purpose, except when fast-forwarding (ie. by running
`git rebase').  Removing it will break this command when
`rebase.abbreviateCommands' is set to true.

This changes todo_list_to_strbuf() to check if a command has an actual
short form, and to ignore it if not.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Junio C Hamano March 30, 2020, 5:50 p.m. UTC | #1
Alban Gruin <alban.gruin@gmail.com> writes:

>  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.

> @@ -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.
Eric Sunshine March 30, 2020, 6:15 p.m. UTC | #2
On Mon, Mar 30, 2020 at 8:43 AM Alban Gruin <alban.gruin@gmail.com> wrote:
> When the sequencer is requested to abbreviate commands, it will replace
> those that does not have a short form (eg. `noop') by a comment mark.

s/does/do/

> `noop' serves no purpose, except when fast-forwarding (ie. by running
> `git rebase').  Removing it will break this command when
> `rebase.abbreviateCommands' is set to true.
>
> This changes todo_list_to_strbuf() to check if a command has an actual
> short form, and to ignore it if not.

Perhaps: s/This changes/Change/

> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index 6fd2674632..79d0c5cb2e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1578,7 +1578,7 @@  static const char *command_to_string(const enum todo_command command)
 
 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;
 }
@@ -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)
+			strbuf_addch(buf, cmd);
 		else
 			strbuf_addstr(buf, command_to_string(item->command));