From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 11 Jan 2012 12:44:32 GMT
Subject: Re: [PATCH 5/8] revert: report fine-grained errors from insn parser
Message-ID: <20120111124432.GF32173@burratino>
In-Reply-To: <1326212039-13806-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -732,7 +732,22 @@ 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_error(const char *message, const char *file,
> +		int lineno, char *error_line)
> +{
> +	const char *suffix = "";
> +	int error_len = strcspn(error_line, " \t\n");
> +
> +	if (error_len > 20) {
> +		error_len = 20;
> +		suffix = "...";
> +	}
> +	return error(_("%s:%d: %s: %.*s%s"), file, lineno, message,
> +		error_len, error_line, suffix);

Since the snippet used in an error message is a single word,
why is it called error_line?  (And why is the signature written
in a way that implies we might modify it, by the way?)

Missing /* TRANSLATORS: ... */ comment.

[...]
> @@ -757,11 +773,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>  	*end_of_object_name = saved;
>  
>  	if (status < 0)
> -		return -1;
> +		return parse_error(_("malformed object name"),
> +				git_path(SEQ_TODO_FILE), lineno, bol);

This is the message I'll get if I misspell "master" as "mister" or try
to cherry-pick HEAD~100000 when the history is not that deep.  When I
read "malformed object name", I'll look for syntax errors and be
confused.  (They are valid syntax denoting commits that just happen
not to exist.)