From: Jeff King <peff@peff.net>
Date: Mon, 01 Jun 2015 16:08:34 GMT
Subject: Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling
Message-ID: <20150601160834.GA15148@peff.net>
In-Reply-To: <1433174031-5471-2-git-send-email-mhagger@alum.mit.edu>

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.

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

-Peff