{"thread":{"id":"37431","subject":"git fsck exit code?","startedAt":"2014-08-27T22:10:12Z","lastAt":"2014-09-15T14:57:33Z","messageCount":21,"participants":["David Turner","Jeff King","Junio C Hamano","Øyvind A. Holm","Jonathan Nieder","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"248451","messageId":"1409177412.15185.3.camel@leckie","threadId":"37431","inReplyTo":null,"subject":"git fsck exit code?","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-08-27T22:10:12Z","receivedAt":"2014-08-27T22:10:12Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"It looks like git fsck exits with 0 status even if there are some\nerrors. The only case where there's a non-zero exit code is if\nverify_pack reports errors -- but not e.g. fsck_object_dir.\n\nIs that really the intended behavior?  I think it would be nice to at\nleast support --exit-code (but probably a sensible exit code ought to be\nthe default).\n"},{"id":"248574","messageId":"20140829185325.GC29456@peff.net","threadId":"37431","inReplyTo":"1409177412.15185.3.camel@leckie","subject":"Re: git fsck exit code?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T18:53:25Z","receivedAt":"2014-08-29T18:53:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:\n\n> It looks like git fsck exits with 0 status even if there are some\n> errors. The only case where there's a non-zero exit code is if\n> verify_pack reports errors -- but not e.g. fsck_object_dir.\n\nIt will also bail non-zero with _certain_ tree errors that cause git to\ndie() rather than fscking more completely.\n\n> Is that really the intended behavior?  I think it would be nice to at\n> least support --exit-code (but probably a sensible exit code ought to be\n> the default).\n\nI don't know that the current behavior is really intended. I agree that\nI would expect a non-zero exit code if there are errors. Fsck also has\nsome warnings. I do not know what those should produce (I'd think a zero\nexit normally, and some option like -Werror to turn them into errors).\n\n-Peff\n"},{"id":"248580","messageId":"xmqqha0v5cgn.fsf@gitster.dls.corp.google.com","threadId":"37431","inReplyTo":"20140829185325.GC29456@peff.net","subject":"Re: git fsck exit code?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-29T19:21:12Z","receivedAt":"2014-08-29T19:21:12Z","isPatch":false,"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, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:\n>\n>> It looks like git fsck exits with 0 status even if there are some\n>> errors. The only case where there's a non-zero exit code is if\n>> verify_pack reports errors -- but not e.g. fsck_object_dir.\n>\n> It will also bail non-zero with _certain_ tree errors that cause git to\n> die() rather than fscking more completely.\n\nEven if git does not die, whenever it says broken link, missing\nobject, or object corrupt, we set errors_found and that variable\naffects the exit status of fsck.  What does \"some errors\" exactly\nmean in the original report?  Dangling objects are *not* errors and\nshould not cause fsck to report an error with its exit status.\n"},{"id":"248583","messageId":"1409343480.19256.2.camel@leckie","threadId":"37431","inReplyTo":"xmqqha0v5cgn.fsf@gitster.dls.corp.google.com","subject":"Re: git fsck exit code?","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-08-29T20:18:00Z","receivedAt":"2014-08-29T20:18:00Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2014-08-29 at 12:21 -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:\n> >\n> >> It looks like git fsck exits with 0 status even if there are some\n> >> errors. The only case where there's a non-zero exit code is if\n> >> verify_pack reports errors -- but not e.g. fsck_object_dir.\n> >\n> > It will also bail non-zero with _certain_ tree errors that cause git to\n> > die() rather than fscking more completely.\n> \n> Even if git does not die, whenever it says broken link, missing\n> object, or object corrupt, we set errors_found and that variable\n> affects the exit status of fsck.  What does \"some errors\" exactly\n> mean in the original report?  Dangling objects are *not* errors and\n> should not cause fsck to report an error with its exit status.\n\nerror in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\nduplicate file entries\n\n(at least -- there might be more, but that's the one that bit me)\n"},{"id":"248585","messageId":"20140829203145.GA510@peff.net","threadId":"37431","inReplyTo":"1409343480.19256.2.camel@leckie","subject":"Re: git fsck exit code?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T20:31:46Z","receivedAt":"2014-08-29T20:31:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 29, 2014 at 04:18:00PM -0400, David Turner wrote:\n\n> > Even if git does not die, whenever it says broken link, missing\n> > object, or object corrupt, we set errors_found and that variable\n> > affects the exit status of fsck.  What does \"some errors\" exactly\n> > mean in the original report?  Dangling objects are *not* errors and\n> > should not cause fsck to report an error with its exit status.\n> \n> error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\n> duplicate file entries\n> \n> (at least -- there might be more, but that's the one that bit me)\n\nI think that we just don't set \"errors_found\" in fsck_obj (nor do we in\nfsck_obj_buffer, but in that case its caller is verify-pack, which\npropagates the return code). Maybe (completely untested):\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d42a27d..29de901 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -388,7 +388,8 @@ static void fsck_sha1_list(void)\n \t\tunsigned char *sha1 = entry->sha1;\n \n \t\tsha1_list.entry[i] = NULL;\n-\t\tfsck_sha1(sha1);\n+\t\tif (fsck_sha1(sha1))\n+\t\t\terrors_found |= ERROR_OBJECT;\n \t\tfree(entry);\n \t}\n \tsha1_list.nr = 0;\n"},{"id":"248586","messageId":"xmqqd2bj58gx.fsf@gitster.dls.corp.google.com","threadId":"37431","inReplyTo":"20140829203145.GA510@peff.net","subject":"Re: git fsck exit code?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-29T20:47:26Z","receivedAt":"2014-08-29T20:47:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Aug 29, 2014 at 04:18:00PM -0400, David Turner wrote:\n>\n>> > Even if git does not die, whenever it says broken link, missing\n>> > object, or object corrupt, we set errors_found and that variable\n>> > affects the exit status of fsck.  What does \"some errors\" exactly\n>> > mean in the original report?  Dangling objects are *not* errors and\n>> > should not cause fsck to report an error with its exit status.\n>> \n>> error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\n>> duplicate file entries\n>> \n>> (at least -- there might be more, but that's the one that bit me)\n>\n> I think that we just don't set \"errors_found\" in fsck_obj (nor do we in\n> fsck_obj_buffer, but in that case its caller is verify-pack, which\n> propagates the return code). Maybe (completely untested):\n\nSounds about right.  David may have more or there may be not.  Let's\nnot forget to collect them and roll the fixes into a single update.\n\nThanks.\n\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index d42a27d..29de901 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -388,7 +388,8 @@ static void fsck_sha1_list(void)\n>  \t\tunsigned char *sha1 = entry->sha1;\n>  \n>  \t\tsha1_list.entry[i] = NULL;\n> -\t\tfsck_sha1(sha1);\n> +\t\tif (fsck_sha1(sha1))\n> +\t\t\terrors_found |= ERROR_OBJECT;\n>  \t\tfree(entry);\n>  \t}\n>  \tsha1_list.nr = 0;\n"},{"id":"248689","messageId":"CAA787rmf7aNJ+ErXk6Lc_hLVDxMV8s2Lx_YmZud83yia4n0VKA@mail.gmail.com","threadId":"37431","inReplyTo":"1409343480.19256.2.camel@leckie","subject":"Re: git fsck exit code?","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2014-08-31T18:54:05Z","receivedAt":"2014-08-31T18:54:05Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On 29 August 2014 22:18, David Turner <dturner@twopensource.com> wrote:\n> On Fri, 2014-08-29 at 12:21 -0700, Junio C Hamano wrote:\n> > Jeff King <peff@peff.net> writes:\n> > > On Wed, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:\n> > > > It looks like git fsck exits with 0 status even if there are\n> > > > some errors. The only case where there's a non-zero exit code is\n> > > > if verify_pack reports errors -- but not e.g. fsck_object_dir.\n> > >\n> > > It will also bail non-zero with _certain_ tree errors that cause\n> > > git to die() rather than fscking more completely.\n> >\n> > Even if git does not die, whenever it says broken link, missing\n> > object, or object corrupt, we set errors_found and that variable\n> > affects the exit status of fsck.  What does \"some errors\" exactly\n> > mean in the original report?  Dangling objects are *not* errors and\n> > should not cause fsck to report an error with its exit status.\n>\n> error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\n> duplicate file entries\n\nI don't think git fsck should return !0 in this case. Yes, it's an\ninconsistency in the repo, but it's sometimes due to erroneous\nconversions from another SCM or some other (non-standard) implementation\nof the git client. I've seen things like this (and other inconsistencies\nin repos, like wrong date formats, non-standard Author fields, unsorted\ntrees, zero-padded file modes and so on), and the thing is, owners of\npublic repos with these errors tend to avoid fixing it because it\nchanges the commit SHAs. If git fsck starts to return !0 on these\nerrors, it will always return error on that repo, which in practise\nmeans that the error code is rendered useless. IMHO git fsck should only\nreturn !0 on errors that can be fixed without changing the commit\nhistory, for example missing or invalid objects.\n\nØyvind\n"},{"id":"248698","messageId":"CAA787rmiiiWwz-n-PgP46_y6L2nhTZZgBtr2H2ZcCA7VA21KYg@mail.gmail.com","threadId":"37431","inReplyTo":"CAA787rmf7aNJ+ErXk6Lc_hLVDxMV8s2Lx_YmZud83yia4n0VKA@mail.gmail.com","subject":"Re: git fsck exit code?","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2014-09-01T11:54:40Z","receivedAt":"2014-09-01T11:54:40Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On 31 August 2014 20:54, Øyvind A. Holm <sunny@sunbase.org> wrote:\n> On 29 August 2014 22:18, David Turner wrote:\n> > error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\n> > duplicate file entries\n>\n> I don't think git fsck should return !0 in this case. Yes, it's an\n> inconsistency in the repo, but it's sometimes due to erroneous\n> conversions from another SCM or some other (non-standard)\n> implementation of the git client. I've seen things like this (and\n> other inconsistencies in repos, like wrong date formats, non-standard\n> Author fields, unsorted trees, zero-padded file modes and so on), and\n> the thing is, owners of public repos with these errors tend to avoid\n> fixing it because it changes the commit SHAs. If git fsck starts to\n> return !0 on these errors, it will always return error on that repo,\n> which in practise means that the error code is rendered useless. IMHO\n> git fsck should only return !0 on errors that can be fixed without\n> changing the commit history, for example missing or invalid objects.\n\nTo elaborate on what I wrote: Of course, if the error is grave enough\nthat Git isn't able to work around it and it severely limits the\nfunctionality of the repo, action needs to be taken regardless of\nwhether the history has to be changed or not. In that case it's OK to\nreturn an error value too.\n\nØyvind\n"},{"id":"248703","messageId":"1409595463.3057.3.camel@leckie","threadId":"37431","inReplyTo":"CAA787rmf7aNJ+ErXk6Lc_hLVDxMV8s2Lx_YmZud83yia4n0VKA@mail.gmail.com","subject":"Re: git fsck exit code?","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-09-01T18:17:43Z","receivedAt":"2014-09-01T18:17:43Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sun, 2014-08-31 at 20:54 +0200, Øyvind A. Holm wrote:\n> On 29 August 2014 22:18, David Turner <dturner@twopensource.com> wrote:\n> > On Fri, 2014-08-29 at 12:21 -0700, Junio C Hamano wrote:\n> > > Jeff King <peff@peff.net> writes:\n> > > > On Wed, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:\n> > > > > It looks like git fsck exits with 0 status even if there are\n> > > > > some errors. The only case where there's a non-zero exit code is\n> > > > > if verify_pack reports errors -- but not e.g. fsck_object_dir.\n> > > >\n> > > > It will also bail non-zero with _certain_ tree errors that cause\n> > > > git to die() rather than fscking more completely.\n> > >\n> > > Even if git does not die, whenever it says broken link, missing\n> > > object, or object corrupt, we set errors_found and that variable\n> > > affects the exit status of fsck.  What does \"some errors\" exactly\n> > > mean in the original report?  Dangling objects are *not* errors and\n> > > should not cause fsck to report an error with its exit status.\n> >\n> > error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains\n> > duplicate file entries\n> \n> I don't think git fsck should return !0 in this case. Yes, it's an\n> inconsistency in the repo, but it's sometimes due to erroneous\n> conversions from another SCM or some other (non-standard) implementation\n> of the git client. I've seen things like this (and other inconsistencies\n> in repos, like wrong date formats, non-standard Author fields, unsorted\n> trees, zero-padded file modes and so on), and the thing is, owners of\n> public repos with these errors tend to avoid fixing it because it\n> changes the commit SHAs. If git fsck starts to return !0 on these\n> errors, it will always return error on that repo, which in practise\n> means that the error code is rendered useless. IMHO git fsck should only\n> return !0 on errors that can be fixed without changing the commit\n> history, for example missing or invalid objects.\n\nWe could have one exit code for errors which can be fixed without\nrewriting history, and another for errors that can't.  Or different\ncommand-line arguments to suppress errors of this sort.\n\nIn my case, I actually could fix the issue, because it was in a\nnewly-created branch; I just rewrote the script that created the branch\nto be a little smarter.\n"},{"id":"249096","messageId":"xmqq4mwgjvt6.fsf_-_@gitster.dls.corp.google.com","threadId":"37431","inReplyTo":"20140829203145.GA510@peff.net","subject":"[PATCH] fsck: exit with non-zero status upon error from fsck_obj()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-09T22:03:33Z","receivedAt":"2014-09-09T22:03:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\nDate: Fri, 29 Aug 2014 16:31:46 -0400\n\nUpon finding a corrupt loose object, we forgot to note the error to\nsignal it with the exit status of the entire process.\n\n[jc: adjusted t1450 and added another test]\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I think your fix is a right one that catches all the \"we can\n   parse minimally for the purpose of 'struct object' class system,\n   but the object is semantically broken\" cases, as fsck_obj() is\n   where such a validation should all happen.\n\n   I can haz a sign off?  Thanks.\n\n builtin/fsck.c  |  3 ++-\n t/t1450-fsck.sh | 30 +++++++++++++++++++++++++-----\n 2 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1affdd5..8abe644 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -389,7 +389,8 @@ static void fsck_sha1_list(void)\n \t\tunsigned char *sha1 = entry->sha1;\n \n \t\tsha1_list.entry[i] = NULL;\n-\t\tfsck_sha1(sha1);\n+\t\tif (fsck_sha1(sha1))\n+\t\t\terrors_found |= ERROR_OBJECT;\n \t\tfree(entry);\n \t}\n \tsha1_list.nr = 0;\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8c739c9..c8dff9c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -101,7 +101,7 @@ test_expect_success 'email with embedded > is not okay' '\n \ttest_when_finished \"remove_object $new\" &&\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n-\tgit fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"error in commit $new\" out\n '\n@@ -113,7 +113,7 @@ test_expect_success 'missing < email delimiter is reported nicely' '\n \ttest_when_finished \"remove_object $new\" &&\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n-\tgit fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"error in commit $new.* - bad name\" out\n '\n@@ -125,7 +125,7 @@ test_expect_success 'missing email is reported nicely' '\n \ttest_when_finished \"remove_object $new\" &&\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n-\tgit fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"error in commit $new.* - missing email\" out\n '\n@@ -137,7 +137,7 @@ test_expect_success '> in name is reported' '\n \ttest_when_finished \"remove_object $new\" &&\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n-\tgit fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"error in commit $new\" out\n '\n@@ -151,11 +151,31 @@ test_expect_success 'integer overflow in timestamps is reported' '\n \ttest_when_finished \"remove_object $new\" &&\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n-\tgit fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"error in commit $new.*integer overflow\" out\n '\n \n+test_expect_success 'malformatted tree object' '\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\ttest_when_finished \"remove_object \\$T\" &&\n+\tT=$(\n+\t\tGIT_INDEX_FILE=test-index &&\n+\t\texport GIT_INDEX_FILE &&\n+\t\trm -f test-index &&\n+\t\t>x &&\n+\t\tgit add x &&\n+\t\tT=$(git write-tree) &&\n+\t\t(\n+\t\t\tgit cat-file tree $T &&\n+\t\t\tgit cat-file tree $T\n+\t\t) |\n+\t\tgit hash-object -w -t tree --stdin\n+\t) &&\n+\ttest_must_fail git fsck 2>out &&\n+\tgrep \"error in tree .*contains duplicate file entries\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\n-- \n2.1.0-449-g93bbe5b\n"},{"id":"249097","messageId":"20140909220709.GA14029@peff.net","threadId":"37431","inReplyTo":"xmqq4mwgjvt6.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] fsck: exit with non-zero status upon error from fsck_obj()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-09T22:07:09Z","receivedAt":"2014-09-09T22:07:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 09, 2014 at 03:03:33PM -0700, Junio C Hamano wrote:\n\n> From: Jeff King <peff@peff.net>\n> Date: Fri, 29 Aug 2014 16:31:46 -0400\n> \n> Upon finding a corrupt loose object, we forgot to note the error to\n> signal it with the exit status of the entire process.\n> \n> [jc: adjusted t1450 and added another test]\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>  * I think your fix is a right one that catches all the \"we can\n>    parse minimally for the purpose of 'struct object' class system,\n>    but the object is semantically broken\" cases, as fsck_obj() is\n>    where such a validation should all happen.\n> \n>    I can haz a sign off?  Thanks.\n\nThanks, I think this is a step forward regardless of other conversation\non the exit code, as it is just harmonizing loose and packed object\nbehavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"249098","messageId":"20140909220933.GB14029@peff.net","threadId":"37431","inReplyTo":"1409595463.3057.3.camel@leckie","subject":"Re: git fsck exit code?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-09T22:09:34Z","receivedAt":"2014-09-09T22:09:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 01, 2014 at 02:17:43PM -0400, David Turner wrote:\n\n> > I don't think git fsck should return !0 in this case. Yes, it's an\n> > inconsistency in the repo, but it's sometimes due to erroneous\n> > conversions from another SCM or some other (non-standard) implementation\n> > of the git client. I've seen things like this (and other inconsistencies\n> > in repos, like wrong date formats, non-standard Author fields, unsorted\n> > trees, zero-padded file modes and so on), and the thing is, owners of\n> > public repos with these errors tend to avoid fixing it because it\n> > changes the commit SHAs. If git fsck starts to return !0 on these\n> > errors, it will always return error on that repo, which in practise\n> > means that the error code is rendered useless. IMHO git fsck should only\n> > return !0 on errors that can be fixed without changing the commit\n> > history, for example missing or invalid objects.\n> \n> We could have one exit code for errors which can be fixed without\n> rewriting history, and another for errors that can't.  Or different\n> command-line arguments to suppress errors of this sort.\n> \n> In my case, I actually could fix the issue, because it was in a\n> newly-created branch; I just rewrote the script that created the branch\n> to be a little smarter.\n\nI think there's obviously some disagreement or room for interpretation\non the exit code. Perhaps a better path forward is to have a\nmachine-readable output from fsck in the first place, and then we can\nannotate each warning/error with extra information that a caller can\nuse.\n\nAs it is now, you have to scrape fsck's stderr stream to figure out what\nhappened (which is a thing I have done, and it felt dirty and wrong).\n\n-Peff\n"},{"id":"249101","messageId":"xmqqvbowigeh.fsf@gitster.dls.corp.google.com","threadId":"37431","inReplyTo":"xmqq4mwgjvt6.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] fsck: exit with non-zero status upon error from fsck_obj()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-09T22:21:42Z","receivedAt":"2014-09-09T22:21:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> From: Jeff King <peff@peff.net>\n> Date: Fri, 29 Aug 2014 16:31:46 -0400\n>\n> Upon finding a corrupt loose object, we forgot to note the error to\n> signal it with the exit status of the entire process.\n>\n> [jc: adjusted t1450 and added another test]\n\nSpoke too soon.  If found another instance where we expected fsck to\nnotice a corruption.  We'd need to squash this in.\n\nBy the way, Jonathan, with dbedf8bf (t1450 (fsck): remove dangling\nobjects, 2010-09-06) you added a 'test_might_fail git fsck' to the\n1450 test that catches an object corruption.  Do you remember if\nthere was some flakiness in this test that necessitated it, or is it\nmerely \"I think this should fail, but it does not, and we may fix it\nsome day but I am not doing that in this patch?\"  Assuming that it\nis the latter, I am including an update to 1450 as well.\n\nThere is another test_might_fail that runs \"git rev-list\" in the\nsame test, but that is not part of the said patch and unrelated to\nthe current topic, so I didn't dig further; we may want to audit the\nhits from \"git grep test_might_fail t/\" as a separate topic (hint,\nhint).\n\n\n t/t1450-fsck.sh        | 2 +-\n t/t4212-log-corrupt.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex c8dff9c..0de755c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -69,7 +69,7 @@ test_expect_success 'object with bad sha1' '\n \tgit update-ref refs/heads/bogus $cmt &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \n-\ttest_might_fail git fsck 2>out &&\n+\ttest_must_fail git fsck 2>out &&\n \tcat out &&\n \tgrep \"$sha.*corrupt\" out\n '\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 58b792b..67bd8ec 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -14,7 +14,7 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'fsck notices broken commit' '\n-\tgit fsck 2>actual &&\n+\ttest_must_fail git fsck 2>actual &&\n \ttest_i18ngrep invalid.author actual\n '\n \n"},{"id":"249102","messageId":"20140909222936.GA701@google.com","threadId":"37431","inReplyTo":"xmqqvbowigeh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fsck: exit with non-zero status upon error from fsck_obj()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-09-09T22:29:36Z","receivedAt":"2014-09-09T22:29:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> By the way, Jonathan, with dbedf8bf (t1450 (fsck): remove dangling\n> objects, 2010-09-06) you added a 'test_might_fail git fsck' to the\n> 1450 test that catches an object corruption.  Do you remember if\n> there was some flakiness in this test that necessitated it, or is it\n> merely \"I think this should fail, but it does not, and we may fix it\n> some day but I am not doing that in this patch?\"\n\nThomas is the person to ask. :)  See v1.6.3-rc0~176^2~3 (Test fsck a\nbit harder, 2009-02-19):\n\n> +\t(git fsck 2>out; true) &&\n\nwhich that cleanup tightened to test_might_fail.\n\nBut yes, I'm pretty sure it was for futureproofing, not for hiding\nflakiness.  I think your patch does the right thing in changing it to\ntest_must_fail now that fsck exits nonzero.\n\nThanks,\nJonathan\n"},{"id":"249277","messageId":"20140912033830.GA5507@peff.net","threadId":"37431","inReplyTo":"20140909220709.GA14029@peff.net","subject":"[PATCH] fsck: return non-zero status on missing ref tips","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-12T03:38:30Z","receivedAt":"2014-09-12T03:38:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"> On Tue, Sep 09, 2014 at 03:03:33PM -0700, Junio C Hamano wrote:\n> \n> > Upon finding a corrupt loose object, we forgot to note the error to\n> > signal it with the exit status of the entire process.\n> > \n> > [jc: adjusted t1450 and added another test]\n> > \n> > Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> > ---\n> > \n> >  * I think your fix is a right one that catches all the \"we can\n> >    parse minimally for the purpose of 'struct object' class system,\n> >    but the object is semantically broken\" cases, as fsck_obj() is\n> >    where such a validation should all happen.\n\nHere's another structural case that we should catch but do not:\n\n-- >8 --\nSubject: fsck: return non-zero status on missing ref tips\n\nFsck tries hard to detect missing objects, and will complain\n(and exit non-zero) about any inter-object links that are\nmissing. However, it will not exit non-zero for any missing\nref tips, meaning that a severely broken repository may\nstill pass \"git fsck && echo ok\".\n\nThe problem is that we use for_each_ref to iterate over the\nref tips, which hides broken tips. It does at least print an\nerror from the refs.c code, but fsck does not ever see the\nref and cannot note the problem in its exit code. We can solve\nthis by using for_each_rawref and noting the error ourselves.\n\nIn addition to adding tests for this case, we add tests for\nall types of missing-object links (all of which worked, but\nwhich we were not testing).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust below here we check that refs/heads/* points only to commit\nobjects. That's also sort-of-structural, but is pretty easy to recover\nfrom without data loss, so I don't think it is as obvious a candidate\nfor a non-zero exit.\n\n builtin/fsck.c  |  3 ++-\n t/t1450-fsck.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 29de901..0928a98 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -489,6 +489,7 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f\n \tobj = parse_object(sha1);\n \tif (!obj) {\n \t\terror(\"%s: invalid sha1 pointer %s\", refname, sha1_to_hex(sha1));\n+\t\terrors_found |= ERROR_REACHABLE;\n \t\t/* We'll continue with the rest despite the error.. */\n \t\treturn 0;\n \t}\n@@ -505,7 +506,7 @@ static void get_default_heads(void)\n {\n \tif (head_points_at && !is_null_sha1(head_sha1))\n \t\tfsck_handle_ref(\"HEAD\", head_sha1, 0, NULL);\n-\tfor_each_ref(fsck_handle_ref, NULL);\n+\tfor_each_rawref(fsck_handle_ref, NULL);\n \tif (include_reflogs)\n \t\tfor_each_reflog(fsck_handle_reflog, NULL);\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 0de755c..b52397a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -302,4 +302,60 @@ test_expect_success 'fsck notices \".git\" in trees' '\n \t)\n '\n \n+# create a static test repo which is broken by omitting\n+# one particular object ($1, which is looked up via rev-parse\n+# in the new repository).\n+create_repo_missing () {\n+\trm -rf missing &&\n+\tgit init missing &&\n+\t(\n+\t\tcd missing &&\n+\t\tgit commit -m one --allow-empty &&\n+\t\tmkdir subdir &&\n+\t\techo content >subdir/file &&\n+\t\tgit add subdir/file &&\n+\t\tgit commit -m two &&\n+\t\tunrelated=$(echo unrelated | git hash-object --stdin -w) &&\n+\t\tgit tag -m foo tag $unrelated &&\n+\t\tsha1=$(git rev-parse --verify \"$1\") &&\n+\t\tpath=$(echo $sha1 | sed 's|..|&/|') &&\n+\t\trm .git/objects/$path\n+\t)\n+}\n+\n+test_expect_success 'fsck notices missing blob' '\n+\tcreate_repo_missing HEAD:subdir/file &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices missing subtree' '\n+\tcreate_repo_missing HEAD:subdir &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices missing root tree' '\n+\tcreate_repo_missing HEAD^{tree} &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices missing parent' '\n+\tcreate_repo_missing HEAD^ &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices missing tagged object' '\n+\tcreate_repo_missing tag^{blob} &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices ref pointing to missing commit' '\n+\tcreate_repo_missing HEAD &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n+test_expect_success 'fsck notices ref pointing to missing tag' '\n+\tcreate_repo_missing tag &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n test_done\n-- \n2.1.0.373.g91ca799\n"},{"id":"249278","messageId":"20140912042939.GA5968@peff.net","threadId":"37431","inReplyTo":"20140912033830.GA5507@peff.net","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-12T04:29:39Z","receivedAt":"2014-09-12T04:29:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc mhagger for packed-refs wisdom]\n\nOn Thu, Sep 11, 2014 at 11:38:30PM -0400, Jeff King wrote:\n\n> Fsck tries hard to detect missing objects, and will complain\n> (and exit non-zero) about any inter-object links that are\n> missing. However, it will not exit non-zero for any missing\n> ref tips, meaning that a severely broken repository may\n> still pass \"git fsck && echo ok\".\n> \n> The problem is that we use for_each_ref to iterate over the\n> ref tips, which hides broken tips. It does at least print an\n> error from the refs.c code, but fsck does not ever see the\n> ref and cannot note the problem in its exit code. We can solve\n> this by using for_each_rawref and noting the error ourselves.\n\nThere's a possibly related problem with packed-refs that I noticed while\nlooking at this.\n\nWhen we call pack-refs, it will refuse to pack any broken loose refs,\nand leave them loose. Which is sane. But when we delete a ref, we need\nto rewrite the packed-refs file, and we omit any broken packed refs. We\nwouldn't have written a broken entry, but we may get broken later (i.e.,\nthe tip object may go missing after the packed-refs file is written).\n\nIf we only have a packed copy of \"refs/heads/master\" and it is broken,\nthen deleting any _other_ unrelated ref will cause refs/heads/master to\nbe dropped from the packed-refs file entirely. We get an error message,\nbut that's easy to miss, and the pointer to master's sha1 is lost\nforever.\n\nThis test (on top of the patch I just sent) demonstrates:\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex b52397a..b0f4545 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -317,6 +317,7 @@ create_repo_missing () {\n \t\tgit commit -m two &&\n \t\tunrelated=$(echo unrelated | git hash-object --stdin -w) &&\n \t\tgit tag -m foo tag $unrelated &&\n+\t\tgit pack-refs --all --prune &&\n \t\tsha1=$(git rev-parse --verify \"$1\") &&\n \t\tpath=$(echo $sha1 | sed 's|..|&/|') &&\n \t\trm .git/objects/$path\n@@ -358,4 +359,10 @@ test_expect_success 'fsck notices ref pointing to missing tag' '\n \ttest_must_fail git -C missing fsck\n '\n \n+test_expect_success 'ref deletion does not eat broken refs' '\n+\tcreate_repo_missing HEAD &&\n+\tgit -C missing update-ref -d refs/tags/tag &&\n+\ttest_must_fail git -C missing fsck\n+'\n+\n test_done\n\n\nThe fsck will succeed even though \"master\" should be broken, because\nwe appear to have no refs at all.\n\nThe fault is in curate_packed_ref_fn, which drops crufty from\npacked-refs as we repack. That in turn is representing an older:\n\n         if (!ref_resolves_to_object(entry))\n                 return 0; /* Skip broken refs */\n\nwhich comes from 624cac3 (refs: change the internal reference-iteration\nAPI, 2013-04-22). But that was just maintaining the existing behavior,\nwhich was using a do_for_each_ref iteration without INCLUDE_BROKEN. So I\nthink this problem has a similar root, though the fix is now slightly\ndifferent.\n\nI am tempted to say that we do not need to do curate_each_ref_fn at all.\nAny entry with a broken sha1 is either:\n\n  1. A truly broken ref, in which case we should make sure to keep it\n     (i.e., it is not cruft at all).\n\n  2. A crufty entry that has been replaced by a loose reference that has\n     not yet been packed. Such a crufty entry may point to broken\n     objects, and that is OK.\n\nIn case 2, we _could_ delete the cruft. But I do not think we need to.\nThe loose ref will take precedence to anybody who actually does a ref\nlookup, so the cruft is not hurting anybody.\n\nDropping curate_packed_ref_fn (as below) fixes the test above. And\nmiraculously does not even seem to conflict with ref patches in pu. :)\n\nAm I missing any case that it is actually helping?\n\ndiff --git a/refs.c b/refs.c\nindex a7853cc..83c2bf7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2484,70 +2484,11 @@ int pack_refs(unsigned int flags)\n }\n \n /*\n- * If entry is no longer needed in packed-refs, add it to the string\n- * list pointed to by cb_data.  Reasons for deleting entries:\n- *\n- * - Entry is broken.\n- * - Entry is overridden by a loose ref.\n- * - Entry does not point at a valid object.\n- *\n- * In the first and third cases, also emit an error message because these\n- * are indications of repository corruption.\n- */\n-static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n-{\n-\tstruct string_list *refs_to_delete = cb_data;\n-\n-\tif (entry->flag & REF_ISBROKEN) {\n-\t\t/* This shouldn't happen to packed refs. */\n-\t\terror(\"%s is broken!\", entry->name);\n-\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\treturn 0;\n-\t}\n-\tif (!has_sha1_file(entry->u.value.sha1)) {\n-\t\tunsigned char sha1[20];\n-\t\tint flags;\n-\n-\t\tif (read_ref_full(entry->name, sha1, 0, &flags))\n-\t\t\t/* We should at least have found the packed ref. */\n-\t\t\tdie(\"Internal error\");\n-\t\tif ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {\n-\t\t\t/*\n-\t\t\t * This packed reference is overridden by a\n-\t\t\t * loose reference, so it is OK that its value\n-\t\t\t * is no longer valid; for example, it might\n-\t\t\t * refer to an object that has been garbage\n-\t\t\t * collected.  For this purpose we don't even\n-\t\t\t * care whether the loose reference itself is\n-\t\t\t * invalid, broken, symbolic, etc.  Silently\n-\t\t\t * remove the packed reference.\n-\t\t\t */\n-\t\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\t\treturn 0;\n-\t\t}\n-\t\t/*\n-\t\t * There is no overriding loose reference, so the fact\n-\t\t * that this reference doesn't refer to a valid object\n-\t\t * indicates some kind of repository corruption.\n-\t\t * Report the problem, then omit the reference from\n-\t\t * the output.\n-\t\t */\n-\t\terror(\"%s does not point to a valid object!\", entry->name);\n-\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\treturn 0;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-/*\n  * Must be called with packed refs already locked (and sorted)\n  */\n static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n-\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *ref_to_delete;\n \tint i, ret;\n \n \t/* Look for a packed ref */\n@@ -2561,15 +2502,6 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tfor (i = 0; i < n; i++)\n \t\tremove_entry(packed, refnames[i]);\n \n-\t/* Remove any other accumulated cruft */\n-\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n-\tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n-\t\tif (remove_entry(packed, ref_to_delete->string) == -1) {\n-\t\t\trollback_packed_refs();\n-\t\t\tdie(\"internal error\");\n-\t\t}\n-\t}\n-\n \t/* Write what remains */\n \tret = commit_packed_refs();\n \tif (ret && err)\n"},{"id":"249279","messageId":"20140912043844.GA5241@peff.net","threadId":"37431","inReplyTo":"20140912042939.GA5968@peff.net","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-12T04:38:44Z","receivedAt":"2014-09-12T04:38:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 12, 2014 at 12:29:39AM -0400, Jeff King wrote:\n\n> Dropping curate_packed_ref_fn (as below) fixes the test above. And\n> miraculously does not even seem to conflict with ref patches in pu. :)\n\nOf course I spoke too soon. The patch I sent is actually based on pu. It\nis easy to make the equivalent change in either \"master\" or \"pu\" (they\nare both just deletions of the same blocks), but the code mutated a\nlittle in between, and there are purely textual conflicts going from one\nto the other.\n\n-Peff\n"},{"id":"249281","messageId":"CAPc5daXMMpqtH=DwLLXgHXVfHThN5MfHwn6dPK6OaZvAQGXT_Q@mail.gmail.com","threadId":"37431","inReplyTo":"20140912042939.GA5968@peff.net","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-12T04:58:45Z","receivedAt":"2014-09-12T04:58:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Sep 11, 2014 at 9:29 PM, Jeff King <peff@peff.net> wrote:\n> [+cc mhagger for packed-refs wisdom]\n>\n> If we only have a packed copy of \"refs/heads/master\" and it is broken,\n> then deleting any _other_ unrelated ref will cause refs/heads/master to\n> be dropped from the packed-refs file entirely. We get an error message,\n> but that's easy to miss, and the pointer to master's sha1 is lost\n> forever.\n\nHmph, and the significance of losing a random 20-byte object name that\nis useless in your repository is? You could of course ask around other\nrepositories (i.e. your origin, others that fork from the same origin,\netc.), and having the name might make it easier to locate the exact\nobject.\n\nBut in such a case, either they have it at the tip (in which case you\ncan just fetch the branch you lost), or they have it reachable from\none of their tips of branches you had shown interest in (that is why\nyou had that lost object in the first place). Either way, you would be\nrunning \"git fetch\" or asking them to send \"git bundle\" output to be\nunbundled at your end, and the way you ask would be by refname, not\nthe object name, so I am not sure if the loss is that grave.\n\nPerhaps I am missing something, of course, though.\n"},{"id":"249282","messageId":"20140912051202.GA6097@peff.net","threadId":"37431","inReplyTo":"CAPc5daXMMpqtH=DwLLXgHXVfHThN5MfHwn6dPK6OaZvAQGXT_Q@mail.gmail.com","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-12T05:12:03Z","receivedAt":"2014-09-12T05:12:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 11, 2014 at 09:58:45PM -0700, Junio C Hamano wrote:\n\n> On Thu, Sep 11, 2014 at 9:29 PM, Jeff King <peff@peff.net> wrote:\n> > [+cc mhagger for packed-refs wisdom]\n> >\n> > If we only have a packed copy of \"refs/heads/master\" and it is broken,\n> > then deleting any _other_ unrelated ref will cause refs/heads/master to\n> > be dropped from the packed-refs file entirely. We get an error message,\n> > but that's easy to miss, and the pointer to master's sha1 is lost\n> > forever.\n> \n> Hmph, and the significance of losing a random 20-byte object name that\n> is useless in your repository is? You could of course ask around other\n> repositories (i.e. your origin, others that fork from the same origin,\n> etc.), and having the name might make it easier to locate the exact\n> object.\n\nBecause your repository is corrupted and broken, and we then forget that\nfact. I.e., it is not a random 20-byte object name. It is the thing that\nyour branch is pointing at, and that is valuable in itself. If you can\nrestore the object (e.g., from another repository), you need to know\nwhich object to restore.\n\nBut I also think corrupting a repository and not noticing is quite bad\nin itself.\n\n> But in such a case, either they have it at the tip (in which case you\n> can just fetch the branch you lost), or they have it reachable from\n> one of their tips of branches you had shown interest in (that is why\n> you had that lost object in the first place). Either way, you would be\n> running \"git fetch\" or asking them to send \"git bundle\" output to be\n> unbundled at your end, and the way you ask would be by refname, not\n> the object name, so I am not sure if the loss is that grave.\n\nYes, but after you get the objects from the other person, what do you\nset your ref to? If I know my tip was at commit X, I can get any set of\nobjects from another untrusted person that includes X, verify what they\nsent me cryptographically, and restore my tip to X.\n\nIf I do not know X, they can send me any random set of objects. I can\nverify that the sha1s are OK and the graph is complete, but I cannot\nverify that the contents are sane. I am effectively just picking their\n\"master\" as my new starting point, and trusting that it has not been\ntampered with.\n\n-Peff\n"},{"id":"249430","messageId":"5416FAD2.5020002@alum.mit.edu","threadId":"37431","inReplyTo":"CAPc5daXMMpqtH=DwLLXgHXVfHThN5MfHwn6dPK6OaZvAQGXT_Q@mail.gmail.com","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-15T14:42:26Z","receivedAt":"2014-09-15T14:42:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2014 06:58 AM, Junio C Hamano wrote:\n> On Thu, Sep 11, 2014 at 9:29 PM, Jeff King <peff@peff.net> wrote:\n>> [+cc mhagger for packed-refs wisdom]\n>>\n>> If we only have a packed copy of \"refs/heads/master\" and it is broken,\n>> then deleting any _other_ unrelated ref will cause refs/heads/master to\n>> be dropped from the packed-refs file entirely. We get an error message,\n>> but that's easy to miss, and the pointer to master's sha1 is lost\n>> forever.\n> \n> Hmph, and the significance of losing a random 20-byte object name that\n> is useless in your repository is? You could of course ask around other\n> repositories (i.e. your origin, others that fork from the same origin,\n> etc.), and having the name might make it easier to locate the exact\n> object.\n> \n> But in such a case, either they have it at the tip (in which case you\n> can just fetch the branch you lost), or they have it reachable from\n> one of their tips of branches you had shown interest in (that is why\n> you had that lost object in the first place). Either way, you would be\n> running \"git fetch\" or asking them to send \"git bundle\" output to be\n> unbundled at your end, and the way you ask would be by refname, not\n> the object name, so I am not sure if the loss is that grave.\n> \n> Perhaps I am missing something, of course, though.\n\nI don't understand your argument.\n\nFirst, you would not just lose the SHA-1 of the object. You would also\nlose the name of the reference that was previously pointing at it.\n\nSecond, the discarded information *is* useful. The more information you\nhave, the more likely you can restore it and/or diagnose the original\ncause of the corruption.\n\nThird, even if the discarded information were not useful, the fact that\n*information has gone missing* is of overwhelming importance, and that\nfact would be forgotten as soon as the warning message scrolls off of\nyour terminal. The reference deletion that triggered the warning might\neven have been done in the background by some other process (e.g., a\nGUI) and the output discarded or shunted into some \"debug\" window that\nthe user would have no reason to look at.\n\nSo I agree with Peff that it would be prudent to preserve the corrupt\nreference at least until the next \"git fsck\", which (a) is run by the\nuser specifically to look for corruption, and (b) can return an error\nresult to make the failure obvious.\n\nThe only thing that is unclear to me is whether the user would be able\nto get rid of the broken reference once it is discovered (short of\nopening packed-refs in an editor).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249431","messageId":"5416FE5D.5050102@alum.mit.edu","threadId":"37431","inReplyTo":"20140912042939.GA5968@peff.net","subject":"Re: [PATCH] fsck: return non-zero status on missing ref tips","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-15T14:57:33Z","receivedAt":"2014-09-15T14:57:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/12/2014 06:29 AM, Jeff King wrote:\n> [+cc mhagger for packed-refs wisdom]\n> \n> On Thu, Sep 11, 2014 at 11:38:30PM -0400, Jeff King wrote:\n> \n>> Fsck tries hard to detect missing objects, and will complain\n>> (and exit non-zero) about any inter-object links that are\n>> missing. However, it will not exit non-zero for any missing\n>> ref tips, meaning that a severely broken repository may\n>> still pass \"git fsck && echo ok\".\n>>\n>> The problem is that we use for_each_ref to iterate over the\n>> ref tips, which hides broken tips. It does at least print an\n>> error from the refs.c code, but fsck does not ever see the\n>> ref and cannot note the problem in its exit code. We can solve\n>> this by using for_each_rawref and noting the error ourselves.\n> \n> There's a possibly related problem with packed-refs that I noticed while\n> looking at this.\n> \n> When we call pack-refs, it will refuse to pack any broken loose refs,\n> and leave them loose. Which is sane. But when we delete a ref, we need\n> to rewrite the packed-refs file, and we omit any broken packed refs. We\n> wouldn't have written a broken entry, but we may get broken later (i.e.,\n> the tip object may go missing after the packed-refs file is written).\n> \n> If we only have a packed copy of \"refs/heads/master\" and it is broken,\n> then deleting any _other_ unrelated ref will cause refs/heads/master to\n> be dropped from the packed-refs file entirely. We get an error message,\n> but that's easy to miss, and the pointer to master's sha1 is lost\n> forever.\n\nI was confused for a while by your observation, because the curate\nfunction has\n\n\tif (read_ref_full(entry->name, sha1, 0, &flags))\n\t\t/* We should at least have found the packed ref. */\n\t\tdie(\"Internal error\");\n\n, which looks like more than \"emit an error message and continue\". But\nin fact the flow never gets this far, because iterating without\nDO_FOR_EACH_INCLUDE_BROKEN doesn't just skip references for which\nREF_ISBROKEN is set, but also (do to a test in do_one_ref()) references\nfor which ref_resolves_to_object() fails. The ultimate source of my\nconfusion is that the word BROKEN has two different meanings in the two\nconstants' names.\n\n> [...]\n> I am tempted to say that we do not need to do curate_each_ref_fn at all.\n> Any entry with a broken sha1 is either:\n> \n>   1. A truly broken ref, in which case we should make sure to keep it\n>      (i.e., it is not cruft at all).\n> \n>   2. A crufty entry that has been replaced by a loose reference that has\n>      not yet been packed. Such a crufty entry may point to broken\n>      objects, and that is OK.\n> \n> In case 2, we _could_ delete the cruft. But I do not think we need to.\n> The loose ref will take precedence to anybody who actually does a ref\n> lookup, so the cruft is not hurting anybody.\n> \n> Dropping curate_packed_ref_fn (as below) fixes the test above. And\n> miraculously does not even seem to conflict with ref patches in pu. :)\n> \n> Am I missing any case that it is actually helping?\n\nSomething inside me screams out in horror that we would pass up an\nopportunity to delete unneeded cruft from the packed-refs file. But I\ncan't think of a rational reason to disagree with you, so as far as I'm\nconcerned your suggestion seems OK.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}