On Thu, Aug 29, 2024 at 11:39 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <ericsunshine@xxxxxxxxxxx> 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).
>
> 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).
How about this?
The "?!LOOP?!" case is particularly serious because that terse
single word does nothing to convey that the loop body should end
with "|| return 1" (or "|| exit 1" in a subshell) to ensure that a
failing command in the body aborts the loop immediately, which is
important since a shell loop does not automatically terminate when
an error occurs within its body. Moreover, unlike &&-chaining
which is ubiquitous in Git tests, the "|| return 1" idiom is
relatively infrequent, thus may be harder for a newcomer to
discover by consulting nearby code.
> > -# 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"?
Yep. Sharp eyes.
