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

On Tue, Jul 20 2021, Taylor Blau wrote:

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

quoted

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.

quoted

 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.

quoted

 	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.

quoted

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