On Sat, Jul 6, 2024 at 7:11 PM Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxxxxx/
