patch, 2 partsbundle tests: use ">file" not ": >file"

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

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

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

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

Ævar Arnfjörð Bjarmason wrote:

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.

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

On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:

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.

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

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

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

below.

Thanks, Taylor

On Tue, Jul 20 2021, Taylor Blau wrote:

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

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

Thanks, used that.

On Thu, Jul 22, 2021 at 01:53:54AM +0200, Ævar Arnfjörð Bjarmason wrote:

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

On Tue, Jul 20, 2021 at 01:52:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

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
      '
      

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

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

On Thu, Jul 22, 2021 at 01:53:29AM +0200, Ævar Arnfjörð Bjarmason wrote:

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

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

On Thu, Jul 22, 2021 at 01:53:27AM +0200, Ævar Arnfjörð Bjarmason wrote:

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