threads / patch / 53092

patch, 8 partst: replace incorrect test_must_fail usage (part 3)

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

## tl;dr

30 messages between Mar 25, 2020 and Mar 27, 2020. Diffs are folded; open one to read it.

replies: 29people: 3as markdown or json

Denton Liu· Mar 25, 2020, 05:54 UTC · lore

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· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

Show changes to t/t5512-ls-remote.sh +1 −1

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· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5512-ls-remote.sh +7 −7

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· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5512-ls-remote.sh +21 −29

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

Eric Sunshine· Mar 25, 2020, 06:08 UTC · re: Denton Liu · lore

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

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

Show 9 quoted lines

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

Show 11 quoted lines

> +       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· Mar 25, 2020, 06:41 UTC · re: Denton Liu · lore

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

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?

Show 20 quoted lines

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

Denton Liu· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

[PATCH 5/8] t5607: reorder `nongit test_must_fail`

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

Show changes to t/t5607-clone-bundle.sh +1 −1

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· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5550-http-fetch-dumb.sh +1 −1

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

Junio C Hamano· Mar 25, 2020, 06:35 UTC · re: Denton Liu · lore

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

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

Show 19 quoted lines

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

>  '

Denton Liu· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5801-remote-helpers.sh +8 −2

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

Junio C Hamano· Mar 25, 2020, 06:31 UTC · re: Denton Liu · lore

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

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

Show 20 quoted lines

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

Eric Sunshine· Mar 25, 2020, 06:36 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 25, 2020 at 2:31 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

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

Denton Liu· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Show changes to t/t5612-clone-refspec.sh +12 −12

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· Mar 25, 2020, 05:54 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5612-clone-refspec.sh +1 −1

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

Junio C Hamano· Mar 25, 2020, 06:43 UTC · re: Denton Liu · lore

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

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

Show 13 quoted lines

> 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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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 <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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5512-ls-remote.sh +1 −1

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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5512-ls-remote.sh +35 −29

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

Eric Sunshine· Mar 26, 2020, 15:24 UTC · re: Denton Liu · lore

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

On Thu, Mar 26, 2020 at 4:28 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 19 quoted lines

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

Denton Liu· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5512-ls-remote.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Show changes to t/t5512-ls-remote.sh +7 −7

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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5550-http-fetch-dumb.sh +1 −3

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

Eric Sunshine· Mar 26, 2020, 15:26 UTC · re: Denton Liu · lore

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

On Thu, Mar 26, 2020 at 4:28 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 15 quoted lines

> 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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5607-clone-bundle.sh +1 −1

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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5612-clone-refspec.sh +1 −1

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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5612-clone-refspec.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Show changes to t/t5612-clone-refspec.sh +12 −12

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· Mar 26, 2020, 08:27 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5801-remote-helpers.sh +8 −2

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

Denton Liu· Mar 27, 2020, 00:43 UTC · re: Denton Liu · lore

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

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

Show changes to t/t5512-ls-remote.sh +1 −1

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· Mar 27, 2020, 17:46 UTC · re: Denton Liu · lore

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

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

Show 18 quoted lines

> 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· Mar 27, 2020, 20:29 UTC · re: Junio C Hamano · lore

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

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· Mar 27, 2020, 21:39 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 7 quoted lines

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