t: replace incorrect test_must_fail usage (part 3)

PATCH Superseded Denton Liu 3 participants 30 messages Mar 25, 2020 – Mar 27, 2020

Share & tools

Denton Liu

author

2020-03-25 05:55 UTC #0 lore raw

cover t: replace incorrect test_must_fail usage (part 3)

The overall scope of these patches is to replace inappropriate uses of test_must_fail. IOW, we should only allow test_must_fail to run on git and test-tool. Ultimately, we will conclude by making test_must_fail error out on non-git commands. An advance view of the final series can be found here[1].

This is the third part. It focuses on t5*.sh.

The first part can be found here[2]. The second part can be found here[3].

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/

Denton Liu (8):
t5512: don't use test_must_fail test_cmp
t5512: generate references with generate_references()
t5512: stop losing return codes of git commands
t5550: remove use of test_might_fail grep
t5607: reorder nongit test_must_fail
t5612: don't use test_must_fail test_cmp
t5612: stop losing return codes of git commands
t5801: teach compare_refs() to accept !

 t/t5512-ls-remote.sh       | 66 +++++++++++++++++---------------------
 t/t5550-http-fetch-dumb.sh |  2 +-
 t/t5607-clone-bundle.sh    |  2 +-
 t/t5612-clone-refspec.sh   | 26 +++++++--------
 t/t5801-remote-helpers.sh  | 10 ++++--
 5 files changed, 52 insertions(+), 54 deletions(-)

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #1 lore raw

PATCH 1/8 t5512: don't use `test_must_fail test_cmp`

The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. Since test_cmp() just wraps an external command, replace test_must_fail test_cmp with ! test_cmp.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5512-ls-remote.sh +1 -1

index 04b35402c7..08b98f12b8 100755
@@ -92,7 +92,7 @@ test_expect_success 'use "origin" when no remote specified' '
 
 test_expect_success 'suppress "From <url>" with -q' '
 	git ls-remote -q 2>actual_err &&
-	test_must_fail test_cmp exp_err actual_err
+	! test_cmp exp_err actual_err
 '
 
 test_expect_success 'use branch.<name>.remote if possible' '

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #2 lore raw

PATCH 2/8 t5512: generate references with generate_references()

In reply to Denton Liu

The expected references are generated using a here-doc with some inline subshells. If one of the git rev-parse invocations within the subshells failed, its return code is swallowed and we won't know about it. Replace these here-docs with generate_references(), which actually reports when git rev-parse fails.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 50 +++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

t/t5512-ls-remote.sh +21 ~~-29~~

index 08b98f12b8..62d02152c7 100755
@@ -4,6 +4,14 @@ test_description='git ls-remote'
 
 . ./test-lib.sh
 
+generate_references () {
+	for i
+	do
+		oid=$(git rev-parse "$i") || return 1
+		printf '%s\t%s\n' "$oid" "$i"
+	done
+}
+
 test_expect_success setup '
 	>file &&
 	git add file &&

@@ -43,34 +51,19 @@ test_expect_success 'ls-remote self' '
 '
 
 test_expect_success 'ls-remote --sort="version:refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark)	refs/tags/mark
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	EOF
+	generate_references refs/tags/mark refs/tags/mark1.1 refs/tags/mark1.2 refs/tags/mark1.10 >expect &&
 	git ls-remote --sort="version:refname" --tags self >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark)	refs/tags/mark
-	EOF
+	generate_references refs/tags/mark1.10 refs/tags/mark1.2 refs/tags/mark1.1 refs/tags/mark >expect &&
 	git ls-remote --sort="-version:refname" --tags self >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-remote --sort="-refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark)	refs/tags/mark
-	EOF
+	generate_references refs/tags/mark1.2 refs/tags/mark1.10 refs/tags/mark1.1 refs/tags/mark >expect &&
 	git ls-remote --sort="-refname" --tags self >actual &&
 	test_cmp expect actual
 '

@@ -212,17 +205,16 @@ test_expect_success 'protocol v2 supports hiderefs' '
 
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
-	cat >expect <<-EOF &&
-	ref: refs/heads/master	HEAD
-	$(git rev-parse HEAD)	HEAD
-	$(git rev-parse refs/heads/master)	refs/heads/master
-	$(git rev-parse HEAD)	refs/remotes/origin/HEAD
-	$(git rev-parse refs/remotes/origin/master)	refs/remotes/origin/master
-	$(git rev-parse refs/tags/mark)	refs/tags/mark
-	$(git rev-parse refs/tags/mark1.1)	refs/tags/mark1.1
-	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
-	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
-	EOF
+	echo "ref: refs/heads/master	HEAD" >expect &&
+	generate_references HEAD \
+		refs/heads/master >>expect &&
+	oid=$(git rev-parse HEAD) &&
+	echo "$oid	refs/remotes/origin/HEAD" >>expect &&
+	generate_references refs/remotes/origin/master \
+		refs/tags/mark \
+		refs/tags/mark1.1 \
+		refs/tags/mark1.10 \
+		refs/tags/mark1.2 >>expect &&
 	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
 	# protocol v0 here.
 	GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&

Signature · 1 line

2.25.0.114.g5b0ca878e0

Eric Sunshine

2020-03-25 06:08 UTC #3 lore raw

Re: [PATCH 2/8] t5512: generate references with generate_references()

Quoted 1 line · Denton Liu

t5512: generate references with generate_references()

This summary doesn't say anything useful. How about this instead?

t5512: stop losing git exit code in here-docs

Quoted 3 lines

The expected references are generated using a here-doc with some inline subshells. If one of the git rev-parse invocations within the subshells failed, its return code is swallowed and we won't know about

s/failed/fails/

Quoted 2 lines

it. Replace these here-docs with generate_references(), which actually reports when git rev-parse fails.

A couple nits below...

t/t5512-ls-remote.sh test_description='git ls-remote' · line 8

Quoted 9 lines

Signed-off-by Denton Liu <[email protected]>

---

t/t5512-ls-remote.sh +5 -0

@@ -4,6 +4,14 @@ test_description='git ls-remote'
+generate_references () {
+       for i
+       do
+               oid=$(git rev-parse "$i") || return 1
+               printf '%s\t%s\n' "$oid" "$i"

I think the more usual way to say this in our test suite would be:

    oid=$(git rev-parse "$i") &&
    printf '%s\t%s\n' "$oid" "$i" || return 1

which has the nice property that someone can come along and insert additional code in the loop body before the final "|| return 1" without having to spend a lot of time trying to work out if the &&-chain is intact or broken.

t/t5512-ls-remote.sh test_expect_success 'ls-remote self' ' · line 52

Quoted 11 lines

t/t5512-ls-remote.sh +3 -6

+       done
+}

@@ -43,34 +51,19 @@ test_expect_success 'ls-remote self' '
 test_expect_success 'ls-remote --sort="version:refname" --tags self' '
-       cat >expect <<-EOF &&
-       $(git rev-parse mark)   refs/tags/mark
-       $(git rev-parse mark1.1)        refs/tags/mark1.1
-       $(git rev-parse mark1.2)        refs/tags/mark1.2
-       $(git rev-parse mark1.10)       refs/tags/mark1.10
-       EOF
+       generate_references refs/tags/mark refs/tags/mark1.1 refs/tags/mark1.2 refs/tags/mark1.10 >expect &&

This gets awfully wide and loses some readability. Perhaps:

    generate_references \
        refs/tags/mark \
        refs/tags/mark1.1 \
        refs/tags/mark1.2 \
        refs/tags/mark1.10 >expect &&

Quoted 3 lines

t/t5512-ls-remote.sh +0 -0

        git ls-remote --sort="version:refname" --tags self >actual &&
        test_cmp expect actual
 '

Junio C Hamano

2020-03-25 06:41 UTC #4 lore raw

Re: [PATCH 2/8] t5512: generate references with generate_references()

In reply to Denton Liu

t/t5512-ls-remote.sh test_description='git ls-remote' · line 8

Quoted 2 lines · Denton Liu

t/t5512-ls-remote.sh +2 -0
+generate_references () {
+	for i

Is it just me who thinks variables used in iteration should be called i, j, etc. only when they are integers?

Quoted 10 lines

t/t5512-ls-remote.sh +6 -0

+	do
+		oid=$(git rev-parse "$i") || return 1
+		printf '%s\t%s\n' "$oid" "$i"
+	done
+}
+
 test_expect_success setup '
 	>file &&
 	git add file &&

t/t5512-ls-remote.sh test_expect_success 'ls-remote self' ' · line 54

Quoted 10 lines

t/t5512-ls-remote.sh +1 -6

@@ -43,34 +51,19 @@ test_expect_success 'ls-remote self' '
 '
 
 test_expect_success 'ls-remote --sort="version:refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark)	refs/tags/mark
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	EOF
+	generate_references refs/tags/mark refs/tags/mark1.1 refs/tags/mark1.2 refs/tags/mark1.10 >expect &&

Hmph, can we avoid overlong lines like this one?

	generate_references >expect <<-EOF
	refs/tags/mark
	refs/tags/mark1.1
	refs/tags/mark1.2
	refs/tags/mark1.10
	EOF

i.e. teaching the helper function to read from its standard input stream, may make it more readable (i.e. it is more obvious what the order of expected output lines are, as you are listing them one by one on a line of its own).

Denton Liu

author

2020-03-25 05:55 UTC #5 lore raw

PATCH 3/8 t5512: stop losing return codes of git commands

In reply to Denton Liu

In a pipe, only the return code of the last command is used. Thus, all other commands will have their return codes masked. Rewrite pipes so that there are no git commands upstream so that their failure is reported.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t5512-ls-remote.sh +7 -7

index 62d02152c7..fea64eb5c2 100755
@@ -21,11 +21,11 @@ test_expect_success setup '
 	git tag mark1.1 &&
 	git tag mark1.2 &&
 	git tag mark1.10 &&
-	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
-	(
-		echo "$(git rev-parse HEAD)	HEAD" &&
-		git show-ref -d	| sed -e "s/ /	/"
-	) >expected.all &&
+	git show-ref --tags -d >expected.tag.raw &&
+	sed -e "s/ /	/" expected.tag.raw >expected.tag &&
+	generate_references HEAD >expected.all &&
+	git show-ref -d	>refs &&
+	sed -e "s/ /	/" refs >>expected.all &&
 
 	git remote add self "$(pwd)/.git"
 '

@@ -173,8 +173,8 @@ do
 		test_config $configsection.hiderefs refs/tags &&
 		git ls-remote . >actual &&
 		test_unconfig $configsection.hiderefs &&
-		git ls-remote . |
-		sed -e "/	refs\/tags\//d" >expect &&
+		git ls-remote . >expect.raw &&
+		sed -e "/	refs\/tags\//d" expect.raw >expect &&
 		test_cmp expect actual
 	'

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #6 lore raw

PATCH 4/8 t5550: remove use of `test_might_fail grep`

In reply to Denton Liu

The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Rewrite the use of test_might_fail() with grep to remove this improper usage.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5550-http-fetch-dumb.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5550-http-fetch-dumb.sh +1 -1

index b811d89cfd..a06294ad8f 100755
@@ -248,7 +248,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '
 '
 
 test_expect_success 'did not use upload-pack service' '
-	test_might_fail grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act &&
+	{ grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act || :; } &&
 	: >exp &&
 	test_cmp exp act
 '

Signature · 1 line

2.25.0.114.g5b0ca878e0

Junio C Hamano

2020-03-25 06:35 UTC #7 lore raw

Re: [PATCH 4/8] t5550: remove use of `test_might_fail grep`

Quoted 14 lines · Denton Liu

The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Rewrite the use of test_might_fail() with grep to remove this improper usage.
Signed-off-by Denton Liu <[email protected]>
---
 t/t5550-http-fetch-dumb.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t5550-http-fetch-dumb.sh +0 -0
index b811d89cfd..a06294ad8f 100755

t/t5550-http-fetch-dumb.sh test_expect_success 'fetch can handle previously-fetched .idx files' ' · line 251

Quoted 5 lines

t/t5550-http-fetch-dumb.sh +1 -1

@@ -248,7 +248,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '
 '
 
 test_expect_success 'did not use upload-pack service' '
-	test_might_fail grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act &&
+	{ grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act || :; } &&

We can use

	sed -n -e "/\/git-upload-pack/p" "$HTTPD_ROOT_PATH/access.log" >actual

instead, but "grep for the pattern but don't worry if we found no match" is also OK.

t/t5550-http-fetch-dumb.sh test_expect_success 'fetch can handle previously-fetched .idx files' ' · line 253

Quoted 2 lines

t/t5550-http-fetch-dumb.sh +0 -0
 	: >exp &&
 	test_cmp exp act

Having said that, if the expectation is not to find any match, shouldn't the whole test be just

! grep "/git-upload-pack" "$HTTPD_ROOT_PATH/access.log"

a single liner? In any case, the use of sq in the original is broken.

Quoted 1 line

t/t5550-http-fetch-dumb.sh +0 -0
 '

Denton Liu

author

2020-03-25 05:55 UTC #8 lore raw

PATCH 5/8 t5607: reorder `nongit test_must_fail`

In reply to Denton Liu

In the future, we plan on only allowing test_must_fail to work on a restricted subset of commands, including git. Reorder the commands so that nongit comes before test_must_fail. This way, test_must_fail operates on a git command.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5607-clone-bundle.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5607-clone-bundle.sh +1 -1

index 9108ff6fbd..6d5a977fcb 100755
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 
 test_expect_success '"verify" needs a worktree' '
 	git bundle create tip.bundle -1 master &&
-	test_must_fail nongit git bundle verify ../tip.bundle 2>err &&
+	nongit test_must_fail git bundle verify ../tip.bundle 2>err &&
 	test_i18ngrep "need a repository" err
 '

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #9 lore raw

PATCH 6/8 t5612: don't use `test_must_fail test_cmp`

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5612-clone-refspec.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5612-clone-refspec.sh +1 -1

index e36ac01661..28373e715a 100755
@@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-	test_must_fail test_cmp expect actual &&
+	! test_cmp expect actual &&
 	test_line_count = 2 actual
 '

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #10 lore raw

PATCH 7/8 t5612: stop losing return codes of git commands

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

t/t5612-clone-refspec.sh +12 ~~-12~~

index 28373e715a..e3b436d8ae 100755
@@ -71,9 +71,9 @@ test_expect_success 'by default all branches will be kept updated' '
 	(
 		cd dir_all &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# follow both master and side
 	git for-each-ref refs/heads >expect &&

@@ -104,9 +104,9 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
 		git fetch --force &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow master
 	git for-each-ref refs/heads/master >expect &&

@@ -126,9 +126,9 @@ test_expect_success '--single-branch while HEAD pointing at master and --no-tags
 	(
 		cd dir_master_no_tags &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow master
 	git for-each-ref refs/heads/master >expect &&

@@ -156,9 +156,9 @@ test_expect_success '--single-branch while HEAD pointing at side' '
 	(
 		cd dir_side &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow side
 	git for-each-ref refs/heads/side >expect &&

@@ -169,9 +169,9 @@ test_expect_success '--single-branch with explicit --branch side' '
 	(
 		cd dir_side2 &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow side
 	git for-each-ref refs/heads/side >expect &&

@@ -223,9 +223,9 @@ test_expect_success '--single-branch with detached' '
 	(
 		cd dir_detached &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# nothing
 	test_must_be_empty actual

Signature · 1 line

2.25.0.114.g5b0ca878e0

Denton Liu

author

2020-03-25 05:55 UTC #11 lore raw

PATCH 8/8 t5801: teach compare_refs() to accept !

In reply to Denton Liu

Before, testing if two refs weren't equal with compare_refs() was done with test_must_fail compare_refs. This was wrong for two reasons. First, test_must_fail should only be used on git commands. Second, negating the error code is a little heavy-handed since in the case where one of the git invocations within compare_refs() fails, we will report success, even though it failed at an unexpected point.

Teach compare_refs() to accept ! as the first argument which would _only_ negate the test_cmp()'s return code.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5801-remote-helpers.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

t/t5801-remote-helpers.sh +8 -2

index 121e5c6edb..0f04b6cddb 100755
@@ -11,9 +11,15 @@ test_description='Test remote-helper import and export commands'
 PATH="$TEST_DIRECTORY/t5801:$PATH"
 
 compare_refs() {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
 	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
 	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
-	test_cmp expect actual
+	eval $fail test_cmp expect actual
 }
 
 test_expect_success 'setup repository' '

@@ -189,7 +195,7 @@ test_expect_success GPG 'push signed tag' '
 	git push origin signed-tag
 	) &&
 	compare_refs local signed-tag^{} server signed-tag^{} &&
-	test_must_fail compare_refs local signed-tag server signed-tag
+	compare_refs ! local signed-tag server signed-tag
 '
 
 test_expect_success GPG 'push signed tag with signed-tags capability' '

Signature · 1 line

2.25.0.114.g5b0ca878e0

Junio C Hamano

2020-03-25 06:31 UTC #12 lore raw

Re: [PATCH 8/8] t5801: teach compare_refs() to accept !

Quoted 15 lines · Denton Liu

compare_refs() {

t/t5801-remote-helpers.sh +7 -1

+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
 	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
 	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
-	test_cmp expect actual
+	eval $fail test_cmp expect actual
 }
 
 test_expect_success 'setup repository' '

t/t5801-remote-helpers.sh test_expect_success GPG 'push signed tag' ' · line 198

Quoted 5 lines

t/t5801-remote-helpers.sh +1 -1

@@ -189,7 +195,7 @@ test_expect_success GPG 'push signed tag' '
 	git push origin signed-tag
 	) &&
 	compare_refs local signed-tag^{} server signed-tag^{} &&
-	test_must_fail compare_refs local signed-tag server signed-tag
+	compare_refs ! local signed-tag server signed-tag

While this is not wrong per-se, I do not know why we cannot just use

! compare_refs local signed-tag server signed-tag

i.e. "we expect these two repositories have different tags"?

It is totally plausible that I am missing something obvious, as it is getting late and I no longer am a night person.

Thanks.

Quoted 3 lines

t/t5801-remote-helpers.sh +0 -0

 '
 
 test_expect_success GPG 'push signed tag with signed-tags capability' '

Eric Sunshine

2020-03-25 06:36 UTC #13 lore raw

Re: [PATCH 8/8] t5801: teach compare_refs() to accept !

Quoted 16 lines · Junio C Hamano

Quoted 4 lines · Denton Liu

compare_refs() {
t/t5801-remote-helpers.sh +3 -0
+     fail= &&
+     if test "x$1" = 'x!'
+     then

t/t5801-remote-helpers.sh test_description='Test remote-helper import and export commands'

Quoted 10 lines

t/t5801-remote-helpers.sh +5 -2

+             fail='!' &&
+             shift
+     fi &&
      git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
      git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
-     test_cmp expect actual
+     eval $fail test_cmp expect actual
 }
-     test_must_fail compare_refs local signed-tag server signed-tag
+     compare_refs ! local signed-tag server signed-tag

Quoted 5 lines

While this is not wrong per-se, I do not know why we cannot just use
! compare_refs local signed-tag server signed-tag
i.e. "we expect these two repositories have different tags"?

As mentioned in the commit message, if one of the git-rev-parse invocations fails unexpectedly, then compare_refs() would return early with a failure code, but the "!" would then (undesirably) turn that failure into a success. We don't want to lose a failure code from git-rev-parse, so the simple use of "! compare_refs ..." is avoided.

Junio C Hamano

2020-03-25 06:43 UTC #14 lore raw

In reply to Denton Liu

Quoted 6 lines · Denton Liu

The overall scope of these patches is to replace inappropriate uses of test_must_fail. IOW, we should only allow test_must_fail to run on git and test-tool. Ultimately, we will conclude by making test_must_fail error out on non-git commands. An advance view of the final series can be found here[1].

Quoted 7 lines

This is the third part. It focuses on t5*.sh.
The first part can be found here[2]. The second part can be found here[3].
[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/

I left comments on individual patches, but I think they were all minor. Mostly the series looks good.

Thanks.

The dl/test-must-fail-fixes-* topics are always fun to read ;-)

Denton Liu

author

2020-03-26 08:28 UTC #15 lore raw

v2 cover t: replace incorrect test_must_fail usage (part 3)

In reply to Denton Liu

This is the third part. It focuses on t5*.sh.

The first part can be found here[2]. The second part can be found here[3].

Changes since v1:

* Broke up long generate_references() lines for clarity in 2/8

* For-loop &&-chain and renamed variable to "ref" in 2/8

* Cleaned up commit message in 2/8

* Rewrote 4/8 with a simpler test case

Denton Liu (8):
t5512: don't use test_must_fail test_cmp
t5512: stop losing git exit code in here-docs
t5512: stop losing return codes of git commands
t5550: simplify no matching line check
t5607: reorder nongit test_must_fail
t5612: don't use test_must_fail test_cmp
t5612: stop losing return codes of git commands
t5801: teach compare_refs() to accept !

 t/t5512-ls-remote.sh       | 80 ++++++++++++++++++++------------------
 t/t5550-http-fetch-dumb.sh |  4 +-
 t/t5607-clone-bundle.sh    |  2 +-
 t/t5612-clone-refspec.sh   | 26 ++++++-------
 t/t5801-remote-helpers.sh  | 10 ++++-
 5 files changed, 66 insertions(+), 56 deletions(-)

Range-diff against v1:

1:  3d7dc8428d = 1:  3d7dc8428d t5512: don't use `test_must_fail test_cmp`
2:  674de50db2 ! 2:  97bc46e4a9 t5512: generate references with generate_references()
    @@ Metadata
     Author: Denton Liu <[email protected]>
     
      ## Commit message ##
    -    t5512: generate references with generate_references()
    +    t5512: stop losing git exit code in here-docs
     
         The expected references are generated using a here-doc with some inline
    -    subshells. If one of the `git rev-parse` invocations within the
    -    subshells failed, its return code is swallowed and we won't know about
    -    it. Replace these here-docs with generate_references(), which actually
    -    reports when `git rev-parse` fails.
    +    command substitutions. If one of the `git rev-parse` invocations within
    +    the command substitutions fails, its return code is swallowed and we
    +    won't know about it. Replace these command substitutions with
    +    generate_references(), which actually reports when `git rev-parse`
    +    fails.
     
      ## t/t5512-ls-remote.sh ##
     @@ t/t5512-ls-remote.sh: test_description='git ls-remote'
    @@ t/t5512-ls-remote.sh: test_description='git ls-remote'
      . ./test-lib.sh
      
     +generate_references () {
    -+	for i
    ++	for ref
     +	do
    -+		oid=$(git rev-parse "$i") || return 1
    -+		printf '%s\t%s\n' "$oid" "$i"
    ++		oid=$(git rev-parse "$ref") &&
    ++		printf '%s\t%s\n' "$oid" "$ref" || return 1
     +	done
     +}
     +
    @@ t/t5512-ls-remote.sh: test_expect_success 'ls-remote self' '
     -	$(git rev-parse mark1.2)	refs/tags/mark1.2
     -	$(git rev-parse mark1.10)	refs/tags/mark1.10
     -	EOF
    -+	generate_references refs/tags/mark refs/tags/mark1.1 refs/tags/mark1.2 refs/tags/mark1.10 >expect &&
    ++	generate_references \
    ++		refs/tags/mark \
    ++		refs/tags/mark1.1 \
    ++		refs/tags/mark1.2 \
    ++		refs/tags/mark1.10 >expect &&
      	git ls-remote --sort="version:refname" --tags self >actual &&
      	test_cmp expect actual
      '
    @@ t/t5512-ls-remote.sh: test_expect_success 'ls-remote self' '
     -	$(git rev-parse mark1.1)	refs/tags/mark1.1
     -	$(git rev-parse mark)	refs/tags/mark
     -	EOF
    -+	generate_references refs/tags/mark1.10 refs/tags/mark1.2 refs/tags/mark1.1 refs/tags/mark >expect &&
    ++	generate_references \
    ++		refs/tags/mark1.10 \
    ++		refs/tags/mark1.2 \
    ++		refs/tags/mark1.1 \
    ++		refs/tags/mark >expect &&
      	git ls-remote --sort="-version:refname" --tags self >actual &&
      	test_cmp expect actual
      '
    @@ t/t5512-ls-remote.sh: test_expect_success 'ls-remote self' '
     -	$(git rev-parse mark1.1)	refs/tags/mark1.1
     -	$(git rev-parse mark)	refs/tags/mark
     -	EOF
    -+	generate_references refs/tags/mark1.2 refs/tags/mark1.10 refs/tags/mark1.1 refs/tags/mark >expect &&
    ++	generate_references \
    ++		refs/tags/mark1.2 \
    ++		refs/tags/mark1.10 \
    ++		refs/tags/mark1.1 \
    ++		refs/tags/mark >expect &&
      	git ls-remote --sort="-refname" --tags self >actual &&
      	test_cmp expect actual
      '
    @@ t/t5512-ls-remote.sh: test_expect_success 'protocol v2 supports hiderefs' '
     -	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
     -	EOF
     +	echo "ref: refs/heads/master	HEAD" >expect &&
    -+	generate_references HEAD \
    ++	generate_references \
    ++		HEAD \
     +		refs/heads/master >>expect &&
     +	oid=$(git rev-parse HEAD) &&
     +	echo "$oid	refs/remotes/origin/HEAD" >>expect &&
    -+	generate_references refs/remotes/origin/master \
    ++	generate_references \
    ++	refs/remotes/origin/master \
     +		refs/tags/mark \
     +		refs/tags/mark1.1 \
     +		refs/tags/mark1.10 \
3:  b3addeb985 = 3:  986ba1dd39 t5512: stop losing return codes of git commands
4:  68c911e29b < -:  ---------- t5550: remove use of `test_might_fail grep`
-:  ---------- > 4:  53e64e7077 t5550: simplify no matching line check
5:  4253f51fea = 5:  4955b701e1 t5607: reorder `nongit test_must_fail`
6:  2bac5f4a29 = 6:  a3d9d3673b t5612: don't use `test_must_fail test_cmp`
7:  b490ccace0 = 7:  bc0e90d5ba t5612: stop losing return codes of git commands
8:  b51f97f6ae = 8:  6a9d3cef2a t5801: teach compare_refs() to accept !

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #16 lore raw

PATCH v2 1/8 t5512: don't use `test_must_fail test_cmp`

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5512-ls-remote.sh +1 -1

index 04b35402c7..08b98f12b8 100755
@@ -92,7 +92,7 @@ test_expect_success 'use "origin" when no remote specified' '
 
 test_expect_success 'suppress "From <url>" with -q' '
 	git ls-remote -q 2>actual_err &&
-	test_must_fail test_cmp exp_err actual_err
+	! test_cmp exp_err actual_err
 '
 
 test_expect_success 'use branch.<name>.remote if possible' '

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #17 lore raw

PATCH v2 2/8 t5512: stop losing git exit code in here-docs

In reply to Denton Liu

The expected references are generated using a here-doc with some inline command substitutions. If one of the git rev-parse invocations within the command substitutions fails, its return code is swallowed and we won't know about it. Replace these command substitutions with generate_references(), which actually reports when git rev-parse fails.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 64 ++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

t/t5512-ls-remote.sh +35 ~~-29~~

index 08b98f12b8..dcb7349b0b 100755
@@ -4,6 +4,14 @@ test_description='git ls-remote'
 
 . ./test-lib.sh
 
+generate_references () {
+	for ref
+	do
+		oid=$(git rev-parse "$ref") &&
+		printf '%s\t%s\n' "$oid" "$ref" || return 1
+	done
+}
+
 test_expect_success setup '
 	>file &&
 	git add file &&

@@ -43,34 +51,31 @@ test_expect_success 'ls-remote self' '
 '
 
 test_expect_success 'ls-remote --sort="version:refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark)	refs/tags/mark
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	EOF
+	generate_references \
+		refs/tags/mark \
+		refs/tags/mark1.1 \
+		refs/tags/mark1.2 \
+		refs/tags/mark1.10 >expect &&
 	git ls-remote --sort="version:refname" --tags self >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark)	refs/tags/mark
-	EOF
+	generate_references \
+		refs/tags/mark1.10 \
+		refs/tags/mark1.2 \
+		refs/tags/mark1.1 \
+		refs/tags/mark >expect &&
 	git ls-remote --sort="-version:refname" --tags self >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-remote --sort="-refname" --tags self' '
-	cat >expect <<-EOF &&
-	$(git rev-parse mark1.2)	refs/tags/mark1.2
-	$(git rev-parse mark1.10)	refs/tags/mark1.10
-	$(git rev-parse mark1.1)	refs/tags/mark1.1
-	$(git rev-parse mark)	refs/tags/mark
-	EOF
+	generate_references \
+		refs/tags/mark1.2 \
+		refs/tags/mark1.10 \
+		refs/tags/mark1.1 \
+		refs/tags/mark >expect &&
 	git ls-remote --sort="-refname" --tags self >actual &&
 	test_cmp expect actual
 '

@@ -212,17 +217,18 @@ test_expect_success 'protocol v2 supports hiderefs' '
 
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
-	cat >expect <<-EOF &&
-	ref: refs/heads/master	HEAD
-	$(git rev-parse HEAD)	HEAD
-	$(git rev-parse refs/heads/master)	refs/heads/master
-	$(git rev-parse HEAD)	refs/remotes/origin/HEAD
-	$(git rev-parse refs/remotes/origin/master)	refs/remotes/origin/master
-	$(git rev-parse refs/tags/mark)	refs/tags/mark
-	$(git rev-parse refs/tags/mark1.1)	refs/tags/mark1.1
-	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
-	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
-	EOF
+	echo "ref: refs/heads/master	HEAD" >expect &&
+	generate_references \
+		HEAD \
+		refs/heads/master >>expect &&
+	oid=$(git rev-parse HEAD) &&
+	echo "$oid	refs/remotes/origin/HEAD" >>expect &&
+	generate_references \
+	refs/remotes/origin/master \
+		refs/tags/mark \
+		refs/tags/mark1.1 \
+		refs/tags/mark1.10 \
+		refs/tags/mark1.2 >>expect &&
 	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
 	# protocol v0 here.
 	GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&

Signature · 1 line

2.26.0.159.g23e2136ad0

Eric Sunshine

2020-03-26 15:24 UTC #18 lore raw

Re: [PATCH v2 2/8] t5512: stop losing git exit code in here-docs

Quoted 11 lines · Denton Liu

The expected references are generated using a here-doc with some inline command substitutions. If one of the git rev-parse invocations within the command substitutions fails, its return code is swallowed and we won't know about it. Replace these command substitutions with generate_references(), which actually reports when git rev-parse fails.
Signed-off-by Denton Liu <[email protected]>
---

t/t5512-ls-remote.sh test_expect_success 'protocol v2 supports hiderefs' ' · line 224

Quoted 8 lines

t/t5512-ls-remote.sh +7 -0

@@ -212,17 +217,18 @@ test_expect_success 'protocol v2 supports hiderefs' '
 test_expect_success 'ls-remote --symref' '
+       echo "$oid      refs/remotes/origin/HEAD" >>expect &&
+       generate_references \
+       refs/remotes/origin/master \
+               refs/tags/mark \
+               refs/tags/mark1.1 \
+               refs/tags/mark1.10 \
+               refs/tags/mark1.2 >>expect &&

Botched indentation of the "refs/remotes/origin/master" line.

Denton Liu

author

2020-03-26 08:28 UTC #19 lore raw

PATCH v2 3/8 t5512: stop losing return codes of git commands

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t5512-ls-remote.sh +7 -7

index dcb7349b0b..8928d1f62d 100755
@@ -21,11 +21,11 @@ test_expect_success setup '
 	git tag mark1.1 &&
 	git tag mark1.2 &&
 	git tag mark1.10 &&
-	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
-	(
-		echo "$(git rev-parse HEAD)	HEAD" &&
-		git show-ref -d	| sed -e "s/ /	/"
-	) >expected.all &&
+	git show-ref --tags -d >expected.tag.raw &&
+	sed -e "s/ /	/" expected.tag.raw >expected.tag &&
+	generate_references HEAD >expected.all &&
+	git show-ref -d	>refs &&
+	sed -e "s/ /	/" refs >>expected.all &&
 
 	git remote add self "$(pwd)/.git"
 '

@@ -185,8 +185,8 @@ do
 		test_config $configsection.hiderefs refs/tags &&
 		git ls-remote . >actual &&
 		test_unconfig $configsection.hiderefs &&
-		git ls-remote . |
-		sed -e "/	refs\/tags\//d" >expect &&
+		git ls-remote . >expect.raw &&
+		sed -e "/	refs\/tags\//d" expect.raw >expect &&
 		test_cmp expect actual
 	'

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #20 lore raw

PATCH v2 4/8 t5550: simplify no matching line check

In reply to Denton Liu

In the 'did not use upload-pack service' test, we have a complicated song-and-dance to ensure that there are no "/git-upload-pack" lines in "$HTTPD_ROOT_PATH/access.log". Simplify this by just checking that grep returns a non-zero exit code.

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Denton Liu <[email protected]>

---
 t/t5550-http-fetch-dumb.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

t/t5550-http-fetch-dumb.sh +1 -3

index b811d89cfd..bcde886b87 100755
@@ -248,9 +248,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '
 '
 
 test_expect_success 'did not use upload-pack service' '
-	test_might_fail grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act &&
-	: >exp &&
-	test_cmp exp act
+	! grep "/git-upload-pack" "$HTTPD_ROOT_PATH/access.log"
 '
 
 test_expect_success 'git client shows text/plain errors' '

Signature · 1 line

2.26.0.159.g23e2136ad0

Eric Sunshine

2020-03-26 15:26 UTC #21 lore raw

Re: [PATCH v2 4/8] t5550: simplify no matching line check

Quoted 9 lines · Denton Liu

In the 'did not use upload-pack service' test, we have a complicated song-and-dance to ensure that there are no "/git-upload-pack" lines in "$HTTPD_ROOT_PATH/access.log". Simplify this by just checking that grep returns a non-zero exit code.
Signed-off-by Denton Liu <[email protected]>
---

t/t5550-http-fetch-dumb.sh test_expect_success 'fetch can handle previously-fetched .idx files' ' · line 250

Quoted 6 lines

t/t5550-http-fetch-dumb.sh +1 -3

@@ -248,9 +248,7 @@ test_expect_success 'fetch can handle previously-fetched .idx files' '
 test_expect_success 'did not use upload-pack service' '
-       test_might_fail grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act &&
-       : >exp &&
-       test_cmp exp act
+       ! grep "/git-upload-pack" "$HTTPD_ROOT_PATH/access.log"
 '

It would have been nice for the commit message to have mentioned that the change is also eliminating an incorrect application of test_might_fail(), but it's probably not worth a re-roll.

Denton Liu

author

2020-03-26 08:28 UTC #22 lore raw

PATCH v2 5/8 t5607: reorder `nongit test_must_fail`

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5607-clone-bundle.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5607-clone-bundle.sh +1 -1

index 9108ff6fbd..6d5a977fcb 100755
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 
 test_expect_success '"verify" needs a worktree' '
 	git bundle create tip.bundle -1 master &&
-	test_must_fail nongit git bundle verify ../tip.bundle 2>err &&
+	nongit test_must_fail git bundle verify ../tip.bundle 2>err &&
 	test_i18ngrep "need a repository" err
 '

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #23 lore raw

PATCH v2 6/8 t5612: don't use `test_must_fail test_cmp`

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5612-clone-refspec.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5612-clone-refspec.sh +1 -1

index e36ac01661..28373e715a 100755
@@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' '
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-	test_must_fail test_cmp expect actual &&
+	! test_cmp expect actual &&
 	test_line_count = 2 actual
 '

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #24 lore raw

PATCH v2 7/8 t5612: stop losing return codes of git commands

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

t/t5612-clone-refspec.sh +12 ~~-12~~

index 28373e715a..e3b436d8ae 100755
@@ -71,9 +71,9 @@ test_expect_success 'by default all branches will be kept updated' '
 	(
 		cd dir_all &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# follow both master and side
 	git for-each-ref refs/heads >expect &&

@@ -104,9 +104,9 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
 		git fetch --force &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow master
 	git for-each-ref refs/heads/master >expect &&

@@ -126,9 +126,9 @@ test_expect_success '--single-branch while HEAD pointing at master and --no-tags
 	(
 		cd dir_master_no_tags &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow master
 	git for-each-ref refs/heads/master >expect &&

@@ -156,9 +156,9 @@ test_expect_success '--single-branch while HEAD pointing at side' '
 	(
 		cd dir_side &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow side
 	git for-each-ref refs/heads/side >expect &&

@@ -169,9 +169,9 @@ test_expect_success '--single-branch with explicit --branch side' '
 	(
 		cd dir_side2 &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# only follow side
 	git for-each-ref refs/heads/side >expect &&

@@ -223,9 +223,9 @@ test_expect_success '--single-branch with detached' '
 	(
 		cd dir_detached &&
 		git fetch &&
-		git for-each-ref refs/remotes/origin |
+		git for-each-ref refs/remotes/origin >refs &&
 		sed -e "/HEAD$/d" \
-		    -e "s|/remotes/origin/|/heads/|" >../actual
+		    -e "s|/remotes/origin/|/heads/|" refs >../actual
 	) &&
 	# nothing
 	test_must_be_empty actual

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-26 08:28 UTC #25 lore raw

PATCH v2 8/8 t5801: teach compare_refs() to accept !

In reply to Denton Liu

Teach compare_refs() to accept ! as the first argument which would _only_ negate the test_cmp()'s return code.

Signed-off-by Denton Liu <[email protected]>

---
 t/t5801-remote-helpers.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

t/t5801-remote-helpers.sh +8 -2

index 121e5c6edb..0f04b6cddb 100755
@@ -11,9 +11,15 @@ test_description='Test remote-helper import and export commands'
 PATH="$TEST_DIRECTORY/t5801:$PATH"
 
 compare_refs() {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
 	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
 	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
-	test_cmp expect actual
+	eval $fail test_cmp expect actual
 }
 
 test_expect_success 'setup repository' '

@@ -189,7 +195,7 @@ test_expect_success GPG 'push signed tag' '
 	git push origin signed-tag
 	) &&
 	compare_refs local signed-tag^{} server signed-tag^{} &&
-	test_must_fail compare_refs local signed-tag server signed-tag
+	compare_refs ! local signed-tag server signed-tag
 '
 
 test_expect_success GPG 'push signed tag with signed-tags capability' '

Signature · 1 line

2.26.0.159.g23e2136ad0

Denton Liu

author

2020-03-27 00:44 UTC #26 lore raw

PATCH v2 9/9 fixup! t5512: stop losing git exit code in here-docs

In reply to Denton Liu

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5512-ls-remote.sh +1 -1

index 8928d1f62d..e98c3a0174 100755
@@ -224,7 +224,7 @@ test_expect_success 'ls-remote --symref' '
 	oid=$(git rev-parse HEAD) &&
 	echo "$oid	refs/remotes/origin/HEAD" >>expect &&
 	generate_references \
-	refs/remotes/origin/master \
+		refs/remotes/origin/master \
 		refs/tags/mark \
 		refs/tags/mark1.1 \
 		refs/tags/mark1.10 \

Signature · 1 line

2.26.0.159.g23e2136ad0

Junio C Hamano

2020-03-27 17:46 UTC #27 lore raw

Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs

Quoted 1 line · Denton Liu

Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs

Forgot to do the final "rebase -i --autosquash"?

Quoted 18 lines

Signed-off-by Denton Liu <[email protected]>

---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5512-ls-remote.sh +1 -1

index 8928d1f62d..e98c3a0174 100755
@@ -224,7 +224,7 @@ test_expect_success 'ls-remote --symref' '
 	oid=$(git rev-parse HEAD) &&
 	echo "$oid	refs/remotes/origin/HEAD" >>expect &&
 	generate_references \
-	refs/remotes/origin/master \
+		refs/remotes/origin/master \
 		refs/tags/mark \
 		refs/tags/mark1.1 \
 		refs/tags/mark1.10 \

Eric Sunshine

2020-03-27 20:29 UTC #28 lore raw

Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs

Quoted 3 lines · Junio C Hamano

Quoted 1 line
Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Forgot to do the final "rebase -i --autosquash"?

My guess is that Denton sent this "fixup!" to have it queued atop [1] in response to the review comment[2] pointing out the bad indentation (rather than re-sending the entire series just for one minor change).

[1]: https://lore.kernel.org/git/[email protected]/
[2]: https://lore.kernel.org/git/CAPig+cTpV2n66nYJdQpG0tYCu5f=PB-a9Boa_NNfVaUOshDVug@mail.gmail.com/

Junio C Hamano

2020-03-27 21:39 UTC #29 lore raw

Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs

Quoted 7 lines · Eric Sunshine

Quoted 3 lines · Junio C Hamano
Quoted 1 line
Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Forgot to do the final "rebase -i --autosquash"?
My guess is that Denton sent this "fixup!" to have it queued atop [1] in response to the review comment[2] pointing out the bad indentation

Ah, [9/9] fooled me (and caused me not to see eight in [1/8]-[8/8] and I mistook they were [1/9]-[8/9].

Usually people do the "oops, here is a hotfix" by marking such a "beyond the original set" patch as [9/8].

In any case, I think I've squashed it to the original. Thanks.