Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 29, 2023, 03:07 UTC
Message-ID: <CAPig+cSMYt81fcU8fRNhN9qZ=95jvRNv9YGhkM6PxgdMywWnCA@mail.gmail.com>
In-Reply-To: <20230329023702.GA1793752@coredump.intra.peff.net>

On Tue, Mar 28, 2023 at 10:37 PM Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

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

Show 14 quoted lines

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