# [PATCH 0/8] t: replace incorrect test_must_fail usage (part 3)

30 messages from 2020-03-25 to 2020-03-27. Participants: Denton Liu, Eric Sunshine, Junio C Hamano.
Thread: https://gitlist.dev/t/53092

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 0/8] t: replace incorrect test_must_fail usage (part 3)
Message-ID: <cover.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/cover.1585115341.git.liu.denton%40gmail.com

```
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/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/

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(-)

-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 1/8] t5512: don't use `test_must_fail test_cmp`
Message-ID: <3d7dc8428d1992d3bc2802f5c828e8d77fe4de3b.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/3d7dc8428d1992d3bc2802f5c828e8d77fe4de3b.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 04b35402c7..08b98f12b8 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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' '
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 3/8] t5512: stop losing return codes of git commands
Message-ID: <b3addeb98547783a31e2e2b876478a5dd96e04ec.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/b3addeb98547783a31e2e2b876478a5dd96e04ec.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 62d02152c7..fea64eb5c2 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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
 	'
 
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 2/8] t5512: generate references with generate_references()
Message-ID: <674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 50 +++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 08b98f12b8..62d02152c7 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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 &&
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 5/8] t5607: reorder `nongit test_must_fail`
Message-ID: <4253f51feaf47029c1c3389e017d49f2148f03c6.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/4253f51feaf47029c1c3389e017d49f2148f03c6.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5607-clone-bundle.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 9108ff6fbd..6d5a977fcb 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -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
 '
 
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 4/8] t5550: remove use of `test_might_fail grep`
Message-ID: <68c911e29b509d75e390aba573921d6ac385fcaf.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/68c911e29b509d75e390aba573921d6ac385fcaf.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5550-http-fetch-dumb.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b811d89cfd..a06294ad8f 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -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
 '
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 8/8] t5801: teach compare_refs() to accept !
Message-ID: <b51f97f6ae37e4b69b9651cbd60a480e5db3e72d.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/b51f97f6ae37e4b69b9651cbd60a480e5db3e72d.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5801-remote-helpers.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 121e5c6edb..0f04b6cddb 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -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' '
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 7/8] t5612: stop losing return codes of git commands
Message-ID: <b490ccace0eb93b0f6d71f71e015afebcc9c9a51.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/b490ccace0eb93b0f6d71f71e015afebcc9c9a51.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index 28373e715a..e3b436d8ae 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -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
-- 
2.25.0.114.g5b0ca878e0


```

## Denton Liu, 2020-03-25 05:54

Subject: [PATCH 6/8] t5612: don't use `test_must_fail test_cmp`
Message-ID: <2bac5f4a290620ef37e3dcf1e05773acd14cb725.1585115341.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/2bac5f4a290620ef37e3dcf1e05773acd14cb725.1585115341.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index e36ac01661..28373e715a 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -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
 '
 
-- 
2.25.0.114.g5b0ca878e0


```

## Eric Sunshine, 2020-03-25 06:08

Subject: Re: [PATCH 2/8] t5512: generate references with generate_references()
Message-ID: <CAPig+cQHZa4yELo3ncCVkbyrKWcPnZ4wjROMMbQGjm8dXK38GA@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcQHZa4yELo3ncCVkbyrKWcPnZ4wjROMMbQGjm8dXK38GA%40mail.gmail.com
In-Reply-To: <674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton@gmail.com>

```
On Wed, Mar 25, 2020 at 1:55 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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

> 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/

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

A couple nits below...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> @@ -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.

> +       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 &&

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

```

## Junio C Hamano, 2020-03-25 06:31

Subject: Re: [PATCH 8/8] t5801: teach compare_refs() to accept !
Message-ID: <xmqqv9mtx7wv.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqv9mtx7wv.fsf%40gitster.c.googlers.com
In-Reply-To: <b51f97f6ae37e4b69b9651cbd60a480e5db3e72d.1585115341.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

>  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

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.

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

```

## Junio C Hamano, 2020-03-25 06:35

Subject: Re: [PATCH 4/8] t5550: remove use of `test_might_fail grep`
Message-ID: <xmqqr1xhx7p6.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqr1xhx7p6.fsf%40gitster.c.googlers.com
In-Reply-To: <68c911e29b509d75e390aba573921d6ac385fcaf.1585115341.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

> 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 <liu.denton@gmail.com>
> ---
>  t/t5550-http-fetch-dumb.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index b811d89cfd..a06294ad8f 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -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.

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

>  '

```

## Eric Sunshine, 2020-03-25 06:36

Subject: Re: [PATCH 8/8] t5801: teach compare_refs() to accept !
Message-ID: <CAPig+cSJVdFEZ_V_FBtdAhR7KqocqXCgkHm1pFMDjBX_NPE+Jw@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcSJVdFEZ_V_FBtdAhR7KqocqXCgkHm1pFMDjBX_NPE%2BJw%40mail.gmail.com
In-Reply-To: <xmqqv9mtx7wv.fsf@gitster.c.googlers.com>

```
On Wed, Mar 25, 2020 at 2:31 AM Junio C Hamano <gitster@pobox.com> wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> >  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_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"?

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:41

Subject: Re: [PATCH 2/8] t5512: generate references with generate_references()
Message-ID: <xmqqmu85x7fo.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqmu85x7fo.fsf%40gitster.c.googlers.com
In-Reply-To: <674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

> +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?

> +	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 &&

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


```

## Junio C Hamano, 2020-03-25 06:43

Subject: Re: [PATCH 0/8] t: replace incorrect test_must_fail usage (part 3)
Message-ID: <xmqqimitx7cq.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqimitx7cq.fsf%40gitster.c.googlers.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

> 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/cover.1576583819.git.liu.denton@gmail.com/
> [3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/

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, 2020-03-26 08:27

Subject: [PATCH v2 0/8] t: replace incorrect test_must_fail usage (part 3)
Message-ID: <cover.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/cover.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585115341.git.liu.denton@gmail.com>

```
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].

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

[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/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/

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 <liu.denton@gmail.com>
     
      ## 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 !
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 1/8] t5512: don't use `test_must_fail test_cmp`
Message-ID: <3d7dc8428d1992d3bc2802f5c828e8d77fe4de3b.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/3d7dc8428d1992d3bc2802f5c828e8d77fe4de3b.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 04b35402c7..08b98f12b8 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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' '
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 2/8] t5512: stop losing git exit code in here-docs
Message-ID: <97bc46e4a9105f13b5284f86907eb1459a9987b6.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/97bc46e4a9105f13b5284f86907eb1459a9987b6.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 64 ++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 08b98f12b8..dcb7349b0b 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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 &&
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 3/8] t5512: stop losing return codes of git commands
Message-ID: <986ba1dd3921251a03c1c4cf92e7439e012b07ed.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/986ba1dd3921251a03c1c4cf92e7439e012b07ed.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index dcb7349b0b..8928d1f62d 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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
 	'
 
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 4/8] t5550: simplify no matching line check
Message-ID: <53e64e7077063a177cfafd97aa01340ee5b4cf1f.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/53e64e7077063a177cfafd97aa01340ee5b4cf1f.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5550-http-fetch-dumb.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b811d89cfd..bcde886b87 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -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' '
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 5/8] t5607: reorder `nongit test_must_fail`
Message-ID: <4955b701e1f961e592e1c0c6567e28cbc12b3c14.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/4955b701e1f961e592e1c0c6567e28cbc12b3c14.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5607-clone-bundle.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 9108ff6fbd..6d5a977fcb 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -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
 '
 
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 6/8] t5612: don't use `test_must_fail test_cmp`
Message-ID: <a3d9d3673b24ab77e38e5c546f16fe9085d69496.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/a3d9d3673b24ab77e38e5c546f16fe9085d69496.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index e36ac01661..28373e715a 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -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
 '
 
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 7/8] t5612: stop losing return codes of git commands
Message-ID: <bc0e90d5baa73312bf77501c910e3457761b77f5.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/bc0e90d5baa73312bf77501c910e3457761b77f5.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index 28373e715a..e3b436d8ae 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -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
-- 
2.26.0.159.g23e2136ad0


```

## Denton Liu, 2020-03-26 08:27

Subject: [PATCH v2 8/8] t5801: teach compare_refs() to accept !
Message-ID: <6a9d3cef2a0daf1b058921bd0cd3397f7753e537.1585209554.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/6a9d3cef2a0daf1b058921bd0cd3397f7753e537.1585209554.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
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 <liu.denton@gmail.com>
---
 t/t5801-remote-helpers.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 121e5c6edb..0f04b6cddb 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -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' '
-- 
2.26.0.159.g23e2136ad0


```

## Eric Sunshine, 2020-03-26 15:24

Subject: Re: [PATCH v2 2/8] t5512: stop losing git exit code in here-docs
Message-ID: <CAPig+cTpV2n66nYJdQpG0tYCu5f=PB-a9Boa_NNfVaUOshDVug@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcTpV2n66nYJdQpG0tYCu5f%3DPB-a9Boa_NNfVaUOshDVug%40mail.gmail.com
In-Reply-To: <97bc46e4a9105f13b5284f86907eb1459a9987b6.1585209554.git.liu.denton@gmail.com>

```
On Thu, Mar 26, 2020 at 4:28 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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 <liu.denton@gmail.com>
> ---
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> @@ -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.

```

## Eric Sunshine, 2020-03-26 15:26

Subject: Re: [PATCH v2 4/8] t5550: simplify no matching line check
Message-ID: <CAPig+cREV63OmEkrqQ_ou=HDHnJZXLcd8j34hqBibiKe6uUnhQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcREV63OmEkrqQ_ou%3DHDHnJZXLcd8j34hqBibiKe6uUnhQ%40mail.gmail.com
In-Reply-To: <53e64e7077063a177cfafd97aa01340ee5b4cf1f.1585209554.git.liu.denton@gmail.com>

```
On Thu, Mar 26, 2020 at 4:28 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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 <liu.denton@gmail.com>
> ---
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> @@ -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, 2020-03-27 00:43

Subject: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Message-ID: <38924d9c4994b5bad93cbbd2659666f6b452a89f.1585269773.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/38924d9c4994b5bad93cbbd2659666f6b452a89f.1585269773.git.liu.denton%40gmail.com
In-Reply-To: <cover.1585209554.git.liu.denton@gmail.com>

```
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 8928d1f62d..e98c3a0174 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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 \
-- 
2.26.0.159.g23e2136ad0


```

## Junio C Hamano, 2020-03-27 17:46

Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Message-ID: <xmqqimipherm.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqimipherm.fsf%40gitster.c.googlers.com
In-Reply-To: <38924d9c4994b5bad93cbbd2659666f6b452a89f.1585269773.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

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

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

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t5512-ls-remote.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 8928d1f62d..e98c3a0174 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -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

Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Message-ID: <CAPig+cR0rNnVRF-Xei-n2HaxLR=9yF=7aya3mdApaqfEXmcPLg@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcR0rNnVRF-Xei-n2HaxLR%3D9yF%3D7aya3mdApaqfEXmcPLg%40mail.gmail.com
In-Reply-To: <xmqqimipherm.fsf@gitster.c.googlers.com>

```
On Fri, Mar 27, 2020 at 1:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> > 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/cover.1585209554.git.liu.denton@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTpV2n66nYJdQpG0tYCu5f=PB-a9Boa_NNfVaUOshDVug@mail.gmail.com/

```

## Junio C Hamano, 2020-03-27 21:39

Subject: Re: [PATCH v2 9/9] fixup! t5512: stop losing git exit code in here-docs
Message-ID: <xmqqwo75eaue.fsf@gitster.c.googlers.com>
URL: https://gitlist.dev/e/xmqqwo75eaue.fsf%40gitster.c.googlers.com
In-Reply-To: <CAPig+cR0rNnVRF-Xei-n2HaxLR=9yF=7aya3mdApaqfEXmcPLg@mail.gmail.com>

```
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Mar 27, 2020 at 1:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > 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.

```