[GSoC PATCH] t3310: replace test -f/-d with test_path_is_file/test_path_is_dir

[GSoC PATCH] t3310: replace test -f/-d with test_path_is_file/test_path_is_dir

[Francesco Paparatto]

Replace old-style path assertions with modern helpers that provide
clearer diagnostic messages on failure. That's because when test -f fails, the
output gives no indication of what went wrong.

These instances were found using: git grep "test -[efd]" t/ as suggested in the microproject ideas.

Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 597df5ebc0..f0054b0a39 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -260,7 +260,7 @@ test_expect_success 'change notes in z' '
 '
 
 test_expect_success 'cannot do merge w/conflicts when previous merge is unfinished' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&
 	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should indicate what is wrong
 	test_grep -q "\\.git/NOTES_MERGE_\\* exists" output
@@ -320,7 +320,7 @@ w notes on 1st commit
 EOF
 
 test_expect_success 'can do merge without conflicts even if previous merge is unfinished (x => w)' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&
 	git notes merge x &&
 	verify_notes w &&
 	# Verify that other notes refs has not changed (x and y)
@@ -564,10 +564,10 @@ EOF
 	# NOTES_MERGE_* refs and .git/NOTES_MERGE_* state files must remain
 	git rev-parse --verify NOTES_MERGE_PARTIAL &&
 	git rev-parse --verify NOTES_MERGE_REF &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# Refs are unchanged
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
 	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
-- 
2.52.0

Re: [GSoC PATCH] t3310: replace test -f/-d with test_path_is_file/test_path_is_dir

[Junio C Hamano]

Francesco Paparatto <francescopaparatto@gmail.com> writes:

> Replace old-style path assertions with modern helpers that provide
> clearer diagnostic messages on failure. That's because when test -f fails, the
> output gives no indication of what went wrong.
>
> These instances were found using: git grep "test -[efd]" t/ as suggested in the microproject ideas.

An overly long line.  Keep the display columns for a line in a patch
e-mail below ~70 columns, so that after getting quoted "> " for a
few times, the result will still fit in 80-column terminals.

>  test_expect_success 'cannot do merge w/conflicts when previous merge is unfinished' '
> -	test -d .git/NOTES_MERGE_WORKTREE &&
> +	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&

OK.

> @@ -320,7 +320,7 @@ w notes on 1st commit
>  EOF
>  
>  test_expect_success 'can do merge without conflicts even if previous merge is unfinished (x => w)' '
> -	test -d .git/NOTES_MERGE_WORKTREE &&
> +	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&

OK.

> @@ -564,10 +564,10 @@ EOF
>  	# NOTES_MERGE_* refs and .git/NOTES_MERGE_* state files must remain
>  	git rev-parse --verify NOTES_MERGE_PARTIAL &&
>  	git rev-parse --verify NOTES_MERGE_REF &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
> +	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
> +	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
> +	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
> +	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&

OK.

>  	# Refs are unchanged
>  	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
>  	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&

Here is a pair of opportunities for others that want more
microproject exercises to make sure we do not ignore failing "git"
invocations.

You do not want to address them, of course.  You've had your share
already ;-)

Thanks.

Re: [GSoC PATCH] t3310: replace test -f/-d with test_path_is_file/test_path_is_dir

[Francesco Paparatto]

Junio C Hamano <gitster@pobox.com> writes:

> An overly long line.  Keep the display columns for a line in a patch
> e-mail below ~70 columns, so that after getting quoted "> " for a
> few times, the result will still fit in 80-column terminals.

Thanks for the review. I'll resend with the commit message
line wrapped.

Francesco

[GSoC PATCH v2] t3310: replace test -f/-d with test_path_is_file/test_path_is_dir

[Francesco Paparatto]

Replace old-style path assertions with modern helpers that
provide clearer diagnostic messages on failure. When test -f
fails, the output gives no indication of what went wrong.

These instances were found using:

  git grep "test -[efd]" t/

as suggested in the microproject ideas.

Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 597df5ebc0..f0054b0a39 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -260,7 +260,7 @@ test_expect_success 'change notes in z' '
 '
 
 test_expect_success 'cannot do merge w/conflicts when previous merge is unfinished' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&
 	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should indicate what is wrong
 	test_grep -q "\\.git/NOTES_MERGE_\\* exists" output
@@ -320,7 +320,7 @@ w notes on 1st commit
 EOF
 
 test_expect_success 'can do merge without conflicts even if previous merge is unfinished (x => w)' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test_path_is_dir .git/NOTES_MERGE_WORKTREE &&
 	git notes merge x &&
 	verify_notes w &&
 	# Verify that other notes refs has not changed (x and y)
@@ -564,10 +564,10 @@ EOF
 	# NOTES_MERGE_* refs and .git/NOTES_MERGE_* state files must remain
 	git rev-parse --verify NOTES_MERGE_PARTIAL &&
 	git rev-parse --verify NOTES_MERGE_REF &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# Refs are unchanged
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
 	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
-- 
2.52.0