Re: [PATCH 1/2] chainlint: make error messages self-explanatory

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

Eric Sunshine <ericsunshine@charter.net> writes:

> "?!LOOP?!" case is particularly serious since it is likely that some
> newcomers are unaware that shell loops do not terminate automatically
> upon error, and it is more difficult for a newcomer to figure out how to
> correct the problem by examining surrounding code since `|| return 1`
> appears in test scrips relatively infrequently (compared, for instance,
> with &&-chaining).

"scrips" -> "scripts"

I'd prefer to see "some newcomes are unaware that" part rewritten
and toned down, as it is not our primary business to help total
newbies to learn shells, it certainly is not what the chain lint
checker should bend over backwards to do.

    ... particularly serious, as it does not convey that returning
    control with "|| return 1" (or "|| exit 1" from a subshell)
    immediately after we detect an error is the canonical way we
    chose in this project to handle errors in a loop.  Because it
    happens relatively infrequently, this norm is harder to figure
    out for a new person on their own than other patterns (like
    &&-chaining).

> Address these shortcomings by emitting human-consumable messages which
> both explain the problem and give a strong hint about how to correct it.

"consumable" -> "readable".

>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ...
>  # Input arguments are pathnames of shell scripts containing test definitions,
>  # or globs referencing a collection of scripts. For each problem discovered,
>  # the pathname of the script containing the test is printed along with the test
> -# name and the test body with a `?!FOO?!` annotation at the location of each
> +# name and the test body with a `?!ERR?!` annotation at the location of each
>  # detected problem, where "FOO" is a tag such as "AMP" which indicates a broken

"FOO" -> "ERR"?

> @@ -619,6 +623,15 @@ sub unwrap {
>  	return $s
>  }
>  
> +sub format_problem {
> +	local $_ = shift;
> +	/^AMP$/ && return "missing '&&'";
> +	/^LOOPRETURN$/ && return "missing '|| return 1'";
> +	/^LOOPEXIT$/ && return "missing '|| exit 1'";
> +	/^HEREDOC$/ && return 'unclosed heredoc';
> +	die("unrecognized problem type '$_'\n");
> +}
> +
>  sub check_test {
>  	my $self = shift @_;
>  	my $title = unwrap(shift @_);
> @@ -641,7 +654,8 @@ sub check_test {
>  	for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
>  		my ($label, $token) = @$_;
>  		my $pos = $token->[2];
> -		$checked .= substr($body, $start, $pos - $start) . " ?!$label?! ";
> +		my $err = format_problem($label, $token);
> +		$checked .= substr($body, $start, $pos - $start) . " ?!ERR $err?! ";
>  		$start = $pos;
>  	}
>  	$checked .= substr($body, $start);

With the hunks omitted before the above two that let us tell between
RETURN vs EXIT, the above two makes the problems much easier to
read.

All the "examples" (self tests) and changes to them looked sensible.

Thanks.