Re: [PATCH] add usage-strings ci check and amend remaining usage strings

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Cc: "Julia Lawall" <julia.lawall@inria.fr>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git <git@vger.kernel.org>
Subject: Re: [PATCH] add usage-strings ci check and amend remaining usage strings
Date: Fri, 25 Feb 2022 16:30:35 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20220222154700.33928-1-chakrabortyabhradeep79@gmail.com>

Hi,

On Tue, 22 Feb 2022, Abhradeep Chakraborty wrote:

> Julia Lawall wrote:
>
> > Of there are some cases that are useful to do statically, with only local
> > information, then using Coccinelle could be useful to get the problem out
> > of the way once and for all.  Coccinelle doesn't support much processing
> > of strings directly, but you can always write some python code to test the
> > contents of a string and to create a new one.
> >
> > Let me know if you want to try this.  You can also check, eg the demo
> > demos/pythontococci.cocci to see how to create code in a python script and
> > then use it in a normal SmPL rule.
> > ...
> > If the context that you are interested in is in a called function or is in
> > the calling context, then Coccinelle might not be the ideal choice.
> > Coccinelle works on one function at a time, so to do anything
> > interprocedural, you have to do some hacks.
>
> Though in this case, `parse-options.c check` method is better [...]

I fear that this is incorrect.

In general, it is my experience that it is a mistake any time a static
check is replaced by a runtime check.

I was ready to let it slide in this instance, but in this case I now have
proof that the `parse-options.c` check is worse than the originally
suggested `sed` chain.

That concrete proof is in the output of
https://github.com/git/git/actions/runs/1890665968, where the combination
of `ac/usage-string-fixups` and `jh/builtin-fsmonitor-part2` causes many,
many failures, but all of those failures have the same root cause: the
runtime check.

With the original `check-usage-strings.sh`, the user inspecting any
failure would see precisely what the issue is, in the `static-analysis`
job's logs. It would display something like this:

	HEAD:builtin/fsmonitor--daemon.c:1507:          N_("Max seconds to wait for background daemon startup")),

With v4 of the patch series, it does not spell out anything in
`static-analysis`. Instead, it causes 8 separate jobs to fail,
it causes failures not only in `t0012-help.sh` but also in
`t7519-status-fsmonitor.sh` and in `t7527-builtin-fsmonitor.sh`.

The purpose of t7519 and t7527 is _not_ to verify those usage strings,
though.

The worst part? Look at the relevant output of t0012 (see
https://github.com/git/git/runs/5312844492?check_suite_focus=true#step:5:4902):

	[...]
	++ git -C sub fsmonitor--daemon -h
	++ exit_code=128
	++ test 128 = 129
	++ echo 'test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h'
	test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h
	++ return 1
	error: last command exited with $?=1
	not ok 81 - fsmonitor--daemon can handle -h
	[...]

Do you see what usage string caused the failure? You can't. And that's
even by design:

	(
		GIT_CEILING_DIRECTORIES=$(pwd) &&
		export GIT_CEILING_DIRECTORIES &&
		test_expect_code 129 git -C sub $builtin -h >output 2>&1
	) &&
	test_i18ngrep usage output

The output is redirected, and since the runtime check added to
`parse-options.c` causes the exit code to be different from the expected
one, the output is never shown.

Arguably the most important job of a regression test is to help software
engineers to diagnose and fix the regression. As quickly and as
conveniently as possible. That means that it is not enough to point out
that there is a regression, the output should be as helpful and concise as
possible to facilitate fixing the problem.

In the above-mentioned case, it was neither as helpful nor as concise as
possible because in the test case that was supposed to identify the
problem, the actual error message was swallowed, and instead of causing
one test failure, it caused a whopping 42 test cases to fail (some of
which even show the error message, but that's not even the purpose of
those test cases).

Since the entire point of this here patch series is to help enforce Git's
rules regarding usage strings, it should expect things like the issue with
`fsmonitor--daemon` _and_ make it as painless to address such an issue.

I am afraid that I have to NAK the `parse-options.c` approach because v1
of this patch series did so much better a job.

Ciao,
Dscho