t3501: use test_path_is_* functions

Commit Message

Khalid Masum March 30, 2022, 9:43 p.m. UTC

From: Labnann <[e-mail]>

Two test -f are present in t3501. They can be replaced with appropriate
helper function: test_path_is_file

Signed-off-by: Labnann <[e-mail]>
---
    [GSoC][PATCH] t3501: Use test_path_is_* Functions
    
    Two test -f are present in t3501. They can be replaced with appropriate
    helper function: test_path_is_file. Which makes the script more readable
    and gives better error messages.
    
    Signed-off-by: Labnann [e-mail]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1195%2FLabnann%2Ft3501-helper-functions-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1195/Labnann/t3501-helper-functions-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1195

 t/t3501-revert-cherry-pick.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

Comments

Ævar Arnfjörð Bjarmason March 30, 2022, 10:11 p.m. UTC | #1

On Wed, Mar 30 2022, Labnan via GitGitGadget wrote:

> From: Labnann <[e-mail]>
>
> Two test -f are present in t3501. They can be replaced with appropriate
> helper function: test_path_is_file
>
> Signed-off-by: Labnann <[e-mail]>

Thanks for contributing to git, this is a much needed area of
improvement.

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 8617efaaf1e..45492a7ed09 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -67,7 +67,7 @@ test_expect_success 'cherry-pick after renaming branch' '
>  	git checkout rename2 &&
>  	git cherry-pick added &&
>  	test $(git rev-parse HEAD^) = $(git rev-parse rename2) &&
> -	test -f opos &&
> +	test_path_is_file opos &&
>  	grep "Add extra line at the end" opos &&
>  	git reflog -1 | grep cherry-pick

While perfect shouldn't be the enemy of the good, I think it would also
be a very nice use of review bandwidth to end up with some good
end-state here if possible.

I.e. a pre-existing issue here is:

 * We are hiding the exit code of git due to using "test", see
   c419562860e (checkout tests: don't ignore "git <cmd>" exit code,
   2022-03-07) for one example of how to fixthat.

 * The "test -f" here is really redundant to begin with, because we'd
   get an error from "grep" if opos didn't exist.

 * Ditto ignoring the exit code of "git reflog -1".

> @@ -78,7 +78,7 @@ test_expect_success 'revert after renaming branch' '
>  	git checkout rename1 &&
>  	git revert added &&
>  	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
> -	test -f spoo &&
> +	test_path_is_file spoo &&
>  	! grep "Add extra line at the end" spoo &&


Same issues here, except that the "test -f" aka "test_path_is_file"
isn't redundant, as the grep is inverted.

It seems to me (other issues aside) that what this test actually wants
to express is something like this:
	
	diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
	index 8617efaaf1e..00096b75376 100755
	--- a/t/t3501-revert-cherry-pick.sh
	+++ b/t/t3501-revert-cherry-pick.sh
	@@ -78,8 +78,9 @@ test_expect_success 'revert after renaming branch' '
	 	git checkout rename1 &&
	 	git revert added &&
	 	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
	-	test -f spoo &&
	-	! grep "Add extra line at the end" spoo &&
	+	git rev-parse initial:oops >expect &&
	+	git rev-parse HEAD:spoo >actual &&
	+	test_cmp expect actual &&
	 	git reflog -1 | grep revert
	 
	 '

I.e. we did a revert of a file we had so that it's the same as in
"initial", but now it's at a different path, which we can exhaustively
do by checking the blob OIDs.

>  	git reflog -1 | grep revert
>  
>
> base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

Patch

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 8617efaaf1e..45492a7ed09 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -67,7 +67,7 @@  test_expect_success 'cherry-pick after renaming branch' '
 	git checkout rename2 &&
 	git cherry-pick added &&
 	test $(git rev-parse HEAD^) = $(git rev-parse rename2) &&
-	test -f opos &&
+	test_path_is_file opos &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -78,7 +78,7 @@  test_expect_success 'revert after renaming branch' '
 	git checkout rename1 &&
 	git revert added &&
 	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
-	test -f spoo &&
+	test_path_is_file spoo &&
 	! grep "Add extra line at the end" spoo &&
 	git reflog -1 | grep revert