Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2024, 17:02 UTC
Message-ID: <xmqqo7buq6b8.fsf@gitster.g>
In-Reply-To: <84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 26 quoted lines

> One of our proposed microprojects is to convert instances of `test -e`
> and related functions to instead use `test_path_exists` or similar. This
> conversion is only feasible when `test -e` is not used as part of a
> control statement, as the replacement is used to _assert_ a condition
> instead of merely testing for it.
>
> Clarify the microproject's description accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  SoC-2024-Microprojects.md | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md
> index 644c0a6..782441f 100644
> --- a/SoC-2024-Microprojects.md
> +++ b/SoC-2024-Microprojects.md
> @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.
>  Find one test script that verifies the presence/absence of
>  files/directories with 'test -(e|f|d|...)' and replace them with the
>  appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper
> -functions.
> +functions. Note that this conversion does not directly apply to control
> +flow constructs like `if test -e ./path; then ...; fi` because the
> +replacements are intended to assert the condition instead of merely
> +testing for it.

Thanks for picking it up. Of course there is one case in which we should use test_path_* helpers to replace such an if...then...fi construct; e.g., c431a235 (t9146: replace test -d/-e/-f with appropriate test_path_is_* function, 2024-02-14) did exactly that.

I am not sure how best to express that in the already crowded description above, though. Rewriting the existing test this way

	Find one test script that uses 'test [!] -(e|f|d|...)' to
	assert the presence/absense of files/directories to make the
	test fail directly with the exit status of such "test"
	commands, and replace them with the appropriate helper
	functions like `test_path_is_file`, that give more
	informative error messages when they fail.

would exclude use of "test -e" as a conditional in control statements, so we could mention what c431a235 did as an exception to the rule, perhaps like

	Note that the above excludes "test -f" and friends used as a
	condition in control statements such as "if test -e path
	...", but as an exception, if such a "if" statement just
	open-codes what these helpers do, replacing it is warranted.

But that does not read very well, even to myself.  Sigh....

Thanks.