Re: [PATCH 12/15] sequencer: handle multi-byte comment characters when writing todo list

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Peff

On 07/03/2024 09:27, Jeff King wrote:
> We already match multi-byte comment characters in parse_insn_line(),
> thanks to the previous commit, yielding a TODO_COMMENT entry. But in
> todo_list_to_strbuf(), we may call command_to_char() to convert that
> back into something we can output.
> 
> We can't just return comment_line_char anymore, since it may require
> multiple bytes. Instead, we'll return "0" for this case, which is the
> same thing we'd return for a command which does not have a single-letter
> abbreviation (e.g., "revert" or "noop"). In that case the caller then
> falls back to outputting the full name via command_to_string(). So we
> can handle TODO_COMMENT there, returning the full string.

If you do re-roll it might be helpful to emphasize that there is only 
one caller.

> Note that there are many other callers of command_to_string(), which
> will now behave differently if they pass TODO_COMMENT. But we would not
> expect that to happen; prior to this commit, the function just calls
> die() in this case. And looking at those callers, that makes sense;
> e.g., do_pick_commit() will only be called when servicing a pick
> command, and should never be called for a comment in the first place.

I've checked the other callers and agree with your analysis. The fact 
that it used to die() also makes it pretty clear that this should be safe.

Best Wishes

Phillip

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   sequencer.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 664986e3b2..9e2851428b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1779,14 +1779,16 @@ static const char *command_to_string(const enum todo_command command)
>   {
>   	if (command < TODO_COMMENT)
>   		return todo_command_info[command].str;
> +	if (command == TODO_COMMENT)
> +		return comment_line_str;
>   	die(_("unknown command: %d"), command);
>   }
>   
>   static char command_to_char(const enum todo_command command)
>   {
>   	if (command < TODO_COMMENT)
>   		return todo_command_info[command].c;
> -	return comment_line_char;
> +	return 0;
>   }
>   
>   static int is_noop(const enum todo_command command)