On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote:
> > - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)"
> > + test_eval_ "fail_117 && $1"
> > + if test $? != 117
> > then
> > - BUG "broken &&-chain or run-away HERE-DOC: $1"
> > + BUG "broken &&-chain: $1"
> > fi
>
> This "here-doc" linter is a cute trick. With seemingly so little
> extra code, it catches a breakage in such an unexpected way.
>
> Even with a very small code footprint, overhead of an extra process
> is still there, and it would be very hard to figure out what it does
> (once you are told what it does, you can probably figure out how it
> works). These make it a "cute trick".
Yes, I thought it was quite clever (and it is attributed to you), but I
agree that I did not quite realize what it was doing until after running
git-blame. I only started with "gee, why didn't we write this in the
simpler way?", which is often a good starting point for archaeology. :)
> While it is a bit sad to see it lost, the resulting code certainly
> is easier to follow, I would think. I do not offhand know how
> valuable detecting unterminated here-doc is, compared to the
> increased clarity of hte code.
I think the complexity is merited _if_ we think it is catching useful
cases. And I do count unterminated here-doc as a useful case, because,
as with broken &&-chains, the failure mode is so nasty (a test will
report success, even though part of the test was simply not run!).
I just think chainlint.pl is doing a good enough job of catching it that
we can rely on it. I'll be curious if Eric has input there on whether it
can do even better, which would remove all of the caveats from the
commit message.
-Peff
