On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote:

On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt [off-list ref] wrote:

quoted

[...]
Instead of improving the detection logic, fix our ".expect" files so
that we do not need any post-processing at all anymore. This allows us
to drop the `-w` flag when diffing so that we can always use diff(1)
now.

Note that we leave the post-processing of `chainlint.pl` output intact.
All we do here is to strip leading line numbers that it would otherwise
generate.

Hmm, okay, but... (see below)

quoted

Having these would cause a rippling effect whenever we add a
new test that sorts into the middle of existing tests and would require
us to renumerate all subsequent lines, which seems rather pointless.

Just an aside, not strictly relevant at this time: Ævar has proposed
that check-chainlint should not be creating conglomerate "test",
"expect", and "actual" files, but should instead let `make` run
chainlint.pl separately on each chainlint self-test file, thus
benefiting from `make`'s innate parallelism rather than baking
parallelism into chainlint.pl itself. More importantly, `make`'s
dependency tracking would ensure that a chainlint self-test file only
gets rechecked if its timestamp changes. That differs from the current
situation in which _all_ of the chainlint self-test files are checked
on _every_ `make test` which is wasteful if none of them have changed.
Anyhow, with his proposed approach, there wouldn't be cascading line
number changes just because a new self-test file was added.

I was indeed also thinking along this way and would tend to agree. I
punted on it as I honestly only really care for fixing the immediate
issue that the post-processing causes for me.

Are you fine with deferring this bigger refactoring?

quoted

Signed-off-by: Patrick Steinhardt <redacted>
---

diff --git a/t/Makefile b/t/Makefile

@@ -103,20 +103,12 @@ check-chainlint:
        $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
                sed -e 's/^[1-9][0-9]* //;/^[   ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \

The commit message claims that this is only stripping the line numbers
which prefix each emitted line, but the `/^[   ]*$$/d` bit is also
deleting blank lines from the output of chainlint.pl. Thus, this ought
to be:

    sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \

Gah, you're right, I missed the second part. Will fix in another round.

Patrick