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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2015, 18:51 UTC
Message-ID: <xmqqr3pswrpc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150603140803.GA23901@peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

Also possibly s/invalid SHA-1/invalid ref/?

Show 15 quoted lines

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

I agree, and further I think the second paragraph is redundant and unnecessary. If you update "... likely that a reference was set to this value" to clarify that the "reference" it talks about is an on-disk entity, not the in-core representation (which can legitimately have NULL_SHA1 to signal "invalid ref", it would be sufficient. I.e.

	... so it is vastly more likely that an on-disk reference
	was set to this value due to a bug ...