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

[Junio C Hamano <gitster@pobox.com>]

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?  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.

> +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.