[GSOC PATCH] t7003: modernize path existence checks using test helpers

[GSOC PATCH] t7003: modernize path existence checks using test helpers

[SoutrikDas]

Replace direct uses of 'test -f' and 'test -d' with
git's helper functions 'test_path_is_file' ,
'test_path_is_missing' and 'test_path_is_dir'

Signed-off-by: SoutrikDas <valusoutrik@gmail.com>
---
 t/t7003-filter-branch.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 5ab4d41ee7..c475769858 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -92,8 +92,8 @@ test_expect_success 'rewrite, renaming a specific file' '
 
 test_expect_success 'test that the file was renamed' '
 	test D = "$(git show HEAD:doh --)" &&
-	! test -f D.t &&
-	test -f doh &&
+	test_path_is_missing D.t &&
+	test_path_is_file doh &&
 	test D = "$(cat doh)"
 '
 
@@ -103,10 +103,10 @@ test_expect_success 'rewrite, renaming a specific directory' '
 
 test_expect_success 'test that the directory was renamed' '
 	test dir/D = "$(git show HEAD:diroh/D.t --)" &&
-	! test -d dir &&
-	test -d diroh &&
-	! test -d diroh/dir &&
-	test -f diroh/D.t &&
+	test_path_is_missing dir &&
+	test_path_is_dir diroh &&
+	test_path_is_missing diroh/dir &&
+	test_path_is_file diroh/D.t &&
 	test dir/D = "$(cat diroh/D.t)"
 '
 
-- 
2.52.0

Re: [GSOC PATCH] t7003: modernize path existence checks using test helpers

[Junio C Hamano]

SoutrikDas <valusoutrik@gmail.com> writes:

> Replace direct uses of 'test -f' and 'test -d' with
> git's helper functions 'test_path_is_file' ,
> 'test_path_is_missing' and 'test_path_is_dir'
>
> Signed-off-by: SoutrikDas <valusoutrik@gmail.com>
> ---
>  t/t7003-filter-branch.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 5ab4d41ee7..c475769858 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -92,8 +92,8 @@ test_expect_success 'rewrite, renaming a specific file' '
>  
>  test_expect_success 'test that the file was renamed' '
>  	test D = "$(git show HEAD:doh --)" &&
> -	! test -f D.t &&
> -	test -f doh &&
> +	test_path_is_missing D.t &&
> +	test_path_is_file doh &&
>  	test D = "$(cat doh)"
>  '
> @@ -103,10 +103,10 @@ test_expect_success 'rewrite, renaming a specific directory' '
>  
>  test_expect_success 'test that the directory was renamed' '
>  	test dir/D = "$(git show HEAD:diroh/D.t --)" &&
> -	! test -d dir &&
> -	test -d diroh &&
> -	! test -d diroh/dir &&
> -	test -f diroh/D.t &&
> +	test_path_is_missing dir &&
> +	test_path_is_dir diroh &&
> +	test_path_is_missing diroh/dir &&
> +	test_path_is_file diroh/D.t &&
>  	test dir/D = "$(cat diroh/D.t)"
>  '

All the checks involving "is-missing" are now stricter than the
original, in that they used to allow "dir" to exist as long as it is
not a directory, etc., but if we audited the code that leads to
these tests can never create a "dir" that is a regular file or
something that is not a directory (which *I* did *NOT*, but
presumably you have already done so?---if so that is worth noting in
the proposed log message), then "test ! -d dir" that is rewritten to
"test_path_is_missing dir" is actually a _better_ test.

Thanks.

Re: [GSOC PATCH] t7003: modernize path existence checks using test helpers

[SoutrikDas]

> All the checks involving "is-missing" are now stricter than the
> original, in that they used to allow "dir" to exist as long as it is
> not a directory, etc., but if we audited the code that leads to
> these tests can never create a "dir" that is a regular file or
> something that is not a directory (which *I* did *NOT*, but
> presumably you have already done so?

At the time of sending the patch v1, I did not do so. Sorry about that.
Now I ran the test from start to 11, since test 12 was the one with two 
of those risky changes, ie `! test -d dir` and `! test -d diroh/dir`

and after doing that I can confirm that there is no non directory dir 
present before test 12 starts. Neither is there a non directory dir 
inside `diroh`

This was the output of ls -la 

drwxr-xr-x  14 soutrik  staff  448 10 Feb 23:16 .
drwxr-xr-x   4 soutrik  staff  128 10 Feb 23:14 ..
drwxr-xr-x@ 14 soutrik  staff  448 10 Feb 23:16 .git
-rw-r--r--   1 soutrik  staff    2 10 Feb 23:15 A.t
-rw-r--r--   1 soutrik  staff    2 10 Feb 23:15 B.t
-rw-r--r--@  1 soutrik  staff  128 10 Feb 23:16 backup-refs
-rw-r--r--@  1 soutrik  staff    2 10 Feb 23:15 C.t
drwxr-xr-x@  3 soutrik  staff   96 10 Feb 23:16 diroh
-rw-r--r--@  1 soutrik  staff    2 10 Feb 23:16 doh
drwxr-xr-x   4 soutrik  staff  128 10 Feb 23:15 drepo
drwxr-xr-x@  6 soutrik  staff  192 10 Feb 23:16 drepo-tree
-rw-r--r--@  1 soutrik  staff    2 10 Feb 23:15 E.t
-rw-r--r--   1 soutrik  staff    2 10 Feb 23:15 G.t
-rw-r--r--   1 soutrik  staff    2 10 Feb 23:15 H.t

And this was the ls -la in the `diroh` directory

drwxr-xr-x@  3 soutrik  staff   96 10 Feb 23:16 .
drwxr-xr-x  14 soutrik  staff  448 10 Feb 23:16 ..
-rw-r--r--@  1 soutrik  staff    6 10 Feb 23:16 D.t

One thing is that I am not sure if what I did is ... the correct way
to test this kind of thing... I just copy pasted all the test commands 
into a big .sh file , did a git init -b main in a temp folder 
and ran the .sh file from there. And then observed the changes.
That big .sh file : https://pastebin.com/9QQD7qYA

> ---if so that is worth noting in
> the proposed log message), then "test ! -d dir" that is rewritten to
> "test_path_is_missing dir" is actually a _better_ test.

I am sending the v2 patch after this message. Should I have just put 
this whole thing into the v2 patch cover mail ? 

Best,
Soutrik

[GSOC PATCH v2] t7003: modernize path existence checks using test helpers

[SoutrikDas]

Replace 'test -f' and 'test -d' with Git's path
helpers. Strengthen the '! test -d dir' and
'! test -d diroh/dir' tests.

Checking the test setup before test 12 confirms
that there are no expected non directories named
'dir' or 'diroh/dir'

Signed-off-by: SoutrikDas <valusoutrik@gmail.com>
---
v2:
Address feedback from Junio C Hamano 
Acknowledge that the rewritten tests are stricter
---
 t/t7003-filter-branch.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 5ab4d41ee7..c475769858 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -92,8 +92,8 @@ test_expect_success 'rewrite, renaming a specific file' '
 
 test_expect_success 'test that the file was renamed' '
 	test D = "$(git show HEAD:doh --)" &&
-	! test -f D.t &&
-	test -f doh &&
+	test_path_is_missing D.t &&
+	test_path_is_file doh &&
 	test D = "$(cat doh)"
 '
 
@@ -103,10 +103,10 @@ test_expect_success 'rewrite, renaming a specific directory' '
 
 test_expect_success 'test that the directory was renamed' '
 	test dir/D = "$(git show HEAD:diroh/D.t --)" &&
-	! test -d dir &&
-	test -d diroh &&
-	! test -d diroh/dir &&
-	test -f diroh/D.t &&
+	test_path_is_missing dir &&
+	test_path_is_dir diroh &&
+	test_path_is_missing diroh/dir &&
+	test_path_is_file diroh/D.t &&
 	test dir/D = "$(cat diroh/D.t)"
 '
 
-- 
2.52.0