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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2024, 15:39 UTC
Message-ID: <xmqq7cbzxrry.fsf@gitster.g>
In-Reply-To: <20240829091625.41297-2-ericsunshine@charter.net>

Eric Sunshine <ericsunshine@charter.net> writes:

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 26 quoted lines

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