Re: [PATCH 1/2] Update shell tests to use semantic functions

[Junio C Hamano <gitster@pobox.com>]

"Mark C. Chu-Carroll via B4 Relay"
<devnull+markchucarroll.fastmail.com@kernel.org> writes:

> -		if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
> +		if test_have_prereq MINGW && test_path_is_file /proc/$shell_pid/winpid

This is Wrong.

First think what the point of "test_path_is_file" and other helpers
is.  Once you understood them, you would never write them as part of
conditional expressions like the above one.

A conditional statement like this "if" does *not* want to shout when
the test condition it used to switch between its "then" (and "else"
if it has it) is not satisfied.  If this "test -f" does not find the
named /proc/$shell_pid/winpid file, it does not mean that we
detected a breakage in the system that is being tested.

After looking at a handful of hunks from the top of this patch, it
seems there are many more similarly wrong conversions among them
than a few that are good, and I won't look at the remainder of the
[1/2] or [2/2].  Sifting the two large patches that lack sign-off
that span across many files and finding salvageable bits is not
something I would spend my weekend on.

This kind of conversion first needs to understand what each
invocation of "test -X" is really trying to check and why.
Brain-less mechanical conversion is not sufficient and will produce
a wrong conversion like the above one.