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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2023, 15:49 UTC
Message-ID: <xmqq7cuzd0cs.fsf@gitster.g>
In-Reply-To: <230329.86wn2zvo9u.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 9 quoted lines

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