Re: [PATCH v2 2/3] sequencer: factor out todo command name parsing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 27, 2019, 17:29 UTC
- Message-ID
- <xmqqh88baqcu.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <c3885c0b8f55957695650c2d1e25f9036d7e80dd.1561644762.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> As I don't want to burden other callers with having to pass in a pointer > to the end of the line the test for an abbreviated command is > changed.
A comma missing somewhere between "As <<REASON>>, <<CONSEQUENCE>>", perhaps after "end of the line"?
Show 23 quoted lines
> This change should not affect the behavior. Instead of testing
> `eol == bol + 1` the new code checks for the end of the line by testing
> for '\n', '\r' or '\0' following the abbreviated name. To avoid reading
> past the end of an empty string it also checks that there is actually a
> single character abbreviation before testing if it matches. This
> prevents it from matching '\0' as the abbreviated name and then trying
> to read another character.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> sequencer.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 919e3153f5..793f86bf9a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
> return todo_list->buf.buf + item->arg_offset;
> }
>
> +static int is_command(enum todo_command command, const char **bol)
> +{This is a tangent, but the reason why the caller of this function is named parse_insn_line() (and not parse_command_line()) is because a "command" often refers "rebase", "cherry-pick" etc., and we need a word to differenciate these from what is to be done as an individual step. Once the codebase stabilizes (read: I am excluding this kind of change outside the scope of a series like this one), we'd need to clean up the names in this file, I think.
Show 6 quoted lines
> + const char *str = todo_command_info[command].str; > + const char nick = todo_command_info[command].c; > + const char *p = *bol + 1; > + > + return skip_prefix(*bol, str, bol) || > + ((nick && **bol == nick) &&
OK, making sure that nick is not NUL is the key to avoid stepping past the NUL after the line that begins at *bol, as explained in the additional paragraph in the proposed log message.
Looking good.
Show 21 quoted lines
> + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
> + (*bol = p));
> +}
> +
> static int parse_insn_line(struct repository *r, struct todo_item *item,
> const char *buf, const char *bol, char *eol)
> {
> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> }
>
> for (i = 0; i < TODO_COMMENT; i++)
> - if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> - item->command = i;
> - break;
> - } else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\t') &&
> - *bol == todo_command_info[i].c) {
> - bol++;
> + if (is_command(i, &bol)) {
> item->command = i;
> break;
> }