Re: [PATCH v2] tests: adjust whitespace in chainlint expectations

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 14, 2023, 08:44 UTC
Message-ID: <CAPig+cTZmiXdPZEVO-F2UzV9YaP6c7r2MfPTC3QWksJa+rM7VA@mail.gmail.com>
In-Reply-To: <aec86a15c69aa276eee4875fad208ee2fc57635a.1702542564.git.ps@pks.im>

On Thu, Dec 14, 2023 at 3:30 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 22 quoted lines

> [...]
> Instead of improving the detection logic, fix our ".expect" files so
> that we do not need any post-processing anymore. This allows us to drop
> the `-w` flag when diffing so that we can always use diff(1) now.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -108,15 +108,7 @@ check-chainlint:
>         } >'$(CHAINLINTTMP_SQ)'/expect && \
>         $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
>                 sed -e 's/^[1-9][0-9]* //;/^[   ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
> -       if test -f ../GIT-BUILD-OPTIONS; then \
> -               . ../GIT-BUILD-OPTIONS; \
> -       fi && \
> -       if test -x ../git$$X; then \
> -               DIFFW="../git$$X --no-pager diff -w --no-index"; \
> -       else \
> -               DIFFW="diff -w -u"; \
> -       fi && \
> -       $$DIFFW '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
> +       diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual

This change feels incomplete. Yes, it "corrects" the whitespace, but check-chainlint still applies sed to remove blank lines and strip the line numbers from the beginning of each line. Based upon the commit message, I had expected that all post-processing of the output would have been eliminated and that the "expect" files would exactly mirror the output of the linter.

As mentioned elsewhere[1], I'm not particularly in favor of this approach, though I won't stand in the way of it either.

[1]: https://lore.kernel.org/git/CAPig+cSkuRfkR2D3JqYcbaJqj485nfD9Nq6pM=vXWB5DJenWpA@mail.gmail.com/