{"thread":{"id":"39515","subject":"[PATCH v3 1/4] t6301: new tests of for-each-ref error handling","startedAt":"2015-06-03T13:51:55Z","lastAt":"2015-06-08T17:37:39Z","messageCount":11,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"262866","messageId":"cover.1433339279.git.mhagger@alum.mit.edu","threadId":"39515","inReplyTo":null,"subject":"[PATCH v3 0/4] Fix how for-each-ref handles broken loose references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-03T13:51:55Z","receivedAt":"2015-06-03T13:51:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This reroll addresses all of the comments about v1 [1] and v2 [2].\nThanks to Stefan, Junio, and Peff for their comments about v2.\n\nChanges since v2:\n\n* Simplify logic flow in read_loose_refs().\n* Remove unnecessary call to hashclr() in read_loose_refs().\n* Improve a comment and commit message.\n\nThis patch series is also available from my GitHub account [3] as\nbranch for-each-ref-errors.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/270429\n[2] http://thread.gmane.org/gmane.comp.version-control.git/270556\n[3] https://github.com/mhagger/git\n\nMichael Haggerty (4):\n  t6301: new tests of for-each-ref error handling\n  for-each-ref: report broken references correctly\n  read_loose_refs(): simplify function logic\n  read_loose_refs(): treat NULL_SHA1 loose references as broken\n\n builtin/for-each-ref.c         |  5 ++++\n refs.c                         | 29 ++++++++++++++++------\n t/t6301-for-each-ref-errors.sh | 56 ++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+), 7 deletions(-)\n create mode 100755 t/t6301-for-each-ref-errors.sh\n\n-- \n2.1.4\n"},{"id":"262864","messageId":"48c6ecbe5a963a9ba89e3c25fc6e075a52ebcdbd.1433339279.git.mhagger@alum.mit.edu","threadId":"39515","inReplyTo":"cover.1433339279.git.mhagger@alum.mit.edu","subject":"[PATCH v3 1/4] t6301: new tests of for-each-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-03T13:51:56Z","receivedAt":"2015-06-03T13:51:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add tests that for-each-ref correctly reports broken loose reference\nfiles and references that point at missing objects. In fact, two of\nthese tests fail, because (1) NULL_SHA1 is not recognized as an\ninvalid reference value, and (2) for-each-ref doesn't respect\nREF_ISBROKEN. Fixes to come.\n\nNote that when for-each-ref is run with a --format option that doesn't\nrequire the object to be looked up, then we should still notice if a\nloose reference file is corrupt or contains NULL_SHA1, but we don't\nnotice if it points at a missing object because we don't do an object\nlookup. This is OK.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t6301-for-each-ref-errors.sh | 56 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 56 insertions(+)\n create mode 100755 t/t6301-for-each-ref-errors.sh\n\ndiff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\nnew file mode 100755\nindex 0000000..cf25244\n--- /dev/null\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -0,0 +1,56 @@\n+#!/bin/sh\n+\n+test_description='for-each-ref errors for broken refs'\n+\n+. ./test-lib.sh\n+\n+ZEROS=$_z40\n+MISSING=abababababababababababababababababababab\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m \"Initial\" &&\n+\tgit tag testtag &&\n+\tgit for-each-ref >full-list &&\n+\tgit for-each-ref --format=\"%(objectname) %(refname)\" >brief-list\n+'\n+\n+test_expect_failure 'Broken refs are reported correctly' '\n+\tr=refs/heads/bogus &&\n+\t: >.git/$r &&\n+\ttest_when_finished \"rm -f .git/$r\" &&\n+\techo \"warning: ignoring broken ref $r\" >broken-err &&\n+\tgit for-each-ref >out 2>err &&\n+\ttest_cmp full-list out &&\n+\ttest_cmp broken-err err\n+'\n+\n+test_expect_failure 'NULL_SHA1 refs are reported correctly' '\n+\tr=refs/heads/zeros &&\n+\techo $ZEROS >.git/$r &&\n+\ttest_when_finished \"rm -f .git/$r\" &&\n+\techo \"warning: ignoring broken ref $r\" >zeros-err &&\n+\tgit for-each-ref >out 2>err &&\n+\ttest_cmp full-list out &&\n+\ttest_cmp zeros-err err &&\n+\tgit for-each-ref --format=\"%(objectname) %(refname)\" >brief-out 2>brief-err &&\n+\ttest_cmp brief-list brief-out &&\n+\ttest_cmp zeros-err brief-err\n+'\n+\n+test_expect_success 'Missing objects are reported correctly' '\n+\tr=refs/heads/missing &&\n+\techo $MISSING >.git/$r &&\n+\ttest_when_finished \"rm -f .git/$r\" &&\n+\techo \"fatal: missing object $MISSING for $r\" >missing-err &&\n+\ttest_must_fail git for-each-ref 2>err &&\n+\ttest_cmp missing-err err &&\n+\t(\n+\t\tcat brief-list &&\n+\t\techo \"$MISSING $r\"\n+\t) | sort -k 2 >missing-brief-expected &&\n+\tgit for-each-ref --format=\"%(objectname) %(refname)\" >brief-out 2>brief-err &&\n+\ttest_cmp missing-brief-expected brief-out &&\n+\ttest_must_be_empty brief-err\n+'\n+\n+test_done\n-- \n2.1.4\n"},{"id":"262867","messageId":"505661bfc79d989cf0a155c7bb865e1a54f9ee07.1433339279.git.mhagger@alum.mit.edu","threadId":"39515","inReplyTo":"cover.1433339279.git.mhagger@alum.mit.edu","subject":"[PATCH v3 2/4] for-each-ref: report broken references correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-03T13:51:57Z","receivedAt":"2015-06-03T13:51:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If there is a loose reference file with invalid contents, \"git\nfor-each-ref\" incorrectly reports the problem as being a missing\nobject with name NULL_SHA1:\n\n    $ echo '12345678' >.git/refs/heads/nonsense\n    $ git for-each-ref\n    fatal: missing object 0000000000000000000000000000000000000000 for refs/heads/nonsense\n\nWith an explicit \"--format\" string, it can even report that the\nreference validly points at NULL_SHA1:\n\n    $ git for-each-ref --format='%(objectname) %(refname)'\n    0000000000000000000000000000000000000000 refs/heads/nonsense\n    $ echo $?\n    0\n\nThis has been broken since\n\n    b7dd2d2 for-each-ref: Do not lookup objects when they will not be used (2009-05-27)\n\n, which changed for-each-ref from using for_each_ref() to using\ngit_for_each_rawref() in order to avoid looking up the referred-to\nobjects unnecessarily. (When \"git for-each-ref\" is given a \"--format\"\nstring that doesn't include information about the pointed-to object,\nit does not look up the object at all, which makes it considerably\nfaster. Iterating with DO_FOR_EACH_INCLUDE_BROKEN is essential to this\noptimization because otherwise for_each_ref() would itself need to\ncheck whether the object exists as part of its brokenness test.)\n\nBut for_each_rawref() includes broken references in the iteration, and\n\"git for-each-ref\" doesn't itself reject references with REF_ISBROKEN.\nThe result is that broken references are processed *as if* they had\nthe value NULL_SHA1, which is the value stored in entries for broken\nreferences.\n\nChange \"git for-each-ref\" to emit warnings for references that are\nREF_ISBROKEN but to otherwise skip them.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/for-each-ref.c         | 5 +++++\n t/t6301-for-each-ref-errors.sh | 2 +-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 83f9cf9..13d2172 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -851,6 +851,11 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f\n \t\t  return 0;\n \t}\n \n+\tif (flag & REF_ISBROKEN) {\n+\t\t  warning(\"ignoring broken ref %s\", refname);\n+\t\t  return 0;\n+\t}\n+\n \tif (*cb->grab_pattern) {\n \t\tconst char **pattern;\n \t\tint namelen = strlen(refname);\ndiff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\nindex cf25244..72d2397 100755\n--- a/t/t6301-for-each-ref-errors.sh\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -14,7 +14,7 @@ test_expect_success setup '\n \tgit for-each-ref --format=\"%(objectname) %(refname)\" >brief-list\n '\n \n-test_expect_failure 'Broken refs are reported correctly' '\n+test_expect_success 'Broken refs are reported correctly' '\n \tr=refs/heads/bogus &&\n \t: >.git/$r &&\n \ttest_when_finished \"rm -f .git/$r\" &&\n-- \n2.1.4\n"},{"id":"262868","messageId":"ab21b4940709f6cfe11a663e4fa4e342e4f1ca41.1433339279.git.mhagger@alum.mit.edu","threadId":"39515","inReplyTo":"cover.1433339279.git.mhagger@alum.mit.edu","subject":"[PATCH v3 3/4] read_loose_refs(): simplify function logic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-03T13:51:58Z","receivedAt":"2015-06-03T13:51:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make it clearer that there are two possible ways to read the\nreference, but that we handle read errors uniformly regardless of\nwhich way it was read.\n\nThis refactoring also makes the following change easier to implement.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 47e4e53..6736424 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1308,19 +1308,24 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\t\t\t create_dir_entry(refs, refname.buf,\n \t\t\t\t\t\t\t  refname.len, 1));\n \t\t} else {\n+\t\t\tint read_ok;\n+\n \t\t\tif (*refs->name) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag = 0;\n-\t\t\t\tif (resolve_gitlink_ref(refs->name, refname.buf, sha1) < 0) {\n-\t\t\t\t\thashclr(sha1);\n-\t\t\t\t\tflag |= REF_ISBROKEN;\n-\t\t\t\t}\n-\t\t\t} else if (read_ref_full(refname.buf,\n-\t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t sha1, &flag)) {\n+\t\t\t\tread_ok = !resolve_gitlink_ref(refs->name,\n+\t\t\t\t\t\t\t       refname.buf, sha1);\n+\t\t\t} else {\n+\t\t\t\tread_ok = !read_ref_full(refname.buf,\n+\t\t\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t\t\t sha1, &flag);\n+\t\t\t}\n+\n+\t\t\tif (!read_ok) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t}\n+\n \t\t\tif (check_refname_format(refname.buf,\n \t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL)) {\n \t\t\t\thashclr(sha1);\n-- \n2.1.4\n"},{"id":"262865","messageId":"eb79f7d96ec74445f7adf62757119273280e09c9.1433339279.git.mhagger@alum.mit.edu","threadId":"39515","inReplyTo":"cover.1433339279.git.mhagger@alum.mit.edu","subject":"[PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-03T13:51:59Z","receivedAt":"2015-06-03T13:51:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n(and the code of other git implementations), so it is vastly more\nlikely that a reference was set to this value due to a software bug\nthan that NULL_SHA1 is the legitimate SHA-1 of an actual object.\nTherefore, if a loose reference has the value NULL_SHA1, consider it\nto be broken.\n\nAmusingly, each of the other 2^160-1 possible SHA-1 values is exactly\nas unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The\ndifference is that most of those other values are also very unlikely\nto be written to a loose reference file by accident, whereas\naccidentally writing NULL_SHA1 to a loose reference file would be an\neasy mistake to make.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c                         | 10 ++++++++++\n t/t6301-for-each-ref-errors.sh |  2 +-\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6736424..83af13d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1324,6 +1324,16 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\tif (!read_ok) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n+\t\t\t} else if (is_null_sha1(sha1)) {\n+\t\t\t\t/*\n+\t\t\t\t * It is so astronomically unlikely\n+\t\t\t\t * that NULL_SHA1 is the SHA-1 of an\n+\t\t\t\t * actual object that we consider its\n+\t\t\t\t * appearance in a loose reference\n+\t\t\t\t * file to be repo corruption\n+\t\t\t\t * (probably due to a software bug).\n+\t\t\t\t */\n+\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t}\n \n \t\t\tif (check_refname_format(refname.buf,\ndiff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\nindex 72d2397..cdb67a0 100755\n--- a/t/t6301-for-each-ref-errors.sh\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -24,7 +24,7 @@ test_expect_success 'Broken refs are reported correctly' '\n \ttest_cmp broken-err err\n '\n \n-test_expect_failure 'NULL_SHA1 refs are reported correctly' '\n+test_expect_success 'NULL_SHA1 refs are reported correctly' '\n \tr=refs/heads/zeros &&\n \techo $ZEROS >.git/$r &&\n \ttest_when_finished \"rm -f .git/$r\" &&\n-- \n2.1.4\n"},{"id":"262869","messageId":"20150603140803.GA23901@peff.net","threadId":"39515","inReplyTo":"eb79f7d96ec74445f7adf62757119273280e09c9.1433339279.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-03T14:08:04Z","receivedAt":"2015-06-03T14:08:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:\n\n> NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n\ns/of/an/ ?\n\n> (and the code of other git implementations), so it is vastly more\n> likely that a reference was set to this value due to a software bug\n> than that NULL_SHA1 is the legitimate SHA-1 of an actual object.\n> Therefore, if a loose reference has the value NULL_SHA1, consider it\n> to be broken.\n> \n> Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly\n> as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The\n> difference is that most of those other values are also very unlikely\n> to be written to a loose reference file by accident, whereas\n> accidentally writing NULL_SHA1 to a loose reference file would be an\n> easy mistake to make.\n\nFWIW, I think this justification (and the comment below) reads better\nthan what you had before.\n\n> diff --git a/refs.c b/refs.c\n> index 6736424..83af13d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1324,6 +1324,16 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n>  \t\t\tif (!read_ok) {\n>  \t\t\t\thashclr(sha1);\n>  \t\t\t\tflag |= REF_ISBROKEN;\n> +\t\t\t} else if (is_null_sha1(sha1)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * It is so astronomically unlikely\n> +\t\t\t\t * that NULL_SHA1 is the SHA-1 of an\n> +\t\t\t\t * actual object that we consider its\n> +\t\t\t\t * appearance in a loose reference\n> +\t\t\t\t * file to be repo corruption\n> +\t\t\t\t * (probably due to a software bug).\n> +\t\t\t\t */\n> +\t\t\t\tflag |= REF_ISBROKEN;\n\nNice. After reading the other thread, I did not think we needed to\nbother with more refactoring here, but I agree this end result flows\nnicely.\n\n-Peff\n"},{"id":"262903","messageId":"xmqqr3pswrpc.fsf@gitster.dls.corp.google.com","threadId":"39515","inReplyTo":"20150603140803.GA23901@peff.net","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-03T18:51:43Z","receivedAt":"2015-06-03T18:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:\n>\n>> NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n>\n> s/of/an/ ?\n\nAlso possibly s/invalid SHA-1/invalid ref/?\n\n>> (and the code of other git implementations), so it is vastly more\n>> likely that a reference was set to this value due to a software bug\n>> than that NULL_SHA1 is the legitimate SHA-1 of an actual object.\n>> Therefore, if a loose reference has the value NULL_SHA1, consider it\n>> to be broken.\n>> \n>> Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly\n>> as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The\n>> difference is that most of those other values are also very unlikely\n>> to be written to a loose reference file by accident, whereas\n>> accidentally writing NULL_SHA1 to a loose reference file would be an\n>> easy mistake to make.\n>\n> FWIW, I think this justification (and the comment below) reads better\n> than what you had before.\n\nI agree, and further I think the second paragraph is redundant and\nunnecessary.  If you update \"... likely that a reference was set to\nthis value\" to clarify that the \"reference\" it talks about is an\non-disk entity, not the in-core representation (which can\nlegitimately have NULL_SHA1 to signal \"invalid ref\", it would be\nsufficient.  I.e.\n\n\t... so it is vastly more likely that an on-disk reference\n\twas set to this value due to a bug ...\n"},{"id":"262915","messageId":"20150603201511.GA31157@peff.net","threadId":"39515","inReplyTo":"xmqqr3pswrpc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-03T20:15:12Z","receivedAt":"2015-06-03T20:15:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:\n> >\n> >> NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n> >\n> > s/of/an/ ?\n> \n> Also possibly s/invalid SHA-1/invalid ref/?\n\nI thought it was trying to express that we use the null sha1 as a\nsentinel value throughout the code, no matter if the value came from a\nref or otherwise.\n\n-Peff\n"},{"id":"262920","messageId":"xmqqmw0gwktt.fsf@gitster.dls.corp.google.com","threadId":"39515","inReplyTo":"20150603201511.GA31157@peff.net","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-03T21:20:14Z","receivedAt":"2015-06-03T21:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:\n>> >\n>> >> NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n>> >\n>> > s/of/an/ ?\n>> \n>> Also possibly s/invalid SHA-1/invalid ref/?\n>\n> I thought it was trying to express that we use the null sha1 as a\n> sentinel value throughout the code, no matter if the value came from a\n> ref or otherwise.\n\nYeah, an invalid object name, not limited to refs, is correct.\n\nThanks.\n"},{"id":"263187","messageId":"55755FC9.7040507@alum.mit.edu","threadId":"39515","inReplyTo":"xmqqmw0gwktt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T09:26:33Z","receivedAt":"2015-06-08T09:26:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/03/2015 11:20 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:\n>>\n>>> Jeff King <peff@peff.net> writes:\n>>>\n>>>> On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:\n>>>>\n>>>>> NULL_SHA1 is used to indicate of \"invalid SHA-1\" throughout our code\n>>>>\n>>>> s/of/an/ ?\n>>>\n>>> Also possibly s/invalid SHA-1/invalid ref/?\n>>\n>> I thought it was trying to express that we use the null sha1 as a\n>> sentinel value throughout the code, no matter if the value came from a\n>> ref or otherwise.\n> \n> Yeah, an invalid object name, not limited to refs, is correct.\n> \n> Thanks.\n\nThanks for all the comments. Taking them into consideration, I suggest\nchanging the last commit message to\n\n--8<-----------------------------------------------------------------\nread_loose_refs(): treat NULL_SHA1 loose references as broken\n\nNULL_SHA1 is used to indicate an \"invalid object name\" throughout our\ncode (and the code of other git implementations), so it is vastly more\nlikely that an on-disk reference was set to this value due to a\nsoftware bug than that NULL_SHA1 is the legitimate SHA-1 of an actual\nobject. Therefore, if a loose reference has the value NULL_SHA1,\nconsider it to be broken.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n--8<-----------------------------------------------------------------\n\nSince the only comments were about this one commit message, I won't send\nan updated patch series to the mailing list unless you request it. You\ncan also get the patches from my GitHub account [1], branch\n\"for-each-ref-errors\".\n\nMichael\n\n[1] https://github.com/mhagger/git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"263271","messageId":"xmqqpp56ayos.fsf@gitster.dls.corp.google.com","threadId":"39515","inReplyTo":"55755FC9.7040507@alum.mit.edu","subject":"Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-08T17:37:39Z","receivedAt":"2015-06-08T17:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Thanks for all the comments. Taking them into consideration, I suggest\n> changing the last commit message to\n> ...\n> Since the only comments were about this one commit message,...\n\nYeah, this round looked good otherwise.  Will amend in-place.\n\nThanks.\n"}]}