[PATCH] tests: prefer host Git to verify chainlint self-checks

[PATCH] tests: prefer host Git to verify chainlint self-checks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.

The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.

To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.

Fix this issue by prefering the host's Git resolved via PATH. If it
doesn't exist, then we fall back to the locally-compiled Git version and
diff as before.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

I've started to dogfood the reftable backend on my local machine and
have converted many repositories to use the reftable backend. This
surfaced the described issue because the repository now sets up the
"extensions.refStorage" extension, and thus "check-chainlint" fails
depending on which versions of Git I'm trying to compile and test.

 t/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..8b7f7aceaa 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -111,7 +111,9 @@ check-chainlint:
 	if test -f ../GIT-BUILD-OPTIONS; then \
 		. ../GIT-BUILD-OPTIONS; \
 	fi && \
-	if test -x ../git$$X; then \
+	if command -v git >/dev/null 2>&1; then \
+		DIFFW="git --no-pager diff -w --no-index"; \
+	elif test -x ../git$$X; then \
 		DIFFW="../git$$X --no-pager diff -w --no-index"; \
 	else \
 		DIFFW="diff -w -u"; \
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> To accomodate for cases where the host system has no Git installation we
> use the locally-compiled version of Git. This can result in problems
> though when the Git project's repository is using extensions that the
> locally-compiled version of Git doesn't understand. It will refuse to
> run and thus cause the checks to fail.
>
> Fix this issue by prefering the host's Git resolved via PATH. If it
> doesn't exist, then we fall back to the locally-compiled Git version and
> diff as before.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> I've started to dogfood the reftable backend on my local machine and
> have converted many repositories to use the reftable backend. This
> surfaced the described issue because the repository now sets up the
> "extensions.refStorage" extension, and thus "check-chainlint" fails
> depending on which versions of Git I'm trying to compile and test.

I do not think "prefer host Git" is necessarily a good idea; falling
back to use host Git is perfectly fine, of course.

Other than that, I agree with the motivation.

>  t/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index 225aaf78ed..8b7f7aceaa 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -111,7 +111,9 @@ check-chainlint:
>  	if test -f ../GIT-BUILD-OPTIONS; then \
>  		. ../GIT-BUILD-OPTIONS; \
>  	fi && \
> -	if test -x ../git$$X; then \
> +	if command -v git >/dev/null 2>&1; then \
> +		DIFFW="git --no-pager diff -w --no-index"; \
> +	elif test -x ../git$$X; then \
>  		DIFFW="../git$$X --no-pager diff -w --no-index"; \
>  	else \
>  		DIFFW="diff -w -u"; \

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

On Tue, Dec 12, 2023 at 11:30:22AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > To accomodate for cases where the host system has no Git installation we
> > use the locally-compiled version of Git. This can result in problems
> > though when the Git project's repository is using extensions that the
> > locally-compiled version of Git doesn't understand. It will refuse to
> > run and thus cause the checks to fail.
> >
> > Fix this issue by prefering the host's Git resolved via PATH. If it
> > doesn't exist, then we fall back to the locally-compiled Git version and
> > diff as before.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >
> > I've started to dogfood the reftable backend on my local machine and
> > have converted many repositories to use the reftable backend. This
> > surfaced the described issue because the repository now sets up the
> > "extensions.refStorage" extension, and thus "check-chainlint" fails
> > depending on which versions of Git I'm trying to compile and test.
> 
> I do not think "prefer host Git" is necessarily a good idea; falling
> back to use host Git is perfectly fine, of course.

Why is that, though? We already use host Git in other parts of our build
infra, and the options we pass to git-diff(1) have been around for ages:

  - `--no-pager` was introduced in 463a849d00 (Add and document a global
    --no-pager option for git., 2007-08-19).

  - `--no-index` is around since at least fcfa33ec90 (diff: make more
    cases implicit --no-index, 2007-02-25).

  - `-w` is around since 0d21efa51c (Teach diff about -b and -w flags,
    2006-06-14).

So all options have pretty much been there since forever. Which means
that if the host has Git around, and the Git version is at least v1.5.3,
then it also understands all options.

Furthermore, we are accessing the Git repository that the user has set
up with host Git in the first place, so I'd think even conceptually it
is the correct thing to do.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> I do not think "prefer host Git" is necessarily a good idea; falling
>> back to use host Git is perfectly fine, of course.
>
> Why is that, though?

Mostly because your "differences in features supported by just-built
one and what happens to be on $PATH can cause problems" cuts both
ways, and as a general principle to work around such issues, using
just-built one is a better discipline.  The features the build
infrastructure ("self-checks" being discussed is a part of it) of a
particular version of Git source depends on are more likely to be
found in the binary that will be built from the matching source,
than what happens to be on $PATH that may be from a year-old version.

As you said, you'd need to accomodate need for those who are
initially porting or building Git on a host without an installed
one.  If we were doing a cross build where just-built on would not
be executable on the host, whatever version on $PATH (or in
/usr/bin) may have to be used, but then in such a case you would not
be testing on host anyway.  For these two reasons, it is a given
that one of the choices has to be to use just-built one.  We can
safely give lower precedence to the host tool.

The only one-and-half practical reasons we may want to fall back to
whatever happens to be on $PATH are:

 - just-built one is so broken that even the simple use by the build
   infrastructure (like "self-checks") does not work (but then it
   becomes "if it is so broken, fix it before even thinking about
   running tests", and that is why I count it as half a reason), or

 - we are bisecting down to an ancient version, and just-built one
   from such a version may not understand the current repository.

so I do think it is an excellent workaround to fall back to a
version of Git with unknown vintage that happens to be on $PATH,
than failing and stopping by relying only on just-built one.

> We already use host Git in other parts of our build
> infra, and the options we pass to git-diff(1) have been around for ages:

It only argues for "trying host one, if available, before just-built
one does not hurt for this particular case".  But I was interested
in laying out a more general principle we can follow in similar
situations in the future.

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Eric Sunshine]

On Wed, Dec 13, 2023 at 10:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> I do not think "prefer host Git" is necessarily a good idea; falling
> >> back to use host Git is perfectly fine, of course.
> >
> > Why is that, though?
>
> Mostly because your "differences in features supported by just-built
> one and what happens to be on $PATH can cause problems" cuts both
> ways [...]

I sent an alternative solution[1] which should sidestep this objection.

As usual, I forgot to use --in-reply-to=<this-thread> when sending the
patch, even though I reminded myself to use it only a minute or so
earlier. Sorry.

[1]: https://lore.kernel.org/git/20231214032248.1615-1-ericsunshine@charter.net/

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

On Wed, Dec 13, 2023 at 10:33:00PM -0500, Eric Sunshine wrote:
> On Wed, Dec 13, 2023 at 10:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > >> I do not think "prefer host Git" is necessarily a good idea; falling
> > >> back to use host Git is perfectly fine, of course.
> > >
> > > Why is that, though?
> >
> > Mostly because your "differences in features supported by just-built
> > one and what happens to be on $PATH can cause problems" cuts both
> > ways [...]
> 
> I sent an alternative solution[1] which should sidestep this objection.
> 
> As usual, I forgot to use --in-reply-to=<this-thread> when sending the
> patch, even though I reminded myself to use it only a minute or so
> earlier. Sorry.
> 
> [1]: https://lore.kernel.org/git/20231214032248.1615-1-ericsunshine@charter.net/

Thanks, I've replied to the thread. I think by now there are three
different ideas:

  - Improve the logic to pick some kind of diff implementation, which is
    my patch series. It would need to be improved so that we also probe
    whether the respective Git executables actually understand the repo
    format so that we can fall back from the just-built Git to system's
    Git.

  - Munge the whitespace of the expected results with some regexes.
    I like that idea better because we can avoid the git-diff(1)
    problem, but find that the result is somewhat hard to read.

  - Fix the ".expect" files so that we can avoid all of these games.

I actually like the last option most. I'll have a go at it and send this
third version out in a bit.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Eric Sunshine]

On Thu, Dec 14, 2023 at 3:13 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Wed, Dec 13, 2023 at 10:33:00PM -0500, Eric Sunshine wrote:
> > On Wed, Dec 13, 2023 at 10:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > Mostly because your "differences in features supported by just-built
> > > one and what happens to be on $PATH can cause problems" cuts both
> > > ways [...]
> >
> > I sent an alternative solution[1] which should sidestep this objection.
>
> Thanks, I've replied to the thread. I think by now there are three
> different ideas:
>
>   - Improve the logic to pick some kind of diff implementation, which is
>     my patch series. It would need to be improved so that we also probe
>     whether the respective Git executables actually understand the repo
>     format so that we can fall back from the just-built Git to system's
>     Git.
>
>   - Munge the whitespace of the expected results with some regexes.
>     I like that idea better because we can avoid the git-diff(1)
>     problem, but find that the result is somewhat hard to read.
>
>   - Fix the ".expect" files so that we can avoid all of these games.
>
> I actually like the last option most. I'll have a go at it and send this
> third version out in a bit.

I sent a reply[1] in the other thread explaining why I'm still leaning
toward `sed` to smooth over these minor differences rather than
churning the "expect" files, especially since the minor differences
are not significant to what is actually being tested. That said, I
won't stand in the way of such a patch to "fix" the "expect" files,
but it feels unnecessary.

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

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

On Thu, Dec 14, 2023 at 03:39:17AM -0500, Eric Sunshine wrote:
> On Thu, Dec 14, 2023 at 3:13 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Wed, Dec 13, 2023 at 10:33:00PM -0500, Eric Sunshine wrote:
> > > On Wed, Dec 13, 2023 at 10:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > > Mostly because your "differences in features supported by just-built
> > > > one and what happens to be on $PATH can cause problems" cuts both
> > > > ways [...]
> > >
> > > I sent an alternative solution[1] which should sidestep this objection.
> >
> > Thanks, I've replied to the thread. I think by now there are three
> > different ideas:
> >
> >   - Improve the logic to pick some kind of diff implementation, which is
> >     my patch series. It would need to be improved so that we also probe
> >     whether the respective Git executables actually understand the repo
> >     format so that we can fall back from the just-built Git to system's
> >     Git.
> >
> >   - Munge the whitespace of the expected results with some regexes.
> >     I like that idea better because we can avoid the git-diff(1)
> >     problem, but find that the result is somewhat hard to read.
> >
> >   - Fix the ".expect" files so that we can avoid all of these games.
> >
> > I actually like the last option most. I'll have a go at it and send this
> > third version out in a bit.
> 
> I sent a reply[1] in the other thread explaining why I'm still leaning
> toward `sed` to smooth over these minor differences rather than
> churning the "expect" files, especially since the minor differences
> are not significant to what is actually being tested. That said, I
> won't stand in the way of such a patch to "fix" the "expect" files,
> but it feels unnecessary.
> 
> [1]: https://lore.kernel.org/git/CAPig+cSkuRfkR2D3JqYcbaJqj485nfD9Nq6pM=vXWB5DJenWpA@mail.gmail.com/

Yeah, our mails indeed crossed. I personally do not mind much which of
our patches land upstream and would be happy with either.

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>   - Fix the ".expect" files so that we can avoid all of these games.
>>
>> I actually like the last option most. I'll have a go at it and send this
>> third version out in a bit.
>
> I sent a reply[1] in the other thread explaining why I'm still leaning
> toward `sed` to smooth over these minor differences rather than
> churning the "expect" files, especially since the minor differences
> are not significant to what is actually being tested.

If it is just one time bulk conversion under t/chainlint/ to match
what the chainlint.pl script produces, with the possibility of
similar bulk updates in the future when the script gets updated, I
tend to agree with Patrick that getting rid of the fuzzy comparison
will be the best way forward.

Especially if the fuzzy comparison is done only to hide differences
between what the old chainlint.sed used to produce and what the
current version produces, that is.  If for some reason the script
started to create subtly different output for other reasons (e.g.,
it may produce different whitespaces on a particular platform, or
with a specific version of perl interpreter), we'd better be aware
of it, instead of blindly ignoring the differences without
inspecting them and verifying that they are benign.

By going through the single conversion pain, it will force us to
think twice before breaking its output stability while updating
chainlint.pl, which would also be a good thing.

THanks.

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Eric Sunshine]

On Thu, Dec 14, 2023 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > I sent a reply[1] in the other thread explaining why I'm still leaning
> > toward `sed` to smooth over these minor differences rather than
> > churning the "expect" files, especially since the minor differences
> > are not significant to what is actually being tested.
>
> If it is just one time bulk conversion under t/chainlint/ to match
> what the chainlint.pl script produces, with the possibility of
> similar bulk updates in the future when the script gets updated, I
> tend to agree with Patrick that getting rid of the fuzzy comparison
> will be the best way forward.

Okay, that's fine. If we take this approach, though, then it would
make sense to eliminate _all_ gratuitous postprocessing of the
"expect" files[1] so that we really are comparing the direct output of
chainlint.pl with the "expect" files, rather than merely munging the
inline whitespace of the "expect" files slightly as Patrick's proposed
patch does[2].

(The only postprocessing of "expect" files which needs to stay is the
bit which removes the "# LINT:" comments which litter the "expect"
files explaining to human readers why the linter should insert a
"???FOO???" annotation at that particular point.)

> Especially if the fuzzy comparison is done only to hide differences
> between what the old chainlint.sed used to produce and what the
> current version produces, that is.  If for some reason the script
> started to create subtly different output for other reasons (e.g.,
> it may produce different whitespaces on a particular platform, or
> with a specific version of perl interpreter), we'd better be aware
> of it, instead of blindly ignoring the differences without
> inspecting them and verifying that they are benign.
>
> By going through the single conversion pain, it will force us to
> think twice before breaking its output stability while updating
> chainlint.pl, which would also be a good thing.

The chainlint self-tests were never meant to be about its general
output stability. They were intended to ensure that the "???FOO???"
annotations are: (1) indeed inserted for the set of linting problems
the tool detects, and (2) inserted at the correct spot in the emitted
output relative to the shell tokens to which the annotation applies.
Minor differences in the tool's output (whether over time or between
platforms) should be immaterial in respect to those correctness goals.

[1]: https://lore.kernel.org/git/CAPig+cTZmiXdPZEVO-F2UzV9YaP6c7r2MfPTC3QWksJa+rM7VA@mail.gmail.com/
[2]: https://lore.kernel.org/git/aec86a15c69aa276eee4875fad208ee2fc57635a.1702542564.git.ps@pks.im/

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> (The only postprocessing of "expect" files which needs to stay is the
> bit which removes the "# LINT:" comments which litter the "expect"
> files explaining to human readers why the linter should insert a
> "???FOO???" annotation at that particular point.)

Yup, that matches my understanding.

> The chainlint self-tests were never meant to be about its general
> output stability. They were intended to ensure that the "???FOO???"
> annotations are: (1) indeed inserted for the set of linting problems
> the tool detects, and (2) inserted at the correct spot in the emitted
> output relative to the shell tokens to which the annotation applies.

Yup.

> Minor differences in the tool's output (whether over time or between
> platforms) should be immaterial in respect to those correctness goals.

But there is no reason to make such immaterial changes to the output
gratuitously when we are updating the tool to improve it, no?

Re: [PATCH] tests: prefer host Git to verify chainlint self-checks

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Thu, Dec 14, 2023 at 01:10:48PM -0500, Eric Sunshine wrote:
> On Thu, Dec 14, 2023 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > I sent a reply[1] in the other thread explaining why I'm still leaning
> > > toward `sed` to smooth over these minor differences rather than
> > > churning the "expect" files, especially since the minor differences
> > > are not significant to what is actually being tested.
> >
> > If it is just one time bulk conversion under t/chainlint/ to match
> > what the chainlint.pl script produces, with the possibility of
> > similar bulk updates in the future when the script gets updated, I
> > tend to agree with Patrick that getting rid of the fuzzy comparison
> > will be the best way forward.
> 
> Okay, that's fine. If we take this approach, though, then it would
> make sense to eliminate _all_ gratuitous postprocessing of the
> "expect" files[1] so that we really are comparing the direct output of
> chainlint.pl with the "expect" files, rather than merely munging the
> inline whitespace of the "expect" files slightly as Patrick's proposed
> patch does[2].
> 
> (The only postprocessing of "expect" files which needs to stay is the
> bit which removes the "# LINT:" comments which litter the "expect"
> files explaining to human readers why the linter should insert a
> "???FOO???" annotation at that particular point.)

Okay. I'll send a v3 to also drop the other post-processing steps then.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] tests: adjust whitespace in chainlint expectations

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 14057 bytes --]

The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.

The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.

To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.

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>
---
 t/Makefile                                    | 10 +-------
 t/chainlint/blank-line-before-esac.expect     |  8 +++----
 t/chainlint/block.expect                      |  4 ++--
 t/chainlint/chain-break-background.expect     |  4 ++--
 t/chainlint/chain-break-return-exit.expect    | 14 +++++------
 t/chainlint/chain-break-status.expect         |  4 ++--
 t/chainlint/chained-subshell.expect           |  4 ++--
 .../command-substitution-subsubshell.expect   |  2 +-
 t/chainlint/dqstring-line-splice.expect       |  4 ++--
 t/chainlint/dqstring-no-interpolate.expect    |  4 ++--
 t/chainlint/empty-here-doc.expect             |  4 ++--
 t/chainlint/exclamation.expect                |  2 +-
 t/chainlint/for-loop-abbreviated.expect       |  2 +-
 t/chainlint/function.expect                   |  4 ++--
 t/chainlint/here-doc.expect                   |  4 ++--
 t/chainlint/loop-detect-status.expect         | 20 ++++++++--------
 t/chainlint/nested-loop-detect-failure.expect | 24 +++++++++----------
 t/chainlint/subshell-here-doc.expect          |  4 ++--
 t/chainlint/token-pasting.expect              | 14 +++++------
 19 files changed, 64 insertions(+), 72 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..3a5d81b7fe 100644
--- 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
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
diff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect
index 48ed4eb124..056e03003d 100644
--- a/t/chainlint/blank-line-before-esac.expect
+++ b/t/chainlint/blank-line-before-esac.expect
@@ -1,11 +1,11 @@
-test_done ( ) {
+test_done () {
 	case "$test_failure" in
-	0 )
+	0)
 		test_at_end_hook_
 
 		exit 0 ;;
 
-	* )
+	*)
 		if test $test_external_has_tap -eq 0
 		then
 			say_color error "# failed $test_failure among $msg"
@@ -14,5 +14,5 @@ test_done ( ) {
 
 		exit 1 ;;
 
-		esac
+	esac
 }
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index a3bcea492a..1c87326364 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -12,9 +12,9 @@
 ) &&
 
 {
-	echo a ; ?!AMP?! echo b
+	echo a; ?!AMP?! echo b
 } &&
-{ echo a ; ?!AMP?! echo b ; } &&
+{ echo a; ?!AMP?! echo b; } &&
 
 {
 	echo "${var}9" &&
diff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect
index 28f9114f42..20d0bb5333 100644
--- a/t/chainlint/chain-break-background.expect
+++ b/t/chainlint/chain-break-background.expect
@@ -1,9 +1,9 @@
 JGIT_DAEMON_PID= &&
 git init --bare empty.git &&
-> empty.git/git-daemon-export-ok &&
+>empty.git/git-daemon-export-ok &&
 mkfifo jgit_daemon_output &&
 {
-	jgit daemon --port="$JGIT_DAEMON_PORT" . > jgit_daemon_output &
+	jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
 	JGIT_DAEMON_PID=$!
 } &&
 test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
index 1732d221c3..4cd18e2edf 100644
--- a/t/chainlint/chain-break-return-exit.expect
+++ b/t/chainlint/chain-break-return-exit.expect
@@ -1,16 +1,16 @@
 case "$(git ls-files)" in
-one ) echo pass one ;;
-* ) echo bad one ; return 1 ;;
+one) echo pass one ;;
+*) echo bad one; return 1 ;;
 esac &&
 (
 	case "$(git ls-files)" in
-	two ) echo pass two ;;
-	* ) echo bad two ; exit 1 ;;
-esac
+	two) echo pass two ;;
+	*) echo bad two; exit 1 ;;
+	esac
 ) &&
 case "$(git ls-files)" in
-dir/two"$LF"one ) echo pass both ;;
-* ) echo bad ; return 1 ;;
+dir/two"$LF"one) echo pass both ;;
+*) echo bad; return 1 ;;
 esac &&
 
 for i in 1 2 3 4 ; do
diff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect
index f4bada9463..e6b3b2193e 100644
--- a/t/chainlint/chain-break-status.expect
+++ b/t/chainlint/chain-break-status.expect
@@ -1,7 +1,7 @@
-OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT" &&
 
-{ test-tool sigchain > actual ; ret=$? ; } &&
+{ test-tool sigchain >actual; ret=$?; } &&
 {
 	test_match_signal 15 "$ret" ||
 	test "$ret" = 3
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index af0369d328..83810ea7ec 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -4,7 +4,7 @@ mkdir sub && (
 	nuff said
 ) &&
 
-cut "-d " -f actual | ( read s1 s2 s3 &&
+cut "-d " -f actual | (read s1 s2 s3 &&
 test -f $s1 ?!AMP?!
 test $(cat $s2) = tree2path1 &&
-test $(cat $s3) = tree3path1 )
+test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect
index ab2f79e845..ec42f2c30c 100644
--- a/t/chainlint/command-substitution-subsubshell.expect
+++ b/t/chainlint/command-substitution-subsubshell.expect
@@ -1,2 +1,2 @@
-OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT"
diff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect
index bf9ced60d4..f8fa3bc969 100644
--- a/t/chainlint/dqstring-line-splice.expect
+++ b/t/chainlint/dqstring-line-splice.expect
@@ -1,3 +1,3 @@
-echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&
-test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&
+echo 'fatal: reword option of --fixup is mutually exclusive with'	'--patch/--interactive/--all/--include/--only' >expect &&
+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
 test_cmp expect actual
diff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect
index 10724987a5..2c1851a3c3 100644
--- a/t/chainlint/dqstring-no-interpolate.expect
+++ b/t/chainlint/dqstring-no-interpolate.expect
@@ -6,6 +6,6 @@ grep "^\.git$" output.txt &&
 (
 	cd client$version &&
 	GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)
-) > output &&
-	cut -d ' ' -f 2 < output | sort > actual &&
+) >output &&
+	cut -d ' ' -f 2 <output | sort >actual &&
 	test_cmp expect actual
diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index e8733c97c6..8507721192 100644
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,4 +1,4 @@
-git ls-tree $tree path > current &&
-cat > expected <<\EOF &&
+git ls-tree $tree path >current &&
+cat >expected <<\EOF &&
 EOF
 test_output
diff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect
index 2d961a58c6..765a35bb4c 100644
--- a/t/chainlint/exclamation.expect
+++ b/t/chainlint/exclamation.expect
@@ -1,4 +1,4 @@
-if ! condition ; then echo nope ; else yep ; fi &&
+if ! condition; then echo nope; else yep; fi &&
 test_prerequisite !MINGW &&
 mail uucp!address &&
 echo !whatever!
diff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect
index a21007a63f..02c0d15cca 100644
--- a/t/chainlint/for-loop-abbreviated.expect
+++ b/t/chainlint/for-loop-abbreviated.expect
@@ -1,5 +1,5 @@
 for it
 do
-	path=$(expr "$it" : ( [^:]*) ) &&
+	path=$(expr "$it" : ([^:]*)) &&
 	git update-index --add "$path" || exit
 done
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index a14388e6b9..dd7c997a3c 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -1,8 +1,8 @@
-sha1_file ( ) {
+sha1_file() {
 	echo "$*" | sed "s#..#.git/objects/&/#"
 } &&
 
-remove_object ( ) {
+remove_object() {
 	file=$(sha1_file "$*") &&
 	test -e "$file" ?!AMP?!
 	rm -f "$file"
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 1df3f78282..91b961242a 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,6 +1,6 @@
 boodle wobba \
-	gorgo snoot \
-	wafta snurb <<EOF &&
+       gorgo snoot \
+       wafta snurb <<EOF &&
 quoth the raven,
 nevermore...
 EOF
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 24da9e86d5..7ce3a34806 100644
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -1,18 +1,18 @@
-( while test $i -le $blobcount
-do
-	printf "Generating blob $i/$blobcount\r" >& 2 &&
+(while test $i -le $blobcount
+ do
+	printf "Generating blob $i/$blobcount\r" >&2 &&
 	printf "blob\nmark :$i\ndata $blobsize\n" &&
 	#test-tool genrandom $i $blobsize &&
 	printf "%-${blobsize}s" $i &&
 	echo "M 100644 :$i $i" >> commit &&
 	i=$(($i+1)) ||
 	echo $? > exit-status
-done &&
-echo "commit refs/heads/main" &&
-echo "author A U Thor <author@email.com> 123456789 +0000" &&
-echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
-echo "data 5" &&
-echo ">2gb" &&
-cat commit ) |
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
 git fast-import --big-file-threshold=2 &&
 test ! -f exit-status
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
index 4793a0e8e1..3461df40e5 100644
--- a/t/chainlint/nested-loop-detect-failure.expect
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -1,31 +1,31 @@
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done ?!LOOP?!
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done || return 1
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done || return 1
 done
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 52789278d1..75d6f607e2 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,7 +1,7 @@
 (
 	echo wobba \
-		gorgo snoot \
-		wafta snurb <<-EOF &&
+	       gorgo snoot \
+	       wafta snurb <<-EOF &&
 	quoth the raven,
 	nevermore...
 	EOF
diff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect
index 342360bcd0..6a387917a7 100644
--- a/t/chainlint/token-pasting.expect
+++ b/t/chainlint/token-pasting.expect
@@ -4,22 +4,22 @@ git config filter.rot13.clean ./rot13.sh &&
 {
     echo "*.t filter=rot13" ?!AMP?!
     echo "*.i ident"
-} > .gitattributes &&
+} >.gitattributes &&
 
 {
     echo a b c d e f g h i j k l m ?!AMP?!
     echo n o p q r s t u v w x y z ?!AMP?!
     echo '$Id$'
-} > test &&
-cat test > test.t &&
-cat test > test.o &&
-cat test > test.i &&
+} >test &&
+cat test >test.t &&
+cat test >test.o &&
+cat test >test.i &&
 git add test test.t test.i &&
 rm -f test test.t test.i &&
 git checkout -- test test.t test.i &&
 
-echo "content-test2" > test2.o &&
-echo "content-test3 - filename with special characters" > "test3 'sq',$x=.o" ?!AMP?!
+echo "content-test2" >test2.o &&
+echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!
 
 downstream_url_for_sed=$(
 	printf "%sn" "$downstream_url" |
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Sunshine]

On Thu, Dec 14, 2023 at 3:30 AM Patrick Steinhardt <ps@pks.im> wrote:
> [...]
> 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/

[PATCH v3] tests: adjust whitespace in chainlint expectations

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 19823 bytes --]

The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.

The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.

To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/Makefile                                    | 12 ++-------
 t/chainlint/blank-line-before-esac.expect     | 12 +++------
 t/chainlint/block.expect                      |  6 ++---
 t/chainlint/chain-break-background.expect     |  4 +--
 t/chainlint/chain-break-continue.expect       |  1 -
 t/chainlint/chain-break-return-exit.expect    | 15 +++++------
 t/chainlint/chain-break-status.expect         |  5 ++--
 t/chainlint/chained-block.expect              |  1 -
 t/chainlint/chained-subshell.expect           |  5 ++--
 .../command-substitution-subsubshell.expect   |  2 +-
 t/chainlint/cuddled.expect                    |  4 ---
 t/chainlint/dqstring-line-splice.expect       |  4 +--
 t/chainlint/dqstring-no-interpolate.expect    |  7 ++---
 t/chainlint/empty-here-doc.expect             |  4 +--
 t/chainlint/exclamation.expect                |  2 +-
 t/chainlint/for-loop-abbreviated.expect       |  2 +-
 t/chainlint/function.expect                   |  6 ++---
 t/chainlint/here-doc-indent-operator.expect   |  2 --
 t/chainlint/here-doc.expect                   |  7 ++---
 t/chainlint/inline-comment.expect             |  1 -
 t/chainlint/loop-detect-failure.expect        |  1 -
 t/chainlint/loop-detect-status.expect         | 20 +++++++-------
 t/chainlint/negated-one-liner.expect          |  1 -
 t/chainlint/nested-here-doc.expect            |  3 ---
 t/chainlint/nested-loop-detect-failure.expect | 27 +++++++++----------
 t/chainlint/not-heredoc.expect                |  1 -
 t/chainlint/one-liner.expect                  |  2 --
 t/chainlint/subshell-here-doc.expect          |  6 ++---
 t/chainlint/token-pasting.expect              | 18 +++++--------
 29 files changed, 65 insertions(+), 116 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..ab620cf732 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,20 +103,12 @@ check-chainlint:
 		echo "# chainlint: $(CHAINLINTTMP_SQ)/tests" && \
 		for i in $(CHAINLINTTESTS); do \
 			echo "# chainlint: $$i" && \
-			sed -e '/^[ 	]*$$/d' chainlint/$$i.expect; \
+			cat chainlint/$$i.expect; \
 		done \
 	} >'$(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
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
diff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect
index 48ed4eb124..7400a1df7e 100644
--- a/t/chainlint/blank-line-before-esac.expect
+++ b/t/chainlint/blank-line-before-esac.expect
@@ -1,18 +1,14 @@
-test_done ( ) {
+test_done () {
 	case "$test_failure" in
-	0 )
+	0)
 		test_at_end_hook_
-
 		exit 0 ;;
-
-	* )
+	*)
 		if test $test_external_has_tap -eq 0
 		then
 			say_color error "# failed $test_failure among $msg"
 			say "1..$test_count"
 		fi
-
 		exit 1 ;;
-
-		esac
+	esac
 }
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index a3bcea492a..569211de08 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -10,12 +10,10 @@
 	} ?!AMP?!
 	baz
 ) &&
-
 {
-	echo a ; ?!AMP?! echo b
+	echo a; ?!AMP?! echo b
 } &&
-{ echo a ; ?!AMP?! echo b ; } &&
-
+{ echo a; ?!AMP?! echo b; } &&
 {
 	echo "${var}9" &&
 	echo "done"
diff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect
index 28f9114f42..20d0bb5333 100644
--- a/t/chainlint/chain-break-background.expect
+++ b/t/chainlint/chain-break-background.expect
@@ -1,9 +1,9 @@
 JGIT_DAEMON_PID= &&
 git init --bare empty.git &&
-> empty.git/git-daemon-export-ok &&
+>empty.git/git-daemon-export-ok &&
 mkfifo jgit_daemon_output &&
 {
-	jgit daemon --port="$JGIT_DAEMON_PORT" . > jgit_daemon_output &
+	jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
 	JGIT_DAEMON_PID=$!
 } &&
 test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
diff --git a/t/chainlint/chain-break-continue.expect b/t/chainlint/chain-break-continue.expect
index 47a3457710..fa00c9fdda 100644
--- a/t/chainlint/chain-break-continue.expect
+++ b/t/chainlint/chain-break-continue.expect
@@ -4,7 +4,6 @@ do
 	test "$path" = "foobar/non-note.txt" && continue
 	test "$path" = "deadbeef" && continue
 	test "$path" = "de/adbeef" && continue
-
 	if test $(expr length "$path") -ne $hexsz
 	then
 		return 1
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
index 1732d221c3..bedc0711e4 100644
--- a/t/chainlint/chain-break-return-exit.expect
+++ b/t/chainlint/chain-break-return-exit.expect
@@ -1,18 +1,17 @@
 case "$(git ls-files)" in
-one ) echo pass one ;;
-* ) echo bad one ; return 1 ;;
+one) echo pass one ;;
+*) echo bad one; return 1 ;;
 esac &&
 (
 	case "$(git ls-files)" in
-	two ) echo pass two ;;
-	* ) echo bad two ; exit 1 ;;
-esac
+	two) echo pass two ;;
+	*) echo bad two; exit 1 ;;
+	esac
 ) &&
 case "$(git ls-files)" in
-dir/two"$LF"one ) echo pass both ;;
-* ) echo bad ; return 1 ;;
+dir/two"$LF"one) echo pass both ;;
+*) echo bad; return 1 ;;
 esac &&
-
 for i in 1 2 3 4 ; do
 	git checkout main -b $i || return $?
 	test_commit $i $i $i tag$i || return $?
diff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect
index f4bada9463..53b3f42e7a 100644
--- a/t/chainlint/chain-break-status.expect
+++ b/t/chainlint/chain-break-status.expect
@@ -1,7 +1,6 @@
-OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT" &&
-
-{ test-tool sigchain > actual ; ret=$? ; } &&
+{ test-tool sigchain >actual; ret=$?; } &&
 {
 	test_match_signal 15 "$ret" ||
 	test "$ret" = 3
diff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect
index 574cdceb07..2416e0fc80 100644
--- a/t/chainlint/chained-block.expect
+++ b/t/chainlint/chained-block.expect
@@ -2,7 +2,6 @@ echo nobody home && {
 	test the doohicky ?!AMP?!
 	right now
 } &&
-
 GIT_EXTERNAL_DIFF=echo git diff | {
 	read path oldfile oldhex oldmode newfile newhex newmode &&
 	test "z$oh" = "z$oldhex"
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index af0369d328..2a44845759 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -3,8 +3,7 @@ mkdir sub && (
 	foo the bar ?!AMP?!
 	nuff said
 ) &&
-
-cut "-d " -f actual | ( read s1 s2 s3 &&
+cut "-d " -f actual | (read s1 s2 s3 &&
 test -f $s1 ?!AMP?!
 test $(cat $s2) = tree2path1 &&
-test $(cat $s3) = tree3path1 )
+test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect
index ab2f79e845..ec42f2c30c 100644
--- a/t/chainlint/command-substitution-subsubshell.expect
+++ b/t/chainlint/command-substitution-subsubshell.expect
@@ -1,2 +1,2 @@
-OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT"
diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect
index c3e0be4047..9558362917 100644
--- a/t/chainlint/cuddled.expect
+++ b/t/chainlint/cuddled.expect
@@ -1,17 +1,13 @@
 (cd foo &&
 	bar
 ) &&
-
 (cd foo ?!AMP?!
 	bar
 ) &&
-
 (
 	cd foo &&
 	bar) &&
-
 (cd foo &&
 	bar) &&
-
 (cd foo ?!AMP?!
 	bar)
diff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect
index bf9ced60d4..f8fa3bc969 100644
--- a/t/chainlint/dqstring-line-splice.expect
+++ b/t/chainlint/dqstring-line-splice.expect
@@ -1,3 +1,3 @@
-echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&
-test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&
+echo 'fatal: reword option of --fixup is mutually exclusive with'	'--patch/--interactive/--all/--include/--only' >expect &&
+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
 test_cmp expect actual
diff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect
index 10724987a5..bd60121b47 100644
--- a/t/chainlint/dqstring-no-interpolate.expect
+++ b/t/chainlint/dqstring-no-interpolate.expect
@@ -1,11 +1,8 @@
 grep "^ ! [rejected][ ]*$BRANCH -> $BRANCH (non-fast-forward)$" out &&
-
 grep "^\.git$" output.txt &&
-
-
 (
 	cd client$version &&
 	GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)
-) > output &&
-	cut -d ' ' -f 2 < output | sort > actual &&
+) >output &&
+	cut -d ' ' -f 2 <output | sort >actual &&
 	test_cmp expect actual
diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index e8733c97c6..8507721192 100644
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,4 +1,4 @@
-git ls-tree $tree path > current &&
-cat > expected <<\EOF &&
+git ls-tree $tree path >current &&
+cat >expected <<\EOF &&
 EOF
 test_output
diff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect
index 2d961a58c6..765a35bb4c 100644
--- a/t/chainlint/exclamation.expect
+++ b/t/chainlint/exclamation.expect
@@ -1,4 +1,4 @@
-if ! condition ; then echo nope ; else yep ; fi &&
+if ! condition; then echo nope; else yep; fi &&
 test_prerequisite !MINGW &&
 mail uucp!address &&
 echo !whatever!
diff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect
index a21007a63f..02c0d15cca 100644
--- a/t/chainlint/for-loop-abbreviated.expect
+++ b/t/chainlint/for-loop-abbreviated.expect
@@ -1,5 +1,5 @@
 for it
 do
-	path=$(expr "$it" : ( [^:]*) ) &&
+	path=$(expr "$it" : ([^:]*)) &&
 	git update-index --add "$path" || exit
 done
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index a14388e6b9..826b2ccc95 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -1,11 +1,9 @@
-sha1_file ( ) {
+sha1_file() {
 	echo "$*" | sed "s#..#.git/objects/&/#"
 } &&
-
-remove_object ( ) {
+remove_object() {
 	file=$(sha1_file "$*") &&
 	test -e "$file" ?!AMP?!
 	rm -f "$file"
 } ?!AMP?!
-
 sha1_file arg && remove_object arg
diff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect
index f92a7ce999..bbaad024d2 100644
--- a/t/chainlint/here-doc-indent-operator.expect
+++ b/t/chainlint/here-doc-indent-operator.expect
@@ -3,9 +3,7 @@ header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
 num_commits: $1
 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data
 EOF
-
 cat >expect << -EOF ?!AMP?!
 this is not indented
 -EOF
-
 cleanup
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 1df3f78282..9e612ac8b1 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,22 +1,19 @@
 boodle wobba \
-	gorgo snoot \
-	wafta snurb <<EOF &&
+       gorgo snoot \
+       wafta snurb <<EOF &&
 quoth the raven,
 nevermore...
 EOF
-
 cat <<-Arbitrary_Tag_42 >foo &&
 snoz
 boz
 woz
 Arbitrary_Tag_42
-
 cat <<"zump" >boo &&
 snoz
 boz
 woz
 zump
-
 horticulture <<\EOF
 gomez
 morticia
diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index 6bad218530..c7fc411f91 100644
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -3,6 +3,5 @@
 	barfoo ?!AMP?! # wrong position for &&
 	flibble "not a # comment"
 ) &&
-
 (cd foo &&
 	flibble "not a # comment")
diff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect
index a66025c39d..7a1a1ff0bf 100644
--- a/t/chainlint/loop-detect-failure.expect
+++ b/t/chainlint/loop-detect-failure.expect
@@ -5,7 +5,6 @@ do
 	git -C r1 add file.$n &&
 	git -C r1 commit -m "$n" || return 1
 done &&
-
 git init r2 &&
 for n in 1000 10000
 do
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 24da9e86d5..7ce3a34806 100644
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -1,18 +1,18 @@
-( while test $i -le $blobcount
-do
-	printf "Generating blob $i/$blobcount\r" >& 2 &&
+(while test $i -le $blobcount
+ do
+	printf "Generating blob $i/$blobcount\r" >&2 &&
 	printf "blob\nmark :$i\ndata $blobsize\n" &&
 	#test-tool genrandom $i $blobsize &&
 	printf "%-${blobsize}s" $i &&
 	echo "M 100644 :$i $i" >> commit &&
 	i=$(($i+1)) ||
 	echo $? > exit-status
-done &&
-echo "commit refs/heads/main" &&
-echo "author A U Thor <author@email.com> 123456789 +0000" &&
-echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
-echo "data 5" &&
-echo ">2gb" &&
-cat commit ) |
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
 git fast-import --big-file-threshold=2 &&
 test ! -f exit-status
diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect
index ad4c2d949e..7ae60eb770 100644
--- a/t/chainlint/negated-one-liner.expect
+++ b/t/chainlint/negated-one-liner.expect
@@ -1,5 +1,4 @@
 ! (foo && bar) &&
 ! (foo && bar) >baz &&
-
 ! (foo; ?!AMP?! bar) &&
 ! (foo; ?!AMP?! bar) >baz
diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index 29b3832a98..93eeded9f4 100644
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -6,7 +6,6 @@ fub <<EOF
 EOF
 formp
 ARBITRARY
-
 (
 	cat <<-\INPUT_END &&
 	fish are mice
@@ -17,7 +16,6 @@ ARBITRARY
 	EOF
 	toink
 	INPUT_END
-
 	cat <<-\EOT ?!AMP?!
 	text goes here
 	data <<EOF
@@ -25,6 +23,5 @@ ARBITRARY
 	EOF
 	more test here
 	EOT
-
 	foobar
 )
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
index 4793a0e8e1..211fdb7479 100644
--- a/t/chainlint/nested-loop-detect-failure.expect
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -1,31 +1,28 @@
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done ?!LOOP?!
 done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done
 done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done || return 1
 done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done || return 1
 done
diff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect
index 2e9bb135fe..47311f4c2d 100644
--- a/t/chainlint/not-heredoc.expect
+++ b/t/chainlint/not-heredoc.expect
@@ -3,7 +3,6 @@ echo ourside &&
 echo "=======" &&
 echo theirside &&
 echo ">>>>>>> theirs" &&
-
 (
 	echo "<<<<<<< ours" &&
 	echo ourside &&
diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect
index 57a7a444c1..cdb45231f1 100644
--- a/t/chainlint/one-liner.expect
+++ b/t/chainlint/one-liner.expect
@@ -1,9 +1,7 @@
 (foo && bar) &&
 (foo && bar) |
 (foo && bar) >baz &&
-
 (foo; ?!AMP?! bar) &&
 (foo; ?!AMP?! bar) |
 (foo; ?!AMP?! bar) >baz &&
-
 (foo "bar; baz")
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 52789278d1..fa7cb0288a 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,15 +1,13 @@
 (
 	echo wobba \
-		gorgo snoot \
-		wafta snurb <<-EOF &&
+	       gorgo snoot \
+	       wafta snurb <<-EOF &&
 	quoth the raven,
 	nevermore...
 	EOF
-
 	cat <<EOF >bip ?!AMP?!
 	fish fly high
 EOF
-
 	echo <<-\EOF >bop
 	gomez
 	morticia
diff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect
index 342360bcd0..cda7d54037 100644
--- a/t/chainlint/token-pasting.expect
+++ b/t/chainlint/token-pasting.expect
@@ -1,26 +1,22 @@
 git config filter.rot13.smudge ./rot13.sh &&
 git config filter.rot13.clean ./rot13.sh &&
-
 {
     echo "*.t filter=rot13" ?!AMP?!
     echo "*.i ident"
-} > .gitattributes &&
-
+} >.gitattributes &&
 {
     echo a b c d e f g h i j k l m ?!AMP?!
     echo n o p q r s t u v w x y z ?!AMP?!
     echo '$Id$'
-} > test &&
-cat test > test.t &&
-cat test > test.o &&
-cat test > test.i &&
+} >test &&
+cat test >test.t &&
+cat test >test.o &&
+cat test >test.i &&
 git add test test.t test.i &&
 rm -f test test.t test.i &&
 git checkout -- test test.t test.i &&
-
-echo "content-test2" > test2.o &&
-echo "content-test3 - filename with special characters" > "test3 'sq',$x=.o" ?!AMP?!
-
+echo "content-test2" >test2.o &&
+echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!
 downstream_url_for_sed=$(
 	printf "%sn" "$downstream_url" |
 	sed -e 's/\/\\/g' -e 's/[[/.*^$]/\&/g'
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Sunshine]

On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@pks.im> 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.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> 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 && \

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

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@pks.im> 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@pks.im>
> > ---
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Sunshine]

On Fri, Dec 15, 2023 at 1:29 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote:
> > 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?

Oh, yes, of course. Please don't read my aside-comment as a suggestion
that you should tackle such a change or that there is any urgent need
to change how this all works. The currently proposed simple
solution(s) to allow you to get past this issue are perfectly
acceptable.

(As a further aside, just for completeness since I already mentioned
part of his plan, Ævar also really intended that the test scripts
themselves, "t/t????-*.sh", should be run individually through
chainlint.pl by the Makefile rather than sending all of them through
it in one go, thus once again taking advantage of `make`'s innate
parallelism, and only checking for &&-chain breakage if a test
script's timestamp has actually changed rather than checking all test
scripts unconditionally on every `make test`[1,2,3].)

[1]: https://lore.kernel.org/git/220901.86bkrzjm6e.gmgdl@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/221122.86cz9fbyln.gmgdl@evledraar.gmail.com/
[3]: https://github.com/avar/git/commit/ff8b4a12b30ac5ca521a64e74b0fd968ab2d4585

[PATCH v4] tests: adjust whitespace in chainlint expectations

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 17894 bytes --]

The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.

The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.

To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.

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 keep some of the post-processing of `chainlint.pl` output
intact to strip leading line numbers generated by the script. 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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/Makefile                                    | 14 +++--------
 t/chainlint/blank-line-before-esac.expect     |  8 +++----
 t/chainlint/blank-line.expect                 |  4 ++++
 t/chainlint/block.expect                      |  4 ++--
 t/chainlint/chain-break-background.expect     |  4 ++--
 t/chainlint/chain-break-return-exit.expect    | 14 +++++------
 t/chainlint/chain-break-status.expect         |  4 ++--
 t/chainlint/chained-subshell.expect           |  4 ++--
 .../command-substitution-subsubshell.expect   |  2 +-
 t/chainlint/dqstring-line-splice.expect       |  6 +++--
 t/chainlint/dqstring-no-interpolate.expect    |  5 ++--
 t/chainlint/empty-here-doc.expect             |  4 ++--
 t/chainlint/exclamation.expect                |  2 +-
 t/chainlint/for-loop-abbreviated.expect       |  2 +-
 t/chainlint/for-loop.expect                   |  1 +
 t/chainlint/function.expect                   |  4 ++--
 t/chainlint/here-doc.expect                   |  4 ++--
 t/chainlint/loop-detect-status.expect         | 20 ++++++++--------
 t/chainlint/nested-cuddled-subshell.expect    |  6 +++++
 t/chainlint/nested-loop-detect-failure.expect | 24 +++++++++----------
 t/chainlint/nested-subshell.expect            |  1 +
 t/chainlint/pipe.expect                       |  2 ++
 t/chainlint/subshell-here-doc.expect          |  4 ++--
 t/chainlint/subshell-one-liner.expect         |  5 ++++
 t/chainlint/t7900-subtree.expect              |  1 +
 t/chainlint/token-pasting.expect              | 14 +++++------
 t/chainlint/while-loop.expect                 |  1 +
 27 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..b7a6fefe28 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,20 +103,12 @@ check-chainlint:
 		echo "# chainlint: $(CHAINLINTTMP_SQ)/tests" && \
 		for i in $(CHAINLINTTESTS); do \
 			echo "# chainlint: $$i" && \
-			sed -e '/^[ 	]*$$/d' chainlint/$$i.expect; \
+			cat chainlint/$$i.expect; \
 		done \
 	} >'$(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
+		sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
+	diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
diff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect
index 48ed4eb124..056e03003d 100644
--- a/t/chainlint/blank-line-before-esac.expect
+++ b/t/chainlint/blank-line-before-esac.expect
@@ -1,11 +1,11 @@
-test_done ( ) {
+test_done () {
 	case "$test_failure" in
-	0 )
+	0)
 		test_at_end_hook_
 
 		exit 0 ;;
 
-	* )
+	*)
 		if test $test_external_has_tap -eq 0
 		then
 			say_color error "# failed $test_failure among $msg"
@@ -14,5 +14,5 @@ test_done ( ) {
 
 		exit 1 ;;
 
-		esac
+	esac
 }
diff --git a/t/chainlint/blank-line.expect b/t/chainlint/blank-line.expect
index f76fde1ffb..b47827d749 100644
--- a/t/chainlint/blank-line.expect
+++ b/t/chainlint/blank-line.expect
@@ -1,4 +1,8 @@
 (
+
 	nothing &&
+
 	something
+
+
 )
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index a3bcea492a..1c87326364 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -12,9 +12,9 @@
 ) &&
 
 {
-	echo a ; ?!AMP?! echo b
+	echo a; ?!AMP?! echo b
 } &&
-{ echo a ; ?!AMP?! echo b ; } &&
+{ echo a; ?!AMP?! echo b; } &&
 
 {
 	echo "${var}9" &&
diff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect
index 28f9114f42..20d0bb5333 100644
--- a/t/chainlint/chain-break-background.expect
+++ b/t/chainlint/chain-break-background.expect
@@ -1,9 +1,9 @@
 JGIT_DAEMON_PID= &&
 git init --bare empty.git &&
-> empty.git/git-daemon-export-ok &&
+>empty.git/git-daemon-export-ok &&
 mkfifo jgit_daemon_output &&
 {
-	jgit daemon --port="$JGIT_DAEMON_PORT" . > jgit_daemon_output &
+	jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
 	JGIT_DAEMON_PID=$!
 } &&
 test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
index 1732d221c3..4cd18e2edf 100644
--- a/t/chainlint/chain-break-return-exit.expect
+++ b/t/chainlint/chain-break-return-exit.expect
@@ -1,16 +1,16 @@
 case "$(git ls-files)" in
-one ) echo pass one ;;
-* ) echo bad one ; return 1 ;;
+one) echo pass one ;;
+*) echo bad one; return 1 ;;
 esac &&
 (
 	case "$(git ls-files)" in
-	two ) echo pass two ;;
-	* ) echo bad two ; exit 1 ;;
-esac
+	two) echo pass two ;;
+	*) echo bad two; exit 1 ;;
+	esac
 ) &&
 case "$(git ls-files)" in
-dir/two"$LF"one ) echo pass both ;;
-* ) echo bad ; return 1 ;;
+dir/two"$LF"one) echo pass both ;;
+*) echo bad; return 1 ;;
 esac &&
 
 for i in 1 2 3 4 ; do
diff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect
index f4bada9463..e6b3b2193e 100644
--- a/t/chainlint/chain-break-status.expect
+++ b/t/chainlint/chain-break-status.expect
@@ -1,7 +1,7 @@
-OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT" &&
 
-{ test-tool sigchain > actual ; ret=$? ; } &&
+{ test-tool sigchain >actual; ret=$?; } &&
 {
 	test_match_signal 15 "$ret" ||
 	test "$ret" = 3
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index af0369d328..83810ea7ec 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -4,7 +4,7 @@ mkdir sub && (
 	nuff said
 ) &&
 
-cut "-d " -f actual | ( read s1 s2 s3 &&
+cut "-d " -f actual | (read s1 s2 s3 &&
 test -f $s1 ?!AMP?!
 test $(cat $s2) = tree2path1 &&
-test $(cat $s3) = tree3path1 )
+test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect
index ab2f79e845..ec42f2c30c 100644
--- a/t/chainlint/command-substitution-subsubshell.expect
+++ b/t/chainlint/command-substitution-subsubshell.expect
@@ -1,2 +1,2 @@
-OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&
 test_match_signal 13 "$OUT"
diff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect
index bf9ced60d4..37eab80738 100644
--- a/t/chainlint/dqstring-line-splice.expect
+++ b/t/chainlint/dqstring-line-splice.expect
@@ -1,3 +1,5 @@
-echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&
-test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&
+
+echo 'fatal: reword option of --fixup is mutually exclusive with'	'--patch/--interactive/--all/--include/--only' >expect &&
+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
 test_cmp expect actual
+
diff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect
index 10724987a5..087eda15e4 100644
--- a/t/chainlint/dqstring-no-interpolate.expect
+++ b/t/chainlint/dqstring-no-interpolate.expect
@@ -6,6 +6,7 @@ grep "^\.git$" output.txt &&
 (
 	cd client$version &&
 	GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)
-) > output &&
-	cut -d ' ' -f 2 < output | sort > actual &&
+) >output &&
+	cut -d ' ' -f 2 <output | sort >actual &&
 	test_cmp expect actual
+
diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index e8733c97c6..8507721192 100644
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,4 +1,4 @@
-git ls-tree $tree path > current &&
-cat > expected <<\EOF &&
+git ls-tree $tree path >current &&
+cat >expected <<\EOF &&
 EOF
 test_output
diff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect
index 2d961a58c6..765a35bb4c 100644
--- a/t/chainlint/exclamation.expect
+++ b/t/chainlint/exclamation.expect
@@ -1,4 +1,4 @@
-if ! condition ; then echo nope ; else yep ; fi &&
+if ! condition; then echo nope; else yep; fi &&
 test_prerequisite !MINGW &&
 mail uucp!address &&
 echo !whatever!
diff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect
index a21007a63f..02c0d15cca 100644
--- a/t/chainlint/for-loop-abbreviated.expect
+++ b/t/chainlint/for-loop-abbreviated.expect
@@ -1,5 +1,5 @@
 for it
 do
-	path=$(expr "$it" : ( [^:]*) ) &&
+	path=$(expr "$it" : ([^:]*)) &&
 	git update-index --add "$path" || exit
 done
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index d65c82129a..d2237f1e38 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -6,6 +6,7 @@
 		bar
 		EOF
 	done ?!AMP?!
+
 	for i in a b c; do
 		echo $i &&
 		cat $i ?!LOOP?!
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index a14388e6b9..dd7c997a3c 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -1,8 +1,8 @@
-sha1_file ( ) {
+sha1_file() {
 	echo "$*" | sed "s#..#.git/objects/&/#"
 } &&
 
-remove_object ( ) {
+remove_object() {
 	file=$(sha1_file "$*") &&
 	test -e "$file" ?!AMP?!
 	rm -f "$file"
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 1df3f78282..91b961242a 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,6 +1,6 @@
 boodle wobba \
-	gorgo snoot \
-	wafta snurb <<EOF &&
+       gorgo snoot \
+       wafta snurb <<EOF &&
 quoth the raven,
 nevermore...
 EOF
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 24da9e86d5..7ce3a34806 100644
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -1,18 +1,18 @@
-( while test $i -le $blobcount
-do
-	printf "Generating blob $i/$blobcount\r" >& 2 &&
+(while test $i -le $blobcount
+ do
+	printf "Generating blob $i/$blobcount\r" >&2 &&
 	printf "blob\nmark :$i\ndata $blobsize\n" &&
 	#test-tool genrandom $i $blobsize &&
 	printf "%-${blobsize}s" $i &&
 	echo "M 100644 :$i $i" >> commit &&
 	i=$(($i+1)) ||
 	echo $? > exit-status
-done &&
-echo "commit refs/heads/main" &&
-echo "author A U Thor <author@email.com> 123456789 +0000" &&
-echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
-echo "data 5" &&
-echo ">2gb" &&
-cat commit ) |
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
 git fast-import --big-file-threshold=2 &&
 test ! -f exit-status
diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect
index 2a86885ee6..3836049cc4 100644
--- a/t/chainlint/nested-cuddled-subshell.expect
+++ b/t/chainlint/nested-cuddled-subshell.expect
@@ -2,18 +2,24 @@
 	(cd foo &&
 		bar
 	) &&
+
 	(cd foo &&
 		bar
 	) ?!AMP?!
+
 	(
 		cd foo &&
 		bar) &&
+
 	(
 		cd foo &&
 		bar) ?!AMP?!
+
 	(cd foo &&
 		bar) &&
+
 	(cd foo &&
 		bar) ?!AMP?!
+
 	foobar
 )
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
index 4793a0e8e1..3461df40e5 100644
--- a/t/chainlint/nested-loop-detect-failure.expect
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -1,31 +1,31 @@
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done ?!LOOP?!
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" ?!LOOP?!
+		echo "$i$j" >"path$i$j" ?!LOOP?!
 	done || return 1
 done &&
 
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
 do
-	for j in 0 1 2 3 4 5 6 7 8 9 ;
+	for j in 0 1 2 3 4 5 6 7 8 9;
 	do
-		echo "$i$j" > "path$i$j" || return 1
+		echo "$i$j" >"path$i$j" || return 1
 	done || return 1
 done
diff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect
index 02e0a9f1bb..73ff28546a 100644
--- a/t/chainlint/nested-subshell.expect
+++ b/t/chainlint/nested-subshell.expect
@@ -4,6 +4,7 @@
 		echo a &&
 		echo b
 	) >file &&
+
 	cd foo &&
 	(
 		echo a ?!AMP?!
diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect
index 2cfc028297..811971b1a3 100644
--- a/t/chainlint/pipe.expect
+++ b/t/chainlint/pipe.expect
@@ -2,7 +2,9 @@
 	foo |
 	bar |
 	baz &&
+
 	fish |
 	cow ?!AMP?!
+
 	sunder
 )
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 52789278d1..75d6f607e2 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,7 +1,7 @@
 (
 	echo wobba \
-		gorgo snoot \
-		wafta snurb <<-EOF &&
+	       gorgo snoot \
+	       wafta snurb <<-EOF &&
 	quoth the raven,
 	nevermore...
 	EOF
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index b7015361bf..8f694990e8 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -2,13 +2,18 @@
 	(foo && bar) &&
 	(foo && bar) |
 	(foo && bar) >baz &&
+
 	(foo; ?!AMP?! bar) &&
 	(foo; ?!AMP?! bar) |
 	(foo; ?!AMP?! bar) >baz &&
+
 	(foo || exit 1) &&
 	(foo || exit 1) |
 	(foo || exit 1) >baz &&
+
 	(foo && bar) ?!AMP?!
+
 	(foo && bar; ?!AMP?! baz) ?!AMP?!
+
 	foobar
 )
diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index 71b3b3bc20..02f3129232 100644
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -15,6 +15,7 @@ main-sub4" &&
 $chkms
 TXT
 ) &&
+
 	subfiles=$(git ls-files) &&
 	check_equal "$subfiles" "$chkms
 $chks"
diff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect
index 342360bcd0..6a387917a7 100644
--- a/t/chainlint/token-pasting.expect
+++ b/t/chainlint/token-pasting.expect
@@ -4,22 +4,22 @@ git config filter.rot13.clean ./rot13.sh &&
 {
     echo "*.t filter=rot13" ?!AMP?!
     echo "*.i ident"
-} > .gitattributes &&
+} >.gitattributes &&
 
 {
     echo a b c d e f g h i j k l m ?!AMP?!
     echo n o p q r s t u v w x y z ?!AMP?!
     echo '$Id$'
-} > test &&
-cat test > test.t &&
-cat test > test.o &&
-cat test > test.i &&
+} >test &&
+cat test >test.t &&
+cat test >test.o &&
+cat test >test.i &&
 git add test test.t test.i &&
 rm -f test test.t test.i &&
 git checkout -- test test.t test.i &&
 
-echo "content-test2" > test2.o &&
-echo "content-test3 - filename with special characters" > "test3 'sq',$x=.o" ?!AMP?!
+echo "content-test2" >test2.o &&
+echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!
 
 downstream_url_for_sed=$(
 	printf "%sn" "$downstream_url" |
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index 1f5eaea0fd..06c1567f48 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -6,6 +6,7 @@
 		bar
 		EOF
 	done ?!AMP?!
+
 	while true; do
 		echo foo &&
 		cat bar ?!LOOP?!
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Sunshine]

On Fri, Dec 15, 2023 at 1:42 AM Patrick Steinhardt <ps@pks.im> 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 keep some of the post-processing of `chainlint.pl` output
> intact to strip leading line numbers generated by the script. 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.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -103,20 +103,12 @@ check-chainlint:
>                 for i in $(CHAINLINTTESTS); do \
>                         echo "# chainlint: $$i" && \
> -                       sed -e '/^[     ]*$$/d' chainlint/$$i.expect; \
> +                       cat chainlint/$$i.expect; \
>                 done \
>         $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
> -               sed -e 's/^[1-9][0-9]* //;/^[   ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
> +               sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
> +       diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual

Thanks, this version looks fine. FWIW, you may consider this:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Aside: I was rather surprised to see this output from git-am when applying v4:

    Applying: tests: adjust whitespace in chainlint expectations
    .git/rebase-apply/patch:205: new blank line at EOF.
    +
    .git/rebase-apply/patch:219: new blank line at EOF.
    +
    warning: 2 lines add whitespace errors.

But upon investigating the two "test" files in question,
dqstring-line-splice.test and dqstring-no-interpolate.test, I recalled
that I had to play tricks to escape the single-quote context created
by the Makefile when generating t/chainlinttmp/tests in order to allow
chainlint.pl to see a double-quoted string. So, the abovementioned
blank lines are indeed expected output from chainlint.pl given the
tricks played.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Aside: I was rather surprised to see this output from git-am when applying v4:
>
>     Applying: tests: adjust whitespace in chainlint expectations
>     .git/rebase-apply/patch:205: new blank line at EOF.
>     +
>     .git/rebase-apply/patch:219: new blank line at EOF.
>     +
>     warning: 2 lines add whitespace errors.
>
> But upon investigating the two "test" files in question,
> dqstring-line-splice.test and dqstring-no-interpolate.test, I recalled
> that I had to play tricks to escape the single-quote context created
> by the Makefile when generating t/chainlinttmp/tests in order to allow
> chainlint.pl to see a double-quoted string. So, the abovementioned
> blank lines are indeed expected output from chainlint.pl given the
> tricks played.

Thanks for being extra careful while reviewing.