Re: [PATCH] t3501: use test_path_is_* functions

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> From: Labnann <khalid.masum.92@gmail.com>
>
> Two test -f are present in t3501. They can be replaced with appropriate
> helper function: test_path_is_file
>
> Signed-off-by: Labnann <khalid.masum.92@gmail.com>

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