[PATCH] t7605: use test_path_is_file instead of test -f

[PATCH] t7605: use test_path_is_file instead of test -f

[Mansi Singh via GitGitGadget]

From: Mansi <mansimaanu8627@gmail.com>

Replace old-style 'test -f' path checks with the modern
test_path_is_file helper in the merge_c1_to_c2_cmds block.

The helper provides clearer failure messages and is the
established convention in Git's test suite.

These instances were found using:
  grep -rn "test -[efd]" t/ --include="*.sh"

Signed-off-by: Mansi <mansimaanu8627@gmail.com>
---
    t7605: use test_path_is_file instead of test -f

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2050%2FMansiSingh17%2Fgsoc-t7605-test-path-helpers-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2050/MansiSingh17/gsoc-t7605-test-path-helpers-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2050

 t/t7605-merge-resolve.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5d56c38546..44de97a480 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -34,9 +34,9 @@ merge_c1_to_c2_cmds='
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
 	git diff --exit-code &&
-	test -f c0.c &&
-	test -f c1.c &&
-	test -f c2.c &&
+	test_path_is_file c0.c &&
+	test_path_is_file c1.c &&
+	test_path_is_file c2.c &&
 	test 3 = $(git ls-tree -r HEAD | wc -l) &&
 	test 3 = $(git ls-files | wc -l)
 '

base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4
-- 
gitgitgadget

Re: [PATCH] t7605: use test_path_is_file instead of test -f

[Lucas Seiki Oshiro]

Hi, Mansi!

> Replace old-style 'test -f' path checks with the modern
> test_path_is_file helper in the merge_c1_to_c2_cmds block.
> 
> The helper provides clearer failure messages and is the
> established convention in Git's test suite.
> 
> These instances were found using:
>  grep -rn "test -[efd]" t/ --include="*.sh"

I don't think this information is relevant to be placed in
the commit description. Perhaps it would better placed after
the scissors mark (the --- after the message) which is sent
to the mailing list but ignored in the final commit.

Btw, since we're in Git we can use Git's special powers to
do that. This is equivalent to your command line:

   $ git grep 'test -[efd]' -- 't/*.sh'

And this may be more useful, separating the output per file
and using pathspecs to filter the tests (files that begin with
"t") from other helper scripts inside t/:

   $ git grep --heading --break 'test -[efd]' -- 't/t*.sh'

I wrote a blog post about git-grep and other tools [1] that
may be useful for you.

> - test -f c0.c &&
> - test -f c1.c &&
> - test -f c2.c &&
> + test_path_is_file c0.c &&
> + test_path_is_file c1.c &&
> + test_path_is_file c2.c &&

The code itself looks good to me!

[1] https://lucasoshiro.github.io/posts-en/2023-02-13-git-debug/#git-grep

[PATCH] t7605: use test_path_is_file instead of test -f

[Mansi Singh via GitGitGadget]

From: Mansi <mansimaanu8627@gmail.com>

Replace old-style 'test -f' path checks with the modern
test_path_is_file helper in the merge_c1_to_c2_cmds block.

The helper provides clearer failure messages and is the
established convention in Git's test suite.

These instances were found using:
  grep -rn "test -[efd]" t/ --include="*.sh"

Signed-off-by: Mansi <mansimaanu8627@gmail.com>
---
    t7605: use test_path_is_file instead of test -f

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2067%2FMansiSingh17%2Ffix-t7605-test-path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2067/MansiSingh17/fix-t7605-test-path-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2067

 t/t7605-merge-resolve.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5d56c38546..44de97a480 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -34,9 +34,9 @@ merge_c1_to_c2_cmds='
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
 	git diff --exit-code &&
-	test -f c0.c &&
-	test -f c1.c &&
-	test -f c2.c &&
+	test_path_is_file c0.c &&
+	test_path_is_file c1.c &&
+	test_path_is_file c2.c &&
 	test 3 = $(git ls-tree -r HEAD | wc -l) &&
 	test 3 = $(git ls-files | wc -l)
 '

base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2
-- 
gitgitgadget

Re: [PATCH] t7605: use test_path_is_file instead of test -f

[Junio C Hamano]

"Mansi Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

The e-mail header seems to imply you are "Mansi Singh".  Do you want
to be known to this community under that name, or just "Mansi"?

> From: Mansi <mansimaanu8627@gmail.com>
>
> Replace old-style 'test -f' path checks with the modern
> test_path_is_file helper in the merge_c1_to_c2_cmds block.
>
> The helper provides clearer failure messages and is the
> established convention in Git's test suite.

OK.

> These instances were found using:
>   grep -rn "test -[efd]" t/ --include="*.sh"

People seem to add the above paragraph to their test-path helper
patches, but unless the coverage of the work is fairly thorough and
you want to say "all the similar issues should be found with this
command and I addressed all of them", I do not see much point saying
how you found one of them and addressed it.

You could have used "git grep -e <pattern> -- t/\*.sh", or you could
have been working to fix something in t7605 and noticed these while
you were doing something else to the file.

I do not see it as too huge a deal and it is probably not a cause to
send in another iteration once it is already written, though.

> Signed-off-by: Mansi <mansimaanu8627@gmail.com>

No matter which name you pick, this should match the identity used
on your in-body "From:" header.  In this message you are using the
same "Mansi" with address, which is good, but see also
Documentation/SubmittingPatches::real-name section.

> diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
> index 5d56c38546..44de97a480 100755
> --- a/t/t7605-merge-resolve.sh
> +++ b/t/t7605-merge-resolve.sh
> @@ -34,9 +34,9 @@ merge_c1_to_c2_cmds='
>  	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
>  	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
>  	git diff --exit-code &&
> -	test -f c0.c &&
> -	test -f c1.c &&
> -	test -f c2.c &&
> +	test_path_is_file c0.c &&
> +	test_path_is_file c1.c &&
> +	test_path_is_file c2.c &&

The patch is quite straight-forward.  Good.

>  	test 3 = $(git ls-tree -r HEAD | wc -l) &&
>  	test 3 = $(git ls-files | wc -l)
>  '
>
> base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2

[PATCH v2] t7605: use test_path_is_file instead of test -f

[Mansi Singh via GitGitGadget]

From: Mansi Singh <mansimaanu8627@gmail.com>

Replace old-style 'test -f' path checks with the modern
test_path_is_file helper in the merge_c1_to_c2_cmds block.

The helper provides clearer failure messages and is the
established convention in Git's test suite.

Signed-off-by: Mansi Singh <mansimaanu8627@gmail.com>
---
    t7605: use test_path_is_file instead of test -f

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2067%2FMansiSingh17%2Ffix-t7605-test-path-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2067/MansiSingh17/fix-t7605-test-path-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2067

Range-diff vs v1:

 1:  8753a5d919 ! 1:  388d8d7118 t7605: use test_path_is_file instead of test -f
     @@
       ## Metadata ##
     -Author: Mansi <mansimaanu8627@gmail.com>
     +Author: Mansi Singh <mansimaanu8627@gmail.com>
      
       ## Commit message ##
          t7605: use test_path_is_file instead of test -f
     @@ Commit message
          The helper provides clearer failure messages and is the
          established convention in Git's test suite.
      
     -    These instances were found using:
     -      grep -rn "test -[efd]" t/ --include="*.sh"
     -
     -    Signed-off-by: Mansi <mansimaanu8627@gmail.com>
     +    Signed-off-by: Mansi Singh <mansimaanu8627@gmail.com>
      
       ## t/t7605-merge-resolve.sh ##
      @@ t/t7605-merge-resolve.sh: merge_c1_to_c2_cmds='


 t/t7605-merge-resolve.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5d56c38546..44de97a480 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -34,9 +34,9 @@ merge_c1_to_c2_cmds='
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
 	git diff --exit-code &&
-	test -f c0.c &&
-	test -f c1.c &&
-	test -f c2.c &&
+	test_path_is_file c0.c &&
+	test_path_is_file c1.c &&
+	test_path_is_file c2.c &&
 	test 3 = $(git ls-tree -r HEAD | wc -l) &&
 	test 3 = $(git ls-files | wc -l)
 '

base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2
-- 
gitgitgadget