{"thread":{"id":"35854","subject":"[PATCH] tree-walk: be more specific about corrupt tree errors","startedAt":"2014-02-12T19:49:57Z","lastAt":"2014-02-13T08:25:40Z","messageCount":2,"participants":["Jeff King","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"234686","messageId":"20140212194956.GA3850@sigill.intra.peff.net","threadId":"35854","inReplyTo":null,"subject":"[PATCH] tree-walk: be more specific about corrupt tree errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-12T19:49:57Z","receivedAt":"2014-02-12T19:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the tree-walker runs into an error, it just calls\ndie(), and the message is always \"corrupt tree file\".\nHowever, we are actually covering several cases here; let's\ngive the user a hint about what happened.\n\nLet's also avoid using the word \"corrupt\", which makes it\nseem like the data bit-rotted on disk. Our sha1 check would\nalready have found that. These errors are ones of data that\nis malformed in the first place.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMichael and I have been looking off-list at some bogus trees (created by\na non-git.git implementation). git-fsck unhelpfully dies during the\ntree-walk:\n\n  $ git fsck\n  Checking object directories: 100% (256/256), done.\n  fatal: corrupt tree file\n\nI think in the long run we want to either teach fsck to avoid the\nregular tree-walker or to set a special \"continue as much as you can\"\nflag. That will let us keep going to find more errors, do our usual fsck\nerror checks (which are more strict), and especially report on _which_\nobject was broken (we can't do that here because we are deep in the call\nstack and may not even have a real object yet).\n\nThis patch at least gives us slightly more specific error messages (both\nfor fsck and for other commands). And it may provide a first step in\nclarity if we follow the \"continue as much as you can\" path.\n\n tree-walk.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 79dba1d..d53b084 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -28,11 +28,13 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned\n \tunsigned int mode, len;\n \n \tif (size < 24 || buf[size - 21])\n-\t\tdie(\"corrupt tree file\");\n+\t\tdie(\"truncated tree file\");\n \n \tpath = get_mode(buf, &mode);\n-\tif (!path || !*path)\n-\t\tdie(\"corrupt tree file\");\n+\tif (!path)\n+\t\tdie(\"malformed mode in tree entry\");\n+\tif (!*path)\n+\t\tdie(\"empty filename in tree entry\");\n \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)\n \tunsigned long len = end - (const unsigned char *)buf;\n \n \tif (size < len)\n-\t\tdie(\"corrupt tree file\");\n+\t\tdie(\"truncated tree file\");\n \tbuf = end;\n \tsize -= len;\n \tdesc->buffer = buf;\n-- \n1.8.5.2.500.g8060133\n"},{"id":"234710","messageId":"52FC8184.5090606@alum.mit.edu","threadId":"35854","inReplyTo":"20140212194956.GA3850@sigill.intra.peff.net","subject":"Re: [PATCH] tree-walk: be more specific about corrupt tree errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-02-13T08:25:40Z","receivedAt":"2014-02-13T08:25:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/12/2014 08:49 PM, Jeff King wrote:\n> When the tree-walker runs into an error, it just calls\n> die(), and the message is always \"corrupt tree file\".\n> However, we are actually covering several cases here; let's\n> give the user a hint about what happened.\n> \n> Let's also avoid using the word \"corrupt\", which makes it\n> seem like the data bit-rotted on disk. Our sha1 check would\n> already have found that. These errors are ones of data that\n> is malformed in the first place.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Michael and I have been looking off-list at some bogus trees (created by\n> a non-git.git implementation). git-fsck unhelpfully dies during the\n> tree-walk:\n> \n>   $ git fsck\n>   Checking object directories: 100% (256/256), done.\n>   fatal: corrupt tree file\n> \n> I think in the long run we want to either teach fsck to avoid the\n> regular tree-walker or to set a special \"continue as much as you can\"\n> flag. That will let us keep going to find more errors, do our usual fsck\n> error checks (which are more strict), and especially report on _which_\n> object was broken (we can't do that here because we are deep in the call\n> stack and may not even have a real object yet).\n> \n> This patch at least gives us slightly more specific error messages (both\n> for fsck and for other commands). And it may provide a first step in\n> clarity if we follow the \"continue as much as you can\" path.\n> \n>  tree-walk.c | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n> \n> diff --git a/tree-walk.c b/tree-walk.c\n> index 79dba1d..d53b084 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -28,11 +28,13 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned\n>  \tunsigned int mode, len;\n>  \n>  \tif (size < 24 || buf[size - 21])\n> -\t\tdie(\"corrupt tree file\");\n> +\t\tdie(\"truncated tree file\");\n>  \n\nI suggest splitting this into two separate checks, because the first\nboolean definitely indicates a truncated file, whereas the second\nboolean could indicate malformedness that is not caused by truncation.\n\nAnother tiny point: I suppose that the number \"24\" comes from\n\n    A one-digit mode\n    SP\n    A one-character filename\n    NUL\n    20-byte SHA1\n\nBut given that you are detecting zero-length filenames a few lines\nlater, I think it makes logical sense to admit for that possibility\nhere, by reducing 24 -> 23.  (I realize that it doesn't change the end\nresult, because the only syntactically correct situation with length=23\nwould be a doubly-broken line that has a one-digit mode *and* a\nzero-length filename, and it's arbitrary which of the forms of\nbrokenness we report in such a case.)\n\n>  \tpath = get_mode(buf, &mode);\n> -\tif (!path || !*path)\n> -\t\tdie(\"corrupt tree file\");\n> +\tif (!path)\n> +\t\tdie(\"malformed mode in tree entry\");\n> +\tif (!*path)\n> +\t\tdie(\"empty filename in tree entry\");\n>  \tlen = strlen(path) + 1;\n>  \n>  \t/* Initialize the descriptor entry */\n> @@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)\n>  \tunsigned long len = end - (const unsigned char *)buf;\n>  \n>  \tif (size < len)\n> -\t\tdie(\"corrupt tree file\");\n> +\t\tdie(\"truncated tree file\");\n>  \tbuf = end;\n>  \tsize -= len;\n>  \tdesc->buffer = buf;\n> \n\nOtherwise, I think this is a nice improvement.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}