Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
- From
Jeff King <peff@peff.net>
- Date
- Jun 3, 2015, 14:08 UTC
- Message-ID
- <20150603140803.GA23901@peff.net>
- In-Reply-To
- <eb79f7d96ec74445f7adf62757119273280e09c9.1433339279.git.mhagger@alum.mit.edu>
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/ ?
Show 12 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.
Show 18 quoted lines
> 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