Re: [PATCH 1/4] tests: run internal chain-linter under "make test"

From: Jeff King <peff@peff.net>
Date: Mar 29, 2023, 23:28 UTC
Message-ID: <20230329232804.GC2314218@coredump.intra.peff.net>
In-Reply-To: <xmqq7cuzd0cs.fsf@gitster.g>

On Wed, Mar 29, 2023 at 08:49:07AM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Now the comment in the Makefile still says (as seen in the context):
> >
> > 	# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
> > 	# checks all tests in all scripts via a single invocation, so tell individual
> > 	# scripts not to "chainlint" themselves
> >
> > But as the commit message notes that's not accurate anymore. We're *not*
> > telling them to turn off chainlint in its entirety, we're telling them
> > to only suppress the chainlint.pl part.
> 
> Correct.  To avoid a confusion we saw in the thread that led to this
> series, we need an explanation that clearly distinguishes the "exit
> 117" one and the "script that parses the shell" one.  If we consider
> that the name "chainlint" refers to the latter, the script, and
> re-read the three lines with that in mind, I think they are OK.  It
> would become even clearer if we insert four words, like so:
> 
> 	`test-chainlint' (which is ...) checks ... via a single
> 	invocation of the "chainlint" script, so tell ...

I had hoped that "chainlint" in that comment would remain sufficient, as the context implies that we're disabling the script. But it's easy enough to expand. I squashed this in:

diff --git a/t/Makefile b/t/Makefile
index 10881affdd0..3e00cdd801d 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -44,7 +44,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual
-# scripts not to "chainlint" themselves
+# scripts not to run the external "chainlint.pl" script themselves
 CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&
 
 all: $(DEFAULT_TEST_TARGET)

> > I haven't looked too deeply, but it seems that we should at least adjust
> > that comment in the Makefile, or perhaps we should rename this "eval
> > 117" thing to be "internal lint" or something, and not "chain lint", to
> > avoid conflating it with chainlint.pl itself.
> 
> I agree that it would help to clarify that we mean by "chainlint"
> the script that parses (or the act of running the script, when it is
> used as a verb), and the above may be a way to do so.  I however do
> not see the need to come up with a new name for the other one, until
> we have a way to toggle it enabled/disabled, which is not something
> the discussion in the other thread found necessary.

Yeah. This distinction is not something anybody who is running or
writing tests should have to worry about (and the fact that it did come
up was a bug that this patch is fixing). So adding an option like
"--no-chain-lint-internal" is just making things more confusing, and
nobody would use it. You'd need it only if you are working on
chainlint.pl itself (and comparing results with the internal linter or
something), and there you are better off running "perl chainlint.pl
t1234-foo.sh" itself.

-Peff