Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Jeff King <peff@peff.net>]

On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:

> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code

s/of/an/ ?

> (and the code of other git implementations), so it is vastly more
> likely that a reference was set to this value due to a software bug
> than that NULL_SHA1 is the legitimate SHA-1 of an actual object.
> Therefore, if a loose reference has the value NULL_SHA1, consider it
> to be broken.
> 
> Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly
> as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The
> difference is that most of those other values are also very unlikely
> to be written to a loose reference file by accident, whereas
> accidentally writing NULL_SHA1 to a loose reference file would be an
> easy mistake to make.

FWIW, I think this justification (and the comment below) reads better
than what you had before.

> diff --git a/refs.c b/refs.c
> index 6736424..83af13d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1324,6 +1324,16 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
>  			if (!read_ok) {
>  				hashclr(sha1);
>  				flag |= REF_ISBROKEN;
> +			} else if (is_null_sha1(sha1)) {
> +				/*
> +				 * It is so astronomically unlikely
> +				 * that NULL_SHA1 is the SHA-1 of an
> +				 * actual object that we consider its
> +				 * appearance in a loose reference
> +				 * file to be repo corruption
> +				 * (probably due to a software bug).
> +				 */
> +				flag |= REF_ISBROKEN;

Nice. After reading the other thread, I did not think we needed to
bother with more refactoring here, but I agree this end result flows
nicely.

-Peff