{"thread":{"id":"44833","subject":"\"git fsck\" not detecting garbage at the end of blob object files...","startedAt":"2017-01-07T12:50:28Z","lastAt":"2018-11-01T03:37:38Z","messageCount":39,"participants":["John Szakmeister","Dennis Kaarsemaker","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Torsten Bögershausen","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308928","messageId":"CAEBDL5Uc39JagdmXUxfxh1TPSK3H5wxoTfjK-pfLRYjciBnHpA@mail.gmail.com","threadId":"44833","inReplyTo":null,"subject":"\"git fsck\" not detecting garbage at the end of blob object files...","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2017-01-07T12:50:19Z","receivedAt":"2017-01-07T12:50:28Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"I was perusing StackOverflow this morning and ran across this\nquestion: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/\n\nIt was a simple question about why \"checking objects\" was not\nappearing, but in it was another issue. The user purposefully\ncorrupted a blob object file to see if `git fsck` would catch it by\ntacking extra data on at the end. `git fsck` happily said everything\nwas okay, but when I played with things locally I found out that `git\ngc` does not like that extra garbage. I'm not sure what the trade-off\nneeds to be here, but my expectation is that if `git fsck` says\neverything is okay, then all operations using that object (file)\nshould work too.\n\nIs that unreasonable? What would be the impact of fixing this issue?\n\n-John\n"},{"id":"308935","messageId":"1483825623.31837.9.camel@kaarsemaker.net","threadId":"44833","inReplyTo":"CAEBDL5Uc39JagdmXUxfxh1TPSK3H5wxoTfjK-pfLRYjciBnHpA@mail.gmail.com","subject":"Re: \"git fsck\" not detecting garbage at the end of blob object files...","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-01-07T21:47:03Z","receivedAt":"2017-01-07T21:47:12Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:\n> I was perusing StackOverflow this morning and ran across this\n> question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/\n> \n> It was a simple question about why \"checking objects\" was not\n> appearing, but in it was another issue. The user purposefully\n> corrupted a blob object file to see if `git fsck` would catch it by\n> tacking extra data on at the end. `git fsck` happily said everything\n> was okay, but when I played with things locally I found out that `git\n> gc` does not like that extra garbage. I'm not sure what the trade-off\n> needs to be here, but my expectation is that if `git fsck` says\n> everything is okay, then all operations using that object (file)\n> should work too.\n> \n> Is that unreasonable? What would be the impact of fixing this issue?\n\nIf you do this with a commit object or tree object, fsck does complain.\nI think it's sensible to do so for blob objects as well.\n\nEditing blob object:\n\nhurricane:/tmp/moo (master)$ hexer .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485 \nhurricane:/tmp/moo (master)$ git status\nOn branch master\nnothing to commit, working tree clean\nhurricane:/tmp/moo (master)$ git fsck\nChecking object directories: 100% (256/256), done.\nhurricane:/tmp/moo (master)$ git gc\nCounting objects: 3, done.\nerror: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'\nfatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt\nerror: failed to run repack\n\nEditing tree object:\n\nhurricane:/tmp/moo (master)$ hexer .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20\nhurricane:/tmp/moo (master +)$ git status\nerror: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'\nfatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt\nerror: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'\nfatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt\nhurricane:/tmp/moo (master +)$ git fsck\nerror: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'\nfatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt\nerror: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'\nfatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt\n\nEditing commit object:\n\nhurricane:/tmp/moo (master)$ echo test >> .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0 \nhurricane:/tmp/moo (master +)$ git status\nerror: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'\nfatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt\nerror: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'\nfatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt\n!(128) hurricane:/tmp/moo (master +)$ git fsck\nerror: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'\nfatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt\nerror: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'\nfatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt\n\nD.\n"},{"id":"308950","messageId":"20170108052619.4ucjamsqad4g5add@sigill.intra.peff.net","threadId":"44833","inReplyTo":"1483825623.31837.9.camel@kaarsemaker.net","subject":"Re: \"git fsck\" not detecting garbage at the end of blob object files...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-08T05:26:20Z","receivedAt":"2017-01-08T05:32:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote:\n\n> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:\n> > I was perusing StackOverflow this morning and ran across this\n> > question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/\n> > \n> > It was a simple question about why \"checking objects\" was not\n> > appearing, but in it was another issue. The user purposefully\n> > corrupted a blob object file to see if `git fsck` would catch it by\n> > tacking extra data on at the end. `git fsck` happily said everything\n> > was okay, but when I played with things locally I found out that `git\n> > gc` does not like that extra garbage. I'm not sure what the trade-off\n> > needs to be here, but my expectation is that if `git fsck` says\n> > everything is okay, then all operations using that object (file)\n> > should work too.\n> > \n> > Is that unreasonable? What would be the impact of fixing this issue?\n> \n> If you do this with a commit object or tree object, fsck does complain.\n> I think it's sensible to do so for blob objects as well.\n\nThe existing extra-garbage check is in unpack_sha1_rest(), which is\ncalled as part of read_sha1_file(). And that's what we hit for commits\nand trees. However, we check the sha1 of blobs using the streaming\ninterface (in case they're large). I think you'd want to put a similar\ncheck into read_istream_loose(). But note if you are grepping for it, it\nis hidden behind a macro; look for read_method_decl(loose).\n\nI'm actually not sure if this should be downgrade to a warning. It's\ntrue that it's a form of corruption, but it doesn't actually prohibit us\nfrom getting the data we need to complete the operation. Arguably fsck\nshould be more picky, but it is just relying on the same parse_object()\ncode path that the rest of git uses.\n\nI doubt anybody cares too much either way, though. It's not like this is\na common thing.\n\nI did notice another interesting case when looking at this. Fsck ends up\nin fsck_loose(), which has the sha1 and path of the loose object. It\npasses the sha1 to fsck_sha1(), and ignores the path entirely!\n\nSo if you have a duplicate copy of the object in a pack, we'd actually\nfind and check the duplicate. This can happen, e.g., if you had a loose\nobject and fetched a thin-pack which made a copy of the loose object to\ncomplete the pack).\n\nProbably fsck_loose() should be more picky about making sure we are\nreading the data from the loose version we found.\n\n-Peff\n"},{"id":"309346","messageId":"CAEBDL5Vf=rvb4fZF87pNYci4sicmzhS_qPJYHHOGcnPTMBhhWg@mail.gmail.com","threadId":"44833","inReplyTo":"20170108052619.4ucjamsqad4g5add@sigill.intra.peff.net","subject":"Re: \"git fsck\" not detecting garbage at the end of blob object files...","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2017-01-13T09:15:42Z","receivedAt":"2017-01-13T09:15:52Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Sun, Jan 8, 2017 at 12:26 AM, Jeff King wrote:\n> On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote:\n>> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:\n>> > I was perusing StackOverflow this morning and ran across this\n>> > question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/\n>> >\n>> > It was a simple question about why \"checking objects\" was not\n>> > appearing, but in it was another issue. The user purposefully\n>> > corrupted a blob object file to see if `git fsck` would catch it by\n>> > tacking extra data on at the end. `git fsck` happily said everything\n>> > was okay, but when I played with things locally I found out that `git\n>> > gc` does not like that extra garbage. I'm not sure what the trade-off\n>> > needs to be here, but my expectation is that if `git fsck` says\n>> > everything is okay, then all operations using that object (file)\n>> > should work too.\n>> >\n>> > Is that unreasonable? What would be the impact of fixing this issue?\n>>\n>> If you do this with a commit object or tree object, fsck does complain.\n>> I think it's sensible to do so for blob objects as well.\n>\n> The existing extra-garbage check is in unpack_sha1_rest(), which is\n> called as part of read_sha1_file(). And that's what we hit for commits\n> and trees. However, we check the sha1 of blobs using the streaming\n> interface (in case they're large). I think you'd want to put a similar\n> check into read_istream_loose(). But note if you are grepping for it, it\n> is hidden behind a macro; look for read_method_decl(loose).\n\nThat's for the pointer.\n\n> I'm actually not sure if this should be downgrade to a warning. It's\n> true that it's a form of corruption, but it doesn't actually prohibit us\n> from getting the data we need to complete the operation. Arguably fsck\n> should be more picky, but it is just relying on the same parse_object()\n> code path that the rest of git uses.\n>\n> I doubt anybody cares too much either way, though. It's not like this is\n> a common thing.\n\nI kind of wonder about that myself too, and I'm not sure what to\nthink about it. On the one hand, I'd like to know about\n*anything* that has changed in an adverse way--it could indicate\na failure somewhere else that needs to be handled. On the other\nhand, scaring the user isn't all that advantageous. I guess I'm\nin the former camp.\n\nAs to whether this is common, yeah, it's probably not. However,\nI was surprised by the number of results that turned up when I\nsearch for \"garbage at end of loose object\".\n\n> I did notice another interesting case when looking at this. Fsck ends up\n> in fsck_loose(), which has the sha1 and path of the loose object. It\n> passes the sha1 to fsck_sha1(), and ignores the path entirely!\n>\n> So if you have a duplicate copy of the object in a pack, we'd actually\n> find and check the duplicate. This can happen, e.g., if you had a loose\n> object and fetched a thin-pack which made a copy of the loose object to\n> complete the pack).\n>\n> Probably fsck_loose() should be more picky about making sure we are\n> reading the data from the loose version we found.\n\nInteresting find! Thanks for the information Peff!\n\n-John\n"},{"id":"309347","messageId":"CAEBDL5WKsMT7XHdX1gAWW3-WaJo7p7R60uogWZEuBCYGsu+s5Q@mail.gmail.com","threadId":"44833","inReplyTo":"1483825623.31837.9.camel@kaarsemaker.net","subject":"Re: \"git fsck\" not detecting garbage at the end of blob object files...","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2017-01-13T09:16:29Z","receivedAt":"2017-01-13T09:16:37Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Sat, Jan 7, 2017 at 4:47 PM, Dennis Kaarsemaker\n wrote:\n> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:\n>> I was perusing StackOverflow this morning and ran across this\n>> question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/\n>>\n>> It was a simple question about why \"checking objects\" was not\n>> appearing, but in it was another issue. The user purposefully\n>> corrupted a blob object file to see if `git fsck` would catch it by\n>> tacking extra data on at the end. `git fsck` happily said everything\n>> was okay, but when I played with things locally I found out that `git\n>> gc` does not like that extra garbage. I'm not sure what the trade-off\n>> needs to be here, but my expectation is that if `git fsck` says\n>> everything is okay, then all operations using that object (file)\n>> should work too.\n>>\n>> Is that unreasonable? What would be the impact of fixing this issue?\n>\n> If you do this with a commit object or tree object, fsck does complain.\n> I think it's sensible to do so for blob objects as well.\n\nAlso very good information. Thanks Dennis!\n\n-John\n"},{"id":"309367","messageId":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","threadId":"44833","inReplyTo":"CAEBDL5Vf=rvb4fZF87pNYci4sicmzhS_qPJYHHOGcnPTMBhhWg@mail.gmail.com","subject":"[PATCH 0/6] loose-object fsck fixes/tightening","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:52:58Z","receivedAt":"2017-01-13T17:53:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 13, 2017 at 04:15:42AM -0500, John Szakmeister wrote:\n\n> > I did notice another interesting case when looking at this. Fsck ends up\n> > in fsck_loose(), which has the sha1 and path of the loose object. It\n> > passes the sha1 to fsck_sha1(), and ignores the path entirely!\n> >\n> > So if you have a duplicate copy of the object in a pack, we'd actually\n> > find and check the duplicate. This can happen, e.g., if you had a loose\n> > object and fetched a thin-pack which made a copy of the loose object to\n> > complete the pack).\n> >\n> > Probably fsck_loose() should be more picky about making sure we are\n> > reading the data from the loose version we found.\n> \n> Interesting find! Thanks for the information Peff!\n\nSo I figured I would knock this out as a fun morning exercise. But\nsheesh, it turned out to be a slog, because most of the functions rely\non map_sha1_file() to convert the sha1 to an object path at the lowest\nlevel.\n\nBut I finally got something working, so here it is. I found another bug\non the way, along with a few cleanups. And then I did the trailing\ngarbage detection at the end, because by that point I knew right where\nit needed to go. :)\n\n [1/6]: t1450: refactor loose-object removal\n [2/6]: sha1_file: fix error message for alternate objects\n [3/6]: t1450: test fsck of packed objects\n [4/6]: sha1_file: add read_loose_object() function\n [5/6]: fsck: parse loose object paths directly\n [6/6]: fsck: detect trailing garbage in all object types\n\n builtin/fsck.c | 46 +++++++++++----\n cache.h | 13 ++++\n sha1_file.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++-----\n t/t1450-fsck.sh | 86 +++++++++++++++++++++++----\n 4 files changed, 284 insertions(+), 41 deletions(-)\n\n-Peff\n"},{"id":"309368","messageId":"20170113175410.qfgpm3ksqa6mkt6j@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 1/6] t1450: refactor loose-object removal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:54:10Z","receivedAt":"2017-01-13T17:54:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 90cf590f5 (fsck: optionally show more helpful info\nfor broken links, 2016-07-17) added a remove_loose_object()\nhelper, but we already had a remove_object() helper that did\nthe same thing. Let's combine these into one.\n\nThe implementations had a few subtle differences, so I've\ntried to take the best of both:\n\n - the original used \"sed\", but the newer version avoids\n spawning an extra process\n\n - the original processed \"$*\", which was nonsense, as it\n assumed only a single sha1. Use \"$1\" to make that more\n clear.\n\n - the newer version ran an extra rev-parse, but it was not\n necessary; it's sole caller already converted the\n argument into a raw sha1\n\n - the original used \"rm -f\", whereas the new one uses\n \"rm\". The latter is better because it may notice a bug\n or other unexpected failure in the test. (The original\n does check that the object exists before we remove it,\n which is good, but that's a subset of the possible\n unexpected conditions).\n\nSigned-off-by: Jeff King \n---\n t/t1450-fsck.sh | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex ee7d4736d..3297d4cb2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -43,13 +43,13 @@ test_expect_success 'HEAD is part of refs, valid objects appear valid' '\n \n test_expect_success 'setup: helpers for corruption tests' '\n \tsha1_file() {\n-\t\techo \"$*\" | sed \"s#..#.git/objects/&/#\"\n+\t\tremainder=${1#??} &&\n+\t\tfirsttwo=${1%$remainder} &&\n+\t\techo \".git/objects/$firsttwo/$remainder\"\n \t} &&\n \n \tremove_object() {\n-\t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n-\t\trm -f \"$file\"\n+\t\trm \"$(sha1_file \"$1\")\"\n \t}\n '\n \n@@ -535,13 +535,6 @@ test_expect_success 'fsck --connectivity-only' '\n \t)\n '\n \n-remove_loose_object () {\n-\tsha1=\"$(git rev-parse \"$1\")\" &&\n-\tremainder=${sha1#??} &&\n-\tfirsttwo=${sha1%$remainder} &&\n-\trm .git/objects/$firsttwo/$remainder\n-}\n-\n test_expect_success 'fsck --name-objects' '\n \trm -rf name-objects &&\n \tgit init name-objects &&\n@@ -550,7 +543,7 @@ test_expect_success 'fsck --name-objects' '\n \t\ttest_commit julius caesar.t &&\n \t\ttest_commit augustus &&\n \t\ttest_commit caesar &&\n-\t\tremove_loose_object $(git rev-parse julius:caesar.t) &&\n+\t\tremove_object $(git rev-parse julius:caesar.t) &&\n \t\ttest_must_fail git fsck --name-objects >out &&\n \t\ttree=$(git rev-parse --verify julius:) &&\n \t\tgrep \"$tree (\\(refs/heads/master\\|HEAD\\)@{[0-9]*}:\" out\n-- \n2.11.0.629.g10075098c\n\n"},{"id":"309369","messageId":"20170113175439.jedroszyilb6idrd@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 2/6] sha1_file: fix error message for alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:54:39Z","receivedAt":"2017-01-13T17:54:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fail to open a corrupt loose object, we report an\nerror and mention the filename via sha1_file_name().\nHowever, that function will always give us a path in the\nlocal repository, whereas the corrupt object may have come\nfrom an alternate. The result is a very misleading error\nmessage.\n\nTeach the open_sha1_file() and stat_sha1_file() helpers to\npass back the path they found, so that we can report it\ncorrectly.\n\nNote that the pointers we return go to static storage (e.g.,\nfrom sha1_file_name()), which is slightly dangerous.\nHowever, these helpers are static local helpers, and the\nnames are used for immediately generating error messages.\nThe simplicity is an acceptable tradeoff for the danger.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 46 +++++++++++++++++++++++++++++++---------------\n t/t1450-fsck.sh | 10 ++++++++++\n 2 files changed, 41 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1eb47f611..c6b990f41 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1630,39 +1630,54 @@ int git_open_cloexec(const char *name, int flags)\n \treturn fd;\n }\n \n-static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n+/*\n+ * Find \"sha1\" as a loose object in the local repository or in an alternate.\n+ * Returns 0 on success, negative on failure.\n+ *\n+ * The \"path\" out-parameter will give the path of the object we found (if any).\n+ * Note that it may point to static storage and is only valid until another\n+ * call to sha1_file_name(), etc.\n+ */\n+static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n+\t\t\t const char **path)\n {\n \tstruct alternate_object_database *alt;\n \n-\tif (!lstat(sha1_file_name(sha1), st))\n+\t*path = sha1_file_name(sha1);\n+\tif (!lstat(*path, st))\n \t\treturn 0;\n \n \tprepare_alt_odb();\n \terrno = ENOENT;\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tconst char *path = alt_sha1_path(alt, sha1);\n-\t\tif (!lstat(path, st))\n+\t\t*path = alt_sha1_path(alt, sha1);\n+\t\tif (!lstat(*path, st))\n \t\t\treturn 0;\n \t}\n \n \treturn -1;\n }\n \n-static int open_sha1_file(const unsigned char *sha1)\n+/*\n+ * Like stat_sha1_file(), but actually open the object and return the\n+ * descriptor. See the caveats on the \"path\" parameter above.\n+ */\n+static int open_sha1_file(const unsigned char *sha1, const char **path)\n {\n \tint fd;\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n \n-\tfd = git_open(sha1_file_name(sha1));\n+\t*path = sha1_file_name(sha1);\n+\tfd = git_open(*path);\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n \n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tconst char *path = alt_sha1_path(alt, sha1);\n-\t\tfd = git_open(path);\n+\t\t*path = alt_sha1_path(alt, sha1);\n+\t\tfd = git_open(*path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n@@ -1674,10 +1689,11 @@ static int open_sha1_file(const unsigned char *sha1)\n \n void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n {\n+\tconst char *path;\n \tvoid *map;\n \tint fd;\n \n-\tfd = open_sha1_file(sha1);\n+\tfd = open_sha1_file(sha1, &path);\n \tmap = NULL;\n \tif (fd >= 0) {\n \t\tstruct stat st;\n@@ -1686,7 +1702,7 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \t\t\t*size = xsize_t(st.st_size);\n \t\t\tif (!*size) {\n \t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(\"object file %s is empty\", sha1_file_name(sha1));\n+\t\t\t\terror(\"object file %s is empty\", path);\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n@@ -2806,8 +2822,9 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t * object even exists.\n \t */\n \tif (!oi->typep && !oi->typename && !oi->sizep) {\n+\t\tconst char *path;\n \t\tstruct stat st;\n-\t\tif (stat_sha1_file(sha1, &st) < 0)\n+\t\tif (stat_sha1_file(sha1, &st, &path) < 0)\n \t\t\treturn -1;\n \t\tif (oi->disk_sizep)\n \t\t\t*oi->disk_sizep = st.st_size;\n@@ -3003,6 +3020,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n {\n \tvoid *data;\n \tconst struct packed_git *p;\n+\tconst char *path;\n+\tstruct stat st;\n \tconst unsigned char *repl = lookup_replace_object_extended(sha1, flag);\n \n \terrno = 0;\n@@ -3018,12 +3037,9 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \t\tdie(\"replacement %s not found for %s\",\n \t\t sha1_to_hex(repl), sha1_to_hex(sha1));\n \n-\tif (has_loose_object(repl)) {\n-\t\tconst char *path = sha1_file_name(sha1);\n-\n+\tif (!stat_sha1_file(repl, &st, &path))\n \t\tdie(\"loose object %s (stored in %s) is corrupt\",\n \t\t sha1_to_hex(repl), path);\n-\t}\n \n \tif ((p = has_packed_and_bad(repl)) != NULL)\n \t\tdie(\"packed object %s (stored in %s) is corrupt\",\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 3297d4cb2..f95174c9d 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -550,4 +550,14 @@ test_expect_success 'fsck --name-objects' '\n \t)\n '\n \n+test_expect_success 'alternate objects are correctly blamed' '\n+\ttest_when_finished \"rm -rf alt.git .git/objects/info/alternates\" &&\n+\tgit init --bare alt.git &&\n+\techo \"../../alt.git/objects\" >.git/objects/info/alternates &&\n+\tmkdir alt.git/objects/12 &&\n+\t>alt.git/objects/12/34567890123456789012345678901234567890 &&\n+\ttest_must_fail git fsck >out 2>&1 &&\n+\tgrep alt.git out\n+'\n+\n test_done\n-- \n2.11.0.629.g10075098c\n\n"},{"id":"309370","messageId":"20170113175555.3xlkk27ozniiql3h@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 3/6] t1450: test fsck of packed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:55:55Z","receivedAt":"2017-01-13T17:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code paths in fsck for packed and loose objects are\nquite different, and it is not immediately obvious that the\npacked case behaves well. In particular:\n\n 1. The fsck_loose() function always returns \"0\" to tell the\n iterator to keep checking more objects. Whereas\n fsck_obj_buffer() (which handles packed objects)\n returns -1. This is OK, because the callback machinery\n for verify_pack() does not stop when it sees a non-zero\n return.\n\n 2. The fsck_loose() function sets the ERROR_OBJECT bit\n when fsck_obj() fails, whereas fsck_obj_buffer() sets it\n only when it sees a corrupt object. This turns out not\n to matter. We don't actually do anything with this bit\n except exit the program with a non-zero code, and that\n is handled already by the non-zero return from the\n function.\n\nSo there are no bugs here, but it was certainly confusing to\nme. And we do not test either of the properties in t1450\n(neither that a non-corruption error will caused a non-zero\nexit for a packed object, nor that we keep going after\nseeing the first error). Let's test both of those\nconditions, so that we'll notice if any of those assumptions\nbecomes invalid.\n\nSigned-off-by: Jeff King \n---\n t/t1450-fsck.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex f95174c9d..c39d42120 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -560,4 +560,25 @@ test_expect_success 'alternate objects are correctly blamed' '\n \tgrep alt.git out\n '\n \n+test_expect_success 'fsck errors in packed objects' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/one &&\n+\tsed \"s/two &&\n+\tone=$(git hash-object -t commit -w one) &&\n+\ttwo=$(git hash-object -t commit -w two) &&\n+\tpack=$(\n+\t\t{\n+\t\t\techo $one &&\n+\t\t\techo $two\n+\t\t} | git pack-objects .git/objects/pack/pack\n+\t) &&\n+\ttest_when_finished \"rm -f .git/objects/pack/pack-$pack.*\" &&\n+\tremove_object $one &&\n+\tremove_object $two &&\n+\ttest_must_fail git fsck 2>out &&\n+\tgrep \"error in commit $one.* - bad name\" out &&\n+\tgrep \"error in commit $two.* - bad name\" out &&\n+\t! grep corrupt out\n+'\n+\n test_done\n-- \n2.11.0.629.g10075098c\n\n"},{"id":"309374","messageId":"20170113175816.jonlumcglz3ajchd@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 4/6] sha1_file: add read_loose_object() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:58:16Z","receivedAt":"2017-01-13T17:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's surprisingly hard to ask the sha1_file code to open a\n_specific_ incarnation of a loose object. Most of the\nfunctions take a sha1, and loop over the various object\ntypes (packed versus loose) and locations (local versus\nalternates) at a low level.\n\nHowever, some tools like fsck need to look at a specific\nfile. This patch gives them a function they can use to open\nthe loose object at a given path.\n\nThe implementation unfortunately ends up repeating bits of\nrelated functions, but there's not a good way around it\nwithout some major refactoring of the whole sha1_file stack.\nWe need to mmap the specific file, then partially read the\nzlib stream to know whether we're streaming or not, and then\nfinally either stream it or copy the data to a buffer.\n\nWe can do that by assembling some of the more arcane\ninternal sha1_file functions, but we end up having to\nessentially reimplement unpack_sha1_file(), along with the\nstreaming bits of check_sha1_signature().\n\nStill, most of the ugliness is contained in the new\nfunction, and the interface is clean enough that it may be\nreusable (though it seems unlikely anything but git-fsck\nwould care about opening a specific file).\n\nSigned-off-by: Jeff King \n---\n cache.h | 13 ++++++\n sha1_file.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 143 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 1b67f078d..33f1c2fa7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1140,6 +1140,19 @@ extern int finalize_object_file(const char *tmpfile, const char *filename);\n \n extern int has_sha1_pack(const unsigned char *sha1);\n \n+/*\n+ * Open the loose object at path, check its sha1, and return the contents,\n+ * type, and size. If the object is a blob, then \"contents\" may return NULL,\n+ * to allow streaming of large blobs.\n+ *\n+ * Returns 0 on success, negative on error (details may be written to stderr).\n+ */\n+int read_loose_object(const char *path,\n+\t\t const unsigned char *expected_sha1,\n+\t\t enum object_type *type,\n+\t\t unsigned long *size,\n+\t\t void **contents);\n+\n /*\n * Return true iff we have an object named sha1, whether local or in\n * an alternate object database, and whether packed or loose. This\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c6b990f41..c0fccb73c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1687,13 +1687,21 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n \treturn -1;\n }\n \n-void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n+/*\n+ * Map the loose object at \"path\" if it is not NULL, or the path found by\n+ * searching for a loose object named \"sha1\".\n+ */\n+static void *map_sha1_file_1(const char *path,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t unsigned long *size)\n {\n-\tconst char *path;\n \tvoid *map;\n \tint fd;\n \n-\tfd = open_sha1_file(sha1, &path);\n+\tif (path)\n+\t\tfd = git_open(path);\n+\telse\n+\t\tfd = open_sha1_file(sha1, &path);\n \tmap = NULL;\n \tif (fd >= 0) {\n \t\tstruct stat st;\n@@ -1712,6 +1720,11 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \treturn map;\n }\n \n+void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n+{\n+\treturn map_sha1_file_1(NULL, sha1, size);\n+}\n+\n unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n@@ -3809,3 +3822,117 @@ int for_each_packed_object(each_packed_object_fn cb, void *data, unsigned flags)\n \t}\n \treturn r ? r : pack_errors;\n }\n+\n+static int check_stream_sha1(git_zstream *stream,\n+\t\t\t const char *hdr,\n+\t\t\t unsigned long size,\n+\t\t\t const char *path,\n+\t\t\t const unsigned char *expected_sha1)\n+{\n+\tgit_SHA_CTX c;\n+\tunsigned char real_sha1[GIT_SHA1_RAWSZ];\n+\tunsigned char buf[4096];\n+\tunsigned long total_read;\n+\tint status = Z_OK;\n+\n+\tgit_SHA1_Init(&c);\n+\tgit_SHA1_Update(&c, hdr, stream->total_out);\n+\n+\t/*\n+\t * We already read some bytes into hdr, but the ones up to the NUL\n+\t * do not count against the object's content size.\n+\t */\n+\ttotal_read = stream->total_out - strlen(hdr) - 1;\n+\n+\t/*\n+\t * This size comparison must be \"<=\" to read the final zlib packets;\n+\t * see the comment in unpack_sha1_rest for details.\n+\t */\n+\twhile (total_read <= size &&\n+\t (status == Z_OK || status == Z_BUF_ERROR)) {\n+\t\tstream->next_out = buf;\n+\t\tstream->avail_out = sizeof(buf);\n+\t\tif (size - total_read < stream->avail_out)\n+\t\t\tstream->avail_out = size - total_read;\n+\t\tstatus = git_inflate(stream, Z_FINISH);\n+\t\tgit_SHA1_Update(&c, buf, stream->next_out - buf);\n+\t\ttotal_read += stream->next_out - buf;\n+\t}\n+\tgit_inflate_end(stream);\n+\n+\tif (status != Z_STREAM_END) {\n+\t\terror(\"corrupt loose object '%s'\", sha1_to_hex(expected_sha1));\n+\t\treturn -1;\n+\t}\n+\n+\tgit_SHA1_Final(real_sha1, &c);\n+\tif (hashcmp(expected_sha1, real_sha1)) {\n+\t\terror(\"sha1 mismatch for %s (expected %s)\", path,\n+\t\t sha1_to_hex(expected_sha1));\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+int read_loose_object(const char *path,\n+\t\t const unsigned char *expected_sha1,\n+\t\t enum object_type *type,\n+\t\t unsigned long *size,\n+\t\t void **contents)\n+{\n+\tint ret = -1;\n+\tint fd = -1;\n+\tvoid *map = NULL;\n+\tunsigned long mapsize;\n+\tgit_zstream stream;\n+\tchar hdr[32];\n+\n+\t*contents = NULL;\n+\n+\tmap = map_sha1_file_1(path, NULL, &mapsize);\n+\tif (!map) {\n+\t\terror_errno(\"unable to mmap %s\", path);\n+\t\tgoto out;\n+\t}\n+\n+\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) {\n+\t\terror(\"unable to unpack header of %s\", path);\n+\t\tgoto out;\n+\t}\n+\n+\t*type = parse_sha1_header(hdr, size);\n+\tif (*type < 0) {\n+\t\terror(\"unable to parse header of %s\", path);\n+\t\tgit_inflate_end(&stream);\n+\t\tgoto out;\n+\t}\n+\n+\tif (*type == OBJ_BLOB) {\n+\t\tif (check_stream_sha1(&stream, hdr, *size, path, expected_sha1) < 0)\n+\t\t\tgoto out;\n+\t} else {\n+\t\t*contents = unpack_sha1_rest(&stream, hdr, *size, expected_sha1);\n+\t\tif (!*contents) {\n+\t\t\terror(\"unable to unpack contents of %s\", path);\n+\t\t\tgit_inflate_end(&stream);\n+\t\t\tgoto out;\n+\t\t}\n+\t\tif (check_sha1_signature(expected_sha1, *contents,\n+\t\t\t\t\t *size, typename(*type))) {\n+\t\t\terror(\"sha1 mismatch for %s (expected %s)\", path,\n+\t\t\t sha1_to_hex(expected_sha1));\n+\t\t\tfree(*contents);\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n+\n+\tret = 0; /* everything checks out */\n+\n+out:\n+\tif (map)\n+\t\tmunmap(map, mapsize);\n+\tif (fd >= 0)\n+\t\tclose(fd);\n+\treturn ret;\n+}\n-- \n2.11.0.629.g10075098c\n\n"},{"id":"309375","messageId":"20170113175944.tdbfqx3e4xhris7m@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 5/6] fsck: parse loose object paths directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T17:59:44Z","receivedAt":"2017-01-13T17:59:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we iterate over the list of loose objects to check, we\nget the actual path of each object. But we then throw it\naway and pass just the sha1 to fsck_sha1(), which will do a\nfresh lookup. Usually it would find the same object, but it\nmay not if an object exists both as a loose and a packed\nobject. We may end up checking the packed object twice, and\nnever look at the loose one.\n\nIn practice this isn't too terrible, because if fsck doesn't\ncomplain, it means you have at least one good copy. But\nsince the point of fsck is to look for corruption, we should\nbe thorough.\n\nThe new read_loose_object() interface can help us get the\ndata from disk, and then we replace parse_object() with\nparse_object_buffer(). As a bonus, our error messages now\nmention the path to a corrupted object, which should make it\neasier to track down errors when they do happen.\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 46 +++++++++++++++++++++++++++++++++-------------\n t/t1450-fsck.sh | 16 ++++++++++++++++\n 2 files changed, 49 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f01b81eeb..4b91ee95e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -362,18 +362,6 @@ static int fsck_obj(struct object *obj)\n \treturn 0;\n }\n \n-static int fsck_sha1(const unsigned char *sha1)\n-{\n-\tstruct object *obj = parse_object(sha1);\n-\tif (!obj) {\n-\t\terrors_found |= ERROR_OBJECT;\n-\t\treturn error(\"%s: object corrupt or missing\",\n-\t\t\t sha1_to_hex(sha1));\n-\t}\n-\tobj->flags |= HAS_OBJ;\n-\treturn fsck_obj(obj);\n-}\n-\n static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,\n \t\t\t unsigned long size, void *buffer, int *eaten)\n {\n@@ -488,9 +476,41 @@ static void get_default_heads(void)\n \t}\n }\n \n+static struct object *parse_loose_object(const unsigned char *sha1,\n+\t\t\t\t\t const char *path)\n+{\n+\tstruct object *obj;\n+\tvoid *contents;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tint eaten;\n+\n+\tif (read_loose_object(path, sha1, &type, &size, &contents) < 0)\n+\t\treturn NULL;\n+\n+\tif (!contents && type != OBJ_BLOB)\n+\t\tdie(\"BUG: read_loose_object streamed a non-blob\");\n+\n+\tobj = parse_object_buffer(sha1, type, size, contents, &eaten);\n+\n+\tif (!eaten)\n+\t\tfree(contents);\n+\treturn obj;\n+}\n+\n static int fsck_loose(const unsigned char *sha1, const char *path, void *data)\n {\n-\tif (fsck_sha1(sha1))\n+\tstruct object *obj = parse_loose_object(sha1, path);\n+\n+\tif (!obj) {\n+\t\terrors_found |= ERROR_OBJECT;\n+\t\terror(\"%s: object corrupt or missing: %s\",\n+\t\t sha1_to_hex(sha1), path);\n+\t\treturn 0; /* keep checking other objects */\n+\t}\n+\n+\tobj->flags = HAS_OBJ;\n+\tif (fsck_obj(obj))\n \t\terrors_found |= ERROR_OBJECT;\n \treturn 0;\n }\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex c39d42120..455c186fe 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -581,4 +581,20 @@ test_expect_success 'fsck errors in packed objects' '\n \t! grep corrupt out\n '\n \n+test_expect_success 'fsck finds problems in duplicate loose objects' '\n+\trm -rf broken-duplicate &&\n+\tgit init broken-duplicate &&\n+\t(\n+\t\tcd broken-duplicate &&\n+\t\ttest_commit duplicate &&\n+\t\t# no \"-d\" here, so we end up with duplicates\n+\t\tgit repack &&\n+\t\t# now corrupt the loose copy\n+\t\tfile=$(sha1_file \"$(git rev-parse HEAD)\") &&\n+\t\trm \"$file\" &&\n+\t\techo broken >\"$file\" &&\n+\t\ttest_must_fail git fsck\n+\t)\n+'\n+\n test_done\n-- \n2.11.0.629.g10075098c\n\n"},{"id":"309376","messageId":"20170113180025.xkwyc6mzxl572jn7@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"[PATCH 6/6] fsck: detect trailing garbage in all object types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-13T18:00:25Z","receivedAt":"2017-01-13T18:00:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a loose tree or commit is read by fsck (or any git\nprogram), unpack_sha1_rest() checks whether there is extra\ncruft at the end of the object file, after the zlib data.\nBlobs that are streamed, however, do not have this check.\n\nFor normal git operations, it's not a big deal. We know the\nsha1 and size checked out, so we have the object bytes we\nwanted. The trailing garbage doesn't affect what we're\ntrying to do.\n\nBut since the point of fsck is to find corruption or other\nproblems, it should be more thorough. This patch teaches its\nloose-sha1 reader to detect extra bytes after the zlib\nstream and complain.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 5 +++++\n t/t1450-fsck.sh | 22 ++++++++++++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c0fccb73c..b77ab6d5c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3864,6 +3864,11 @@ static int check_stream_sha1(git_zstream *stream,\n \t\terror(\"corrupt loose object '%s'\", sha1_to_hex(expected_sha1));\n \t\treturn -1;\n \t}\n+\tif (stream->avail_in) {\n+\t\terror(\"garbage at end of loose object '%s'\",\n+\t\t sha1_to_hex(expected_sha1));\n+\t\treturn -1;\n+\t}\n \n \tgit_SHA1_Final(real_sha1, &c);\n \tif (hashcmp(expected_sha1, real_sha1)) {\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 455c186fe..8975b4d1b 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -597,4 +597,26 @@ test_expect_success 'fsck finds problems in duplicate loose objects' '\n \t)\n '\n \n+test_expect_success 'fsck detects trailing loose garbage (commit)' '\n+\tgit cat-file commit HEAD >basis &&\n+\techo bump-commit-sha1 >>basis &&\n+\tcommit=$(git hash-object -w -t commit basis) &&\n+\tfile=$(sha1_file $commit) &&\n+\ttest_when_finished \"remove_object $commit\" &&\n+\tchmod +w \"$file\" &&\n+\techo garbage >>\"$file\" &&\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep \"garbage.*$commit\" out\n+'\n+\n+test_expect_success 'fsck detects trailing loose garbage (blob)' '\n+\tblob=$(echo trailing | git hash-object -w --stdin) &&\n+\tfile=$(sha1_file $blob) &&\n+\ttest_when_finished \"remove_object $blob\" &&\n+\tchmod +w \"$file\" &&\n+\techo garbage >>\"$file\" &&\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep \"garbage.*$blob\" out\n+'\n+\n test_done\n-- \n2.11.0.629.g10075098c\n"},{"id":"309715","messageId":"CAEBDL5XfZDipTNf73q1bNN+xatEvLD29uicSim-a7bqUV1Z=NQ@mail.gmail.com","threadId":"44833","inReplyTo":"20170113175258.e66taigy4wpokohk@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] loose-object fsck fixes/tightening","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2017-01-19T11:18:09Z","receivedAt":"2017-01-19T11:19:19Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Fri, Jan 13, 2017 at 12:52 PM, Jeff King wrote:\n> On Fri, Jan 13, 2017 at 04:15:42AM -0500, John Szakmeister wrote:\n>\n>> > I did notice another interesting case when looking at this. Fsck ends up\n>> > in fsck_loose(), which has the sha1 and path of the loose object. It\n>> > passes the sha1 to fsck_sha1(), and ignores the path entirely!\n>> >\n>> > So if you have a duplicate copy of the object in a pack, we'd actually\n>> > find and check the duplicate. This can happen, e.g., if you had a loose\n>> > object and fetched a thin-pack which made a copy of the loose object to\n>> > complete the pack).\n>> >\n>> > Probably fsck_loose() should be more picky about making sure we are\n>> > reading the data from the loose version we found.\n>>\n>> Interesting find! Thanks for the information Peff!\n>\n> So I figured I would knock this out as a fun morning exercise. But\n> sheesh, it turned out to be a slog, because most of the functions rely\n> on map_sha1_file() to convert the sha1 to an object path at the lowest\n> level.\n\nYeah, I discovered the same thing when I took a look at it a week or so ago. :-(\n\n> But I finally got something working, so here it is. I found another bug\n> on the way, along with a few cleanups. And then I did the trailing\n> garbage detection at the end, because by that point I knew right where\n> it needed to go. :)\n\nI don't know if my opinion counts for much, but the changes look good to me.\n\n-John\n"},{"id":"362012","messageId":"878t2fkxrn.fsf@evledraar.gmail.com","threadId":"44833","inReplyTo":"20170113175944.tdbfqx3e4xhris7m@sigill.intra.peff.net","subject":"Infinite loop regression in git-fsck in v2.12.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-30T20:03:24Z","receivedAt":"2018-10-30T20:03:31Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"While playing around with having a GIT_TEST_FSCK=true as I suggested in\nhttps://public-inbox.org/git/20181030184331.27264-3-avarab@gmail.com/ I\nfound that we've had an infinite loop in git-fsck since c68b489e56\n(\"fsck: parse loose object paths directly\", 2017-01-13)\n\nIn particular in the while() loop added by f6371f9210 (\"sha1_file: add\nread_loose_object() function\", 2017-01-13) in the check_stream_sha1()\nfunction.\n\nTo reproduce just:\n\n (\n cd t &&\n ./t5000-tar-tree.sh -d &&\n git -C trash\\ directory.t5000-tar-tree/ fsck\n )\n\nBefore we'd print:\n\n error: sha1 mismatch 19f9c8273ec45a8938e6999cb59b3ff66739902a\n error: 19f9c8273ec45a8938e6999cb59b3ff66739902a: object corrupt or missing\n Checking object directories: 100% (256/256), done.\n missing blob 19f9c8273ec45a8938e6999cb59b3ff66739902a\n\nNow we just hang on:\n\n Checking object directories: 9% (24/256)\n\nI have no idea if this makes sense, but this fixes it and we pass all\nthe fsck tests with it:\n\n diff --git a/sha1-file.c b/sha1-file.c\n index dd0b6aa873..fffc31458e 100644\n --- a/sha1-file.c\n +++ b/sha1-file.c\n @@ -2182,7 +2182,7 @@ static int check_stream_sha1(git_zstream *stream,\n \tgit_hash_ctx c;\n \tunsigned char real_sha1[GIT_MAX_RAWSZ];\n \tunsigned char buf[4096];\n -\tunsigned long total_read;\n +\tunsigned long total_read, last_total_read;\n \tint status = Z_OK;\n\n \tthe_hash_algo->init_fn(&c);\n @@ -2193,6 +2193,7 @@ static int check_stream_sha1(git_zstream *stream,\n \t * do not count against the object's content size.\n \t */\n \ttotal_read = stream->total_out - strlen(hdr) - 1;\n +\tlast_total_read = total_read;\n\n \t/*\n \t * This size comparison must be \"<=\" to read the final zlib packets;\n @@ -2207,6 +2208,9 @@ static int check_stream_sha1(git_zstream *stream,\n \t\tstatus = git_inflate(stream, Z_FINISH);\n \t\tthe_hash_algo->update_fn(&c, buf, stream->next_out - buf);\n \t\ttotal_read += stream->next_out - buf;\n +\t\tif (last_total_read == total_read)\n +\t\t\treturn -1;\n +\t\tlast_total_read = total_read;\n \t}\n \tgit_inflate_end(stream);\n\n\nI.e. we get into a loop where total_read isn't increasing. We no longer\nprint \"sha1 mismatch\" but maybe that's an emergent effect of something\nelse. Haven't checked.\n\nThe test is easy, just add a 'git fsck' at the end of t5000-tar-tree.sh,\nbut more generally it seems having something like GIT_TEST_FSCK=true is\na good idea. We do a bunch of stress testing of the object store in the\ntest suite that we're unlikely to encounter in the wild.\n\nOf course my idea of how to do that in my\n<20181030184331.27264-3-avarab@gmail.com> would be counterproductive,\ni.e. it seems we want to catch all the cases where there's a bad fsck,\njust that it returns in a certain way.\n\nSo maybe a good approach would be that we'd annotate all those test\nwhose fsck fails with \"this is how it should fail\", and run those tests\nunder GIT_TEST_FSCK=true, and GIT_TEST_FSCK=true would also be asserting\nthat no tests other than those marked as failing the fsck check at the\nend fail it.\n"},{"id":"362013","messageId":"20181030213505.GA11319@sigill.intra.peff.net","threadId":"44833","inReplyTo":"878t2fkxrn.fsf@evledraar.gmail.com","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T21:35:05Z","receivedAt":"2018-10-30T21:35:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2018 at 09:03:24PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> While playing around with having a GIT_TEST_FSCK=true as I suggested in\n> https://public-inbox.org/git/20181030184331.27264-3-avarab@gmail.com/ I\n> found that we've had an infinite loop in git-fsck since c68b489e56\n> (\"fsck: parse loose object paths directly\", 2017-01-13)\n> \n> In particular in the while() loop added by f6371f9210 (\"sha1_file: add\n> read_loose_object() function\", 2017-01-13) in the check_stream_sha1()\n> function.\n> \n> To reproduce just:\n> \n> (\n> cd t &&\n> ./t5000-tar-tree.sh -d &&\n> git -C trash\\ directory.t5000-tar-tree/ fsck\n> )\n\nThanks, I was easily able to reproduce.\n\n> Before we'd print:\n> \n> error: sha1 mismatch 19f9c8273ec45a8938e6999cb59b3ff66739902a\n> error: 19f9c8273ec45a8938e6999cb59b3ff66739902a: object corrupt or missing\n> Checking object directories: 100% (256/256), done.\n> missing blob 19f9c8273ec45a8938e6999cb59b3ff66739902a\n\nThe problem isn't actually a sha1 mismatch, though that's what\nparse_object() will report. The issue is actually that the file is\ntruncated. So zlib does not say \"this is corrupt\", but rather \"I need\nmore bytes to keep going\". And unfortunately it returns Z_BUF_ERROR both\nfor \"I need more bytes\" (in which we know we are truncated, because we\nfed the whole mmap'd file in the first place) as well as \"I need more\noutput buffer space\" (which just means we should keep looping!).\n\nSo we need to distinguish those cases. I think this is the simplest fix:\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex dd0b6aa873..a7ff5fe25d 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -2199,6 +2199,7 @@ static int check_stream_sha1(git_zstream *stream,\n \t * see the comment in unpack_sha1_rest for details.\n \t */\n \twhile (total_read <= size &&\n+\t stream->avail_in > 0 &&\n \t (status == Z_OK || status == Z_BUF_ERROR)) {\n \t\tstream->next_out = buf;\n \t\tstream->avail_out = sizeof(buf);\n\n> I have no idea if this makes sense, but this fixes it and we pass all\n> the fsck tests with it:\n> \n> diff --git a/sha1-file.c b/sha1-file.c\n> index dd0b6aa873..fffc31458e 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -2182,7 +2182,7 @@ static int check_stream_sha1(git_zstream *stream,\n> \tgit_hash_ctx c;\n> \tunsigned char real_sha1[GIT_MAX_RAWSZ];\n> \tunsigned char buf[4096];\n> -\tunsigned long total_read;\n> +\tunsigned long total_read, last_total_read;\n> \tint status = Z_OK;\n> \n> \tthe_hash_algo->init_fn(&c);\n> @@ -2193,6 +2193,7 @@ static int check_stream_sha1(git_zstream *stream,\n> \t * do not count against the object's content size.\n> \t */\n> \ttotal_read = stream->total_out - strlen(hdr) - 1;\n> +\tlast_total_read = total_read;\n\nThis works just by checking that we are making forward progress in the\noutput buffer. I think that would _probably_ be OK for this case, since\nwe know we have all of the input available. But in a case where we're\nfeeding the input in a stream, it would not be. It's possible there that\nwe would not create any output in one round, but would do so after\nfeeding more input bytes.\n\nI think the patch I showed above addresses the root cause more directly.\nI'll wrap that up in a real commit, but I think there may be some\nrelated work:\n\n - \"git show 19f9c827\" does complain with \"sha1 mismatch\" (which isn't\n strictly correct, but is probably good enough). However, \"git\n cat-file blob 19f9c827\" exits non-zero without printing anything. It\n probably should complain more loudly.\n\n - the offending loop comes from f6371f9210. But that commit was mostly\n cargo-culting other parts of sha1-file.c. I'm worried that this bug\n exists elsewhere, too. I'll dig around to see if I can find other\n instances.\n\n-Peff\n"},{"id":"362014","messageId":"877ehzksjd.fsf@evledraar.gmail.com","threadId":"44833","inReplyTo":"878t2fkxrn.fsf@evledraar.gmail.com","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-30T21:56:22Z","receivedAt":"2018-10-30T21:56:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 30 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> The test is easy, just add a 'git fsck' at the end of t5000-tar-tree.sh,\n> but more generally it seems having something like GIT_TEST_FSCK=true is\n> a good idea. We do a bunch of stress testing of the object store in the\n> test suite that we're unlikely to encounter in the wild.\n>\n> Of course my idea of how to do that in my\n> <20181030184331.27264-3-avarab@gmail.com> would be counterproductive,\n> i.e. it seems we want to catch all the cases where there's a bad fsck,\n> just that it returns in a certain way.\n>\n> So maybe a good approach would be that we'd annotate all those test\n> whose fsck fails with \"this is how it should fail\", and run those tests\n> under GIT_TEST_FSCK=true, and GIT_TEST_FSCK=true would also be asserting\n> that no tests other than those marked as failing the fsck check at the\n> end fail it.\n\nWIP patch for doing that:\n\n diff --git a/Makefile b/Makefile\n index b08d5ea258..ca624c381f 100644\n --- a/Makefile\n +++ b/Makefile\n @@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-example-decorate.o\n +TEST_BUILTINS_OBJS += test-env-bool.o\n TEST_BUILTINS_OBJS += test-genrandom.o\n TEST_BUILTINS_OBJS += test-hashmap.o\n TEST_BUILTINS_OBJS += test-index-version.o\n diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n index 5df8b682aa..c4481085c4 100644\n --- a/t/helper/test-tool.c\n +++ b/t/helper/test-tool.c\n @@ -17,6 +17,7 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-fsmonitor\", cmd__dump_fsmonitor },\n \t{ \"dump-split-index\", cmd__dump_split_index },\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n +\t{ \"env-bool\", cmd__env_bool },\n \t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"genrandom\", cmd__genrandom },\n \t{ \"hashmap\", cmd__hashmap },\n diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n index 71f470b871..f7845fbc56 100644\n --- a/t/helper/test-tool.h\n +++ b/t/helper/test-tool.h\n @@ -13,6 +13,7 @@ int cmd__dump_cache_tree(int argc, const char **argv);\n int cmd__dump_fsmonitor(int argc, const char **argv);\n int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n +int cmd__env_bool(int argc, const char **argv);\n int cmd__example_decorate(int argc, const char **argv);\n int cmd__genrandom(int argc, const char **argv);\n int cmd__hashmap(int argc, const char **argv);\n diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n index 635918505d..92fbce2920 100755\n --- a/t/t1305-config-include.sh\n +++ b/t/t1305-config-include.sh\n @@ -313,4 +313,8 @@ test_expect_success 'include cycles are detected' '\n \ttest_i18ngrep \"exceeded maximum include depth\" stderr\n '\n\n +GIT_FSCK_FAILS=true\n +GIT_FSCK_FAILS_TEST='\n +\ttest_i18ngrep \"exceeded maximum include depth\" fsck.err\n +'\n test_done\n diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh\n index 14520913af..06abf84ef4 100755\n --- a/t/t3103-ls-tree-misc.sh\n +++ b/t/t3103-ls-tree-misc.sh\n @@ -22,4 +22,10 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n \ttest_must_fail git ls-tree -r HEAD\n '\n\n +GIT_FSCK_FAILS=true\n +GIT_FSCK_FAILS_TEST='\n +\ttest_i18ngrep \"invalid sha1 pointer in cache-tree\" fsck.err &&\n +\ttest_i18ngrep \"broken link from\" fsck.out &&\n +\ttest_i18ngrep \"missing tree\" fsck.out\n +'\n test_done\n diff --git a/t/test-lib.sh b/t/test-lib.sh\n index 897e6fcc94..d4ebb94998 100644\n --- a/t/test-lib.sh\n +++ b/t/test-lib.sh\n @@ -454,6 +454,8 @@ GIT_EXIT_OK=\n trap 'die' EXIT\n trap 'exit $?' INT\n\n +GIT_FSCK_FAILS=\n +\n # The user-facing functions are loaded from a separate file so that\n # test_perf subshells can have them too\n . \"$TEST_DIRECTORY/test-lib-functions.sh\"\n @@ -790,6 +792,25 @@ test_at_end_hook_ () {\n }\n\n test_done () {\n +\tif test_have_prereq TEST_FSCK\n +\tthen\n +\t\tdesc='git fsck at end (due to GIT_TEST_FSCK)'\n +\t\tif test -n \"$GIT_FSCK_FAILS\"\n +\t\tthen\n +\t\t\ttest_expect_success \"$desc (expected to fail)\" '\n +\t\t\t\ttest_must_fail git fsck 2>fsck.err >fsck.out\n +\t\t\t'\n +\t\t\ttest_expect_success \"$descriptor (expected to fail) -- assert failure mode\" \"\n +\t\t\t\ttest_path_exists fsck.err &&\n +\t\t\t\ttest_path_exists fsck.out &&\n +\t\t\t\t$GIT_FSCK_FAILS_TEST\n +\t\t\t\"\n +\t\telse\n +\t\t\ttest_expect_success \"$desc\" '\n +\t\t\t\tgit fsck\n +\t\t\t'\n +\t\tfi\n +\tfi\n \tGIT_EXIT_OK=t\n\n \tif test -z \"$HARNESS_ACTIVE\"\n @@ -1268,3 +1289,5 @@ test_lazy_prereq CURL '\n test_lazy_prereq SHA1 '\n \ttest $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n '\n +\n +test_lazy_prereq TEST_FSCK 'test-tool env-bool GIT_TEST_FSCK'\n\nCould be made prettier by turning that work in test_done() into a\nutility function, but is (I think) worth the effort to do.\n\nJeff: Gotta turn in for the night, but maybe Something you're maybe\ninterested in carrying forward for this fix? It's not that much work to\nmark up the failing tests, there's 10-20 of them from some quick\neyeballing.\n"},{"id":"362040","messageId":"xmqq4ld3134f.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181030213505.GA11319@sigill.intra.peff.net","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-30T22:28:00Z","receivedAt":"2018-10-30T22:28:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The problem isn't actually a sha1 mismatch, though that's what\n> parse_object() will report. The issue is actually that the file is\n> truncated. So zlib does not say \"this is corrupt\", but rather \"I need\n> more bytes to keep going\". And unfortunately it returns Z_BUF_ERROR both\n> for \"I need more bytes\" (in which we know we are truncated, because we\n> fed the whole mmap'd file in the first place) as well as \"I need more\n> output buffer space\" (which just means we should keep looping!).\n>\n> So we need to distinguish those cases. I think this is the simplest fix:\n>\n> diff --git a/sha1-file.c b/sha1-file.c\n> index dd0b6aa873..a7ff5fe25d 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -2199,6 +2199,7 @@ static int check_stream_sha1(git_zstream *stream,\n> \t * see the comment in unpack_sha1_rest for details.\n> \t */\n> \twhile (total_read <= size &&\n> +\t stream->avail_in > 0 &&\n> \t (status == Z_OK || status == Z_BUF_ERROR)) {\n> \t\tstream->next_out = buf;\n> \t\tstream->avail_out = sizeof(buf);\n\nHmph. If the last round consumed the final input byte and needed\noutput space of N bytes, but only M (< N) bytes of the output space\nwas available, then it would have reduced both avail_in and\navail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib\nrefrain from consuming that final byte (leaving avail_in to at least\none) and give us Z_BUF_ERROR in such a case?\n\n> This works just by checking that we are making forward progress in the\n> output buffer. I think that would _probably_ be OK for this case, since\n> we know we have all of the input available. But in a case where we're\n> feeding the input in a stream, it would not be. It's possible there that\n> we would not create any output in one round, but would do so after\n> feeding more input bytes.\n\nYes, exactly.\n\n> I think the patch I showed above addresses the root cause more directly.\n> I'll wrap that up in a real commit, but I think there may be some\n> related work:\n>\n> - \"git show 19f9c827\" does complain with \"sha1 mismatch\" (which isn't\n> strictly correct, but is probably good enough). However, \"git\n> cat-file blob 19f9c827\" exits non-zero without printing anything. It\n> probably should complain more loudly.\n>\n> - the offending loop comes from f6371f9210. But that commit was mostly\n> cargo-culting other parts of sha1-file.c. I'm worried that this bug\n> exists elsewhere, too. I'll dig around to see if I can find other\n> instances.\n\nThanks.\n"},{"id":"362041","messageId":"20181030225603.GA5889@sigill.intra.peff.net","threadId":"44833","inReplyTo":"xmqq4ld3134f.fsf@gitster-ct.c.googlers.com","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T22:56:03Z","receivedAt":"2018-10-30T22:56:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 07:28:00AM +0900, Junio C Hamano wrote:\n\n> > So we need to distinguish those cases. I think this is the simplest fix:\n> >\n> > diff --git a/sha1-file.c b/sha1-file.c\n> > index dd0b6aa873..a7ff5fe25d 100644\n> > --- a/sha1-file.c\n> > +++ b/sha1-file.c\n> > @@ -2199,6 +2199,7 @@ static int check_stream_sha1(git_zstream *stream,\n> > \t * see the comment in unpack_sha1_rest for details.\n> > \t */\n> > \twhile (total_read <= size &&\n> > +\t stream->avail_in > 0 &&\n> > \t (status == Z_OK || status == Z_BUF_ERROR)) {\n> > \t\tstream->next_out = buf;\n> > \t\tstream->avail_out = sizeof(buf);\n> \n> Hmph. If the last round consumed the final input byte and needed\n> output space of N bytes, but only M (< N) bytes of the output space\n> was available, then it would have reduced both avail_in and\n> avail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib\n> refrain from consuming that final byte (leaving avail_in to at least\n> one) and give us Z_BUF_ERROR in such a case?\n\nHmm, yeah, good thinking. I think zlib could consume that final byte\ninto its internal buffer.\n\nAs part of my digging, I looked at how the loose streaming code handles\nthis. It checks that when we see Z_BUF_ERROR, we actually did run out of\noutput bytes (so if we didn't, then we know it's not the case we\nexpected to be looping on).\n\nI have some patches almost ready to send; I'll use that technique.\n\n-Peff\n"},{"id":"362043","messageId":"20181030230833.GA12950@sigill.intra.peff.net","threadId":"44833","inReplyTo":"877ehzksjd.fsf@evledraar.gmail.com","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T23:08:33Z","receivedAt":"2018-10-30T23:08:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2018 at 10:56:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > So maybe a good approach would be that we'd annotate all those test\n> > whose fsck fails with \"this is how it should fail\", and run those tests\n> > under GIT_TEST_FSCK=true, and GIT_TEST_FSCK=true would also be asserting\n> > that no tests other than those marked as failing the fsck check at the\n> > end fail it.\n> [...]\n> Jeff: Gotta turn in for the night, but maybe Something you're maybe\n> interested in carrying forward for this fix? It's not that much work to\n> mark up the failing tests, there's 10-20 of them from some quick\n> eyeballing.\n\nFor this fix, I'd much rather add a specific test to the existing fsck\ntests. Otherwise, we're relying on what a bunch of other tests happen to\nbe doing now, but there's little hope that they won't get refactored in\na way that puts a gap in our test coverage.\n\nIOW, I think of things like GIT_TEST_FSCK as a kind of shotgun approach.\nThey may find things, and we should fix them and make sure it runs\nclean. But ultimately, specific cases of interest should get their own\ntests.\n\n-Peff\n"},{"id":"362044","messageId":"20181030231232.GA6141@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181030225603.GA5889@sigill.intra.peff.net","subject":"Re: Infinite loop regression in git-fsck in v2.12.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T23:12:32Z","receivedAt":"2018-10-30T23:12:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2018 at 06:56:03PM -0400, Jeff King wrote:\n\n> > > \twhile (total_read <= size &&\n> > > +\t stream->avail_in > 0 &&\n> > > \t (status == Z_OK || status == Z_BUF_ERROR)) {\n> > > \t\tstream->next_out = buf;\n> > > \t\tstream->avail_out = sizeof(buf);\n> > \n> > Hmph. If the last round consumed the final input byte and needed\n> > output space of N bytes, but only M (< N) bytes of the output space\n> > was available, then it would have reduced both avail_in and\n> > avail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib\n> > refrain from consuming that final byte (leaving avail_in to at least\n> > one) and give us Z_BUF_ERROR in such a case?\n> \n> Hmm, yeah, good thinking. I think zlib could consume that final byte\n> into its internal buffer.\n> \n> As part of my digging, I looked at how the loose streaming code handles\n> this. It checks that when we see Z_BUF_ERROR, we actually did run out of\n> output bytes (so if we didn't, then we know it's not the case we\n> expected to be looping on).\n> \n> I have some patches almost ready to send; I'll use that technique.\n\nAnd here they are.\n\n [1/3]: t1450: check large blob in trailing-garbage test\n [2/3]: check_stream_sha1(): handle input underflow\n [3/3]: cat-file: handle streaming failures consistently\n\n builtin/cat-file.c | 16 ++++++++++++----\n sha1-file.c | 3 ++-\n t/t1450-fsck.sh | 23 +++++++++++++++++++++--\n 3 files changed, 35 insertions(+), 7 deletions(-)\n\n-Peff\n"},{"id":"362045","messageId":"20181030231850.GA32038@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181030231232.GA6141@sigill.intra.peff.net","subject":"[PATCH 1/3] t1450: check large blob in trailing-garbage test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T23:18:51Z","receivedAt":"2018-10-30T23:18:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit cce044df7f (fsck: detect trailing garbage in all\nobject types, 2017-01-13) added two tests of trailing\ngarbage in a loose object file: one with a commit and one\nwith a blob. The point of having two is that blobs would\nfollow a different code path that streamed the contents,\ninstead of loading it into a buffer as usual.\n\nAt the time, merely being a blob was enough to trigger the\nstreaming code path. But since 7ac4f3a007 (fsck: actually\nfsck blob data, 2018-05-02), we now only stream blobs that\nare actually large. So since then, the streaming code path\nis not tested at all for this case.\n\nWe can restore the original intent of the test by tweaking\ncore.bigFileThreshold to make our small blob seem large.\nThere's no easy way to externally verify that we followed\nthe streaming code path, but I did check before/after using\na temporary debug statement.\n\nSigned-off-by: Jeff King \n---\nI prepared this series on master, but it occurs to me you may want to\napply patch 2 on top of f6371f9210 or thereabouts, which introduced the\nbug it fixes. If so, then obviously this one doesn't make sense back\nthen, and should go on top of 7ac4f3a007. It should be semantically\nindependent, though there may be a minor text conflict.\n\n t/t1450-fsck.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 0f2dd26f74..3421f12e8a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -673,13 +673,13 @@ test_expect_success 'fsck detects trailing loose garbage (commit)' '\n \ttest_i18ngrep \"garbage.*$commit\" out\n '\n \n-test_expect_success 'fsck detects trailing loose garbage (blob)' '\n+test_expect_success 'fsck detects trailing loose garbage (large blob)' '\n \tblob=$(echo trailing | git hash-object -w --stdin) &&\n \tfile=$(sha1_file $blob) &&\n \ttest_when_finished \"remove_object $blob\" &&\n \tchmod +w \"$file\" &&\n \techo garbage >>\"$file\" &&\n-\ttest_must_fail git fsck 2>out &&\n+\ttest_must_fail git -c core.bigfilethreshold=5 fsck 2>out &&\n \ttest_i18ngrep \"garbage.*$blob\" out\n '\n \n-- \n2.19.1.1235.g6b27db57c2\n\n"},{"id":"362046","messageId":"20181030232312.GB32038@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181030231232.GA6141@sigill.intra.peff.net","subject":"[PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T23:23:12Z","receivedAt":"2018-10-30T23:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This commit fixes an infinite loop when fscking large\ntruncated loose objects.\n\nThe check_stream_sha1() function takes an mmap'd loose\nobject buffer and streams 4k of output at a time, checking\nits sha1. The loop quits when we've output enough bytes (we\nknow the size from the object header), or when zlib tells us\nanything except Z_OK or Z_BUF_ERROR.\n\nThe latter is expected because zlib may run out of room in\nour 4k buffer, and that is how it tells us to process the\noutput and loop again.\n\nBut Z_BUF_ERROR also covers another case: one in which zlib\ncannot make forward progress because it needs more _input_.\nThis should never happen in this loop, because though we're\nstreaming the output, we have the entire deflated input\navailable in the mmap'd buffer. But since we don't check\nthis case, we'll just loop infinitely if we do see a\ntruncated object, thinking that zlib is asking for more\noutput space.\n\nIt's tempting to fix this by checking stream->avail_in as\npart of the loop condition (and quitting if all of our bytes\nhave been consumed). But that assumes that once zlib has\nconsumed the input, there is nothing left to do. That's not\nnecessarily the case: it may have read our input into its\ninternal state, but still have bytes to output.\n\nInstead, let's continue on Z_BUF_ERROR only when we see the\ncase we're expecting: the previous round filled our output\nbuffer completely. If it didn't (and we still saw\nZ_BUF_ERROR), we know something is wrong and should break\nout of the loop.\n\nThe bug comes from commit f6371f9210 (sha1_file: add\nread_loose_object() function, 2017-01-13), which\nreimplemented some of the existing loose object functions.\nSo it's worth checking if this bug was inherited from any of\nthose. The answers seems to be no. The two obvious\ncandidates are both OK:\n\n 1. unpack_sha1_rest(); this doesn't need to loop on\n Z_BUF_ERROR at all, since it allocates the expected\n output buffer in advance (which we can't do since we're\n explicitly streaming here)\n\n 2. check_object_signature(); the streaming path relies on\n the istream interface, which uses read_istream_loose()\n for this case. That function uses a similar \"is our\n output buffer full\" check with Z_BUF_ERROR (which is\n where I stole it from for this patch!)\n\nReported-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \nSigned-off-by: Jeff King \n---\n sha1-file.c | 3 ++-\n t/t1450-fsck.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex dd0b6aa873..2daf7d9935 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -2199,7 +2199,8 @@ static int check_stream_sha1(git_zstream *stream,\n \t * see the comment in unpack_sha1_rest for details.\n \t */\n \twhile (total_read <= size &&\n-\t (status == Z_OK || status == Z_BUF_ERROR)) {\n+\t (status == Z_OK ||\n+\t\t(status == Z_BUF_ERROR && !stream->avail_out))) {\n \t\tstream->next_out = buf;\n \t\tstream->avail_out = sizeof(buf);\n \t\tif (size - total_read < stream->avail_out)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 3421f12e8a..b5677d26a4 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -683,6 +683,25 @@ test_expect_success 'fsck detects trailing loose garbage (large blob)' '\n \ttest_i18ngrep \"garbage.*$blob\" out\n '\n \n+test_expect_success 'fsck detects truncated loose object' '\n+\t# make it big enough that we know we will truncate in the data\n+\t# portion, not the header\n+\ttest-tool genrandom truncate 4096 >file &&\n+\tblob=$(git hash-object -w file) &&\n+\tfile=$(sha1_file $blob) &&\n+\ttest_when_finished \"remove_object $blob\" &&\n+\ttest_copy_bytes 1024 <\"$file\" >tmp &&\n+\trm \"$file\" &&\n+\tmv -f tmp \"$file\" &&\n+\n+\t# check both regular and streaming code paths\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep corrupt.*$blob out &&\n+\n+\ttest_must_fail git -c core.bigfilethreshold=128 fsck 2>out &&\n+\ttest_i18ngrep corrupt.*$blob out\n+'\n+\n # for each of type, we have one version which is referenced by another object\n # (and so while unreachable, not dangling), and another variant which really is\n # dangling.\n-- \n2.19.1.1235.g6b27db57c2\n\n"},{"id":"362047","messageId":"20181030232337.GC32038@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181030231232.GA6141@sigill.intra.peff.net","subject":"[PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-30T23:23:38Z","receivedAt":"2018-10-30T23:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are three ways to convince cat-file to stream a blob:\n\n - cat-file -p $blob\n\n - cat-file blob $blob\n\n - echo $batch | cat-file --batch\n\nIn the first two, we simply exit with the error code of\nstreaw_blob_to_fd(). That means that an error will cause us\nto exit with \"-1\" (which we try to avoid) without printing\nany kind of error message (which is confusing to the user).\n\nInstead, let's match the third case, which calls die() on an\nerror. Unfortunately we cannot be more specific, as\nstream_blob_to_fd() does not tell us whether the problem was\non reading (e.g., a corrupt object) or on writing (e.g.,\nENOSPC). That might be an opportunity for future work, but\nfor now we will at least exit with a sane message and exit\ncode.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 8d97c84725..0d403eb77d 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode,\n \treturn 0;\n }\n \n+static int stream_blob(const struct object_id *oid)\n+{\n+\tif (stream_blob_to_fd(1, oid, NULL, 0))\n+\t\tdie(\"unable to stream %s to stdout\", oid_to_hex(oid));\n+\treturn 0;\n+}\n+\n static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\tint unknown_type)\n {\n@@ -132,7 +139,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t}\n \n \t\tif (type == OBJ_BLOB)\n-\t\t\treturn stream_blob_to_fd(1, &oid, NULL, 0);\n+\t\t\treturn stream_blob(&oid);\n \t\tbuf = read_object_file(&oid, &type, &size);\n \t\tif (!buf)\n \t\t\tdie(\"Cannot read object %s\", obj_name);\n@@ -155,7 +162,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\t\toidcpy(&blob_oid, &oid);\n \n \t\t\tif (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB)\n-\t\t\t\treturn stream_blob_to_fd(1, &blob_oid, NULL, 0);\n+\t\t\t\treturn stream_blob(&blob_oid);\n \t\t\t/*\n \t\t\t * we attempted to dereference a tag to a blob\n \t\t\t * and failed; there may be new dereference\n@@ -319,8 +326,9 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t\tBUG(\"invalid cmdmode: %c\", opt->cmdmode);\n \t\t\tbatch_write(opt, contents, size);\n \t\t\tfree(contents);\n-\t\t} else if (stream_blob_to_fd(1, oid, NULL, 0) < 0)\n-\t\t\tdie(\"unable to stream %s to stdout\", oid_to_hex(oid));\n+\t\t} else {\n+\t\t\tstream_blob(oid);\n+\t\t}\n \t}\n \telse {\n \t\tenum object_type type;\n-- \n2.19.1.1235.g6b27db57c2\n"},{"id":"362058","messageId":"xmqqpnvqyc9x.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181030232312.GB32038@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T04:23:54Z","receivedAt":"2018-10-31T04:23:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The bug comes from commit f6371f9210 (sha1_file: add\n> read_loose_object() function, 2017-01-13), which\n> reimplemented some of the existing loose object functions.\n> So it's worth checking if this bug was inherited from any of\n> those. The answers seems to be no. The two obvious\n> candidates are both OK:\n>\n> 1. unpack_sha1_rest(); this doesn't need to loop on\n> Z_BUF_ERROR at all, since it allocates the expected\n> output buffer in advance (which we can't do since we're\n> explicitly streaming here)\n>\n> 2. check_object_signature(); the streaming path relies on\n> the istream interface, which uses read_istream_loose()\n> for this case. That function uses a similar \"is our\n> output buffer full\" check with Z_BUF_ERROR (which is\n> where I stole it from for this patch!)\n\nSee 692f0bc7 to find who did the fix you stole from, and for what\nkind of breakage the original fix was made.\n\nBy the way, a very similar loop for pack_non_delta istream iterates\nwhile total_read is smaller than sz, but it does not have the same\ncheck upon BUF_ERROR to see if we've read everything.\n\n\n"},{"id":"362062","messageId":"20181031043051.GA5601@sigill.intra.peff.net","threadId":"44833","inReplyTo":"xmqqpnvqyc9x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T04:30:51Z","receivedAt":"2018-10-31T04:30:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 01:23:54PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The bug comes from commit f6371f9210 (sha1_file: add\n> > read_loose_object() function, 2017-01-13), which\n> > reimplemented some of the existing loose object functions.\n> > So it's worth checking if this bug was inherited from any of\n> > those. The answers seems to be no. The two obvious\n> > candidates are both OK:\n> >\n> > 1. unpack_sha1_rest(); this doesn't need to loop on\n> > Z_BUF_ERROR at all, since it allocates the expected\n> > output buffer in advance (which we can't do since we're\n> > explicitly streaming here)\n> >\n> > 2. check_object_signature(); the streaming path relies on\n> > the istream interface, which uses read_istream_loose()\n> > for this case. That function uses a similar \"is our\n> > output buffer full\" check with Z_BUF_ERROR (which is\n> > where I stole it from for this patch!)\n> \n> See 692f0bc7 to find who did the fix you stole from, and for what\n> kind of breakage the original fix was made.\n\nHeh. I did not dig into it, but actually thought \"I'll bet Junio had to\nget this right when he wrote the streaming code. No wonder he spotted my\nmistake so quickly!\".\n\n> By the way, a very similar loop for pack_non_delta istream iterates\n> while total_read is smaller than sz, but it does not have the same\n> check upon BUF_ERROR to see if we've read everything.\n\nIndeed. Did you find that one by inspection, or did you peek at:\n\n https://public-inbox.org/git/20130325202114.GD16019@sigill.intra.peff.net/\n\n? :)\n\n-Peff\n"},{"id":"362064","messageId":"xmqq36smybbq.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181031043051.GA5601@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T04:44:25Z","receivedAt":"2018-10-31T04:44:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> See 692f0bc7 to find who did the fix you stole from, and for what\n>> kind of breakage the original fix was made.\n>\n> Heh. I did not dig into it, but actually thought \"I'll bet Junio had to\n> get this right when he wrote the streaming code. No wonder he spotted my\n> mistake so quickly!\".\n>\n>> By the way, a very similar loop for pack_non_delta istream iterates\n>> while total_read is smaller than sz, but it does not have the same\n>> check upon BUF_ERROR to see if we've read everything.\n>\n> Indeed. Did you find that one by inspection, or did you peek at:\n>\n> https://public-inbox.org/git/20130325202114.GD16019@sigill.intra.peff.net/\n\nI looked for BUF_ERROR in the streaming.c and found two instances in\na very similar looking loop with a subtle differnce, and the\ndifference was due to one of them getting fixed in the past while\nthe other one was left intact as written at its inception.\n\nI should have looked for that message to read the part below\nthree-dash mark. Or we may want to transplant that comment somehow\nto the function so next person will not be puzzled like I did?\n"},{"id":"362067","messageId":"20181031050338.GB5601@sigill.intra.peff.net","threadId":"44833","inReplyTo":"xmqq36smybbq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T05:03:39Z","receivedAt":"2018-10-31T05:03:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 01:44:25PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> See 692f0bc7 to find who did the fix you stole from, and for what\n> >> kind of breakage the original fix was made.\n> >\n> > Heh. I did not dig into it, but actually thought \"I'll bet Junio had to\n> > get this right when he wrote the streaming code. No wonder he spotted my\n> > mistake so quickly!\".\n> >\n> >> By the way, a very similar loop for pack_non_delta istream iterates\n> >> while total_read is smaller than sz, but it does not have the same\n> >> check upon BUF_ERROR to see if we've read everything.\n> >\n> > Indeed. Did you find that one by inspection, or did you peek at:\n> >\n> > https://public-inbox.org/git/20130325202114.GD16019@sigill.intra.peff.net/\n> \n> I looked for BUF_ERROR in the streaming.c and found two instances in\n> a very similar looking loop with a subtle differnce, and the\n> difference was due to one of them getting fixed in the past while\n> the other one was left intact as written at its inception.\n> \n> I should have looked for that message to read the part below\n> three-dash mark. Or we may want to transplant that comment somehow\n> to the function so next person will not be puzzled like I did?\n\nHmm. Reading that function, I am not sure if it actually might need\nfixing.\n\nMight we actually get Z_BUF_ERROR asking for more input if zlib reads to\nthe end of the pack window? That is probably quite unlikely in practice,\nbut in theory you could feed a very large buffer for the output and use\na very small pack window.\n\nSo I do not think we can use the same logic in that loop. But at the\nsame time, what prevents use_pack() from getting to the very end of the\npack and saying \"I have no bytes left for you\"? And then we'd loop\ninfinitely, feeding zlib nothing.\n\nI'm not sure what the solution is. I do not think this works:\n\ndiff --git a/streaming.c b/streaming.c\nindex d1e6b2dce6..a92a85ed10 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -394,6 +394,9 @@ static read_method_decl(pack_non_delta)\n \t\tmapped = use_pack(st->u.in_pack.pack, &window,\n \t\t\t\t st->u.in_pack.pos, &st->z.avail_in);\n \n+\t\tif (!st->z.avail_in)\n+\t\t\tbreak;\n+\n \t\tst->z.next_out = (unsigned char *)buf + total_read;\n \t\tst->z.avail_out = sz - total_read;\n \t\tst->z.next_in = mapped;\n\nbecause we may have read to the very end but still have bytes to output.\n\nThough hrm. I think use_pack() will always tell us about the trailing\n20-byte hash in the \"avail\" window. Which means we should never\nlegitimately get to 0 there, because it means that either:\n\n 1. We're reading the trailing hash, which cannot possibly be right (in\n most cases I'd expect zlib to barf at that point anyway, but of\n course it's possible to have a hash that is valid zlib data ;) ).\n\n 2. We're truncated _before_ the hash, so we really did read to EOF,\n and there are no more bytes. I suspect we may actually detect this\n case upon opening the pack (since we do peek at the trailer then),\n but again that could be fooled by coincidence.\n\nI guess that's not the whole story, though. use_pack() tries to promise\nat least 20 bytes (to simplify some of the other parsing routines). So\nwe shouldn't actually ever get \"0\" here. If we really are that close to\nthe end of the pack, we'd hit this logic in use_pack:\n\n if (offset > (p->pack_size - the_hash_algo->rawsz))\n\tdie(\"offset beyond end of packfile (truncated pack?)\");\n\nSo actually, I think this code is OK as-is. We will always have at least\n20 bytes of input, or use_pack() will die.\n\nPhew. I almost just deleted all of the above, because now I think I'm\nready to write that comment you asked for. ;) But I left it since maybe\nit makes sense to follow my thought process.\n\n-Peff\n"},{"id":"362069","messageId":"20181031051316.GC5601@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181031050338.GB5601@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T05:13:16Z","receivedAt":"2018-10-31T05:13:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 01:03:39AM -0400, Jeff King wrote:\n\n> Phew. I almost just deleted all of the above, because now I think I'm\n> ready to write that comment you asked for. ;) But I left it since maybe\n> it makes sense to follow my thought process.\n\nSo here it is in a more succinct form.\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] read_istream_pack_non_delta(): document input handling\n\nTwice now we have scratched our heads about why the loose streaming code\nneeds the protection added by 692f0bc7ae (avoid infinite loop in\nread_istream_loose, 2013-03-25), but the similar code in its pack\ncounterpart does not.\n\nThe short answer is that use_pack() will die before it lets us run out\nof bytes. Note that this could mean reading garbage (including the\ntrailing hash) from the packfile in some cases of corruption, but that's\nOK. zlib will notice and complain (and if not, certainly the end result\nwill not match the object hash we expect).\n\nLet's leave a comment this time to document our findings.\n\nSigned-off-by: Jeff King \n---\n streaming.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/streaming.c b/streaming.c\nindex d1e6b2dce6..ac7c7a22f9 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -408,6 +408,15 @@ static read_method_decl(pack_non_delta)\n \t\t\tst->z_state = z_done;\n \t\t\tbreak;\n \t\t}\n+\n+\t\t/*\n+\t\t * Unlike the loose object case, we do not have to worry here\n+\t\t * about running out of input bytes and spinning infinitely. If\n+\t\t * we get Z_BUF_ERROR due to too few input bytes, then we'll\n+\t\t * replenish them in the next use_pack() call when we loop. If\n+\t\t * we truly hit the end of the pack (i.e., because it's corrupt\n+\t\t * or truncated), then use_pack() catches that and will die().\n+\t\t */\n \t\tif (status != Z_OK && status != Z_BUF_ERROR) {\n \t\t\tgit_inflate_end(&st->z);\n \t\t\tst->z_state = z_error;\n-- \n2.19.1.1298.g19f18f2a22\n\n"},{"id":"362070","messageId":"xmqqk1lywulg.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181031051316.GC5601@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] check_stream_sha1(): handle input underflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T05:31:07Z","receivedAt":"2018-10-31T05:31:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Oct 31, 2018 at 01:03:39AM -0400, Jeff King wrote:\n>\n>> Phew. I almost just deleted all of the above, because now I think I'm\n>> ready to write that comment you asked for. ;) But I left it since maybe\n>> it makes sense to follow my thought process.\n>\n> So here it is in a more succinct form.\n\nThanks.\n\n> +\n> +\t\t/*\n> +\t\t * Unlike the loose object case, we do not have to worry here\n> +\t\t * about running out of input bytes and spinning infinitely. If\n> +\t\t * we get Z_BUF_ERROR due to too few input bytes, then we'll\n> +\t\t * replenish them in the next use_pack() call when we loop. If\n> +\t\t * we truly hit the end of the pack (i.e., because it's corrupt\n> +\t\t * or truncated), then use_pack() catches that and will die().\n> +\t\t */\n> \t\tif (status != Z_OK && status != Z_BUF_ERROR) {\n> \t\t\tgit_inflate_end(&st->z);\n> \t\t\tst->z_state = z_error;\n\nReads well. Will apply.\n"},{"id":"362097","messageId":"20181031124208.29451-1-avarab@gmail.com","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"[PATCH 0/3] Add a GIT_TEST_FSCK test mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-31T12:42:05Z","receivedAt":"2018-10-31T12:42:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This goes on top Jeff's \"cat-file: handle streaming failures\nconsistently\" and implements the test mode I suggested in\nhttps://public-inbox.org/git/877ehzksjd.fsf@evledraar.gmail.com/\n\nIn the process I didn't find any other bugs than the 2.12..2.19\nregression which is already fixed, but as noted in 3/3 I think it's\nworth it to stress test fsck like this. I'll be adding this to my\nregular build.\n\nÆvar Arnfjörð Bjarmason (3):\n tests: add a \"env-bool\" helper to test-tool\n tests: mark those tests where \"git fsck\" fails at the end\n tests: add a special test setup that runs \"git fsck\" before exiting\n\n Makefile | 1 +\n t/README | 5 ++++\n t/helper/test-env-bool.c | 9 +++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0000-basic.sh | 26 +++++++++++++++++++\n t/t1006-cat-file.sh | 5 ++++\n t/t1305-config-include.sh | 4 +++\n t/t1404-update-ref-errors.sh | 4 +++\n t/t1410-reflog.sh | 4 +++\n t/t1515-rev-parse-outside-repo.sh | 4 +++\n t/t3008-ls-files-lazy-init-name-hash.sh | 4 +++\n t/t3103-ls-tree-misc.sh | 6 +++++\n t/t3430-rebase-merges.sh | 6 +++++\n t/t4046-diff-unmerged.sh | 4 +++\n t/t4058-diff-duplicates.sh | 5 ++++\n t/t4212-log-corrupt.sh | 6 +++++\n t/t5000-tar-tree.sh | 5 ++++\n t/t5300-pack-object.sh | 5 ++++\n t/t5303-pack-corruption-resilience.sh | 8 ++++++\n t/t5307-pack-missing-commit.sh | 7 ++++++\n t/t5312-prune-corruption.sh | 4 +++\n t/t5504-fetch-receive-strict.sh | 4 +++\n t/t5601-clone.sh | 8 ++++++\n t/t6007-rev-list-cherry-pick-file.sh | 4 +++\n t/t6011-rev-list-with-bad-commit.sh | 7 ++++++\n t/t6030-bisect-porcelain.sh | 6 +++++\n t/t7007-show.sh | 6 +++++\n t/t7106-reset-unborn-branch.sh | 4 +++\n t/t7415-submodule-names.sh | 4 +++\n t/t7416-submodule-dash-url.sh | 4 +++\n t/t7417-submodule-path-url.sh | 4 +++\n t/t7509-commit-authorship.sh | 4 +++\n t/t8003-blame-corner-cases.sh | 4 +++\n t/t9130-git-svn-authors-file.sh | 7 ++++++\n t/test-lib-functions.sh | 2 ++\n t/test-lib.sh | 33 +++++++++++++++++++++++++\n 37 files changed, 225 insertions(+)\n create mode 100644 t/helper/test-env-bool.c\n\n-- \n2.19.1.899.g0250525e69\n\n"},{"id":"362098","messageId":"20181031124208.29451-2-avarab@gmail.com","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"[PATCH 1/3] tests: add a \"env-bool\" helper to test-tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-31T12:42:06Z","receivedAt":"2018-10-31T12:42:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This new helper is a wrapper around the git_env_bool() function. There\nare various GIT_TEST_* variables described in \"Running tests with\nspecial setups\" in t/README that use git_env_bool().\n\nA GIT_TEST_* variable implemented in shellscript won't have access to\nthe same semantics (historically we've used \"test -n\" for many of\nthese).\n\nSo let's add this helper so we can expose the same environment\nvariable behavior without exposing the implementation detail of\nwhether that variable happens to be checked in C or shellscript.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 1 +\n t/helper/test-env-bool.c | 9 +++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n 4 files changed, 12 insertions(+)\n create mode 100644 t/helper/test-env-bool.c\n\ndiff --git a/Makefile b/Makefile\nindex b08d5ea258..ca624c381f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-example-decorate.o\n+TEST_BUILTINS_OBJS += test-env-bool.o\n TEST_BUILTINS_OBJS += test-genrandom.o\n TEST_BUILTINS_OBJS += test-hashmap.o\n TEST_BUILTINS_OBJS += test-index-version.o\ndiff --git a/t/helper/test-env-bool.c b/t/helper/test-env-bool.c\nnew file mode 100644\nindex 0000000000..956b0aa88e\n--- /dev/null\n+++ b/t/helper/test-env-bool.c\n@@ -0,0 +1,9 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"config.h\"\n+\n+int cmd__env_bool(int argc, const char **argv)\n+{\n+\tassert(argc == 2);\n+\treturn !git_env_bool(argv[1], 0);\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 5df8b682aa..c4481085c4 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -17,6 +17,7 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-fsmonitor\", cmd__dump_fsmonitor },\n \t{ \"dump-split-index\", cmd__dump_split_index },\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n+\t{ \"env-bool\", cmd__env_bool },\n \t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"genrandom\", cmd__genrandom },\n \t{ \"hashmap\", cmd__hashmap },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 71f470b871..f7845fbc56 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -13,6 +13,7 @@ int cmd__dump_cache_tree(int argc, const char **argv);\n int cmd__dump_fsmonitor(int argc, const char **argv);\n int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n+int cmd__env_bool(int argc, const char **argv);\n int cmd__example_decorate(int argc, const char **argv);\n int cmd__genrandom(int argc, const char **argv);\n int cmd__hashmap(int argc, const char **argv);\n-- \n2.19.1.899.g0250525e69\n\n"},{"id":"362099","messageId":"20181031124208.29451-3-avarab@gmail.com","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"[PATCH 2/3] tests: mark those tests where \"git fsck\" fails at the end","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-31T12:42:07Z","receivedAt":"2018-10-31T12:42:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Mark the tests where \"git fsck\" fails at the end with extra test code\nto check the fsck output. There fsck.{err,out} has been created for\nus.\n\nA later change will add the support for GIT_TEST_FSCK_TESTS. They're\nbeing added first to ensure the test suite will never fail with\nGIT_TEST_FSCK=true during bisect.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1006-cat-file.sh | 5 +++++\n t/t1305-config-include.sh | 4 ++++\n t/t1404-update-ref-errors.sh | 4 ++++\n t/t1410-reflog.sh | 4 ++++\n t/t1515-rev-parse-outside-repo.sh | 4 ++++\n t/t3008-ls-files-lazy-init-name-hash.sh | 4 ++++\n t/t3103-ls-tree-misc.sh | 6 ++++++\n t/t3430-rebase-merges.sh | 6 ++++++\n t/t4046-diff-unmerged.sh | 4 ++++\n t/t4058-diff-duplicates.sh | 5 +++++\n t/t4212-log-corrupt.sh | 6 ++++++\n t/t5000-tar-tree.sh | 5 +++++\n t/t5300-pack-object.sh | 5 +++++\n t/t5303-pack-corruption-resilience.sh | 8 ++++++++\n t/t5307-pack-missing-commit.sh | 7 +++++++\n t/t5312-prune-corruption.sh | 4 ++++\n t/t5504-fetch-receive-strict.sh | 4 ++++\n t/t5601-clone.sh | 8 ++++++++\n t/t6007-rev-list-cherry-pick-file.sh | 4 ++++\n t/t6011-rev-list-with-bad-commit.sh | 7 +++++++\n t/t6030-bisect-porcelain.sh | 6 ++++++\n t/t7007-show.sh | 6 ++++++\n t/t7106-reset-unborn-branch.sh | 4 ++++\n t/t7415-submodule-names.sh | 4 ++++\n t/t7416-submodule-dash-url.sh | 4 ++++\n t/t7417-submodule-path-url.sh | 4 ++++\n t/t7509-commit-authorship.sh | 4 ++++\n t/t8003-blame-corner-cases.sh | 4 ++++\n t/t9130-git-svn-authors-file.sh | 7 +++++++\n 29 files changed, 147 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 43c4be1e5e..12b69e6fbe 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -588,4 +588,9 @@ test_expect_success 'cat-file --unordered works' '\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"unable to unpack header of\" fsck.err &&\n+\ttest_i18ngrep \"object corrupt or missing\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 635918505d..890d307d4e 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -313,4 +313,8 @@ test_expect_success 'include cycles are detected' '\n \ttest_i18ngrep \"exceeded maximum include depth\" stderr\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"exceeded maximum include depth\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 51a4f4c0ac..6095b2d4b9 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -618,4 +618,8 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '\n \ttest_cmp unchanged actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid sha1 pointer\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 388b0611d8..43b8e0c9c5 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -368,4 +368,8 @@ test_expect_success 'continue walking past root commits' '\n \t)\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid reflog entry\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh\nindex 3ec2971ee5..1d8fc3ad70 100755\n--- a/t/t1515-rev-parse-outside-repo.sh\n+++ b/t/t1515-rev-parse-outside-repo.sh\n@@ -42,4 +42,8 @@ test_expect_success 'rev-parse --resolve-git-dir' '\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"not a git repository\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t3008-ls-files-lazy-init-name-hash.sh b/t/t3008-ls-files-lazy-init-name-hash.sh\nindex 64f047332b..7fb2e5c177 100755\n--- a/t/t3008-ls-files-lazy-init-name-hash.sh\n+++ b/t/t3008-ls-files-lazy-init-name-hash.sh\n@@ -24,4 +24,8 @@ test_expect_success 'no buffer overflow in lazy_init_name_hash' '\n \ttest-tool lazy-init-name-hash -m\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"notice: No default references\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh\nindex 14520913af..b7d8ae2e81 100755\n--- a/t/t3103-ls-tree-misc.sh\n+++ b/t/t3103-ls-tree-misc.sh\n@@ -22,4 +22,10 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n \ttest_must_fail git ls-tree -r HEAD\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid sha1 pointer in cache-tree\" fsck.err &&\n+\ttest_i18ngrep \"broken link from.*tree\" fsck.out &&\n+\ttest_i18ngrep \"missing tree\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex aa7bfc88ec..efac3a792b 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -396,4 +396,10 @@ test_expect_success 'with --autosquash and --exec' '\n \tgrep \"G: +G\" actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"broken link from.*commit\" fsck.out &&\n+\ttest_i18ngrep \"to.*tree\" fsck.out &&\n+\ttest_i18ngrep \"missing tree\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh\nindex ff7cfd884a..d868bc44a9 100755\n--- a/t/t4046-diff-unmerged.sh\n+++ b/t/t4046-diff-unmerged.sh\n@@ -84,4 +84,8 @@ test_expect_success 'diff-files -3' '\n \ttest_cmp diff-files-3.expect diff-files-3.actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"notice: No default references\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex c24ee175ef..9c79410dc0 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -76,4 +76,9 @@ test_expect_success 'diff-tree with renames' '\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"zeroPaddedFilemode\" fsck.err &&\n+\ttest_i18ngrep \"duplicateEntries\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 03b952c90d..5f36c58a61 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -85,4 +85,10 @@ test_expect_success 'absurdly far-in-future date' '\n \tgit log -1 --format=%ad $commit\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"badDate\" fsck.err &&\n+\ttest_i18ngrep \"badDateOverflow\" fsck.err &&\n+\ttest_i18ngrep \"missingSpaceBeforeDate\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..88c768f232 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -408,4 +408,9 @@ test_expect_success TAR_HUGE,TIME_IS_64BIT,TIME_T_IS_64BIT 'system tar can read\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"corrupt loose object\" fsck.err &&\n+\ttest_i18ngrep \"object corrupt or missing\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 6c620cd540..b4ef9a447a 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -493,4 +493,9 @@ test_expect_success \\\n 'test_must_fail git -c core.bigfilethreshold=1 index-pack -o bad.idx test-3.pack 2>msg &&\n test_i18ngrep \"SHA1 COLLISION FOUND\" msg'\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"sha1 mismatch for\" fsck.err &&\n+\ttest_i18ngrep \"object corrupt or missing\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 41e6dc4dcf..79c9b307d0 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -400,4 +400,12 @@ test_expect_success \\\n 'printf \"\\0\\1\\1X\\0\" > tail_garbage_opcode &&\n test_must_fail test-tool delta -p /dev/null tail_garbage_opcode /dev/null'\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"pack checksum mismatch\" fsck.err &&\n+\ttest_i18ngrep \"index CRC mismatch.*at offset 12\" fsck.err &&\n+\ttest_i18ngrep \"cannot unpack.*at offset 12\" fsck.err &&\n+\ttest_i18ngrep \"failed to read delta base object.*at offset 12\" fsck.err &&\n+\ttest_i18ngrep \"failed to read delta base object.*at offset 2032\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh\nindex dacb440b27..2780f4ceeb 100755\n--- a/t/t5307-pack-missing-commit.sh\n+++ b/t/t5307-pack-missing-commit.sh\n@@ -36,4 +36,11 @@ test_expect_success 'pack-objects notices corruption' '\n \ttest_must_fail git pack-objects --revs pack\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid sha1 pointer\" fsck.err &&\n+\ttest_i18ngrep \"broken link from.*commit\" fsck.out &&\n+\ttest_i18ngrep \"to.*commit\" fsck.out &&\n+\ttest_i18ngrep \"missing commit\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex da9d59940d..898d8906bc 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -111,4 +111,8 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid sha1 pointer\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 7bc706873c..945a060992 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -348,4 +348,8 @@ test_expect_success \\\n \tgrep \"Cannot demote unterminatedheader\" act\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"missingEmail\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex f1a49e94f5..35eca5881b 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -726,6 +726,14 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\n '\n \n+# We might have \"test_done\" through lib-httpd.sh. Need to tes\n+# GIT_TEST_FSCK_TESTS here.\n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"object.*is a tree, not a blob\" fsck.err &&\n+\ttest_i18ngrep \"object.*is a commit, not a blob\" fsck.err &&\n+\ttest_i18ngrep \"error in tree.*: broken links\" fsck.err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh\nindex f0268372d2..a86ba0900b 100755\n--- a/t/t6007-rev-list-cherry-pick-file.sh\n+++ b/t/t6007-rev-list-cherry-pick-file.sh\n@@ -266,4 +266,8 @@ test_expect_success '--cherry-pick avoids looking at full diffs' '\n \tgit rev-list --cherry-pick ...shy-diff\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"missing blob\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\nindex 545b461e51..30d39ce925 100755\n--- a/t/t6011-rev-list-with-bad-commit.sh\n+++ b/t/t6011-rev-list-with-bad-commit.sh\n@@ -55,5 +55,12 @@ test_expect_success 'first commit is still available' \\\n git log $first_commit\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"pack checksum mismatch\" fsck.err &&\n+\ttest_i18ngrep \"index CRC mismatch for object.*at offset 487\" fsck.err &&\n+\ttest_i18ngrep \"inflate: data stream error.*incorrect data check\" fsck.err &&\n+\ttest_i18ngrep \"cannot unpack.*at offset 487\" fsck.err\n+'\n+\n test_done\n \ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex f84ff941c3..5da668ed06 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -911,4 +911,10 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"broken link from.*tree\" fsck.out &&\n+\ttest_i18ngrep \"to.*tree\" fsck.out &&\n+\ttest_i18ngrep \"missing tree\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex 42d3db6246..d25cee8a72 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -128,4 +128,10 @@ test_expect_success 'show --graph is forbidden' '\n test_must_fail git show --graph HEAD\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"broken link from.*tag\" fsck.out &&\n+\ttest_i18ngrep \"to.*blob\" fsck.out &&\n+\ttest_i18ngrep \"missing blob\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh\nindex ecb85c3b82..0719261fe7 100755\n--- a/t/t7106-reset-unborn-branch.sh\n+++ b/t/t7106-reset-unborn-branch.sh\n@@ -64,4 +64,8 @@ test_expect_success 'reset --hard' '\n \ttest_path_is_missing a\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"missing tree\" fsck.out\n+'\n+\n test_done\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex 293e2e1963..3d8fa7831f 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -191,4 +191,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n \t)\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"gitmodulesName\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh\nindex 1cd2c1c1ea..e6c885784e 100755\n--- a/t/t7416-submodule-dash-url.sh\n+++ b/t/t7416-submodule-dash-url.sh\n@@ -46,4 +46,8 @@ test_expect_success 'fsck rejects unprotected dash' '\n \tgrep gitmodulesUrl err\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"gitmodulesUrl\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh\nindex 756af8c4d6..8362442908 100755\n--- a/t/t7417-submodule-path-url.sh\n+++ b/t/t7417-submodule-path-url.sh\n@@ -25,4 +25,8 @@ test_expect_success 'fsck rejects unprotected dash' '\n \tgrep gitmodulesPath err\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"gitmodulesPath\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t7509-commit-authorship.sh b/t/t7509-commit-authorship.sh\nindex 500ab2fe72..cdcbfed61a 100755\n--- a/t/t7509-commit-authorship.sh\n+++ b/t/t7509-commit-authorship.sh\n@@ -174,4 +174,8 @@ test_expect_success '--reset-author with CHERRY_PICK_HEAD' '\n \ttest_cmp expect actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"invalid reflog entry\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex c92a47b6d5..3a8affc3e7 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -275,4 +275,8 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '\n \tgrep \"A U Thor\" actual\n '\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"missingNameBeforeEmail\" fsck.err\n+'\n+\n test_done\ndiff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh\nindex cb764bcadc..0c0c42c72c 100755\n--- a/t/t9130-git-svn-authors-file.sh\n+++ b/t/t9130-git-svn-authors-file.sh\n@@ -128,4 +128,11 @@ test_expect_success 'authors-file imported user without email' '\n \n test_debug 'GIT_DIR=gitconfig.clone/.git git log'\n \n+GIT_TEST_FSCK_TESTS='\n+\ttest_i18ngrep \"object.*is a tree, not a blob\" fsck.err &&\n+\ttest_i18ngrep \"object.*is a commit, not a blob\" fsck.err &&\n+\ttest_i18ngrep \"tree.*: broken links\" fsck.err &&\n+\ttest_i18ngrep \"missingTaggerEntry\" fsck.err\n+'\n+\n test_done\n-- \n2.19.1.899.g0250525e69\n\n"},{"id":"362100","messageId":"20181031124208.29451-4-avarab@gmail.com","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"[PATCH 3/3] tests: add a special test setup that runs \"git fsck\" before exiting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-31T12:42:08Z","receivedAt":"2018-10-31T12:42:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add the ability to run the tests with GIT_TEST_FSCK=true in the\nenvironment. If set we'll run \"git fsck\" at the end of every test, and\nthose tests that fail need to annotate what their failure was.\n\nThe goal is to detect regressions in fsck that our tests might\notherwise miss. We had one such regression in c68b489e56 (\"fsck: parse\nloose object paths directly\", 2017-01-13) released with Git 2.12.0,\nwhich wasn't spotted more than a year and a half later during the\n2.20.0 window.\n\nAs it turns out there already was a test for what triggerd that bug\nall along in the form of t5000-tar-tree.sh, we just weren't running\n\"git fsck\" at the end[1].\n\nThat specific bug has been fixed in (\"check_stream_sha1(): handle\ninput underflow\", 2018-10-30)[1], but since we have a demonstrable\nhistory of not anticipating which tests which would make \"git fsck\"\nfail need to be made part of the \"git fsck\" test suite let's add this\ntest mode to cover potential blind spots. The \"git fsck\" command is\nalso something where we might expect that during our RC windows users\naren't actively testing on already corrupt repositories, so \"in the\nwild\" test coverage will be spotty, so we need all the help we can\nget.\n\n1. https://public-inbox.org/git/878t2fkxrn.fsf@evledraar.gmail.com/\n2. https://public-inbox.org/git/20181030232312.GB32038@sigill.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 5 +++++\n t/t0000-basic.sh | 26 ++++++++++++++++++++++++++\n t/test-lib-functions.sh | 2 ++\n t/test-lib.sh | 33 +++++++++++++++++++++++++++++++++\n 4 files changed, 66 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 8847489640..092f78b3d7 100644\n--- a/t/README\n+++ b/t/README\n@@ -343,6 +343,11 @@ of the index for the whole test suite by bypassing the default number of\n cache entries and thread minimums. Setting this to 1 will make the\n index loading single threaded.\n \n+GIT_TEST_FSCK= if true arranges for \"git fsck\" to be run at\n+the end of the test scripts. Those tests that fail will need to set a\n+\"GIT_TEST_FSCK_TESTS\" variable before we enter \"test_done\" with a test\n+fragment to test that fsck.{out,err} is the expected failure.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4d23373526..8e667e6691 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -19,6 +19,7 @@ modification *should* take notice and update the test vectors here.\n '\n \n . ./test-lib.sh\n+unset GIT_TEST_FSCK\n \n try_local_x () {\n \tlocal x=\"local\" &&\n@@ -393,6 +394,31 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' \"\n \t)\n \"\n \n+test_expect_success 'GIT_TEST_FSCK=true' \"\n+\ttest_when_finished 'sane_unset GIT_TEST_FSCK' &&\n+\tGIT_TEST_FSCK=true &&\n+\texport GIT_TEST_FSCK &&\n+\trun_sub_test_lib_test run-git-fsck-test \\\n+\t\t'--run basic' --run='1 3 5' <<-\\\\EOF &&\n+\tfor i in 1 2 3 4 5 6\n+\tdo\n+\t\ttest_expect_success \\\"passing test #\\$i\\\" 'true'\n+\tdone\n+\tGIT_TEST_FSCK=true test_done\n+\tEOF\n+\tcheck_sub_test_lib_test run-git-fsck-test <<-\\\\EOF\n+\t> ok 1 - passing test #1\n+\t> ok 2 # skip passing test #2 (--run)\n+\t> ok 3 - passing test #3\n+\t> ok 4 # skip passing test #4 (--run)\n+\t> ok 5 - passing test #5\n+\t> ok 6 # skip passing test #6 (--run)\n+\t> ok 7 # skip git fsck at end (due to GIT_TEST_FSCK) (expected to succeed) (--run)\n+\t> # passed all 7 test(s)\n+\t> 1..7\n+\tEOF\n+\"\n+\n test_expect_success '--run basic' \"\n \trun_sub_test_lib_test run-basic \\\n \t\t'--run basic' --run='1 3 5' <<-\\\\EOF &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..7d002ff5aa 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -470,6 +470,7 @@ test_expect_success () {\n # Usage: test_external description command arguments...\n # Example: test_external 'Perl API' perl ../path/to/test.pl\n test_external () {\n+\tunset GIT_TEST_FSCK\n \ttest \"$#\" = 4 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 3 ||\n \terror >&5 \"bug in the test script: not 3 or 4 parameters to test_external\"\n@@ -511,6 +512,7 @@ test_external () {\n # Like test_external, but in addition tests that the command generated\n # no output on stderr.\n test_external_without_stderr () {\n+\tunset GIT_TEST_FSCK\n \t# The temporary file has no (and must have no) security\n \t# implications.\n \ttmp=${TMPDIR:-/tmp}\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 897e6fcc94..5f7f5595e3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -454,6 +454,8 @@ GIT_EXIT_OK=\n trap 'die' EXIT\n trap 'exit $?' INT\n \n+GIT_TEST_FSCK_TESTS=\n+\n # The user-facing functions are loaded from a separate file so that\n # test_perf subshells can have them too\n . \"$TEST_DIRECTORY/test-lib-functions.sh\"\n@@ -789,7 +791,36 @@ test_at_end_hook_ () {\n \t:\n }\n \n+_test_done_fsck() {\n+\tdesc='git fsck at end (due to GIT_TEST_FSCK)'\n+\tif test -n \"$GIT_TEST_FSCK_TESTS\"\n+\tthen\n+\t\ttest_expect_success \"$desc (expected to fail)\" '\n+\t\t\ttest_must_fail git fsck 2>fsck.err >fsck.out\n+\t\t'\n+\t\ttest_expect_success \"$desc (expected to fail) -- assert failure mode\" \"\n+\t\t\ttest_path_exists fsck.err &&\n+\t\t\ttest_path_exists fsck.out &&\n+\t\t\t$GIT_TEST_FSCK_TESTS\n+\t\t\"\n+\telse\n+\t\ttest_expect_success \"$desc (expected to succeed)\" '\n+\t\t\tgit fsck\n+\t\t'\n+\tfi\n+}\n+\n test_done () {\n+\t# Don't want to run this under TEST_NO_CREATE_REPO, otherwise\n+\t# we end up sloowly running \"git fsck\" against git.git\n+\tif test -z \"$TEST_NO_CREATE_REPO\" &&\n+\t\t # test -n first so all --verbose output isn't\n+\t\t # polluted with this check\n+\t\t test -n \"$GIT_TEST_FSCK\" &&\n+\t\t test_have_prereq TEST_FSCK\n+\tthen\n+\t\t_test_done_fsck\n+\tfi\n \tGIT_EXIT_OK=t\n \n \tif test -z \"$HARNESS_ACTIVE\"\n@@ -1268,3 +1299,5 @@ test_lazy_prereq CURL '\n test_lazy_prereq SHA1 '\n \ttest $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n '\n+\n+test_lazy_prereq TEST_FSCK 'test-tool env-bool GIT_TEST_FSCK'\n-- \n2.19.1.899.g0250525e69\n\n"},{"id":"362103","messageId":"20181031133341.GA5070@tor.lan","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-10-31T13:33:41Z","receivedAt":"2018-10-31T13:34:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote:\n> There are three ways to convince cat-file to stream a blob:\n> \n> - cat-file -p $blob\n> \n> - cat-file blob $blob\n> \n> - echo $batch | cat-file --batch\n> \n> In the first two, we simply exit with the error code of\n> streaw_blob_to_fd(). That means that an error will cause us\n> to exit with \"-1\" (which we try to avoid) without printing\n> any kind of error message (which is confusing to the user).\n> \n> Instead, let's match the third case, which calls die() on an\n> error. Unfortunately we cannot be more specific, as\n> stream_blob_to_fd() does not tell us whether the problem was\n> on reading (e.g., a corrupt object) or on writing (e.g.,\n> ENOSPC). That might be an opportunity for future work, but\n> for now we will at least exit with a sane message and exit\n> code.\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/cat-file.c | 16 ++++++++++++----\n> 1 file changed, 12 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 8d97c84725..0d403eb77d 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode,\n> \treturn 0;\n> }\n> \n> +static int stream_blob(const struct object_id *oid)\n\nSorry for nit-picking:\ncould this be renamed into stream_blob_to_stdout() ?\n\n> +{\n> +\tif (stream_blob_to_fd(1, oid, NULL, 0))\n\nAnd I wonder if we could make things clearer:\n s/1/STDOUT_FILENO/\n \n (Stolen from fast-import.c)\n\n> +\t\tdie(\"unable to stream %s to stdout\", oid_to_hex(oid));\n> +\treturn 0;\n> +}\n> +\n[]\n"},{"id":"362108","messageId":"xmqqva5iurd7.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181031133341.GA5070@tor.lan","subject":"Re: [PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T14:23:48Z","receivedAt":"2018-10-31T14:23:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> +static int stream_blob(const struct object_id *oid)\n>\n> Sorry for nit-picking:\n> could this be renamed into stream_blob_to_stdout() ?\n\nI think that name makes sense, even though stream_blob() is just\nfine for a fuction that takes a single parameter oid, as there is no\nother sane choice than streaming to the standard output stream the\nblob data.\n\n>> +{\n>> +\tif (stream_blob_to_fd(1, oid, NULL, 0))\n>\n> And I wonder if we could make things clearer:\n> s/1/STDOUT_FILENO/\n\nWhat would benefit from symbolic constant more in that function call\nmay be CAN_SEEK thing, but s/1/STDOUT_FILENO/ adds negative value to\nthat line, I would think. The name of the function already makes it\nclear this is sending the output to a file descriptor, and an\ninteger 1 that specifies a file descriptor cannot mean anything\nother than the standard output stream.\n"},{"id":"362109","messageId":"20181031143747.GA9994@sigill.intra.peff.net","threadId":"44833","inReplyTo":"xmqqva5iurd7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T14:37:47Z","receivedAt":"2018-10-31T14:37:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 11:23:48PM +0900, Junio C Hamano wrote:\n\n> Torsten Bögershausen writes:\n> \n> >> +static int stream_blob(const struct object_id *oid)\n> >\n> > Sorry for nit-picking:\n> > could this be renamed into stream_blob_to_stdout() ?\n> \n> I think that name makes sense, even though stream_blob() is just\n> fine for a fuction that takes a single parameter oid, as there is no\n> other sane choice than streaming to the standard output stream the\n> blob data.\n\nI was trying to keep the name small since it is a static-local\nconvenience helper. I'd rather write it as:\n\n stream_blob(1, oid);\n\nthan change the name. ;)\n\n> >> +{\n> >> +\tif (stream_blob_to_fd(1, oid, NULL, 0))\n> >\n> > And I wonder if we could make things clearer:\n> > s/1/STDOUT_FILENO/\n> \n> What would benefit from symbolic constant more in that function call\n> may be CAN_SEEK thing, but s/1/STDOUT_FILENO/ adds negative value to\n> that line, I would think. The name of the function already makes it\n> clear this is sending the output to a file descriptor, and an\n> integer 1 that specifies a file descriptor cannot mean anything\n> other than the standard output stream.\n\nYes, I'd agree (there are very few cases where I think STDOUT_FILENO\nactually increases the readability, since it is usually pretty clear\nfrom the context when something is a descriptor).\n\n-Peff\n"},{"id":"362116","messageId":"20181031173859.GA717@flurp.local","threadId":"44833","inReplyTo":"20181030232337.GC32038@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-31T17:38:59Z","receivedAt":"2018-10-31T17:39:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote:\n> There are three ways to convince cat-file to stream a blob:\n> \n> - cat-file -p $blob\n> \n> - cat-file blob $blob\n> \n> - echo $batch | cat-file --batch\n> \n> In the first two, we simply exit with the error code of\n> streaw_blob_to_fd(). That means that an error will cause us\n\nYour \"m\" got confused and ended up upside-down.\n\n> to exit with \"-1\" (which we try to avoid) without printing\n> any kind of error message (which is confusing to the user).\n> \n> Instead, let's match the third case, which calls die() on an\n> error. Unfortunately we cannot be more specific, as\n> stream_blob_to_fd() does not tell us whether the problem was\n> on reading (e.g., a corrupt object) or on writing (e.g.,\n> ENOSPC). That might be an opportunity for future work, but\n> for now we will at least exit with a sane message and exit\n> code.\n> \n> Signed-off-by: Jeff King \n"},{"id":"362129","messageId":"20181031202930.GB13021@sigill.intra.peff.net","threadId":"44833","inReplyTo":"20181031173859.GA717@flurp.local","subject":"Re: [PATCH 3/3] cat-file: handle streaming failures consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T20:29:30Z","receivedAt":"2018-10-31T20:29:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2018 at 01:38:59PM -0400, Eric Sunshine wrote:\n\n> On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote:\n> > There are three ways to convince cat-file to stream a blob:\n> > \n> > - cat-file -p $blob\n> > \n> > - cat-file blob $blob\n> > \n> > - echo $batch | cat-file --batch\n> > \n> > In the first two, we simply exit with the error code of\n> > streaw_blob_to_fd(). That means that an error will cause us\n> \n> Your \"m\" got confused and ended up upside-down.\n\nHeh. I'm not sure how I managed that. They're not exactly next to each\nother on a qwerty keyboard.\n\n-Peff\n"},{"id":"362146","messageId":"xmqq36slv56r.fsf@gitster-ct.c.googlers.com","threadId":"44833","inReplyTo":"20181031124208.29451-3-avarab@gmail.com","subject":"Re: [PATCH 2/3] tests: mark those tests where \"git fsck\" fails at the end","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-01T03:37:32Z","receivedAt":"2018-11-01T03:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Mark the tests where \"git fsck\" fails at the end with extra test code\n> to check the fsck output. There fsck.{err,out} has been created for\n> us.\n>\n> A later change will add the support for GIT_TEST_FSCK_TESTS. They're\n> being added first to ensure the test suite will never fail with\n> GIT_TEST_FSCK=true during bisect.\n\nI am sympathetic to what step 3/3 (eh, rather, an earlier \"let's not\nleave the repository in corrupt state, as that would make it\ninconvenient for us to later append new tests\") wants to do, but not\nthis one---these markings at the end makes it inconvenient for us to\nlater add new tests to these script before them.\n\n"}]}