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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 14, 2023, 18:10 UTC
Message-ID: <CAPig+cQvcSeSKVE=0kDyNiSztNAgVwhfAzoL5K7uYHEKe=0f_A@mail.gmail.com>
In-Reply-To: <xmqqbkaspxn6.fsf@gitster.g>

On Thu, Dec 14, 2023 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

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

Show 12 quoted lines

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