Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling

Jeff King [off-list ref] writes:

On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:

quoted

Add tests that for-each-ref correctly reports broken loose reference
files and references that point at missing objects. In fact, two of
these tests fail, because (1) NULL_SHA1 is not recognized as an
invalid reference value, and (2) for-each-ref doesn't respect
REF_ISBROKEN. Fixes to come.

This whole series looks straightforward and correct to me. Thanks for a
pleasant read. I have two minor comments on the tests:

quoted

--- /dev/null
+++ b/t/t6301-for-each-ref-errors.sh

@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='for-each-ref errors for broken refs'
+
+. ./test-lib.sh
+
+ZEROS=0000000000000000000000000000000000000000
+MISSING=abababababababababababababababababababab

The test suite provides $_z40, so you can skip $ZEROS. I don't think
it's a big deal, though, and it may be nicer to have it explicitly next
to $MISSING here.

Yeah, ZEROS=$_z40 next to MISSING=abababa... may not be too bad.

quoted

+test_expect_success 'Missing objects are reported correctly' '
+	r=refs/heads/missing &&
+	echo $MISSING >.git/$r &&
+	test_when_finished "rm -f .git/$r" &&
+	echo "fatal: missing object $MISSING for $r" >missing-err &&
+	test_must_fail git for-each-ref 2>err &&
+	test_cmp missing-err err
+'

Due to b7dd2d2 (that you mentioned in the message for patch 2), we only
sometimes notice the missing objects. Is it worth testing that:

  git for-each-ref --format='%(refname)'

does _not_ barf here?

Sound like a good thing to check, too.

Thanks.