Re: [PATCH v3 1/2] sequencer: factor out parsing of todo commands

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

On 22/06/2026 18:00, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Move the code that parses todo commands into a separate function so
>> that it can be shared with "git status" in the next commit. As we
>> know the input is NUL terminated we do not pass a pointer to the end
>> of the line and instead test for a blank line by looking for NUL, CR
>> LF, or LF. We use starts_with() instead of starts_with_mem() for the
>> same reason. This results in slightly different behavior when there
>> a CR at the start of the line that is not followed by LF. Previously
>> such a line was treated as a comment rather than an invalid line.
> 
> Meaning that the input validation is tighter than before? 

Yes

> I think
> it is fine in this case, as I do not see a reason why anybody wants
> to use a lone CR as comment introducer.

Agreed. In the unlikely event that core.commentChar starts with a CR we 
still treat the line as a comment, but we don't treat lines starting 
with a CR as a comment anymore. I think that behavior was a lazy way of 
handling empty lines with CR LF line endings.

Thanks

Phillip
>> +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
>> +{
>> +	const char *s = *p;
>> +
>> +	for (int i = 0; i < TODO_COMMENT; i++)
>> +		if (is_command(i, p)) {
>> +			*cmd = i;
>> +			return true;
>> +		}
>> +
>> +	if (starts_with(s, comment_line_str)) {
>> +		*cmd = TODO_COMMENT;
>> +		return true;
>> +	} else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
>> +		*cmd = TODO_COMMENT;
>> +		return true;
>> +	}
>> +
>> +	return false;
>>   }
> 
> I notice that the order of noticing concrete comments and comment
> lines are swapped relative to the original.  There is no inherently
> "natural" order between them, so the change is perfectly OK.  I just
> got confused slightly while reading it until I realized that is what
> you did.
> 
>>   static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>> @@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>>   {
>>   	struct object_id commit_oid;
>>   	char *end_of_object_name;
>> -	int i, saved, status, padding;
>> +	int saved, status, padding;
>>   
>>   	item->flags = 0;
>>   
>>   	/* left-trim */
>>   	bol += strspn(bol, " \t");
>>   
>> -	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
>> -		item->command = TODO_COMMENT;
>> -		item->commit = NULL;
>> -		item->arg_offset = bol - buf;
>> -		item->arg_len = eol - bol;
>> -		return 0;
>> -	}
>> -
>> -	for (i = 0; i < TODO_COMMENT; i++)
>> -		if (is_command(i, &bol)) {
>> -			item->command = i;
>> -			break;
>> -		}
>> -	if (i >= TODO_COMMENT)
>> +	if (!sequencer_parse_todo_command(&bol, &item->command))
>>   		return error(_("invalid command '%.*s'"),
>>   			     (int)strcspn(bol, " \t\r\n"), bol);
>> +
>> +	if (item->command == TODO_COMMENT) {
>> +		item->commit = NULL;
>> +		item->arg_offset = bol - buf;
>> +		item->arg_len = eol - bol;
>> +		return 0;
>> +	}
> 
> And the extra stuff that are only relevant to a comment line is
> naturally processed by the caller.  OK.
> 
> Thanks.  Looking good so far.
>