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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/01/2015 06:08 PM, Jeff King wrote:
> 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.

Dang, I knew about that variable but just forgot it. I'll make this change.

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

It makes sense. I will add it, with --format='%(objectname) %(refname)'
for added fun.

Thanks for the review!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu