{"thread":{"id":"39498","subject":"[PATCH v2 0/3] Fix how for-each-ref handles broken loose references","startedAt":"2015-06-02T15:57:24Z","lastAt":"2015-06-03T09:09:03Z","messageCount":8,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"262737","messageId":"1433260647-18181-1-git-send-email-mhagger@alum.mit.edu","threadId":"39498","inReplyTo":null,"subject":"[PATCH v2 0/3] Fix how for-each-ref handles broken loose references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T15:57:24Z","receivedAt":"2015-06-02T15:57:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a reroll of [1]. Thanks to Peff and Junio for their comments\nabout v1.\n\nChanges since v1:\n\n* Use $_z40 to initialize $ZEROS in test.\n\n* Test that \"git for-each-ref --format=\"%(objectname) %(refname)\"\n *doesn't* notice references that point at missing objects.\n\nThis patch series is also available from my GitHub account [2] as\nbranch for-each-ref-errors.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/270429\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (3):\n t6301: new tests of for-each-ref error handling\n for-each-ref: report broken references correctly\n read_loose_refs(): treat NULL_SHA1 loose references as broken\n\n builtin/for-each-ref.c | 5 ++++\n refs.c | 7 ++++++\n t/t6301-for-each-ref-errors.sh | 56 ++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 68 insertions(+)\n create mode 100755 t/t6301-for-each-ref-errors.sh\n\n-- \n2.1.4\n"},{"id":"262739","messageId":"1433260647-18181-2-git-send-email-mhagger@alum.mit.edu","threadId":"39498","inReplyTo":"1433260647-18181-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/3] t6301: new tests of for-each-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T15:57:25Z","receivedAt":"2015-06-02T15:57:25Z","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 \n---\n\nNotes (discussion):\n Note that a reference that points at NULL_SHA1 is reported as \"broken\"\n rather than \"missing\". This is because NULL_SHA1 is manifestly bogus,\n whereas we have no systematic basis for rejecting any other\n 40-character hexadecimal string without doing an object lookup.\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":"262740","messageId":"1433260647-18181-3-git-send-email-mhagger@alum.mit.edu","threadId":"39498","inReplyTo":"1433260647-18181-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/3] for-each-ref: report broken references correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T15:57:26Z","receivedAt":"2015-06-02T15:57:26Z","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 \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":"262738","messageId":"1433260647-18181-4-git-send-email-mhagger@alum.mit.edu","threadId":"39498","inReplyTo":"1433260647-18181-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T15:57:27Z","receivedAt":"2015-06-02T15:57:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"NULL_SHA1 is never a valid value for a reference. If a loose reference\nhas that value, mark it as broken.\n\nWhy check NULL_SHA1 and not the nearly 2^160 other SHA-1s that are\nalso invalid in a given repository? Because (a) it is cheap to test\nfor NULL_SHA1, and (b) NULL_SHA1 is often used as a \"SHA-1 is invalid\"\nvalue inside of Git client source code (not only ours!), and\naccidentally writing it to a loose reference file would be an easy\nmistake to make.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 7 +++++++\n t/t6301-for-each-ref-errors.sh | 2 +-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 47e4e53..c28fde1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1321,6 +1321,13 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t}\n+\n+\t\t\tif (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {\n+\t\t\t\t/* NULL_SHA1 is never a valid reference value. */\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);\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":"262748","messageId":"CAGZ79kZG95HB-siR_jMORPuqcL2J_YEwg0VPHE=8XN7gWD4JkA@mail.gmail.com","threadId":"39498","inReplyTo":"1433260647-18181-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-02T17:28:35Z","receivedAt":"2015-06-02T17:28:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 2, 2015 at 8:57 AM, Michael Haggerty wrote:\n> NULL_SHA1 is never a valid value for a reference. If a loose reference\n> has that value, mark it as broken.\n>\n> Why check NULL_SHA1 and not the nearly 2^160 other SHA-1s that are\n> also invalid in a given repository? Because (a) it is cheap to test\n> for NULL_SHA1, and (b) NULL_SHA1 is often used as a \"SHA-1 is invalid\"\n\nI don't quite agree with the reasoning here. Just because it's cheap doesn't\nmean it's right. ;) But I fully agree with (b) so this still makes sense.\n\n\n> value inside of Git client source code (not only ours!), and\n> accidentally writing it to a loose reference file would be an easy\n> mistake to make.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 7 +++++++\n> t/t6301-for-each-ref-errors.sh | 2 +-\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 47e4e53..c28fde1 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1321,6 +1321,13 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n> hashclr(sha1);\n> flag |= REF_ISBROKEN;\n> }\n> +\n> + if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {\n\nWhy do we do the extra check for !(flag & REF_ISBROKEN) here?\n\n> + /* NULL_SHA1 is never a valid reference value. ...\n\n... *by our definition*, because we believe it helps detecting\nerrors/mistakes in the future.\n\n*/\n> + hashclr(sha1);\n\nWhile this code looks consistent to the rest around, at closer inspection\nthis feels a bit redundant to me. If is_null_sha1(sha1) is true, then\nhashclr(sha1); doesn't change the state. Or did I miss a subtle side effect?\n\n> + flag |= REF_ISBROKEN;\n> + }\n> +\n> if (check_refname_format(refname.buf,\n> REFNAME_ALLOW_ONELEVEL)) {\n> hashclr(sha1);\n> diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\n> index 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> test_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> r=refs/heads/zeros &&\n> echo $ZEROS >.git/$r &&\n> test_when_finished \"rm -f .git/$r\" &&\n> --\n> 2.1.4\n>\n"},{"id":"262772","messageId":"xmqq7frl3m6i.fsf@gitster.dls.corp.google.com","threadId":"39498","inReplyTo":"1433260647-18181-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-02T20:11:33Z","receivedAt":"2015-06-02T20:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> NULL_SHA1 is never a valid value for a reference. If a loose reference\n> has that value, mark it as broken.\n>\n> Why check NULL_SHA1 and not the nearly 2^160 other SHA-1s that are\n> also invalid in a given repository? Because (a) it is cheap to test\n> for NULL_SHA1, and (b) NULL_SHA1 is often used as a \"SHA-1 is invalid\"\n> value inside of Git client source code (not only ours!), and\n> accidentally writing it to a loose reference file would be an easy\n> mistake to make.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 7 +++++++\n> t/t6301-for-each-ref-errors.sh | 2 +-\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 47e4e53..c28fde1 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1321,6 +1321,13 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n> \t\t\t\thashclr(sha1);\n> \t\t\t\tflag |= REF_ISBROKEN;\n> \t\t\t}\n> +\n> +\t\t\tif (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {\n> +\t\t\t\t/* NULL_SHA1 is never a valid reference value. */\n> +\t\t\t\thashclr(sha1);\n\nDo you need to clear after checking with is_null_sha1()?\n\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> diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\n> index 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"},{"id":"262778","messageId":"556E1BBE.7000708@alum.mit.edu","threadId":"39498","inReplyTo":"CAGZ79kZG95HB-siR_jMORPuqcL2J_YEwg0VPHE=8XN7gWD4JkA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T21:10:22Z","receivedAt":"2015-06-02T21:10:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/02/2015 07:28 PM, Stefan Beller wrote:\n> On Tue, Jun 2, 2015 at 8:57 AM, Michael Haggerty wrote:\n>> NULL_SHA1 is never a valid value for a reference. If a loose reference\n>> has that value, mark it as broken.\n>>\n>> Why check NULL_SHA1 and not the nearly 2^160 other SHA-1s that are\n>> also invalid in a given repository? Because (a) it is cheap to test\n>> for NULL_SHA1, and (b) NULL_SHA1 is often used as a \"SHA-1 is invalid\"\n> \n> I don't quite agree with the reasoning here. Just because it's cheap doesn't\n> mean it's right. ;) But I fully agree with (b) so this still makes sense.\n\nIts cheapness improves the cost/benefit ratio of adding this check.\n\n>> value inside of Git client source code (not only ours!), and\n>> accidentally writing it to a loose reference file would be an easy\n>> mistake to make.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> refs.c | 7 +++++++\n>> t/t6301-for-each-ref-errors.sh | 2 +-\n>> 2 files changed, 8 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 47e4e53..c28fde1 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1321,6 +1321,13 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n>> hashclr(sha1);\n>> flag |= REF_ISBROKEN;\n>> }\n>> +\n>> + if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {\n> \n> Why do we do the extra check for !(flag & REF_ISBROKEN) here?\n\nThat was an attempt to avoid calling is_null_sha1() unnecessarily. I\nthink I can make this go away and make the code clearer in general by\nrestructuring the logic a little bit. I will do that in the next round.\n\n>> + /* NULL_SHA1 is never a valid reference value. ...\n> \n> ... *by our definition*, because we believe it helps detecting\n> errors/mistakes in the future.\n\nIt's not even by our definition. It is just astronomically more likely\nthat NULL_SHA1 got set there due to an error than that it is the SHA-1\nof legitimate content. In fact it is so unlikely that we use NULL_SHA1\nthroughout our code to indicate \"invalid SHA-1\", ignoring the\ntheoretical possibility that it could appear some day as a real SHA-1.\n\nI'll try to explain this point better in the comment.\n\n> */\n>> + hashclr(sha1);\n> \n> While this code looks consistent to the rest around, at closer inspection\n> this feels a bit redundant to me. If is_null_sha1(sha1) is true, then\n> hashclr(sha1); doesn't change the state. Or did I miss a subtle side effect?\n\nYou're right, there is no reason to call hashclr() here.\n\n>> + flag |= REF_ISBROKEN;\n>> + }\n>> +\n>> [...]\n\nThanks for your review!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"262852","messageId":"20150603090903.GE32000@peff.net","threadId":"39498","inReplyTo":"556E1BBE.7000708@alum.mit.edu","subject":"Re: [PATCH v2 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-03T09:09:03Z","receivedAt":"2015-06-03T09:09:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 02, 2015 at 11:10:22PM +0200, Michael Haggerty wrote:\n\n> >> +\n> >> + if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {\n> > \n> > Why do we do the extra check for !(flag & REF_ISBROKEN) here?\n> \n> That was an attempt to avoid calling is_null_sha1() unnecessarily. I\n> think I can make this go away and make the code clearer in general by\n> restructuring the logic a little bit. I will do that in the next round.\n\nIf you get rid of the useless hashclr(), then this just becomes:\n\n if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1))\n\tflag |= REF_ISBROKEN;\n\nThe reason for the initial check seems pretty obvious then (but it would\nalso be OK without it; is_null_sha1 is not that expensive).\n\n-Peff\n"}]}