Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Malformed object name
> 3. Object name does not refer to a valid commit
>
> Since we would like to make the instruction sheet user-editable in the
> future (much like the 'rebase -i' sheet), report more fine-grained
> parse errors prefixed with the filename and line number.

Seems like a sensible idea.  In other words, the infrastructure that
parses .git/sequencer/todo is meant to handle arbitrary user input
some day, so it can be used as the implementation of "git rebase
--interactive" and "git sequence --edit", and currently it is
suboptimal for that purpose because the parse error messages just say

	error: Could not parse line 5.

This patch shifts responsibility to parse_insn_line(), which can come
up with a more detailed message like

	error: .git/sequencer/todo:5: unrecognized action "frobnicate"

Once the operator is allowed to edit the sequence, the message might
be adjusted to something meaning

	error: <sequence you just gave me>:5: unrecognized action "frobnicate"

instead of exposing an implementation detail, or maybe some day "git
sequence --edit" could even re-launch the editor with an error message
in a comment before the problematic line and the cursor pointing
there.  And for now, pointing to the explicit filename is useful since
this should only happen if there was filesystem corruption, tampering,
or a git bug.

By the way, an example of the output before and after the patch would
have been useful in saving some trouble of figuring this out.

Ok, onward to the patch.

[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>  	return 0;
>  }
>  
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> +			struct replay_insn_list *item, int lineno)
>  {
> +	const char *todo_file = git_path(SEQ_TODO_FILE);

This idiom is _still_ scary.  I don't want to have to shout about
this, but why didn't the commit message at least acknowledge it to put
the reader's mind at ease when this has come up again and again?

[...]
> +		bol += strlen("revert ");
> +	} else {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Unrecognized action: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);

Ah, so my example above was wrong: the actual message is

	error: .git/sequencer/todo:5: Unrecognized action: the quick bro
	wn fox jumps over the lazy dog

I guess that's fine.  Is it intended?

> +	}
>  
>  	/* Eat up extra spaces/ tabs before object name */
> -	padding = strspn(bol, " \t");
> -	if (!padding)
> -		return -1;
> -	bol += padding;
> +	bol += strspn(bol, " \t");

What does this have to do with the stated goal of the patch?  It seems
to me that an unrelated and unadvertised bugfix snuck in.

[...]
> @@ -741,12 +744,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>  	status = get_sha1(bol, commit_sha1);
>  	*end_of_object_name = saved;
>  
> -	if (status < 0)
> -		return -1;
> +	if (status < 0) {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Malformed object name: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);
> +	}

This seems a little repetitive --- maybe it would make sense to
factor out a function to emit errors of the form

	error: file:lineno: message: line

By the way, this is gross.  Probably get_sha1 should grow a variant
that takes a buffer and a length.

[...]
>  	item->operand = lookup_commit_reference(commit_sha1);
> -	if (!item->operand)
> -		return -1;
> +	if (!item->operand) {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Not a valid commit: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);
> +	}

Hmm, this one can be emitted even when there was no corruption or
internal error, because the user removed a commit she was
cherry-picking and the gc-ed before a "git cherry-pick --continue".
Alternatively, it can happen because the repository has grown very
crowded and what used to be an unambiguous commit name no longer is
one (not enough digits).  Will the error message be intuitive in these
situations?

[...]
> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>  
>  	for (i = 1; *p; i++) {
>  		char *eol = strchrnul(p, '\n');
> -		if (parse_insn_line(p, eol, &item) < 0)
> -			return error(_("Could not parse line %d."), i);
> +		if (parse_insn_line(p, eol, &item, i) < 0)
> +			return -1;

Not related to this patch, but why the "< 0" test?  It makes this
reader worry that there is some unusual return value convention that
he should be taking into account.

To sum up: the idea looks promising, but this patch doesn't seem to be
ready yet.

Hope that helps,
Jonathan