On Wed, Mar 30 2022, Labnan via GitGitGadget wrote: > From: Labnann <khalid.masum.92@xxxxxxxxx> > > 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@xxxxxxxxx> 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
