Re: [PATCH] chainlint.pl: recognize test bodies defined via heredoc

From: Jeff King <peff@peff.net>
Date: Jul 6, 2024, 23:11 UTC
Message-ID: <20240706231128.GA746087@coredump.intra.peff.net>
In-Reply-To: <xmqqr0c6makb.fsf@gitster.g>

On Sat, Jul 06, 2024 at 03:15:32PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I'll post some patches in a moment:
> >
> >   [1/3]: chainlint.pl: fix line number reporting
> >   [2/3]: t/chainlint: add test_expect_success call to test snippets
> >   [3/3]: t/chainlint: add tests for test body in heredoc
> >
> > with the idea that we'd apply your patch here on top of what Junio has
> > queued in jk/test-body-in-here-doc, and then these three on top.
> 
> Would the final form be to have Eric's preparatory enhancement to
> chainlint and then these three first, and finally the "here-docs"
> conversion I queued from you earlier?

I had planned on top (leaving a brief moment where chainlint would ignore the new format), but either way is fine.

My biggest question is around my patch 1 above:

  - is it worth squashing in to Eric's patch? I didn't want to do that
    without getting his OK on the approach.

  - instead of bumping the line number in the caller, should the lexer
    record the line number of the here-doc to be used later?

  - the test harness in the Makefile strips the line numbers from the
    chainlint output, so it's hard to verify those fixes. I saw them
    only because the combination of the two bugs meant that the here-doc
    had a "line 0" in it, which was enough to confuse the "sed"
    invocation in the Makefile.

    I did manually verify that it is OK after my fix, but do we want
    that to be part of the chainlint tests? Just leaving the line
    numbers in is a maintenance nightmare, since it depends on the order
    of concatenating all of the tests together (so our "expect" files
    would depend on all of the previous tests). But if we wanted to get
    fancy, we could perhaps store relative offsets in the expect file. I
    think it gets pretty complicated, though, since we print only
    problematic lines.

I was going to give it a few days for Eric to chime in on those points, and then assemble a final version for you to apply (but I certainly don't mind if you want to pick up what's here in the meantime).

-Peff