| Message ID | 20240307092747.GL2080210@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | allow multi-byte core.commentChar | expand |
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)
On Fri, Mar 08, 2024 at 10:20:45AM +0000, Phillip Wood wrote: > 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. Thanks, will do. -Peff
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)
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. 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. Signed-off-by: Jeff King <peff@peff.net> --- sequencer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)