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 mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
