On Tue, Mar 28, 2023 at 10:37 PM Jeff King <peff@xxxxxxxx> wrote:
> 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)"
> > > - 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.
> > 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?
