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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Jul 6, 2024 at 7:11 PM Jeff King <peff@peff.net> wrote:
> 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.

Given the effort you put into the commit message and diagnosing my
bugs, my knee-jerk response is that it would be nice to keep your
patch separate so you retain authorship. But it also would be
irresponsible for us to let my buggy patch into the project history
as-is since you caught the problems at review time. So, squashing your
fixes in seems like the correct 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?

It would be more robust to do so, but I suspect things will be fine
for a long time even without such an enhancement. But I also agree
with your commentary in patch [1/3] that it probably would be easy to
latch the line number at the point at which the heredoc body is
latched.

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

Given the way the Makefile currently concatenates all the self-tests,
it would indeed be a nightmare to retain the line numbers. In the long
run, we probably ought someday to adopt Ævar's idea of checking the
self-test files individually[*] rather than en masse. With that
approach, it may make sense to revisit whether or not line numbers
should be present in the "expected" files.

[*] https://lore.kernel.org/git/CAPig+cSBjsosRqoAafYN94Cco8+7SdUt0ND_jHS+jVPoM4K0JA@mail.gmail.com/