On Tue, Mar 28, 2023 at 10:37 PM Jeff King [off-list ref] wrote:

On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote:

quoted

quoted

-           if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)"
-                   BUG "broken &&-chain or run-away HERE-DOC: $1"

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. :)

I never realized what the "OK-" part of "OK-117" was doing either. I
guess I should have gone spelunking through history to find out,
though it wasn't high-priority for me to know with regards to my work
on chainlint.pl, so I never got around to it. I suspect that the "OK-"
trick was discussed and added during the period I was off-list.

quoted

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.

It was an intentional design choice, for the sake of simplicity, _not_
to make chainlint.pl a shell syntax checker, despite the fact that it
is genuinely parsing shell code. After all, the shell itself -- by
which test code will ultimately be executed -- is more than capable of
diagnosing syntax errors, so why burden chainlint.pl with all that
extra baggage? Instead, chainlint.pl is focussed on detecting semantic
problems -- such as broken &&-chain and missing `return 1` -- which
are of importance to _this_ project.

So, it was very much deliberate that chainlint.pl does not diagnose an
unclosed here-doc. When making that design choice, though, I wasn't
aware of 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22),
thus didn't know that our test framework had been allowing such
problems to slip through silently[1].

That said, it doesn't look too hard to make chainlint.pl diagnose an
unclosed here-doc.

[1]: Why is that, by the way? Is `eval` not complaining about the
unclosed here-doc? Is that something that can be fixed more generally?
Are there other syntactic problems that go unnoticed?