Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
Michael Haggerty mhag...@alum.mit.edu writes: Thanks for all the comments. Taking them into consideration, I suggest changing the last commit message to ... Since the only comments were about this one commit message,... Yeah, this round looked good otherwise. Will amend in-place. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
On 06/03/2015 11:20 PM, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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/? I thought it was trying to express that we use the null sha1 as a sentinel value throughout the code, no matter if the value came from a ref or otherwise. Yeah, an invalid object name, not limited to refs, is correct. Thanks. Thanks for all the comments. Taking them into consideration, I suggest changing the last commit message to --8- read_loose_refs(): treat NULL_SHA1 loose references as broken NULL_SHA1 is used to indicate an invalid object name throughout our code (and the code of other git implementations), so it is vastly more likely that an on-disk 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. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --8- Since the only comments were about this one commit message, I won't send an updated patch series to the mailing list unless you request it. You can also get the patches from my GitHub account [1], branch for-each-ref-errors. Michael [1] https://github.com/mhagger/git -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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/? I thought it was trying to express that we use the null sha1 as a sentinel value throughout the code, no matter if the value came from a ref or otherwise. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
Jeff King p...@peff.net writes: 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/? (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 ... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
Jeff King p...@peff.net writes: On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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/? I thought it was trying to express that we use the null sha1 as a sentinel value throughout the code, no matter if the value came from a ref or otherwise. Yeah, an invalid object name, not limited to refs, is correct. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
