git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Michael HaggertyNext: Junio C Hamano
Message 6 of 11 in “Fix how for-each-ref handles broken loose references”
  1. 0/4 Fix how for-each-ref handles broken loose referencesMichael Haggerty, Jun 3, 2015
  2. 1/4 t6301: new tests of for-each-ref error handlingMichael Haggerty, Jun 3, 2015
  3. 2/4 for-each-ref: report broken references correctlyMichael Haggerty, Jun 3, 2015
  4. 3/4 read_loose_refs(): simplify function logicMichael Haggerty, Jun 3, 2015
  5. 4/4 read_loose_refs(): treat NULL_SHA1 loose references as brokenMichael Haggerty, Jun 3, 2015
  6. Jeff KingJun 3, 2015
  7. Junio C HamanoJun 3, 2015
  8. Jeff KingJun 3, 2015
  9. Junio C HamanoJun 3, 2015
  10. Michael HaggertyJun 8, 2015
  11. Junio C HamanoJun 8, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.