On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote: > On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > [...] > > 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) > > > 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? > > Signed-off-by: Patrick Steinhardt <ps@xxxxxx> > > --- > > 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
Attachment:
signature.asc
Description: PGP signature
