{"thread":{"id":"39560","subject":"[PATCH 0/2] fsck: don't ignore broken reflog entries","startedAt":"2015-06-08T13:40:03Z","lastAt":"2015-06-08T17:08:09Z","messageCount":11,"participants":["Michael Haggerty","Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"263207","messageId":"cover.1433769878.git.mhagger@alum.mit.edu","threadId":"39560","inReplyTo":null,"subject":"[PATCH 0/2] fsck: don't ignore broken reflog entries","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T13:40:03Z","receivedAt":"2015-06-08T13:40:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, if a reflog entry's old or new SHA-1 was not resolvable to\nan object, that SHA-1 was silently ignored. Instead, report such cases\nas errors.\n\nThis patch series is also available from my GitHub account [1], branch\n\"fsck-reflog-entries\".\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (2):\n  fsck_handle_reflog_sha1(): new function\n  fsck: report errors if reflog entries point at invalid objects\n\n builtin/fsck.c | 34 ++++++++++++++++++++--------------\n 1 file changed, 20 insertions(+), 14 deletions(-)\n\n-- \n2.1.4\n"},{"id":"263209","messageId":"1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu","threadId":"39560","inReplyTo":"cover.1433769878.git.mhagger@alum.mit.edu","subject":"[PATCH 1/2] fsck_handle_reflog_sha1(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T13:40:04Z","receivedAt":"2015-06-08T13:40:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"New function, extracted from fsck_handle_reflog_ent(). The extra\nis_null_sha1() test for the new reference is currently unnecessary, as\nreflogs are deleted when the reference itself is deleted. But it\ndoesn't hurt, either.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fsck.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 4e8e2ee..b1b6c60 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -451,28 +451,29 @@ static void fsck_dir(int i, char *path)\n \n static int default_refs;\n \n-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+static void fsck_handle_reflog_sha1(unsigned char *sha1)\n {\n \tstruct object *obj;\n \n-\tif (verbose)\n-\t\tfprintf(stderr, \"Checking reflog %s->%s\\n\",\n-\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1));\n-\n-\tif (!is_null_sha1(osha1)) {\n-\t\tobj = lookup_object(osha1);\n+\tif (!is_null_sha1(sha1)) {\n+\t\tobj = lookup_object(sha1);\n \t\tif (obj) {\n \t\t\tobj->used = 1;\n \t\t\tmark_object_reachable(obj);\n \t\t}\n \t}\n-\tobj = lookup_object(nsha1);\n-\tif (obj) {\n-\t\tobj->used = 1;\n-\t\tmark_object_reachable(obj);\n-\t}\n+}\n+\n+static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n+{\n+\tif (verbose)\n+\t\tfprintf(stderr, \"Checking reflog %s->%s\\n\",\n+\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1));\n+\n+\tfsck_handle_reflog_sha1(osha1);\n+\tfsck_handle_reflog_sha1(nsha1);\n \treturn 0;\n }\n \n-- \n2.1.4\n"},{"id":"263208","messageId":"2ba9ee48a062b049c8b64fed4caba32c42f03031.1433769878.git.mhagger@alum.mit.edu","threadId":"39560","inReplyTo":"cover.1433769878.git.mhagger@alum.mit.edu","subject":"[PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T13:40:05Z","receivedAt":"2015-06-08T13:40:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, if a reflog entry's old or new SHA-1 was not resolvable to\nan object, that SHA-1 was silently ignored. Instead, report such cases\nas errors.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fsck.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b1b6c60..2679793 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -451,7 +451,7 @@ static void fsck_dir(int i, char *path)\n \n static int default_refs;\n \n-static void fsck_handle_reflog_sha1(unsigned char *sha1)\n+static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1)\n {\n \tstruct object *obj;\n \n@@ -460,6 +460,9 @@ static void fsck_handle_reflog_sha1(unsigned char *sha1)\n \t\tif (obj) {\n \t\t\tobj->used = 1;\n \t\t\tmark_object_reachable(obj);\n+\t\t} else {\n+\t\t\terror(\"%s: invalid reflog entry %s\", refname, sha1_to_hex(sha1));\n+\t\t\terrors_found |= ERROR_REACHABLE;\n \t\t}\n \t}\n }\n@@ -468,19 +471,21 @@ static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n+\tconst char *refname = cb_data;\n+\n \tif (verbose)\n \t\tfprintf(stderr, \"Checking reflog %s->%s\\n\",\n \t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1));\n \n-\tfsck_handle_reflog_sha1(osha1);\n-\tfsck_handle_reflog_sha1(nsha1);\n+\tfsck_handle_reflog_sha1(refname, osha1);\n+\tfsck_handle_reflog_sha1(refname, nsha1);\n \treturn 0;\n }\n \n static int fsck_handle_reflog(const char *logname, const struct object_id *oid,\n \t\t\t      int flag, void *cb_data)\n {\n-\tfor_each_reflog_ent(logname, fsck_handle_reflog_ent, NULL);\n+\tfor_each_reflog_ent(logname, fsck_handle_reflog_ent, (void *)logname);\n \treturn 0;\n }\n \n-- \n2.1.4\n"},{"id":"263211","messageId":"b3e391d36b36b67aca1fa5db4350f219@www.dscho.org","threadId":"39560","inReplyTo":"1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-08T14:18:53Z","receivedAt":"2015-06-08T14:18:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn 2015-06-08 08:40, Michael Haggerty wrote:\n> New function, extracted from fsck_handle_reflog_ent(). The extra\n> is_null_sha1() test for the new reference is currently unnecessary, as\n> reflogs are deleted when the reference itself is deleted. But it\n> doesn't hurt, either.\n\nThis patch is probably easier to read with the `--patience` flag (at least I find the patch obviously good in that form):\n\n-- snipsnap --\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 4e8e2ee..b1b6c60 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -451,28 +451,29 @@ static void fsck_dir(int i, char *path)\n \n static int default_refs;\n \n+static void fsck_handle_reflog_sha1(unsigned char *sha1)\n+{\n+\tstruct object *obj;\n+\n+\tif (!is_null_sha1(sha1)) {\n+\t\tobj = lookup_object(sha1);\n+\t\tif (obj) {\n+\t\t\tobj->used = 1;\n+\t\t\tmark_object_reachable(obj);\n+\t\t}\n+\t}\n+}\n+\n static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n-\tstruct object *obj;\n-\n \tif (verbose)\n \t\tfprintf(stderr, \"Checking reflog %s->%s\\n\",\n \t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1));\n \n-\tif (!is_null_sha1(osha1)) {\n-\t\tobj = lookup_object(osha1);\n-\t\tif (obj) {\n-\t\t\tobj->used = 1;\n-\t\t\tmark_object_reachable(obj);\n-\t\t}\n-\t}\n-\tobj = lookup_object(nsha1);\n-\tif (obj) {\n-\t\tobj->used = 1;\n-\t\tmark_object_reachable(obj);\n-\t}\n+\tfsck_handle_reflog_sha1(osha1);\n+\tfsck_handle_reflog_sha1(nsha1);\n \treturn 0;\n }\n \n-- \n1.8.5.2.msysgit.0.4.gd08ed18\n"},{"id":"263212","messageId":"e9d2f8cc371aa41e5d9095abd3cb7edb@www.dscho.org","threadId":"39560","inReplyTo":"2ba9ee48a062b049c8b64fed4caba32c42f03031.1433769878.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-08T14:27:09Z","receivedAt":"2015-06-08T14:27:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn 2015-06-08 08:40, Michael Haggerty wrote:\n> Previously, if a reflog entry's old or new SHA-1 was not resolvable to\n> an object, that SHA-1 was silently ignored. Instead, report such cases\n> as errors.\n\nI like the idea, but I am a bit uncertain whether it would constitute \"too backwards-incompatible\" a change to make this an error. I think it could be argued both ways: it *is* an improvement, but it could also possibly disrupt scripts that work pretty nicely at the moment.\n\nMy fsck-api branch will help with this, of course, as users whose scripts break could be (temporarily) demote the error to a warning. I planned to work on it this week and would be happy to rebase it onto this here patch series.\n\nCiao,\nDscho\n"},{"id":"263219","messageId":"xmqqwpzeck72.fsf@gitster.dls.corp.google.com","threadId":"39560","inReplyTo":"1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-08T15:07:45Z","receivedAt":"2015-06-08T15:07:45Z","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> New function, extracted from fsck_handle_reflog_ent(). The extra\n> is_null_sha1() test for the new reference is currently unnecessary, as\n> reflogs are deleted when the reference itself is deleted. But it\n> doesn't hurt, either.\n\nI think we would crash with today's code in such a situation, but\nwouldn't we want to diagnose a 0{40} object name on the \"new\" side\nof the reflog entry as an error in the endgame state?\n\nI do share uneasiness with Dscho that this is tightening what used\nto be an OK state into an error, but I haven't thought about how\nserious it would be.\n\nThanks.\n"},{"id":"263221","messageId":"5575B03C.6040008@alum.mit.edu","threadId":"39560","inReplyTo":"e9d2f8cc371aa41e5d9095abd3cb7edb@www.dscho.org","subject":"Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T15:09:48Z","receivedAt":"2015-06-08T15:09:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/08/2015 04:27 PM, Johannes Schindelin wrote:\n> On 2015-06-08 08:40, Michael Haggerty wrote:\n>> Previously, if a reflog entry's old or new SHA-1 was not resolvable\n>> to an object, that SHA-1 was silently ignored. Instead, report such\n>> cases as errors.\n> \n> I like the idea, but I am a bit uncertain whether it would constitute\n> \"too backwards-incompatible\" a change to make this an error. I think\n> it could be argued both ways: it *is* an improvement, but it could\n> also possibly disrupt scripts that work pretty nicely at the moment.\n\nWhat kind of script are you worried about? One that mucks around inside\nthe object database / reflog files? If people do that, all bets are off,\nno? Plus,\n\n* This change only causes fsck to output an extra line (and exit with\n  a a non-zero retcode).\n\n* Repair is only a\n\n      git reflog expire --expire-unreachable=now --all\n\n  away, I think.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"263222","messageId":"5575B1F7.8030400@alum.mit.edu","threadId":"39560","inReplyTo":"xmqqwpzeck72.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-08T15:17:11Z","receivedAt":"2015-06-08T15:17:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/08/2015 05:07 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> New function, extracted from fsck_handle_reflog_ent(). The extra\n>> is_null_sha1() test for the new reference is currently unnecessary, as\n>> reflogs are deleted when the reference itself is deleted. But it\n>> doesn't hurt, either.\n> \n> I think we would crash with today's code in such a situation, but\n> wouldn't we want to diagnose a 0{40} object name on the \"new\" side\n> of the reflog entry as an error in the endgame state?\n\nGood point. new_sha1 == NULL_SHA1 should be diagnosed and reported with\na distinct error message.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"263236","messageId":"c5720357601be135485ef546cae7ffdb@www.dscho.org","threadId":"39560","inReplyTo":"5575B03C.6040008@alum.mit.edu","subject":"Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-08T16:00:09Z","receivedAt":"2015-06-08T16:00:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn 2015-06-08 17:09, Michael Haggerty wrote:\n> On 06/08/2015 04:27 PM, Johannes Schindelin wrote:\n>> On 2015-06-08 08:40, Michael Haggerty wrote:\n>>> Previously, if a reflog entry's old or new SHA-1 was not resolvable\n>>> to an object, that SHA-1 was silently ignored. Instead, report such\n>>> cases as errors.\n>>\n>> I like the idea, but I am a bit uncertain whether it would constitute\n>> \"too backwards-incompatible\" a change to make this an error. I think\n>> it could be argued both ways: it *is* an improvement, but it could\n>> also possibly disrupt scripts that work pretty nicely at the moment.\n> \n> What kind of script are you worried about?\n\nI was concerned about scripts that work on repositories whose reflogs become inconsistent for whatever reason (that happened a lot to me in the past, IIRC it had something to do with bare repositories and/or shared object databases).\n\nNow, if I was to run a script in, say, cron to verify that all of my repositories (possibly on a network drive, for shared team use), I could imagine that I actually want to error out if the reflogs become inconsistent. But then, I could also imagine that I care more about the script being quiet when everything is okay except for the reflogs. \n\n> * This change only causes fsck to output an extra line (and exit with\n>   a a non-zero retcode).\n\nIt is that non-zero exit status that would make my hypothetical cron script start to fail.\n\n> * Repair is only a\n> \n>       git reflog expire --expire-unreachable=now --all\n> \n>   away, I think.\n\nTrue.\n\nPlus, as I mentioned, it could be considered a bug fix that fsck now reports this problem.\n\nThe more I think about it, the more I think it is actually a bug fix.\n\nThanks,\nDscho\n"},{"id":"263254","messageId":"20150608165643.GA6863@peff.net","threadId":"39560","inReplyTo":"c5720357601be135485ef546cae7ffdb@www.dscho.org","subject":"Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-08T16:56:44Z","receivedAt":"2015-06-08T16:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 08, 2015 at 06:00:09PM +0200, Johannes Schindelin wrote:\n\n> >> I like the idea, but I am a bit uncertain whether it would constitute\n> >> \"too backwards-incompatible\" a change to make this an error. I think\n> >> it could be argued both ways: it *is* an improvement, but it could\n> >> also possibly disrupt scripts that work pretty nicely at the moment.\n> > \n> > What kind of script are you worried about?\n> \n> I was concerned about scripts that work on repositories whose reflogs\n> become inconsistent for whatever reason (that happened a lot to me in\n> the past, IIRC it had something to do with bare repositories and/or\n> shared object databases).\n\nI think these repositories are already broken. You cannot run `git gc`\nin such a repository, as it will barf when trying to walk the reflog\ntips during `git repack`.\n\nWe run into this exact situation at GitHub because of our shared object\ndatabases. Our per-fork repack code basically has to do:\n\n  if ! git repack ...; then\n    git reflog expire --expire-unreachable=all --all &&\n    git repack ... ||\n    die \"ok, it really is broken\"\n  fi\n\n-Peff\n"},{"id":"263259","messageId":"9adde72489de0b1ad66bd2c4730b18e9@www.dscho.org","threadId":"39560","inReplyTo":"20150608165643.GA6863@peff.net","subject":"Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-08T17:08:09Z","receivedAt":"2015-06-08T17:08:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-06-08 18:56, Jeff King wrote:\n> On Mon, Jun 08, 2015 at 06:00:09PM +0200, Johannes Schindelin wrote:\n> \n>> >> I like the idea, but I am a bit uncertain whether it would constitute\n>> >> \"too backwards-incompatible\" a change to make this an error. I think\n>> >> it could be argued both ways: it *is* an improvement, but it could\n>> >> also possibly disrupt scripts that work pretty nicely at the moment.\n>> >\n>> > What kind of script are you worried about?\n>>\n>> I was concerned about scripts that work on repositories whose reflogs\n>> become inconsistent for whatever reason (that happened a lot to me in\n>> the past, IIRC it had something to do with bare repositories and/or\n>> shared object databases).\n> \n> I think these repositories are already broken. You cannot run `git gc`\n> in such a repository, as it will barf when trying to walk the reflog\n> tips during `git repack`.\n> \n> We run into this exact situation at GitHub because of our shared object\n> databases. Our per-fork repack code basically has to do:\n> \n>   if ! git repack ...; then\n>     git reflog expire --expire-unreachable=all --all &&\n>     git repack ... ||\n>     die \"ok, it really is broken\"\n>   fi\n\nGood point. So if I needed any more convincing that Michael's patch is a bug fix (as opposed to a backwards-incompatible change), this did it.\n\nCiao,\nDscho\n"}]}