Re: [PATCH v2] t9146: replace test -d/-e/-f with appropriate test_path_is_* function

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2024, 20:31 UTC
Message-ID: <xmqq7cj95ssb.fsf@gitster.g>
In-Reply-To: <pull.1661.v2.git.1707765433663.gitgitgadget@gmail.com>

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> The helper functions test_path_is_* provide better debugging
> information than test -d/-e/-f.

Correct.

> Replace "if ! test -d then <error message>" with "test_path_exists"
> and "test -d" with "test_path_is_dir" at places where we check for
> existent directories.

The former could result in misconversion, if the intention of the test was "we cannot have directory here; a regular file is OK", so we have to be a bit more careful than mechanical conversion.

> Replace "test -f" with "test_path_is_file" at places where we check
> for existent files.

OK.

> Replace "test ! -e" with "test_path_is_missing" where we check for
> non-existent directories.

OK.

Show 8 quoted lines

>  		for i in a b c d d/e d/e/f "weird file name"
>  		do
> -			if ! test -d "$i"
> -			then
> -				echo >&2 "$i does not exist" &&
> -				exit 1
> -			fi
> +			test_path_exists "$i" || exit 1

We were saying that we are OK if "$i" existed as a file (not a directory), but now we complain regardless of what "$i" is. Is that closer to what the test originally wanted to do? Just checking.

Show 13 quoted lines

>  		done
>  	)
>  '
> @@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '
>  		git svn fetch &&
>  		for i in a b c d d/e d/e/f "weird file name"
>  		do
> -			if test -d "$i"
> -			then
> -				echo >&2 "$i exists" &&
> -				exit 1
> -			fi
> +			test_path_is_missing "$i" || exit 1

Ditto; are we sure the intention of the original is that nothing should be at "$i" (instead of "as long as it is not a directory, we are OK")? Just checking.

The same comment applies to all conversions to test_path_exists and test_path_is_missing where the original was not "test -e" or "! test -e". The other ones, like the change from "test -f" to "test_path_is_file", looked all correct.

Thanks.