Re: [PATCH 2/3] sequencer: factor out todo command name parsing

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

Hi René

On 25/06/2019 18:03, René Scharfe wrote:
> Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Factor out the code that parses the name of the command at the start of
>> each line in the todo file into it's own function so that it can be used
>> in the next commit.
> 
> "Factor out" sounds like functionality is intended to not be changed...

Indeed, I thought I should have explained a bit more in the commit
message after I sent it. I think it is unchanged but it's not
immediately obvious.

>>
>> 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)
>> +{
>> +	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) &&
>> +		 (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
>                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ... but this adds support for LF, CR and NUL as separators after short
> commands...

They're there to detect the end of the line, as I don't want to have to
pass in eol from other callers. The check that the nick name is not '\0'
means that if we pass in an empty string we do not read past the end as
the test will fail before it gets to the "*p == ..." part.

> 
>> +		 (*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') &&
>                             ^^^^^^^^^^^^^^
> ... while this removes the check against the string's length.
> 
> Is this safe? It probably (hopefully?) is, as skip_prefix() requires bol
> to point to a NUL-terminated string already.

Yes the string is NUL-terminated

> But is_command() could do the old (and shorter) eol check just as well,
> so the next question is: Why this change?

For the next patch so other callers don't have to find the end of the
line if they don't need it. The new checks for '\n' etc. replace the
'bol + 1 == eol' part.

I'll reroll with a better commit message

Best Wishes

Phillip

> 
>> -			   *bol == todo_command_info[i].c) {
>> -			bol++;
>> +		if (is_command(i, &bol)) {
>>  			item->command = i;
>>  			break;
>>  		}
>>
>