[PATCH 0/2] bundle tests: modernize, fix missing coverage & test_cmp

[PATCH 0/2] bundle tests: modernize, fix missing coverage & test_cmp

[Ævar Arnfjörð Bjarmason]

A trivial series to improve the bundle tests a bit. This is split off
from some larger changes to "git bundle" I have cooking, where the
"test_cmp" in 2/2 helped assert & catch regressions.

Ævar Arnfjörð Bjarmason (2):
  bundle tests: use ">file" not ": >file"
  bundle tests: use test_cmp instead of grep

 t/t5607-clone-bundle.sh | 74 ++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 16 deletions(-)

-- 
2.32.0.874.ge7a9d58bfcf

[PATCH 1/2] bundle tests: use ">file" not ": >file"

[Ævar Arnfjörð Bjarmason]

Change redundant uses of ":" on the LHS of a ">" to the more commonly
use ">file" pattern.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5607-clone-bundle.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index f4c383cd5ce..c9323a08fe8 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '
 '
 
 test_expect_success 'empty bundle file is rejected' '
-	: >empty-bundle &&
+	>empty-bundle &&
 	test_must_fail git fetch empty-bundle
 '
 
 # This triggers a bug in older versions where the resulting line (with
 # --pretty=oneline) was longer than a 1024-char buffer.
 test_expect_success 'ridiculously long subject in boundary' '
-	: >file4 &&
+	>file4 &&
 	test_tick &&
 	git add file4 &&
 	printf "%01200d\n" 0 | git commit -F - &&
@@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '
 '
 
 test_expect_success 'prerequisites with an empty commit message' '
-	: >file1 &&
+	>file1 &&
 	git add file1 &&
 	test_tick &&
 	git commit --allow-empty-message -m "" &&
-- 
2.32.0.874.ge7a9d58bfcf

[PATCH 2/2] bundle tests: use test_cmp instead of grep

[Ævar Arnfjörð Bjarmason]

Change the bundle tests to fully compare the expected "git ls-remote"
or "git bundle list-heads" output, instead of merely grepping it.

This avoids subtle regressions in the tests. In
f62e0a39b6 (t5704 (bundle): add tests for bundle --stdin, 2010-04-19)
the "bundle --stdin <rev-list options>" test was added to make sure we
didn't include the tag.

But since the --stdin mode didn't work until 5bb0fd2cab (bundle:
arguments can be read from stdin, 2021-01-11) our grepping of
"master" (later "main") missed the important part of the test.

Namely that we should not include the "refs/tags/tag" tag in that
case. Since the test only grepped for "main" in the output we'd miss a
regression in that code.

So let's use test_cmp instead, and also in the other nearby tests
where it's easy.

This does make things a bit more verbose in the case of the test
that's checking the bundle header, since it's different under SHA1 and
SHA256. I think this makes test easier to follow.

I've got some WIP changes to extend the "git bundle" command to dump
parts of the header out, which are easier to understand if we test the
output explicitly like this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5607-clone-bundle.sh | 68 +++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index c9323a08fe8..a7f18407e32 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -29,11 +29,21 @@ test_expect_success '"verify" needs a worktree' '
 
 test_expect_success 'annotated tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
-	git ls-remote bundle > output &&
-	grep tag output &&
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse tag)	refs/tags/tag
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote bundle >actual &&
+	test_cmp expect actual &&
+
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
-	git ls-remote bundle > output &&
-	! grep tag output
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote bundle >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'die if bundle file cannot be created' '
@@ -43,14 +53,20 @@ test_expect_success 'die if bundle file cannot be created' '
 
 test_expect_success 'bundle --stdin' '
 	echo main | git bundle create stdin-bundle.bdl --stdin &&
-	git ls-remote stdin-bundle.bdl >output &&
-	grep main output
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote stdin-bundle.bdl >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'bundle --stdin <rev-list options>' '
 	echo main | git bundle create hybrid-bundle.bdl --stdin tag &&
-	git ls-remote hybrid-bundle.bdl >output &&
-	grep main output
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote stdin-bundle.bdl >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'empty bundle file is rejected' '
@@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '
 	printf "%01200d\n" 0 | git commit -F - &&
 	test_commit fifth &&
 	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
-	git bundle list-heads long-subject-bundle.bdl >heads &&
-	test -s heads &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main) HEAD
+	EOF
+	git bundle list-heads long-subject-bundle.bdl >actual &&
+	test_cmp expect actual &&
+
 	git fetch long-subject-bundle.bdl &&
-	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
-	grep "^-$OID_REGEX " boundary
+
+	cat >expect.common <<-EOF &&
+	-$(git log --pretty=format:"%H %s" -1 HEAD^)
+	$(git rev-parse HEAD) HEAD
+	EOF
+	if test_have_prereq SHA1
+	then
+		cp expect.common expect
+	else
+		echo @object-format=sha256 >expect
+		cat expect.common >>expect
+	fi &&
+	if test_have_prereq SHA1
+	then
+		head -n 3 long-subject-bundle.bdl >bundle-header
+	else
+		head -n 4 long-subject-bundle.bdl >bundle-header
+	fi &&
+	grep -v "^#" bundle-header >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'prerequisites with an empty commit message' '
@@ -103,7 +141,11 @@ test_expect_success 'fetch SHA-1 from bundle' '
 
 test_expect_success 'git bundle uses expected default format' '
 	git bundle create bundle HEAD^.. &&
-	head -n1 bundle | grep "^# v$(test_oid version) git bundle$"
+	cat >expect <<-EOF &&
+	# v$(test_oid version) git bundle
+	EOF
+	head -n1 bundle >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'git bundle v3 has expected contents' '
-- 
2.32.0.874.ge7a9d58bfcf

Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep

[Taylor Blau]

On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:
> So let's use test_cmp instead, and also in the other nearby tests
> where it's easy.

I took a look at this patch carefully to make sure that this
transformation also improved the readability, too.

Looking around, I think that this was a good improvement in readability,
but also hardened the tests (for the reasons that you mentioned). One
tiny note below.

>  test_expect_success 'empty bundle file is rejected' '
> @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '
>  	printf "%01200d\n" 0 | git commit -F - &&
>  	test_commit fifth &&
>  	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
> -	git bundle list-heads long-subject-bundle.bdl >heads &&
> -	test -s heads &&
> +	cat >expect <<-EOF &&
> +	$(git rev-parse main) HEAD
> +	EOF
> +	git bundle list-heads long-subject-bundle.bdl >actual &&
> +	test_cmp expect actual &&

This is quite readable, but the assertion below gets much more
complicated as a result of the change.

>  	git fetch long-subject-bundle.bdl &&
> -	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
> -	grep "^-$OID_REGEX " boundary
> +
> +	cat >expect.common <<-EOF &&
> +	-$(git log --pretty=format:"%H %s" -1 HEAD^)
> +	$(git rev-parse HEAD) HEAD
> +	EOF
> +	if test_have_prereq SHA1
> +	then
> +		cp expect.common expect
> +	else
> +		echo @object-format=sha256 >expect
> +		cat expect.common >>expect
> +	fi &&

Here we're setting up expect, but I think flipping the order might make
things a little easier to follow. Maybe something like this:

    rm expect &&
    if ! test_have_prereq SHA1
    then
      echo "@object-format=sha256" >expect
    fi &&
    cat >>expect <<-EOF &&
    -$(git log --pretty=format:"%H %s" -1 HEAD^)
    $(git rev-parse HEAD) HEAD
    EOF &&

Or, if you wanted to go further, you could do something like:

    cat >expect <<-EOF
    $(test_have_prereq SHA1 || echo "@object-format=sha256")
    -$(git log --pretty=format:"%H %s" -1 HEAD^)
    $(git rev-parse HEAD) HEAD
    EOF

which is arguably a little tighter (although I find the
echo-in-a-heredoc thing to be kind of ugly).

> +	if test_have_prereq SHA1
> +	then
> +		head -n 3 long-subject-bundle.bdl >bundle-header
> +	else
> +		head -n 4 long-subject-bundle.bdl >bundle-header
> +	fi &&
> +	grep -v "^#" bundle-header >actual &&

Here I would suggest getting rid of the bundle-header intermediary and
instead writing:

    if test_have_prereq SHA1
    then
      head -n 3 long-subject-bundle.bdl
    else
      head -n 4 long-subject-bundle.bdl
    fi | grep -v "^#" >actual

and then having your

> +	test_cmp expect actual

below.

Thanks,
Taylor

Re: [PATCH 0/2] bundle tests: modernize, fix missing coverage & test_cmp

[Taylor Blau]

On Tue, Jul 20, 2021 at 01:52:07PM +0200, Ævar Arnfjörð Bjarmason wrote:
> A trivial series to improve the bundle tests a bit. This is split off
> from some larger changes to "git bundle" I have cooking, where the
> "test_cmp" in 2/2 helped assert & catch regressions.

Thanks. Both patches looked good to me, and I left a couple of minor
suggestions on the latter patch to potentially clean things up a little
bit.

But please feel free to ignore them, and only pick them up if you feel
they do improve the readability. Since I would be happy to see this
picked up as-is, it has my:

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH 1/2] bundle tests: use ">file" not ": >file"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change redundant uses of ":" on the LHS of a ">" to the more commonly

While the changes are correct, I am not sure if the adjective
"redundant" would apply.  The word refers to uses of two or more
when one would suffice, but in this case, you use one explicit no-op
command ":" when you need zero.

> use ">file" pattern.

"use" -> "used".

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5607-clone-bundle.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> index f4c383cd5ce..c9323a08fe8 100755
> --- a/t/t5607-clone-bundle.sh
> +++ b/t/t5607-clone-bundle.sh
> @@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '
>  '
>  
>  test_expect_success 'empty bundle file is rejected' '
> -	: >empty-bundle &&
> +	>empty-bundle &&
>  	test_must_fail git fetch empty-bundle
>  '
>  
>  # This triggers a bug in older versions where the resulting line (with
>  # --pretty=oneline) was longer than a 1024-char buffer.
>  test_expect_success 'ridiculously long subject in boundary' '
> -	: >file4 &&
> +	>file4 &&
>  	test_tick &&
>  	git add file4 &&
>  	printf "%01200d\n" 0 | git commit -F - &&
> @@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '
>  '
>  
>  test_expect_success 'prerequisites with an empty commit message' '
> -	: >file1 &&
> +	>file1 &&
>  	git add file1 &&
>  	test_tick &&
>  	git commit --allow-empty-message -m "" &&

RE: [PATCH 1/2] bundle tests: use ">file" not ": >file"

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> Change redundant uses of ":" on the LHS of a ">" to the more commonly
> use ">file" pattern.

While this is redundant in bash, it's not redundant in zsh.

Probably not a big deal since I don't think the test suite can run in
zsh right now, but it's taking us on the opposite direction.

-- 
Felipe Contreras

[PATCH v2 0/2] bundle tests: modernize, fix missing coverage & test_cmp

[Ævar Arnfjörð Bjarmason]

A trivial series to improve the bundle tests a bit. This is split off
from some larger changes to "git bundle" I have cooking, where the
"test_cmp" in 2/2 helped assert & catch regressions.

Ævar Arnfjörð Bjarmason (2):
  bundle tests: use ">file" not ": >file"
  bundle tests: use test_cmp instead of grep

 t/t5607-clone-bundle.sh | 72 ++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 16 deletions(-)

Range-diff against v1:
1:  746d727113b ! 1:  2a77f80725d bundle tests: use ">file" not ": >file"
    @@ Metadata
      ## Commit message ##
         bundle tests: use ">file" not ": >file"
     
    -    Change redundant uses of ":" on the LHS of a ">" to the more commonly
    -    use ">file" pattern.
    +    Change uses of ":" on the LHS of a ">" to the more commonly used
    +    ">file" pattern in t/t5607-clone-bundle.sh.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  062f34abf1a ! 2:  d5504fd764c bundle tests: use test_cmp instead of grep
    @@ t/t5607-clone-bundle.sh: test_expect_success 'ridiculously long subject in bound
     -	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
     -	grep "^-$OID_REGEX " boundary
     +
    -+	cat >expect.common <<-EOF &&
    ++	if ! test_have_prereq SHA1
    ++	then
    ++		echo "@object-format=sha256"
    ++	fi >expect &&
    ++	cat >>expect <<-EOF &&
     +	-$(git log --pretty=format:"%H %s" -1 HEAD^)
     +	$(git rev-parse HEAD) HEAD
     +	EOF
    ++
     +	if test_have_prereq SHA1
     +	then
    -+		cp expect.common expect
    -+	else
    -+		echo @object-format=sha256 >expect
    -+		cat expect.common >>expect
    -+	fi &&
    -+	if test_have_prereq SHA1
    -+	then
    -+		head -n 3 long-subject-bundle.bdl >bundle-header
    ++		head -n 3 long-subject-bundle.bdl
     +	else
    -+		head -n 4 long-subject-bundle.bdl >bundle-header
    -+	fi &&
    -+	grep -v "^#" bundle-header >actual &&
    ++		head -n 4 long-subject-bundle.bdl
    ++	fi | grep -v "^#" >actual &&
    ++
     +	test_cmp expect actual
      '
      
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v2 1/2] bundle tests: use ">file" not ": >file"

[Ævar Arnfjörð Bjarmason]

Change uses of ":" on the LHS of a ">" to the more commonly used
">file" pattern in t/t5607-clone-bundle.sh.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5607-clone-bundle.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index f4c383cd5ce..c9323a08fe8 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '
 '
 
 test_expect_success 'empty bundle file is rejected' '
-	: >empty-bundle &&
+	>empty-bundle &&
 	test_must_fail git fetch empty-bundle
 '
 
 # This triggers a bug in older versions where the resulting line (with
 # --pretty=oneline) was longer than a 1024-char buffer.
 test_expect_success 'ridiculously long subject in boundary' '
-	: >file4 &&
+	>file4 &&
 	test_tick &&
 	git add file4 &&
 	printf "%01200d\n" 0 | git commit -F - &&
@@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '
 '
 
 test_expect_success 'prerequisites with an empty commit message' '
-	: >file1 &&
+	>file1 &&
 	git add file1 &&
 	test_tick &&
 	git commit --allow-empty-message -m "" &&
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v2 2/2] bundle tests: use test_cmp instead of grep

[Ævar Arnfjörð Bjarmason]

Change the bundle tests to fully compare the expected "git ls-remote"
or "git bundle list-heads" output, instead of merely grepping it.

This avoids subtle regressions in the tests. In
f62e0a39b6 (t5704 (bundle): add tests for bundle --stdin, 2010-04-19)
the "bundle --stdin <rev-list options>" test was added to make sure we
didn't include the tag.

But since the --stdin mode didn't work until 5bb0fd2cab (bundle:
arguments can be read from stdin, 2021-01-11) our grepping of
"master" (later "main") missed the important part of the test.

Namely that we should not include the "refs/tags/tag" tag in that
case. Since the test only grepped for "main" in the output we'd miss a
regression in that code.

So let's use test_cmp instead, and also in the other nearby tests
where it's easy.

This does make things a bit more verbose in the case of the test
that's checking the bundle header, since it's different under SHA1 and
SHA256. I think this makes test easier to follow.

I've got some WIP changes to extend the "git bundle" command to dump
parts of the header out, which are easier to understand if we test the
output explicitly like this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5607-clone-bundle.sh | 66 +++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index c9323a08fe8..ed0d911e953 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -29,11 +29,21 @@ test_expect_success '"verify" needs a worktree' '
 
 test_expect_success 'annotated tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
-	git ls-remote bundle > output &&
-	grep tag output &&
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse tag)	refs/tags/tag
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote bundle >actual &&
+	test_cmp expect actual &&
+
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
-	git ls-remote bundle > output &&
-	! grep tag output
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote bundle >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'die if bundle file cannot be created' '
@@ -43,14 +53,20 @@ test_expect_success 'die if bundle file cannot be created' '
 
 test_expect_success 'bundle --stdin' '
 	echo main | git bundle create stdin-bundle.bdl --stdin &&
-	git ls-remote stdin-bundle.bdl >output &&
-	grep main output
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote stdin-bundle.bdl >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'bundle --stdin <rev-list options>' '
 	echo main | git bundle create hybrid-bundle.bdl --stdin tag &&
-	git ls-remote hybrid-bundle.bdl >output &&
-	grep main output
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	git ls-remote stdin-bundle.bdl >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'empty bundle file is rejected' '
@@ -67,11 +83,31 @@ test_expect_success 'ridiculously long subject in boundary' '
 	printf "%01200d\n" 0 | git commit -F - &&
 	test_commit fifth &&
 	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
-	git bundle list-heads long-subject-bundle.bdl >heads &&
-	test -s heads &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main) HEAD
+	EOF
+	git bundle list-heads long-subject-bundle.bdl >actual &&
+	test_cmp expect actual &&
+
 	git fetch long-subject-bundle.bdl &&
-	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
-	grep "^-$OID_REGEX " boundary
+
+	if ! test_have_prereq SHA1
+	then
+		echo "@object-format=sha256"
+	fi >expect &&
+	cat >>expect <<-EOF &&
+	-$(git log --pretty=format:"%H %s" -1 HEAD^)
+	$(git rev-parse HEAD) HEAD
+	EOF
+
+	if test_have_prereq SHA1
+	then
+		head -n 3 long-subject-bundle.bdl
+	else
+		head -n 4 long-subject-bundle.bdl
+	fi | grep -v "^#" >actual &&
+
+	test_cmp expect actual
 '
 
 test_expect_success 'prerequisites with an empty commit message' '
@@ -103,7 +139,11 @@ test_expect_success 'fetch SHA-1 from bundle' '
 
 test_expect_success 'git bundle uses expected default format' '
 	git bundle create bundle HEAD^.. &&
-	head -n1 bundle | grep "^# v$(test_oid version) git bundle$"
+	cat >expect <<-EOF &&
+	# v$(test_oid version) git bundle
+	EOF
+	head -n1 bundle >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'git bundle v3 has expected contents' '
-- 
2.32.0.955.ge7c5360f7e7

Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20 2021, Taylor Blau wrote:

> On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> So let's use test_cmp instead, and also in the other nearby tests
>> where it's easy.
>
> I took a look at this patch carefully to make sure that this
> transformation also improved the readability, too.
>
> Looking around, I think that this was a good improvement in readability,
> but also hardened the tests (for the reasons that you mentioned). One
> tiny note below.
>
>>  test_expect_success 'empty bundle file is rejected' '
>> @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '
>>  	printf "%01200d\n" 0 | git commit -F - &&
>>  	test_commit fifth &&
>>  	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
>> -	git bundle list-heads long-subject-bundle.bdl >heads &&
>> -	test -s heads &&
>> +	cat >expect <<-EOF &&
>> +	$(git rev-parse main) HEAD
>> +	EOF
>> +	git bundle list-heads long-subject-bundle.bdl >actual &&
>> +	test_cmp expect actual &&
>
> This is quite readable, but the assertion below gets much more
> complicated as a result of the change.
>
>>  	git fetch long-subject-bundle.bdl &&
>> -	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
>> -	grep "^-$OID_REGEX " boundary
>> +
>> +	cat >expect.common <<-EOF &&
>> +	-$(git log --pretty=format:"%H %s" -1 HEAD^)
>> +	$(git rev-parse HEAD) HEAD
>> +	EOF
>> +	if test_have_prereq SHA1
>> +	then
>> +		cp expect.common expect
>> +	else
>> +		echo @object-format=sha256 >expect
>> +		cat expect.common >>expect
>> +	fi &&
>
> Here we're setting up expect, but I think flipping the order might make
> things a little easier to follow. Maybe something like this:
>
>     rm expect &&
>     if ! test_have_prereq SHA1
>     then
>       echo "@object-format=sha256" >expect
>     fi &&
>     cat >>expect <<-EOF &&
>     -$(git log --pretty=format:"%H %s" -1 HEAD^)
>     $(git rev-parse HEAD) HEAD
>     EOF &&

Thanks, I used pretty much that as-is for v2.

> Or, if you wanted to go further, you could do something like:
>
>     cat >expect <<-EOF
>     $(test_have_prereq SHA1 || echo "@object-format=sha256")
>     -$(git log --pretty=format:"%H %s" -1 HEAD^)
>     $(git rev-parse HEAD) HEAD
>     EOF
>
> which is arguably a little tighter (although I find the
> echo-in-a-heredoc thing to be kind of ugly).

This one won't work because you'll have an empty line at the start under
SHA-1.

>> +	if test_have_prereq SHA1
>> +	then
>> +		head -n 3 long-subject-bundle.bdl >bundle-header
>> +	else
>> +		head -n 4 long-subject-bundle.bdl >bundle-header
>> +	fi &&
>> +	grep -v "^#" bundle-header >actual &&
>
> Here I would suggest getting rid of the bundle-header intermediary and
> instead writing:
>
>     if test_have_prereq SHA1
>     then
>       head -n 3 long-subject-bundle.bdl
>     else
>       head -n 4 long-subject-bundle.bdl
>     fi | grep -v "^#" >actual
>
> and then having your

Thanks, used that.

Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep

[Taylor Blau]

On Thu, Jul 22, 2021 at 01:53:54AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > Or, if you wanted to go further, you could do something like:
> >
> >     cat >expect <<-EOF
> >     $(test_have_prereq SHA1 || echo "@object-format=sha256")
> >     -$(git log --pretty=format:"%H %s" -1 HEAD^)
> >     $(git rev-parse HEAD) HEAD
> >     EOF
> >
> > which is arguably a little tighter (although I find the
> > echo-in-a-heredoc thing to be kind of ugly).
>
> This one won't work because you'll have an empty line at the start under
> SHA-1.

Ah, you're totally right: good catch. I think it's avoidable by smashing
the first two lines into one. If the subshell prints nothing, then the
first line will start with "-$(git log ...)", otherwise, if it does
print something, then the echo will print a newline to separate it from
the "-$(git log ...) output which will go on the second line.

But that is definitely uglier than my first suggestion, so I'm glad that
you didn't try and make what I wrote above work :).

Thanks,
Taylor

Re: [PATCH v2 2/2] bundle tests: use test_cmp instead of grep

[Taylor Blau]

On Thu, Jul 22, 2021 at 01:53:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Change the bundle tests to fully compare the expected "git ls-remote"
> or "git bundle list-heads" output, instead of merely grepping it.

Thanks; this version makes all of those tests more readable. I did have
one idle thought while reading, but what you wrote is right (so I'm just
thinking out loud as opposed to anything else).

> -	sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary &&
> -	grep "^-$OID_REGEX " boundary
> +
> +	if ! test_have_prereq SHA1
> +	then
> +		echo "@object-format=sha256"
> +	fi >expect &&
> +	cat >>expect <<-EOF &&

On my first read, I worried that this 'cat >>expect' when we're in
SHA-1 mode would append to the 'expect' we generated a few lines up. But
it doesn't, since we redirect the output of the 'if' statement into
'expect' instead of doing the redirect attached to the echo.

Subtle, but that's right. Good.

Thanks,
Taylor

Re: [PATCH v2 0/2] bundle tests: modernize, fix missing coverage & test_cmp

[Taylor Blau]

On Thu, Jul 22, 2021 at 01:53:27AM +0200, Ævar Arnfjörð Bjarmason wrote:
> A trivial series to improve the bundle tests a bit. This is split off
> from some larger changes to "git bundle" I have cooking, where the
> "test_cmp" in 2/2 helped assert & catch regressions.

Thanks, this version looks great to me, so I'd be happy to see it get
picked up.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor