git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
@ 2020-10-20 19:53 Joey S
  2020-10-21 13:25 ` Phillip Wood
  2020-10-26 20:50 ` Jonathan Nieder
  0 siblings, 2 replies; 16+ messages in thread
From: Joey S @ 2020-10-20 19:53 UTC (permalink / raw)
  To: git

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(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 00e09a375c..fdb450e446 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -19,7 +19,7 @@ test_expect_success 'setup' '
 test_expect_success TTY 'some commands use a pager' '
 	rm -f paginated.out &&
 	test_terminal git log &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_failure TTY 'pager runs from subdir' '
@@ -65,49 +65,49 @@ test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
 test_expect_success TTY 'some commands do not use a pager' '
 	rm -f paginated.out &&
 	test_terminal git rev-list HEAD &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success 'no pager when stdout is a pipe' '
 	rm -f paginated.out &&
 	git log | cat &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success 'no pager when stdout is a regular file' '
 	rm -f paginated.out &&
 	git log >file &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git --paginate rev-list uses a pager' '
 	rm -f paginated.out &&
 	test_terminal git --paginate rev-list HEAD &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 	rm -f file paginated.out &&
 	git --paginate log | cat &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'no pager with --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git --no-pager log &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'configuration can disable pager' '
 	rm -f paginated.out &&
 	test_unconfig pager.grep &&
 	test_terminal git grep initial &&
-	test -e paginated.out &&
+	test_path_is_file paginated.out &&

 	rm -f paginated.out &&
 	test_config pager.grep false &&
 	test_terminal git grep initial &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'configuration can enable pager (from subdir)' '
@@ -122,107 +122,107 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 		test_terminal git bundle unbundle ../test.bundle
 	) &&
 	{
-		test -e paginated.out ||
-		test -e subdir/paginated.out
+		test_path_is_file paginated.out ||
+		test_path_is_file subdir/paginated.out
 	}
 '

 test_expect_success TTY 'git tag -l defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git tag -l &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -l respects pager.tag' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag -l &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag -l respects --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag --no-pager tag -l &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag with no args defaults to paging' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag with no args respects pager.tag' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag --contains defaults to paging' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag --contains &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag --contains respects pager.tag' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag --contains &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag -a defaults to not paging' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git tag -am message newtag &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag -a ignores pager.tag' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag tag -am message newtag &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag -a respects --paginate' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git --paginate tag -am message newtag &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git branch defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git branch &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git branch respects pager.branch' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.branch=false branch &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git branch respects --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git --no-pager branch &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
@@ -232,8 +232,8 @@ test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
 		touch editor.used
 	EOF
 	EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
-	! test -e paginated.out &&
-	test -e editor.used
+	test_path_is_missing paginated.out &&
+	test_path_is_file editor.used
 '

 test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
@@ -242,13 +242,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	test_when_finished "git branch -D other" &&
 	test_terminal git -c pager.branch branch --set-upstream-to=other &&
 	test_when_finished "git branch --unset-upstream" &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git config ignores pager.config when setting' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config foo.bar bar &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git config --edit ignores pager.config' '
@@ -257,33 +257,33 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
 		touch editor.used
 	EOF
 	EDITOR=./editor test_terminal git -c pager.config config --edit &&
-	! test -e paginated.out &&
-	test -e editor.used
+	test_path_is_missing paginated.out &&
+	test_path_is_file editor.used
 '

 test_expect_success TTY 'git config --get ignores pager.config' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config --get foo.bar &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git config --get-urlmatch defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git -c http."https://foo.com/".bar=foo \
 			  config --get-urlmatch http https://foo.com &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config --get-all respects pager.config' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config=false config --get-all foo.bar &&
-	! test -e paginated.out
+	test_path_is_missing paginated.out
 '

 test_expect_success TTY 'git config --list defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git config --list &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '


@@ -392,7 +392,7 @@ test_default_pager() {
 			export PATH &&
 			$full_command
 		) &&
-		test -e default_pager_used
+		test_path_is_file default_pager_used
 	"
 }

@@ -406,7 +406,7 @@ test_PAGER_overrides() {
 		PAGER='wc >PAGER_used' &&
 		export PAGER &&
 		$full_command &&
-		test -e PAGER_used
+		test_path_is_file PAGER_used
 	"
 }

@@ -432,7 +432,7 @@ test_core_pager() {
 		export PAGER &&
 		test_config core.pager 'wc >core.pager_used' &&
 		$full_command &&
-		${if_local_config}test -e core.pager_used
+		${if_local_config}test_path_is_file core.pager_used
 	"
 }

@@ -464,7 +464,7 @@ test_pager_subdir_helper() {
 			cd sub &&
 			$full_command
 		) &&
-		${if_local_config}test -e core.pager_used
+		${if_local_config}test_path_is_file core.pager_used
 	"
 }

@@ -477,7 +477,7 @@ test_GIT_PAGER_overrides() {
 		GIT_PAGER='wc >GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
-		test -e GIT_PAGER_used
+		test_path_is_file GIT_PAGER_used
 	"
 }

@@ -489,7 +489,7 @@ test_doesnt_paginate() {
 		GIT_PAGER='wc >GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
-		! test -e GIT_PAGER_used
+		test_path_is_missing GIT_PAGER_used
 	"
 }

--
2.29.0.rc2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-20 19:53 [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script Joey S
@ 2020-10-21 13:25 ` Phillip Wood
  2020-10-21 18:42   ` Junio C Hamano
  2020-10-26 20:50 ` Jonathan Nieder
  1 sibling, 1 reply; 16+ messages in thread
From: Phillip Wood @ 2020-10-21 13:25 UTC (permalink / raw)
  To: Joey S, git

Hi Joey

On 20/10/2020 20:53, Joey S wrote:
> 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.

This is a good summary of the changes and importantly explains why we're 
making the changes

> Signed-off-by: Joey Salazar <jgsal@protonmail.com>

The patch looks fine to me now

Thanks

Phillip

> ---
>   t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
>   1 file changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 00e09a375c..fdb450e446 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -19,7 +19,7 @@ test_expect_success 'setup' '
>   test_expect_success TTY 'some commands use a pager' '
>   	rm -f paginated.out &&
>   	test_terminal git log &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_failure TTY 'pager runs from subdir' '
> @@ -65,49 +65,49 @@ test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
>   test_expect_success TTY 'some commands do not use a pager' '
>   	rm -f paginated.out &&
>   	test_terminal git rev-list HEAD &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success 'no pager when stdout is a pipe' '
>   	rm -f paginated.out &&
>   	git log | cat &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success 'no pager when stdout is a regular file' '
>   	rm -f paginated.out &&
>   	git log >file &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git --paginate rev-list uses a pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --paginate rev-list HEAD &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success 'no pager even with --paginate when stdout is a pipe' '
>   	rm -f file paginated.out &&
>   	git --paginate log | cat &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'no pager with --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --no-pager log &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'configuration can disable pager' '
>   	rm -f paginated.out &&
>   	test_unconfig pager.grep &&
>   	test_terminal git grep initial &&
> -	test -e paginated.out &&
> +	test_path_is_file paginated.out &&
> 
>   	rm -f paginated.out &&
>   	test_config pager.grep false &&
>   	test_terminal git grep initial &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'configuration can enable pager (from subdir)' '
> @@ -122,107 +122,107 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
>   		test_terminal git bundle unbundle ../test.bundle
>   	) &&
>   	{
> -		test -e paginated.out ||
> -		test -e subdir/paginated.out
> +		test_path_is_file paginated.out ||
> +		test_path_is_file subdir/paginated.out
>   	}
>   '
> 
>   test_expect_success TTY 'git tag -l defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git tag -l &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -l respects pager.tag' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag -l &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -l respects --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag --no-pager tag -l &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag with no args defaults to paging' '
>   	# no args implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git tag &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag with no args respects pager.tag' '
>   	# no args implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag --contains defaults to paging' '
>   	# --contains implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git tag --contains &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag --contains respects pager.tag' '
>   	# --contains implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag --contains &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a defaults to not paging' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git tag -am message newtag &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a ignores pager.tag' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag tag -am message newtag &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a respects --paginate' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git --paginate tag -am message newtag &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git branch defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git branch &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git branch respects pager.branch' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.branch=false branch &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git branch respects --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --no-pager branch &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
> @@ -232,8 +232,8 @@ test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
>   		touch editor.used
>   	EOF
>   	EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
> -	! test -e paginated.out &&
> -	test -e editor.used
> +	test_path_is_missing paginated.out &&
> +	test_path_is_file editor.used
>   '
> 
>   test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
> @@ -242,13 +242,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
>   	test_when_finished "git branch -D other" &&
>   	test_terminal git -c pager.branch branch --set-upstream-to=other &&
>   	test_when_finished "git branch --unset-upstream" &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git config ignores pager.config when setting' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config config foo.bar bar &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git config --edit ignores pager.config' '
> @@ -257,33 +257,33 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
>   		touch editor.used
>   	EOF
>   	EDITOR=./editor test_terminal git -c pager.config config --edit &&
> -	! test -e paginated.out &&
> -	test -e editor.used
> +	test_path_is_missing paginated.out &&
> +	test_path_is_file editor.used
>   '
> 
>   test_expect_success TTY 'git config --get ignores pager.config' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config config --get foo.bar &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git config --get-urlmatch defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git -c http."https://foo.com/".bar=foo \
>   			  config --get-urlmatch http https://foo.com &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config --get-all respects pager.config' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config=false config --get-all foo.bar &&
> -	! test -e paginated.out
> +	test_path_is_missing paginated.out
>   '
> 
>   test_expect_success TTY 'git config --list defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git config --list &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
> 
> @@ -392,7 +392,7 @@ test_default_pager() {
>   			export PATH &&
>   			$full_command
>   		) &&
> -		test -e default_pager_used
> +		test_path_is_file default_pager_used
>   	"
>   }
> 
> @@ -406,7 +406,7 @@ test_PAGER_overrides() {
>   		PAGER='wc >PAGER_used' &&
>   		export PAGER &&
>   		$full_command &&
> -		test -e PAGER_used
> +		test_path_is_file PAGER_used
>   	"
>   }
> 
> @@ -432,7 +432,7 @@ test_core_pager() {
>   		export PAGER &&
>   		test_config core.pager 'wc >core.pager_used' &&
>   		$full_command &&
> -		${if_local_config}test -e core.pager_used
> +		${if_local_config}test_path_is_file core.pager_used
>   	"
>   }
> 
> @@ -464,7 +464,7 @@ test_pager_subdir_helper() {
>   			cd sub &&
>   			$full_command
>   		) &&
> -		${if_local_config}test -e core.pager_used
> +		${if_local_config}test_path_is_file core.pager_used
>   	"
>   }
> 
> @@ -477,7 +477,7 @@ test_GIT_PAGER_overrides() {
>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>   		export GIT_PAGER &&
>   		$full_command &&
> -		test -e GIT_PAGER_used
> +		test_path_is_file GIT_PAGER_used
>   	"
>   }
> 
> @@ -489,7 +489,7 @@ test_doesnt_paginate() {
>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>   		export GIT_PAGER &&
>   		$full_command &&
> -		! test -e GIT_PAGER_used
> +		test_path_is_missing GIT_PAGER_used
>   	"
>   }
> 
> --
> 2.29.0.rc2
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-21 13:25 ` Phillip Wood
@ 2020-10-21 18:42   ` Junio C Hamano
  2020-10-21 21:42     ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2020-10-21 18:42 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Joey S, git

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Joey
>
> On 20/10/2020 20:53, Joey S wrote:
>> 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.
>
> This is a good summary of the changes and importantly explains why
> we're making the changes
>
>> Signed-off-by: Joey Salazar <jgsal@protonmail.com>
>
> The patch looks fine to me now
>
> Thanks
>
> Phillip

I'll probably have to hand-edit the authorship before applying
(check the From: of the e-mail) so that the identity on the
signed-off-by trailer matches the author, but other than that the
patch looks quite good to me, too.

Thanks, both.

>
>> ---
>>   t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
>>   1 file changed, 42 insertions(+), 42 deletions(-)
>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
>> index 00e09a375c..fdb450e446 100755
>> --- a/t/t7006-pager.sh
>> +++ b/t/t7006-pager.sh
>> @@ -19,7 +19,7 @@ test_expect_success 'setup' '
>>   test_expect_success TTY 'some commands use a pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git log &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_failure TTY 'pager runs from subdir' '
>> @@ -65,49 +65,49 @@ test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
>>   test_expect_success TTY 'some commands do not use a pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git rev-list HEAD &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success 'no pager when stdout is a pipe' '
>>   	rm -f paginated.out &&
>>   	git log | cat &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success 'no pager when stdout is a regular file' '
>>   	rm -f paginated.out &&
>>   	git log >file &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git --paginate rev-list uses a pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git --paginate rev-list HEAD &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success 'no pager even with --paginate when stdout is
>> a pipe' '
>>   	rm -f file paginated.out &&
>>   	git --paginate log | cat &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'no pager with --no-pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git --no-pager log &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'configuration can disable pager' '
>>   	rm -f paginated.out &&
>>   	test_unconfig pager.grep &&
>>   	test_terminal git grep initial &&
>> -	test -e paginated.out &&
>> +	test_path_is_file paginated.out &&
>>   	rm -f paginated.out &&
>>   	test_config pager.grep false &&
>>   	test_terminal git grep initial &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'configuration can enable pager (from
>> subdir)' '
>> @@ -122,107 +122,107 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
>>   		test_terminal git bundle unbundle ../test.bundle
>>   	) &&
>>   	{
>> -		test -e paginated.out ||
>> -		test -e subdir/paginated.out
>> +		test_path_is_file paginated.out ||
>> +		test_path_is_file subdir/paginated.out
>>   	}
>>   '
>>   test_expect_success TTY 'git tag -l defaults to paging' '
>>   	rm -f paginated.out &&
>>   	test_terminal git tag -l &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git tag -l respects pager.tag' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag=false tag -l &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag -l respects --no-pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag --no-pager tag -l &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag with no args defaults to paging'
>> '
>>   	# no args implies -l so this should page like -l
>>   	rm -f paginated.out &&
>>   	test_terminal git tag &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git tag with no args respects pager.tag'
>> '
>>   	# no args implies -l so this should page like -l
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag=false tag &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag --contains defaults to paging' '
>>   	# --contains implies -l so this should page like -l
>>   	rm -f paginated.out &&
>>   	test_terminal git tag --contains &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git tag --contains respects pager.tag' '
>>   	# --contains implies -l so this should page like -l
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag=false tag --contains &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag -a defaults to not paging' '
>>   	test_when_finished "git tag -d newtag" &&
>>   	rm -f paginated.out &&
>>   	test_terminal git tag -am message newtag &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag -a ignores pager.tag' '
>>   	test_when_finished "git tag -d newtag" &&
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag tag -am message newtag &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag -a respects --paginate' '
>>   	test_when_finished "git tag -d newtag" &&
>>   	rm -f paginated.out &&
>>   	test_terminal git --paginate tag -am message newtag &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git tag as alias ignores pager.tag with
>> -a' '
>>   	test_when_finished "git tag -d newtag" &&
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git tag as alias respects pager.tag with
>> -l' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git branch defaults to paging' '
>>   	rm -f paginated.out &&
>>   	test_terminal git branch &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git branch respects pager.branch' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.branch=false branch &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git branch respects --no-pager' '
>>   	rm -f paginated.out &&
>>   	test_terminal git --no-pager branch &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git branch --edit-description ignores
>> pager.branch' '
>> @@ -232,8 +232,8 @@ test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
>>   		touch editor.used
>>   	EOF
>>   	EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
>> -	! test -e paginated.out &&
>> -	test -e editor.used
>> +	test_path_is_missing paginated.out &&
>> +	test_path_is_file editor.used
>>   '
>>   test_expect_success TTY 'git branch --set-upstream-to ignores
>> pager.branch' '
>> @@ -242,13 +242,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
>>   	test_when_finished "git branch -D other" &&
>>   	test_terminal git -c pager.branch branch --set-upstream-to=other &&
>>   	test_when_finished "git branch --unset-upstream" &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git config ignores pager.config when
>> setting' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.config config foo.bar bar &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git config --edit ignores pager.config' '
>> @@ -257,33 +257,33 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
>>   		touch editor.used
>>   	EOF
>>   	EDITOR=./editor test_terminal git -c pager.config config --edit &&
>> -	! test -e paginated.out &&
>> -	test -e editor.used
>> +	test_path_is_missing paginated.out &&
>> +	test_path_is_file editor.used
>>   '
>>   test_expect_success TTY 'git config --get ignores pager.config' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.config config --get foo.bar &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git config --get-urlmatch defaults to
>> paging' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c http."https://foo.com/".bar=foo \
>>   			  config --get-urlmatch http https://foo.com &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>>   test_expect_success TTY 'git config --get-all respects
>> pager.config' '
>>   	rm -f paginated.out &&
>>   	test_terminal git -c pager.config=false config --get-all foo.bar &&
>> -	! test -e paginated.out
>> +	test_path_is_missing paginated.out
>>   '
>>   test_expect_success TTY 'git config --list defaults to paging' '
>>   	rm -f paginated.out &&
>>   	test_terminal git config --list &&
>> -	test -e paginated.out
>> +	test_path_is_file paginated.out
>>   '
>> 
>> @@ -392,7 +392,7 @@ test_default_pager() {
>>   			export PATH &&
>>   			$full_command
>>   		) &&
>> -		test -e default_pager_used
>> +		test_path_is_file default_pager_used
>>   	"
>>   }
>> @@ -406,7 +406,7 @@ test_PAGER_overrides() {
>>   		PAGER='wc >PAGER_used' &&
>>   		export PAGER &&
>>   		$full_command &&
>> -		test -e PAGER_used
>> +		test_path_is_file PAGER_used
>>   	"
>>   }
>> @@ -432,7 +432,7 @@ test_core_pager() {
>>   		export PAGER &&
>>   		test_config core.pager 'wc >core.pager_used' &&
>>   		$full_command &&
>> -		${if_local_config}test -e core.pager_used
>> +		${if_local_config}test_path_is_file core.pager_used
>>   	"
>>   }
>> @@ -464,7 +464,7 @@ test_pager_subdir_helper() {
>>   			cd sub &&
>>   			$full_command
>>   		) &&
>> -		${if_local_config}test -e core.pager_used
>> +		${if_local_config}test_path_is_file core.pager_used
>>   	"
>>   }
>> @@ -477,7 +477,7 @@ test_GIT_PAGER_overrides() {
>>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>>   		export GIT_PAGER &&
>>   		$full_command &&
>> -		test -e GIT_PAGER_used
>> +		test_path_is_file GIT_PAGER_used
>>   	"
>>   }
>> @@ -489,7 +489,7 @@ test_doesnt_paginate() {
>>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>>   		export GIT_PAGER &&
>>   		$full_command &&
>> -		! test -e GIT_PAGER_used
>> +		test_path_is_missing GIT_PAGER_used
>>   	"
>>   }
>> --
>> 2.29.0.rc2
>> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-21 18:42   ` Junio C Hamano
@ 2020-10-21 21:42     ` Joey Salazar
  2020-10-21 21:57       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Joey Salazar @ 2020-10-21 21:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Phillip Wood, git

Hi Phillip, Junio,

Thank you for your reviews and support, they're much appreciated and I'm happy the patch looks ok.

> I'll probably have to hand-edit the authorship before applying
> (check the From: of the e-mail) so that the identity on the
> signed-off-by trailer matches the author, but other than that the
> patch looks quite good to me, too.

My big apologies Junio, I mistook "authorship" to mean my `git config --global user.name` and `git config --global user.email` settings.

Should I resend the patch email?

Thank you all,
Joey


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, October 21, 2020 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Phillip Wood phillip.wood123@gmail.com writes:
>
> > Hi Joey
> > On 20/10/2020 20:53, Joey S wrote:
> >
> > > 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.
> >
> > This is a good summary of the changes and importantly explains why
> > we're making the changes
> >
> > > Signed-off-by: Joey Salazar jgsal@protonmail.com
> >
> > The patch looks fine to me now
> > Thanks
> > Phillip
>
> I'll probably have to hand-edit the authorship before applying
> (check the From: of the e-mail) so that the identity on the
> signed-off-by trailer matches the author, but other than that the
> patch looks quite good to me, too.
>
> Thanks, both.
>
> > > t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
> > > 1 file changed, 42 insertions(+), 42 deletions(-)
> > > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> > > index 00e09a375c..fdb450e446 100755
> > > --- a/t/t7006-pager.sh
> > > +++ b/t/t7006-pager.sh
> > > @@ -19,7 +19,7 @@ test_expect_success 'setup' '
> > > test_expect_success TTY 'some commands use a pager' '
> > > rm -f paginated.out &&
> > > test_terminal git log &&
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_failure TTY 'pager runs from subdir' '
> > >     @@ -65,49 +65,49 @@ test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
> > >     test_expect_success TTY 'some commands do not use a pager' '
> > >     rm -f paginated.out &&
> > >     test_terminal git rev-list HEAD &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success 'no pager when stdout is a pipe' '
> > >     rm -f paginated.out &&
> > >     git log | cat &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success 'no pager when stdout is a regular file' '
> > >     rm -f paginated.out &&
> > >     git log >file &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git --paginate rev-list uses a pager' '
> > >     rm -f paginated.out &&
> > >     test_terminal git --paginate rev-list HEAD &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success 'no pager even with --paginate when stdout is
> > >     a pipe' '
> > >     rm -f file paginated.out &&
> > >     git --paginate log | cat &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'no pager with --no-pager' '
> > >     rm -f paginated.out &&
> > >     test_terminal git --no-pager log &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'configuration can disable pager' '
> > >     rm -f paginated.out &&
> > >     test_unconfig pager.grep &&
> > >     test_terminal git grep initial &&
> > >
> > >
> > > -   test -e paginated.out &&
> > >
> > > -   test_path_is_file paginated.out &&
> > >     rm -f paginated.out &&
> > >     test_config pager.grep false &&
> > >     test_terminal git grep initial &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'configuration can enable pager (from
> > >     subdir)' '
> > >     @@ -122,107 +122,107 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
> > >     test_terminal git bundle unbundle ../test.bundle
> > >     ) &&
> > >     {
> > >
> > >
> > > -       test -e paginated.out ||
> > >
> > >
> > > -       test -e subdir/paginated.out
> > >
> > >
> > >
> > > -       test_path_is_file paginated.out ||
> > >
> > >
> > > -       test_path_is_file subdir/paginated.out
> > >
> > >
> > >     }
> > >     '
> > >     test_expect_success TTY 'git tag -l defaults to paging' '
> > >     rm -f paginated.out &&
> > >     test_terminal git tag -l &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag -l respects pager.tag' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag=false tag -l &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag -l respects --no-pager' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag --no-pager tag -l &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag with no args defaults to paging'
> > >     '
> > >
> > >     no args implies -l so this should page like -l
> > >
> > >     ===============================================
> > >
> > >     rm -f paginated.out &&
> > >     test_terminal git tag &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag with no args respects pager.tag'
> > >     '
> > >
> > >     no args implies -l so this should page like -l
> > >
> > >     ===============================================
> > >
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag=false tag &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag --contains defaults to paging' '
> > >
> > >     --contains implies -l so this should page like -l
> > >
> > >     ==================================================
> > >
> > >     rm -f paginated.out &&
> > >     test_terminal git tag --contains &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag --contains respects pager.tag' '
> > >
> > >     --contains implies -l so this should page like -l
> > >
> > >     ==================================================
> > >
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag=false tag --contains &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag -a defaults to not paging' '
> > >     test_when_finished "git tag -d newtag" &&
> > >     rm -f paginated.out &&
> > >     test_terminal git tag -am message newtag &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag -a ignores pager.tag' '
> > >     test_when_finished "git tag -d newtag" &&
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag tag -am message newtag &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag -a respects --paginate' '
> > >     test_when_finished "git tag -d newtag" &&
> > >     rm -f paginated.out &&
> > >     test_terminal git --paginate tag -am message newtag &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag as alias ignores pager.tag with
> > >     -a' '
> > >     test_when_finished "git tag -d newtag" &&
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git tag as alias respects pager.tag with
> > >     -l' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git branch defaults to paging' '
> > >     rm -f paginated.out &&
> > >     test_terminal git branch &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git branch respects pager.branch' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.branch=false branch &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git branch respects --no-pager' '
> > >     rm -f paginated.out &&
> > >     test_terminal git --no-pager branch &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git branch --edit-description ignores
> > >     pager.branch' '
> > >     @@ -232,8 +232,8 @@ test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
> > >     touch editor.used
> > >     EOF
> > >     EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
> > >
> > >
> > > -   ! test -e paginated.out &&
> > > -   test -e editor.used
> > >
> > > -   test_path_is_missing paginated.out &&
> > > -   test_path_is_file editor.used
> > >     '
> > >     test_expect_success TTY 'git branch --set-upstream-to ignores
> > >     pager.branch' '
> > >     @@ -242,13 +242,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
> > >     test_when_finished "git branch -D other" &&
> > >     test_terminal git -c pager.branch branch --set-upstream-to=other &&
> > >     test_when_finished "git branch --unset-upstream" &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git config ignores pager.config when
> > >     setting' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.config config foo.bar bar &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git config --edit ignores pager.config' '
> > >     @@ -257,33 +257,33 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
> > >     touch editor.used
> > >     EOF
> > >     EDITOR=./editor test_terminal git -c pager.config config --edit &&
> > >
> > >
> > > -   ! test -e paginated.out &&
> > > -   test -e editor.used
> > >
> > > -   test_path_is_missing paginated.out &&
> > > -   test_path_is_file editor.used
> > >     '
> > >     test_expect_success TTY 'git config --get ignores pager.config' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.config config --get foo.bar &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git config --get-urlmatch defaults to
> > >     paging' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c http."https://foo.com/".bar=foo \
> > >     config --get-urlmatch http https://foo.com &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >     test_expect_success TTY 'git config --get-all respects
> > >     pager.config' '
> > >     rm -f paginated.out &&
> > >     test_terminal git -c pager.config=false config --get-all foo.bar &&
> > >
> > >
> > > -   ! test -e paginated.out
> > >
> > > -   test_path_is_missing paginated.out
> > >     '
> > >     test_expect_success TTY 'git config --list defaults to paging' '
> > >     rm -f paginated.out &&
> > >     test_terminal git config --list &&
> > >
> > >
> > > -   test -e paginated.out
> > >
> > > -   test_path_is_file paginated.out
> > >     '
> > >
> > >
> > > @@ -392,7 +392,7 @@ test_default_pager() {
> > > export PATH &&
> > > $full_command
> > > ) &&
> > >
> > > -       test -e default_pager_used
> > >
> > >
> > >
> > > -       test_path_is_file default_pager_used
> > >
> > >
> > >     "
> > >     }
> > >     @@ -406,7 +406,7 @@ test_PAGER_overrides() {
> > >     PAGER='wc >PAGER_used' &&
> > >     export PAGER &&
> > >     $full_command &&
> > >
> > >
> > > -       test -e PAGER_used
> > >
> > >
> > >
> > > -       test_path_is_file PAGER_used
> > >
> > >
> > >     "
> > >     }
> > >     @@ -432,7 +432,7 @@ test_core_pager() {
> > >     export PAGER &&
> > >     test_config core.pager 'wc >core.pager_used' &&
> > >     $full_command &&
> > >
> > >
> > > -       ${if_local_config}test -e core.pager_used
> > >
> > >
> > >
> > > -       ${if_local_config}test_path_is_file core.pager_used
> > >
> > >
> > >     "
> > >     }
> > >     @@ -464,7 +464,7 @@ test_pager_subdir_helper() {
> > >     cd sub &&
> > >     $full_command
> > >     ) &&
> > >
> > >
> > > -       ${if_local_config}test -e core.pager_used
> > >
> > >
> > >
> > > -       ${if_local_config}test_path_is_file core.pager_used
> > >
> > >
> > >     "
> > >     }
> > >     @@ -477,7 +477,7 @@ test_GIT_PAGER_overrides() {
> > >     GIT_PAGER='wc >GIT_PAGER_used' &&
> > >     export GIT_PAGER &&
> > >     $full_command &&
> > >
> > >
> > > -       test -e GIT_PAGER_used
> > >
> > >
> > >
> > > -       test_path_is_file GIT_PAGER_used
> > >
> > >
> > >     "
> > >     }
> > >     @@ -489,7 +489,7 @@ test_doesnt_paginate() {
> > >     GIT_PAGER='wc >GIT_PAGER_used' &&
> > >     export GIT_PAGER &&
> > >     $full_command &&
> > >
> > >
> > > -       ! test -e GIT_PAGER_used
> > >
> > >
> > >
> > > -       test_path_is_missing GIT_PAGER_used
> > >
> > >
> > >     "
> > >     }
> > >     --
> > >     2.29.0.rc2
> > >



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-21 21:42     ` Joey Salazar
@ 2020-10-21 21:57       ` Junio C Hamano
  2020-10-21 22:21         ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2020-10-21 21:57 UTC (permalink / raw)
  To: Joey Salazar; +Cc: Phillip Wood, git

Joey Salazar <jgsal@protonmail.com> writes:

> Hi Phillip, Junio,
>
> Thank you for your reviews and support, they're much appreciated and I'm happy the patch looks ok.
>
>> I'll probably have to hand-edit the authorship before applying
>> (check the From: of the e-mail) so that the identity on the
>> signed-off-by trailer matches the author, but other than that the
>> patch looks quite good to me, too.
>
> My big apologies Junio, I mistook "authorship" to mean my `git config --global user.name` and `git config --global user.email` settings.
>
> Should I resend the patch email?

No need to apologize and no need to resend. You may want to send a
patch to yourself to prepare your procedure, but that is something
you can do without sending patches to the list.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-21 21:57       ` Junio C Hamano
@ 2020-10-21 22:21         ` Joey Salazar
  0 siblings, 0 replies; 16+ messages in thread
From: Joey Salazar @ 2020-10-21 22:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Phillip Wood, git

Will do, thank you.



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, October 21, 2020 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Joey Salazar jgsal@protonmail.com writes:
>
> > Hi Phillip, Junio,
> > Thank you for your reviews and support, they're much appreciated and I'm happy the patch looks ok.
> >
> > > I'll probably have to hand-edit the authorship before applying
> > > (check the From: of the e-mail) so that the identity on the
> > > signed-off-by trailer matches the author, but other than that the
> > > patch looks quite good to me, too.
> >
> > My big apologies Junio, I mistook "authorship" to mean my `git config --global user.name` and `git config --global user.email` settings.
> > Should I resend the patch email?
>
> No need to apologize and no need to resend. You may want to send a
> patch to yourself to prepare your procedure, but that is something
> you can do without sending patches to the list.
>
> Thanks.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-20 19:53 [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script Joey S
  2020-10-21 13:25 ` Phillip Wood
@ 2020-10-26 20:50 ` Jonathan Nieder
  2020-10-26 21:24   ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2020-10-26 20:50 UTC (permalink / raw)
  To: Joey S; +Cc: git

Hi,

Joey S wrote:

> 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
 	"
 }
 

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-26 20:50 ` Jonathan Nieder
@ 2020-10-26 21:24   ` Junio C Hamano
  2020-10-26 21:46     ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2020-10-26 21:24 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Joey S, git

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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 for a great suggestion.  The if_local_config thing was
inherited and not a problem introduced by Joey, but it is a good
idea to clean it up at the same time, I think.



> 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
>  	"
>  }
>  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-26 21:24   ` Junio C Hamano
@ 2020-10-26 21:46     ` Joey Salazar
  2020-10-26 22:02       ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Joey Salazar @ 2020-10-26 21:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Nieder, git

Thank you both for the comments and update, squashing both commits make sense to me as well, yet I can incorporate the changes in a new patch (v3) if that would be preferred.

Thanks,
Joey

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, October 26, 2020 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Jonathan Nieder jrnieder@gmail.com writes:
>
> > 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 for a great suggestion. The if_local_config thing was
> inherited and not a problem introduced by Joey, but it is a good
> idea to clean it up at the same time, I think.
>
> > 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
> >
> >
> >     "
> >     }
> >



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-26 21:46     ` Joey Salazar
@ 2020-10-26 22:02       ` Jonathan Nieder
  2020-10-26 23:00         ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2020-10-26 22:02 UTC (permalink / raw)
  To: Joey Salazar; +Cc: Junio C Hamano, git

(administrivia: the convention on this mailing list is not to top-post.
When your mailer puts the cursor at the top of the reply, that's a
signal to go through and delete the parts you don't want to reply to,
before putting your reply below the bit you do want to reply to)

Joey Salazar wrote:

> Thank you both for the comments and update, squashing both commits
> make sense to me as well, yet I can incorporate the changes in a new
> patch (v3) if that would be preferred.

Thanks, Joey.  I think we'd prefer a v3 since that gives you a place
to put the commit message for the combined patch.

Or if you prefer for Junio to take care of the squashing, an
alternative is to write the proposed commit message here.

Thanks for your patient work,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-26 22:02       ` Jonathan Nieder
@ 2020-10-26 23:00         ` Joey Salazar
  2020-10-27  0:04           ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Joey Salazar @ 2020-10-26 23:00 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git


On Monday, October 26, 2020 4:02 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

> (administrivia: the convention on this mailing list is not to top-post.
> When your mailer puts the cursor at the top of the reply, that's a
> signal to go through and delete the parts you don't want to reply to,
> before putting your reply below the bit you do want to reply to)
>
Understood, thank you.

> Thanks, Joey. I think we'd prefer a v3 since that gives you a place
> to put the commit message for the combined patch.
>
> Or if you prefer for Junio to take care of the squashing, an
> alternative is to write the proposed commit message here.

I've locally incorporated the changes and remain open to either option. The proposed amended commit message would be;
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.
For instances when `${if_local_config}` is either '' or '! ' then
`test_path_is_file` will diagnose the directory and print a message if
and only if the result `is false` goes away.

Signed-off-by: Joey Salazar <jgsal@protonmail.com>

Does this capture the idea correctly?

> Thanks for your patient work,
> Jonathan
Thank you for all your input and guidance.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-26 23:00         ` Joey Salazar
@ 2020-10-27  0:04           ` Joey Salazar
  2020-10-27  0:14             ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Joey Salazar @ 2020-10-27  0:04 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git


On Monday, October 26, 2020 5:00 PM, Joey Salazar <jgsal@protonmail.com> wrote:

> For instances when `${if_local_config}` is either '' or '! ' then
> `test_path_is_file` will diagnose the directory and print a message if
> and only if the result `is false` goes away.
>
If `if test -n '$pager_wanted'` is checking if pager_wanted=true before diagnosing core.pager_used, then would;

For other instances when '$pager_wanted' is not empty then `test_path_is_file`
will diagnose the directory and print a message.

be more accurate?





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-27  0:04           ` Joey Salazar
@ 2020-10-27  0:14             ` Jonathan Nieder
  2020-10-27 23:45               ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2020-10-27  0:14 UTC (permalink / raw)
  To: Joey Salazar; +Cc: Junio C Hamano, git

Joey Salazar wrote:
> On Monday, October 26, 2020 5:00 PM, Joey Salazar <jgsal@protonmail.com> wrote:

>> 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.
>> For instances when `${if_local_config}` is either '' or '! ' then
>> `test_path_is_file` will diagnose the directory and print a message if
>> and only if the result `is false` goes away.
>
> If `if test -n '$pager_wanted'` is checking if pager_wanted=true
> before diagnosing core.pager_used, then would;
>
> For other instances when '$pager_wanted' is not empty then `test_path_is_file`
> will diagnose the directory and print a message.
>
> be more accurate?

Yes, but because it restates what the patch says instead of describing
the "why", it's at the wrong level of abstraction.

Starting from

    t7006: Use test_path_is_* functions in test script

    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.

I think what would make sense is to add a second paragraph describing
why the existing code uses ${if_local_config} and why what the new
code is doing is better.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-27  0:14             ` Jonathan Nieder
@ 2020-10-27 23:45               ` Joey Salazar
  2020-10-28  0:19                 ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Joey Salazar @ 2020-10-27 23:45 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git

Jonathan Nieder wrote:

> > If `if test -n '$pager_wanted'` is checking if pager_wanted=true
> > before diagnosing core.pager_used, then would;
> > For other instances when '$pager_wanted' is not empty then `test_path_is_file`
> > will diagnose the directory and print a message.
> > be more accurate?
>
> Yes, but because it restates what the patch says instead of describing
> the "why", it's at the wrong level of abstraction.
> I think what would make sense is to add a second paragraph describing
> why the existing code uses ${if_local_config} and why what the new
> code is doing is better.

I see, thank you. I'm now thinking of a paragraph like this (thank you Emily Shaffer for your guidance in the IRC channel);
Messages from checks to `${if_local_config}` are also printed when the
result is false, which can be confusing. Improve readability by
removing `${if_local_config}` checks and print messages only when a
pager is wanted.

> Thanks,
> Jonathan

Thank you for your patient input and feedback.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-27 23:45               ` Joey Salazar
@ 2020-10-28  0:19                 ` Jonathan Nieder
  2020-10-28  2:45                   ` Joey Salazar
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2020-10-28  0:19 UTC (permalink / raw)
  To: Joey Salazar; +Cc: Junio C Hamano, git

Joey Salazar wrote:

> I see, thank you. I'm now thinking of a paragraph like this (thank
> you Emily Shaffer for your guidance in the IRC channel);
>
> Messages from checks to `${if_local_config}` are also printed when the
> result is false, which can be confusing. Improve readability by
> removing `${if_local_config}` checks and print messages only when a
> pager is wanted.

I think that's on the right track, though I'm having trouble
understanding the details of it.  If this goes as part of the same
patch, we'd want to focus on why we're making this change in the
context of this commit.  Using an example can make the idea clearer:

	A subtlety: one advantage of helpers such as test_path_is_missing is
	that they print a diagnostic if and only if the condition fails, which
	can make the output from a failing test easier to read.  Unfortunately,
	some helpers in this test communicate whether a configured pager is
	expected to run using a shell constract that doesn't have that property:

		my_generic_helper () {
			...
			${if_local_config}test -e core.pager_used
		}

		...
		if_local_config='! '
		my_generic_helper

	Rewriting this to "${if_local_config}test_path_is_file core.pager_used"
	would print a diagnostic when the file is absent, which is the opposite
	of what we want.  Make the logic more explicit instead, using "test" to
	check a variable core_pager_wanted that is nonempty when core.pager is
	expected to be used.

That said, it looks like js/t7006-cleanup is in "next", indicating
that it has finished being reviewed and is now safe to build on (see
https://git-scm.com/docs/SubmittingPatches for more on this subject),
so it would be even better to make this a patch on top of the existing
v2 patch after all.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
  2020-10-28  0:19                 ` Jonathan Nieder
@ 2020-10-28  2:45                   ` Joey Salazar
  0 siblings, 0 replies; 16+ messages in thread
From: Joey Salazar @ 2020-10-28  2:45 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git

Jonathan Nieder wrote:

> A subtlety: one advantage of helpers such as test_path_is_missing is
> that they print a diagnostic if and only if the condition fails, which
> can make the output from a failing test easier to read. Unfortunately,
> some helpers in this test communicate whether a configured pager is
> expected to run using a shell constract that doesn't have that property:
>
> my_generic_helper () {
> ...
> ${if_local_config}test -e core.pager_used
> }
>
> ...
> if_local_config='! '
> my_generic_helper
>
> Rewriting this to "${if_local_config}test_path_is_file core.pager_used"
> would print a diagnostic when the file is absent, which is the opposite
> of what we want. Make the logic more explicit instead, using "test" to
> check a variable core_pager_wanted that is nonempty when core.pager is
> expected to be used.

Thank you for this explanation, much appreciated.

> That said, it looks like js/t7006-cleanup is in "next", indicating
> that it has finished being reviewed and is now safe to build on (see
> https://git-scm.com/docs/SubmittingPatches for more on this subject),
> so it would be even better to make this a patch on top of the existing
> v2 patch after all.

A proposed commit msg for patch v1 in the branch js/t7006-cleanup that explains "what was wrong in the previous patches and how the problem was corrected" would be;
The previous patch introduces `test_path_is_file` and helper functions
for instances that include checking if a configured pager is expected
to run using a shell construct. This prints a diagnostic message even
when the file is absent, as opposed to only when the file exists and
has been checked.
Use "test" to first check if a pager is wanted when `core.pager` is
expected to be used, then diagnose `core.pager_used` and print a
diagnostic message as appropriate.

Should I include an explicit mention to the removal of `${if_local_config}`?

Should I start a new email thread with "[PATCH v1]" on further discussion?

Thank you,
Joey

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-10-28 23:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20 19:53 [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script Joey S
2020-10-21 13:25 ` Phillip Wood
2020-10-21 18:42   ` Junio C Hamano
2020-10-21 21:42     ` Joey Salazar
2020-10-21 21:57       ` Junio C Hamano
2020-10-21 22:21         ` Joey Salazar
2020-10-26 20:50 ` Jonathan Nieder
2020-10-26 21:24   ` Junio C Hamano
2020-10-26 21:46     ` Joey Salazar
2020-10-26 22:02       ` Jonathan Nieder
2020-10-26 23:00         ` Joey Salazar
2020-10-27  0:04           ` Joey Salazar
2020-10-27  0:14             ` Jonathan Nieder
2020-10-27 23:45               ` Joey Salazar
2020-10-28  0:19                 ` Jonathan Nieder
2020-10-28  2:45                   ` Joey Salazar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).