[2/8] t5512: generate references with generate_references()
diff mbox series

Message ID 674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton@gmail.com
State New
Headers show
Series
  • t: replace incorrect test_must_fail usage (part 3)
Related show

Commit Message

Denton Liu March 25, 2020, 5:54 a.m. UTC
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(-)

Comments

Eric Sunshine March 25, 2020, 6:08 a.m. UTC | #1
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 March 25, 2020, 6:41 a.m. UTC | #2
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).

Patch
diff mbox series

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