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

From: Junio C Hamano <[email-masked]>
Date: Wed, 29 Mar 2023 08:49:07 -0700

Ævar Arnfjörð Bjarmason <[email-masked]> 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 ...

> So if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still
> turn all of it off, but an invokation of chainlint.pl for the scripts as
> a batch is no longer thought to make the "eval 117" trick redundant.

Yes, I think that is the idea reached by the discussion in the other
thread that led to this series.

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

Thanks.