Skip to contentSkip to conversation

t3501: use test_path_is_* functions

PATCH In review Labnan via GitGitGadget 4 participants 16 messages Mar 30, 2022 – Apr 22, 2022

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 t3501: use test_path_is_* functions

From: Labnann <[email protected]>

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

---
[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.

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(-)
t/t3501-revert-cherry-pick.sh +2 -2
index 8617efaaf1e..45492a7ed09 100755
@@ -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
 

base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 6 lines · Labnan via GitGitGadget

From: Labnann <[email protected]>

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

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

t/t3501-revert-cherry-pick.sh test_expect_success 'cherry-pick after renaming branch' ' · line 72

Quoted 12 lines
t/t3501-revert-cherry-pick.sh +1 -1
index 8617efaaf1e..45492a7ed09 100755
@@ -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".

t/t3501-revert-cherry-pick.sh test_expect_success 'revert after renaming branch' ' · line 82

Quoted 7 lines
t/t3501-revert-cherry-pick.sh +1 -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.

Quoted 4 lines
t/t3501-revert-cherry-pick.sh +0 -0
 	git reflog -1 | grep revert
 

base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

#2 lore raw

v2 cover t3501: remove test and test -f

Hello!
Thess remove test -f from t3501 and test command. It uses test_cmp to hide exit code of git.

Can I get help with "git reflog -1 | grep revert"? About which helper function I can use to hide the exit code.

Thank you.

Labnann (3):
t3501: use test_path_is_* functions
t3501: don't ignore "git <cmd>" exit code
t3501: test cherry-pick revert with oids

 t/t3501-revert-cherry-pick.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Range-diff against v1:

1:  8b7d38a66f = 1:  8b7d38a66f t3501: use test_path_is_* functions
-:  ---------- > 2:  04b0bf1c5d t3501: don't ignore "git <cmd>" exit code
-:  ---------- > 3:  36445b40fb t3501: test cherry-pick revert with oids
Signature · 1 line
2.35.1
#3 lore raw

PATCH v2 1/3 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

---
 t/t3501-revert-cherry-pick.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
t/t3501-revert-cherry-pick.sh +2 -2
index 8617efaaf1..45492a7ed0 100755
@@ -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
 
Signature · 1 line
2.35.1
#4 lore raw

PATCH v2 2/3 t3501: don't ignore "git <cmd>" exit code

In reply to Labnann

without this change this test will become flaky e.g under SANITIZE=leak if some (but not all) memory leaks revealed by these commands, according to c419562860e

---
 t/t3501-revert-cherry-pick.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
t/t3501-revert-cherry-pick.sh +8 -2
index 45492a7ed0..bd19c272d6 100755
@@ -44,6 +44,12 @@ test_expect_success setup '
 	git tag rename2
 '
 
+test_cmp_rev_parse () {
+	git rev-parse $1 >expect &&
+	git rev-parse $2 >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success 'cherry-pick --nonsense' '
 
 	pos=$(git rev-parse HEAD) &&
@@ -66,7 +72,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_cmp_rev_parse HEAD^ rename2 &&
 	test_path_is_file opos &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
@@ -77,7 +83,7 @@ test_expect_success 'revert after renaming branch' '
 
 	git checkout rename1 &&
 	git revert added &&
-	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
+	test_cmp_rev_parse HEAD^ rename1 &&
 	test_path_is_file spoo &&
 	! grep "Add extra line at the end" spoo &&
 	git reflog -1 | grep revert
Signature · 1 line
2.35.1
#5 lore raw

Re: [PATCH v2 2/3] t3501: don't ignore "git <cmd>" exit code

Quoted 3 lines · Labnann

without this change this test will become flaky e.g under SANITIZE=leak if some (but not all) memory leaks revealed by these commands, according to c419562860e

The body of the proposed log message is *not* a continuation (the latter half) of a sentence that the title started, but should be a full sentence on its own. Capitalize the first word as usual.

Also, the leak check should not be the primary reason to do this change. We do not want to miss "git rev-parse" used in these tests starts to fail, regardless of why they fail. Perhaps like this instead?

Otherwise, we would not notice when "git rev-parse" starts segfaulting without emitting any output. The 'test' command would end up being just "test =", which yields success.

t/t3501-revert-cherry-pick.sh test_expect_success setup ' · line 51

Quoted 5 lines
t/t3501-revert-cherry-pick.sh +5 -0
+test_cmp_rev_parse () {
+	git rev-parse $1 >expect &&
+	git rev-parse $2 >actual &&
+	test_cmp expect actual
+}

To me, it looks like we can just use test_cmp_rev that already exists, but am I missing some subtlety?

If not (then we need to explain why in the proposed commit log message), at least we should place $1 and $2 inside a pair of double-quotes. For the current callers, what they pass happen not to need such quoting, but once we write a helper function, we are helping _future_ callers as well, and we should be reasonably prepared for them.

t/t3501-revert-cherry-pick.sh test_expect_success 'cherry-pick after renaming branch' ' · line 75

Quoted 9 lines

test_expect_success 'cherry-pick --nonsense' '

t/t3501-revert-cherry-pick.sh +1 -1
 	pos=$(git rev-parse HEAD) &&
@@ -66,7 +72,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_cmp_rev_parse HEAD^ rename2 &&

OK.

t/t3501-revert-cherry-pick.sh test_expect_success 'revert after renaming branch' ' · line 86

Quoted 9 lines
t/t3501-revert-cherry-pick.sh +1 -1
 	test_path_is_file opos &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
@@ -77,7 +83,7 @@ test_expect_success 'revert after renaming branch' '
 
 	git checkout rename1 &&
 	git revert added &&
-	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
+	test_cmp_rev_parse HEAD^ rename1 &&

OK.

t/t3501-revert-cherry-pick.sh test_expect_success 'revert after renaming branch' ' · line 89

Quoted 3 lines
t/t3501-revert-cherry-pick.sh +0 -0
 	test_path_is_file spoo &&
 	! grep "Add extra line at the end" spoo &&
 	git reflog -1 | grep revert

Thanks.

#6 lore raw

PATCH v2 3/3 t3501: test cherry-pick revert with oids

In reply to Labnann

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

---
 t/t3501-revert-cherry-pick.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +3 -5
index bd19c272d6..08103923ab 100755
@@ -72,8 +72,7 @@ test_expect_success 'cherry-pick after renaming branch' '
 
 	git checkout rename2 &&
 	git cherry-pick added &&
-	test_cmp_rev_parse HEAD^ rename2 &&
-	test_path_is_file opos &&
+	test_cmp_rev_parse rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -83,9 +82,8 @@ test_expect_success 'revert after renaming branch' '
 
 	git checkout rename1 &&
 	git revert added &&
-	test_cmp_rev_parse HEAD^ rename1 &&
-	test_path_is_file spoo &&
-	! grep "Add extra line at the end" spoo &&
+	test_cmp_rev_parse rename1 HEAD^ &&
+	test_cmp_rev_parse initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '
Signature · 1 line
2.35.1
#7 lore raw

Re: [PATCH v2 3/3] t3501: test cherry-pick revert with oids

Quoted 3 lines · Labnann

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

Don't indent the first line by a space, and capitalize the first letter of the full sentence as usual. End the sentence with a full stop.

I did not quite get the "blob OIDs" reference; does that refer to the changes in both hunks?

Quoted 10 lines
---
 t/t3501-revert-cherry-pick.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +0 -0
index bd19c272d6..08103923ab 100755

t/t3501-revert-cherry-pick.sh test_expect_success 'cherry-pick after renaming branch' ' · line 76

Quoted 7 lines
t/t3501-revert-cherry-pick.sh +1 -2
@@ -72,8 +72,7 @@ test_expect_success 'cherry-pick after renaming branch' '
 
 	git checkout rename2 &&
 	git cherry-pick added &&
-	test_cmp_rev_parse HEAD^ rename2 &&
-	test_path_is_file opos &&
+	test_cmp_rev_parse rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&

This change is not quite explained anywhere. Why do we have to swap HEAD^ and rename2? I agree that we do not have to make sure that opos exists since we are going to run "grep" on it in a later step anyway, and the lack of that file will be detected as a failure, but these two changes deserve mention in the proposed log message.

t/t3501-revert-cherry-pick.sh test_expect_success 'revert after renaming branch' ' · line 86

Quoted 11 lines
t/t3501-revert-cherry-pick.sh +2 -3
 	git reflog -1 | grep cherry-pick
 
@@ -83,9 +82,8 @@ test_expect_success 'revert after renaming branch' '
 
 	git checkout rename1 &&
 	git revert added &&
-	test_cmp_rev_parse HEAD^ rename1 &&
-	test_path_is_file spoo &&
-	! grep "Add extra line at the end" spoo &&
+	test_cmp_rev_parse rename1 HEAD^ &&
+	test_cmp_rev_parse initial:oops HEAD:spoo &&

Again, did we need to swap and if so why?

So instead of allowing spoo to be some random garbage as long as it does not have the "Add extra line" message, we can exactly predict what the contents of the correct result, which is to end up with the identical contents in oops of the initial commit. OK, that makes sense.

Swap the order of revisions being compared in two tests for such and such reasons. In one test, stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway. In the other test, instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit.

or something like that, perhaps.

Strictly speaking, this used to check the working tree files but now it checks the contents of the resulting commit. I wonder if we need to ensure that the HEAD, the index and the working tree are all updated the same way, or if that is too basic, obvious, and (most importantly) already tested elsewhere?

Thanks.

Quoted 3 lines
t/t3501-revert-cherry-pick.sh +0 -0
 	git reflog -1 | grep revert
 
 '
#8 lore raw

v3 cover t3501: remove redundant file checking and stop ignoring git <cmd> exit code

In reply to Labnann

From: Labnan <[email protected]>

T3501 contains two test cases where we used to check if a file exists using test -f which is not necessary because we already have grep after it. Also the use of git rev-parse can be hidden using test_cmp_rev.

This patch tries to resolve these two issues in T3501.

Sorry for the extra email that was not sent to the right place. I am still struggling to work with the mailing list.

Labnann (1):
t3501: remove redundant file check and stop ignoring git <cmd> exit
code

 t/t3501-revert-cherry-pick.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Range-diff against v2:

1:  8b7d38a66f < -:  ---------- t3501: use test_path_is_* functions
2:  04b0bf1c5d < -:  ---------- t3501: don't ignore "git <cmd>" exit code
3:  36445b40fb < -:  ---------- t3501: test cherry-pick revert with oids
-:  ---------- > 1:  aeb28e3249 t3501: remove redundant file check and stop ignoring git <cmd> exit code
Signature · 1 line
2.35.1.windows.2
#9 lore raw

PATCH v3 1/1 t3501: remove redundant file check and stop ignoring git <cmd> exit code

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway.

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit.

In both tests, we would not notice when "git rev-parse" starts segfaulting without emitting any output. The 'test' command would end up being just "test =", which yields success. Therefore we could use test_cmp_rev

---
 t/t3501-revert-cherry-pick.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +3 -5
index 8617efaaf1..ad8f0cae5a 100755
@@ -66,8 +66,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_cmp_rev rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -77,9 +76,8 @@ 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 &&
+	test_cmp_rev rename1 HEAD^ &&
+	test_cmp_rev initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '
Signature · 1 line
2.35.1.windows.2
#10 lore raw

Re: [PATCH v3 1/1] t3501: remove redundant file check and stop ignoring git <cmd> exit code

Quoted 3 lines · Labnann

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway.

OK.

Quoted 5 lines

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit.

Makes sense. The code removes "test -f spoo", which is not explained in the above, and I do not think we want to. We'd notice a breakage where revert leaves the right result in HEAD but fails to match the working tree files if we leave it there.

Quoted 4 lines

In both tests, we would not notice when "git rev-parse" starts segfaulting without emitting any output. The 'test' command would end up being just "test =", which yields success. Therefore we could use test_cmp_rev

The two sentences are right. The conclusion is a bit iffy; it is not "we could", which implies that the current one is OK but it is OK to also rewrite it to use test_cmp_rev.

Use the 'test_cmp_rev' helper to make sure we will notice such a breakage.

or something like that, perhaps?

Quoted 1 line

Is Labnann your name? I do not know where you are from so forgive me if it is. We prefer people to sign with their real names (cf. Documentation/SubmittingPatches[[real-name]]) on this line (and because the name/e-mail on this line must match the author's ident, the same preference applies there, too).

Quoted 20 lines
---
 t/t3501-revert-cherry-pick.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +1 -2
index 8617efaaf1..ad8f0cae5a 100755
@@ -66,8 +66,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_cmp_rev rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -77,9 +76,8 @@ test_expect_success 'revert after renaming branch' '
 

t/t3501-revert-cherry-pick.sh test_expect_success 'revert after renaming branch' ' · line 83

Quoted 10 lines
t/t3501-revert-cherry-pick.sh +2 -3
 	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 &&
+	test_cmp_rev rename1 HEAD^ &&
+	test_cmp_rev initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '

The diff looks almost perfect here (modulo that "test -f spoo" is better kept, I would think).

Thanks.

#11 lore raw

v4 cover t3501: remove test -f and stop ignoring git <cmd> exit code

In reply to Labnann

Resending this email because the previous email did not seem to reach gitgitgadget's PR and the mailing list.

T3501 contains two test cases where we used to check if a file exists using test -f which is not necessary because we already have helper function for it. In fact in one test case test -f is redundant because we grep right after it. Also the use of git rev-parse can cause segfault.

This patch tries to resolve these two issues in T3501.

Khalid Masum (1):
t3501: remove test -f and stop ignoring git <cmd> exit code

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

Range-diff against v3:
1: aeb28e3249 ! 1: 1090429b86 t3501: remove redundant file check and stop ignoring git <cmd> exit code
@@
## Metadata ##
-Author: Labnann <[email protected]>
+Author: Khalid Masum <[email protected]>

## Commit message ##
- t3501: remove redundant file check and stop ignoring git <cmd> exit code
+ t3501: remove test -f and stop ignoring git <cmd> exit code

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in
@@ Commit message

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can
- expect the exact outcome---after the successful revert of
- "added", the contents of file "spoo" should become identical to
- what was in file "oops" in the "initial" commit.
+ expect the exact outcome---after the successful revert of "added", the
+ contents of file "spoo" should become identical to what was in file
+ "oops" in the "initial" commit. This test also contains 'test -f' that
+ verifies presence of a file, but we have a helper function to do the same
+ thing. Replace it with appropriate helper function 'test_path_is_file'
+ for better readability and better error messages.

- In both tests, we would not notice when "git rev-parse" starts
- segfaulting without emitting any output. The 'test' command would
- end up being just "test =", which yields success. Therefore we could
- use test_cmp_rev
+ In both tests, we will not notice when "git rev-parse" starts segfaulting
+ without emitting any output.  The 'test' command will end up being just
+ "test =", which yields success. Use the 'test_cmp_rev' helper to make
+ sure we will notice such a breakage.

- Signed-off-by: Labnann <[email protected]>
+ Signed-off-by: Khalid Masum <[email protected]>

## t/t3501-revert-cherry-pick.sh ##
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick after renaming branch' '
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'revert after renaming branch
- test -f spoo &&
- ! grep "Add extra line at the end" spoo &&
+ test_cmp_rev rename1 HEAD^ &&
++ test_path_is_file spoo &&
+ test_cmp_rev initial:oops HEAD:spoo &&
git reflog -1 | grep revert

Signature · 1 line
2.35.1.windows.2
#12 lore raw

PATCH v4 1/1 t3501: remove test -f and stop ignoring git <cmd> exit code

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway.

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit. This test also contains 'test -f' that verifies presence of a file, but we have a helper function to do the same thing. Replace it with appropriate helper function 'test_path_is_file' for better readability and better error messages.

In both tests, we will not notice when "git rev-parse" starts segfaulting without emitting any output.  The 'test' command will end up being just "test =", which yields success. Use the 'test_cmp_rev' helper to make sure we will notice such a breakage.

---
 t/t3501-revert-cherry-pick.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +4 -5
index 8617efaaf1..9eb19204ac 100755
@@ -66,8 +66,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_cmp_rev rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -77,9 +76,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 &&
+	test_cmp_rev rename1 HEAD^ &&
+	test_path_is_file spoo &&
+	test_cmp_rev initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '
Signature · 1 line
2.35.1.windows.2
#13 lore raw

v4 cover t3501: remove test -f and stop ignoring git <cmd> exit code

In reply to Labnann

T3501 contains two test cases where we used to check if a file exists using test -f which is not necessary because we already have helper function for it. In fact in one test case test -f is redundant because we grep right after it. Also the use of git rev-parse can cause segfault.

This patch tries to resolve these two issues in T3501.

Khalid Masum (1):
t3501: remove test -f and stop ignoring git <cmd> exit code

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

Range-diff against v3:
1: aeb28e3249 ! 1: 1090429b86 t3501: remove redundant file check and stop ignoring git <cmd> exit code
@@
## Metadata ##
-Author: Labnann <[email protected]>
+Author: Khalid Masum <[email protected]>

## Commit message ##
- t3501: remove redundant file check and stop ignoring git <cmd> exit code
+ t3501: remove test -f and stop ignoring git <cmd> exit code

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in
@@ Commit message

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can
- expect the exact outcome---after the successful revert of
- "added", the contents of file "spoo" should become identical to
- what was in file "oops" in the "initial" commit.
+ expect the exact outcome---after the successful revert of "added", the
+ contents of file "spoo" should become identical to what was in file
+ "oops" in the "initial" commit. This test also contains 'test -f' that
+ verifies presence of a file, but we have a helper function to do the same
+ thing. Replace it with appropriate helper function 'test_path_is_file'
+ for better readability and better error messages.

- In both tests, we would not notice when "git rev-parse" starts
- segfaulting without emitting any output. The 'test' command would
- end up being just "test =", which yields success. Therefore we could
- use test_cmp_rev
+ In both tests, we will not notice when "git rev-parse" starts segfaulting
+ without emitting any output.  The 'test' command will end up being just
+ "test =", which yields success. Use the 'test_cmp_rev' helper to make
+ sure we will notice such a breakage.

- Signed-off-by: Labnann <[email protected]>
+ Signed-off-by: Khalid Masum <[email protected]>

## t/t3501-revert-cherry-pick.sh ##
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick after renaming branch' '
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'revert after renaming branch
- test -f spoo &&
- ! grep "Add extra line at the end" spoo &&
+ test_cmp_rev rename1 HEAD^ &&
++ test_path_is_file spoo &&
+ test_cmp_rev initial:oops HEAD:spoo &&
git reflog -1 | grep revert

Signature · 1 line
2.35.1.windows.2
#14 lore raw

PATCH v4 1/1 t3501: remove test -f and stop ignoring git <cmd> exit code

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway.

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit. This test also contains 'test -f' that verifies presence of a file, but we have a helper function to do the same thing. Replace it with appropriate helper function 'test_path_is_file' for better readability and better error messages.

In both tests, we will not notice when "git rev-parse" starts segfaulting without emitting any output.  The 'test' command will end up being just "test =", which yields success. Use the 'test_cmp_rev' helper to make sure we will notice such a breakage.

---
 t/t3501-revert-cherry-pick.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +4 -5
index 8617efaaf1..9eb19204ac 100755
@@ -66,8 +66,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_cmp_rev rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -77,9 +76,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 &&
+	test_cmp_rev rename1 HEAD^ &&
+	test_path_is_file spoo &&
+	test_cmp_rev initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '
Signature · 1 line
2.35.1.windows.2
#15 lore raw

PATCH v2 1/1 t3501: remove test -f and stop ignoring git <cmd> exit code

In reply to Labnan via GitGitGadget

From: Khalid Masum <[email protected]>

In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway.

In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit. This test also contains 'test -f' that verifies presence of a file, but we have a helper function to do the same thing. Replace it with appropriate helper function 'test_path_is_file' for better readability and better error messages.

In both tests, we will not notice when "git rev-parse" starts segfaulting without emitting any output.  The 'test' command will end up being just "test =", which yields success. Use the 'test_cmp_rev' helper to make sure we will notice such a breakage.

---
t3501: remove redundant test -f and use of git rev-parse

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.

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

Range-diff vs v1:

1: 8b7d38a66f8 ! 1: 1090429b865 t3501: use test_path_is_* functions
@@
## Metadata ##
-Author: Labnann <[email protected]>
+Author: Khalid Masum <[email protected]>

## Commit message ##
- t3501: use test_path_is_* functions
+ t3501: remove test -f and stop ignoring git <cmd> exit code

- Two test -f are present in t3501. They can be replaced with appropriate
- helper function: test_path_is_file
+ In the test 'cherry-pick after renaming branch', stop checking for
+ the presence of a file (opos) because we are going to "grep" in it in
+ the same test and the lack of it will be noticed as a failure anyway.

- Signed-off-by: Labnann <[email protected]>
+ In the test 'revert after renaming branch', instead of allowing any
+ random contents as long as a known phrase is not there in it, we can
+ expect the exact outcome---after the successful revert of "added", the
+ contents of file "spoo" should become identical to what was in file
+ "oops" in the "initial" commit. This test also contains 'test -f' that
+ verifies presence of a file, but we have a helper function to do the same
+ thing. Replace it with appropriate helper function 'test_path_is_file'
+ for better readability and better error messages.
+
+ In both tests, we will not notice when "git rev-parse" starts segfaulting
+ without emitting any output.  The 'test' command will end up being just
+ "test =", which yields success. Use the 'test_cmp_rev' helper to make
+ sure we will notice such a breakage.
+
+ Signed-off-by: Khalid Masum <[email protected]>

## t/t3501-revert-cherry-pick.sh ##
@@ t/t3501-revert-cherry-pick.sh: 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 $(git rev-parse HEAD^) = $(git rev-parse rename2) &&
- test -f opos &&
-+ test_path_is_file opos &&
++ test_cmp_rev rename2 HEAD^ &&
grep "Add extra line at the end" opos &&
git reflog -1 | grep cherry-pick

@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'revert after renaming branch' '
+
git checkout rename1 &&
git revert added &&
- test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
+- test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
- test -f spoo &&
+- ! grep "Add extra line at the end" spoo &&
++ test_cmp_rev rename1 HEAD^ &&
+ test_path_is_file spoo &&
- ! grep "Add extra line at the end" spoo &&
++ test_cmp_rev initial:oops HEAD:spoo &&
git reflog -1 | grep revert

+ '

 t/t3501-revert-cherry-pick.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
t/t3501-revert-cherry-pick.sh +4 -5
index 8617efaaf1e..9eb19204ac7 100755
@@ -66,8 +66,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_cmp_rev rename2 HEAD^ &&
 	grep "Add extra line at the end" opos &&
 	git reflog -1 | grep cherry-pick
 
@@ -77,9 +76,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 &&
+	test_cmp_rev rename1 HEAD^ &&
+	test_path_is_file spoo &&
+	test_cmp_rev initial:oops HEAD:spoo &&
 	git reflog -1 | grep revert
 
 '

base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b

Signature · 1 line
gitgitgadget