{"thread":{"id":"39484","subject":"[PATCH 1/3] t6301: new tests of for-each-ref error handling","startedAt":"2015-06-01T15:53:48Z","lastAt":"2015-06-02T15:46:41Z","messageCount":7,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"262610","messageId":"1433174031-5471-1-git-send-email-mhagger@alum.mit.edu","threadId":"39484","inReplyTo":null,"subject":"[PATCH 0/3] Fix how for-each-ref handles broken loose references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-01T15:53:48Z","receivedAt":"2015-06-01T15:53:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, \"git for-each-ref\" fails to handle REF_ISBROKEN references\ncorrectly, treating them instead as valid references that point at\nNULL_SHA1. Sometimes this makes for-each-ref emit the wrong error\nmessage; sometimes it even appears to complete successfully.\n\nInstead, emit warnings for broken references but otherwise ignore\nthem. Also, add special-case code to defend against an actual\nNULL_SHA1 in a loose reference file being treated as valid. (We've\nseen that problem on our servers before, probably caused by the Ruby\nGit client that we used to use.)\n\nThis patch series was prepared against \"maint\" as it might be suitable\nfor that branch, but it also applies cleanly against \"master\".\n\nThis patch series is also available from my GitHub account [1], as\nbranch for-each-ref-errors.\n\n[1] 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 | 45 ++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 57 insertions(+)\n create mode 100755 t/t6301-for-each-ref-errors.sh\n\n-- \n2.1.4\n"},{"id":"262609","messageId":"1433174031-5471-2-git-send-email-mhagger@alum.mit.edu","threadId":"39484","inReplyTo":"1433174031-5471-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/3] t6301: new tests of for-each-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-01T15:53:49Z","receivedAt":"2015-06-01T15:53:49Z","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\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 | 45 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 45 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..dc68947\n--- /dev/null\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='for-each-ref errors for broken refs'\n+\n+. ./test-lib.sh\n+\n+ZEROS=0000000000000000000000000000000000000000\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+'\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+'\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+'\n+\n+test_done\n-- \n2.1.4\n"},{"id":"262611","messageId":"1433174031-5471-3-git-send-email-mhagger@alum.mit.edu","threadId":"39484","inReplyTo":"1433174031-5471-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/3] for-each-ref: report broken references correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-01T15:53:50Z","receivedAt":"2015-06-01T15:53:50Z","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 dc68947..b9af9a9 100755\n--- a/t/t6301-for-each-ref-errors.sh\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -13,7 +13,7 @@ test_expect_success setup '\n \tgit for-each-ref >full-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":"262612","messageId":"1433174031-5471-4-git-send-email-mhagger@alum.mit.edu","threadId":"39484","inReplyTo":"1433174031-5471-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-01T15:53:51Z","receivedAt":"2015-06-01T15:53:51Z","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 <mhagger@alum.mit.edu>\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 b9af9a9..f737cce 100755\n--- a/t/t6301-for-each-ref-errors.sh\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -23,7 +23,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":"262613","messageId":"20150601160834.GA15148@peff.net","threadId":"39484","inReplyTo":"1433174031-5471-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T16:08:34Z","receivedAt":"2015-06-01T16:08:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:\n\n> Add tests that for-each-ref correctly reports broken loose reference\n> files and references that point at missing objects. In fact, two of\n> these tests fail, because (1) NULL_SHA1 is not recognized as an\n> invalid reference value, and (2) for-each-ref doesn't respect\n> REF_ISBROKEN. Fixes to come.\n\nThis whole series looks straightforward and correct to me. Thanks for a\npleasant read. I have two minor comments on the tests:\n\n> --- /dev/null\n> +++ b/t/t6301-for-each-ref-errors.sh\n> @@ -0,0 +1,45 @@\n> +#!/bin/sh\n> +\n> +test_description='for-each-ref errors for broken refs'\n> +\n> +. ./test-lib.sh\n> +\n> +ZEROS=0000000000000000000000000000000000000000\n> +MISSING=abababababababababababababababababababab\n\nThe test suite provides $_z40, so you can skip $ZEROS. I don't think\nit's a big deal, though, and it may be nicer to have it explicitly next\nto $MISSING here.\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> +'\n\nDue to b7dd2d2 (that you mentioned in the message for patch 2), we only\nsometimes notice the missing objects. Is it worth testing that:\n\n  git for-each-ref --format='%(refname)'\n\ndoes _not_ barf here?\n\n-Peff\n"},{"id":"262624","messageId":"xmqqa8wj72qz.fsf@gitster.dls.corp.google.com","threadId":"39484","inReplyTo":"20150601160834.GA15148@peff.net","subject":"Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T17:33:08Z","receivedAt":"2015-06-01T17:33:08Z","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 Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:\n>\n>> Add tests that for-each-ref correctly reports broken loose reference\n>> files and references that point at missing objects. In fact, two of\n>> these tests fail, because (1) NULL_SHA1 is not recognized as an\n>> invalid reference value, and (2) for-each-ref doesn't respect\n>> REF_ISBROKEN. Fixes to come.\n>\n> This whole series looks straightforward and correct to me. Thanks for a\n> pleasant read. I have two minor comments on the tests:\n>\n>> --- /dev/null\n>> +++ b/t/t6301-for-each-ref-errors.sh\n>> @@ -0,0 +1,45 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='for-each-ref errors for broken refs'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +ZEROS=0000000000000000000000000000000000000000\n>> +MISSING=abababababababababababababababababababab\n>\n> The test suite provides $_z40, so you can skip $ZEROS. I don't think\n> it's a big deal, though, and it may be nicer to have it explicitly next\n> to $MISSING here.\n\nYeah, ZEROS=$_z40 next to MISSING=abababa... may not be too bad.\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>> +'\n>\n> Due to b7dd2d2 (that you mentioned in the message for patch 2), we only\n> sometimes notice the missing objects. Is it worth testing that:\n>\n>   git for-each-ref --format='%(refname)'\n>\n> does _not_ barf here?\n\nSound like a good thing to check, too.\n\nThanks.\n"},{"id":"262736","messageId":"556DCFE1.8070303@alum.mit.edu","threadId":"39484","inReplyTo":"20150601160834.GA15148@peff.net","subject":"Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-02T15:46:41Z","receivedAt":"2015-06-02T15:46:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/01/2015 06:08 PM, Jeff King wrote:\n> On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:\n> \n>> Add tests that for-each-ref correctly reports broken loose reference\n>> files and references that point at missing objects. In fact, two of\n>> these tests fail, because (1) NULL_SHA1 is not recognized as an\n>> invalid reference value, and (2) for-each-ref doesn't respect\n>> REF_ISBROKEN. Fixes to come.\n> \n> This whole series looks straightforward and correct to me. Thanks for a\n> pleasant read. I have two minor comments on the tests:\n> \n>> --- /dev/null\n>> +++ b/t/t6301-for-each-ref-errors.sh\n>> @@ -0,0 +1,45 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='for-each-ref errors for broken refs'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +ZEROS=0000000000000000000000000000000000000000\n>> +MISSING=abababababababababababababababababababab\n> \n> The test suite provides $_z40, so you can skip $ZEROS. I don't think\n> it's a big deal, though, and it may be nicer to have it explicitly next\n> to $MISSING here.\n\nDang, I knew about that variable but just forgot it. I'll make this change.\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>> +'\n> \n> Due to b7dd2d2 (that you mentioned in the message for patch 2), we only\n> sometimes notice the missing objects. Is it worth testing that:\n> \n>   git for-each-ref --format='%(refname)'\n> \n> does _not_ barf here?\n\nIt makes sense. I will add it, with --format='%(objectname) %(refname)'\nfor added fun.\n\nThanks for the review!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}