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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:
>
>> 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:
>
>> --- /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.

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