Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 26, 2020, 20:50 UTC
Message-ID: <20201026205028.GC2645313@google.com>
In-Reply-To: <KHJW7elqEfVsIp1V0WKPRVAB5xqCDJjjqLv8flthlDiSsSWjND-VVGG2zL-xOYMstk-q0JR3OiSggcMlFgzkIKm2podjzAyamb0pW-wx1ZY=@protonmail.com>

Hi,

Joey S wrote:

Show 8 quoted lines

> Modernize the test by replacing `test -e` instances with
> `test_path_is_file` helper functions, and `! test -e` with
> `test_path_is_missing`, for better readability and diagnostic messages.
>
> Signed-off-by: Joey Salazar <jgsal@protonmail.com>
> ---
>  t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
>  1 file changed, 42 insertions(+), 42 deletions(-)

Grepping for 'test -' in the file after applying this patch, I find no more instances.

[...]

> -	test -e paginated.out
> +	test_path_is_file paginated.out

This kind of change ensures that if the file exists but as a directory, the test will diagnose it, which is a nice thing.

Overall (as someone who's worked a bit with this test script before), looks good to me. Thanks for your work.

> -		${if_local_config}test -e core.pager_used
> +		${if_local_config}test_path_is_file core.pager_used

This bit is a little subtle: ${if_local_config} is either '' or '! ', and in the latter case the benefit of test_path_is_file printing a message if and only if the result is false goes away.

Would the following, squashed in, make sense?

Thanks, Jonathan

diff --git i/t/t7006-pager.sh w/t/t7006-pager.sh
index fdb450e446a..11327944741 100755
--- i/t/t7006-pager.sh
+++ w/t/t7006-pager.sh
@@ -411,13 +411,13 @@ test_PAGER_overrides() {
 }
 
 test_core_pager_overrides() {
-	if_local_config=
+	pager_wanted=true
 	used_if_wanted='overrides PAGER'
 	test_core_pager "$@"
 }
 
 test_local_config_ignored() {
-	if_local_config='! '
+	pager_wanted=
 	used_if_wanted='is not used'
 	test_core_pager "$@"
 }
@@ -432,18 +432,23 @@ test_core_pager() {
 		export PAGER &&
 		test_config core.pager 'wc >core.pager_used' &&
 		$full_command &&
-		${if_local_config}test_path_is_file core.pager_used
+		if test -n '$pager_wanted'
+		then
+			test_path_is_file core.pager_used
+		else
+			test_path_is_missing core.pager_used
+		fi
 	"
 }
 
 test_core_pager_subdir() {
-	if_local_config=
+	pager_wanted=true
 	used_if_wanted='overrides PAGER'
 	test_pager_subdir_helper "$@"
 }
 
 test_no_local_config_subdir() {
-	if_local_config='! '
+	pager_wanted=
 	used_if_wanted='is not used'
 	test_pager_subdir_helper "$@"
 }
@@ -464,7 +469,12 @@ test_pager_subdir_helper() {
 			cd sub &&
 			$full_command
 		) &&
-		${if_local_config}test_path_is_file core.pager_used
+		if test -n '$pager_wanted'
+		then
+			test_path_is_file core.pager_used
+		else
+			test_path_is_missing core.pager_used
+		fi
 	"
 }