{"thread":{"id":"44161","subject":"[PATCH 0/2] tree-walk improvements","startedAt":"2016-09-26T19:33:02Z","lastAt":"2016-09-28T18:02:56Z","messageCount":22,"participants":["David Turner","Stefan Beller","Junio C Hamano","Jeff King","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"302620","messageId":"1474918365-10937-1-git-send-email-novalis@novalis.org","threadId":"44161","inReplyTo":null,"subject":"[PATCH 0/2] tree-walk improvements","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-26T19:32:43Z","receivedAt":"2016-09-26T19:33:02Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"The first patch is a re-roll of Peff's patch from 2014 -- here's\nthe archive message:\n\nhttp://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html\n\nNot sure why this wasn't applied then, but I thought it looked pretty\ngood, so I added a few tests.\n\nHopefully the encoding works correctly on these patches. If not, you\ncan fetch from\nhttps://github.com/novalis/git/\non branch dturner/bad-trees\n\nEmail address note 1: my employer wants me to use my company address,\nbut not my company computer, for patches I write on work time.  This\nmeans that I'm going to continue corresponding from\nnovalis@novalis.org, but will send patches with the @twosigma.com\naddress in the author line.\n\nEmail address note 2: I'm not subscribed to the mailing list these\ndays, so please CC me (at novalis@novalis.org) on replies.\n\nDavid Turner (1):\n  fsck: handle bad trees like other errors\n\nJeff King (1):\n  tree-walk: be more specific about corrupt tree errors\n\n fsck.c                                             |  18 +++--\n t/t1007-hash-object.sh                             |  15 +++-\n t/t1007/tree-with-empty-filename                   | Bin 0 -> 28 bytes\n t/t1007/tree-with-malformed-mode                   | Bin 0 -> 39 bytes\n t/t1450-fsck.sh                                    |  17 ++++-\n .../307e300745b82417cc1a903f875c7d22e45ef907       |   4 +\n .../f506a346749bb96f52d8605ffba9fb93d46b5ffd       | Bin 0 -> 45 bytes\n tree-walk.c                                        |  83 ++++++++++++++++++---\n tree-walk.h                                        |   8 ++\n 9 files changed, 125 insertions(+), 20 deletions(-)\n create mode 100644 t/t1007/tree-with-empty-filename\n create mode 100644 t/t1007/tree-with-malformed-mode\n create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd\n\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302621","messageId":"1474918365-10937-2-git-send-email-novalis@novalis.org","threadId":"44161","inReplyTo":"1474918365-10937-1-git-send-email-novalis@novalis.org","subject":"[PATCH 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-26T19:32:44Z","receivedAt":"2016-09-26T19:33:16Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen 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: David Turner <dturner@twosigma.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1007-hash-object.sh           |  15 +++++++++++++--\n t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes\n t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes\n tree-walk.c                      |  12 +++++++-----\n 4 files changed, 20 insertions(+), 7 deletions(-)\n create mode 100644 t/t1007/tree-with-empty-filename\n create mode 100644 t/t1007/tree-with-malformed-mode\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex acca9ac..cd10c73 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -183,9 +183,20 @@ for args in \"-w --stdin-paths\" \"--stdin-paths -w\"; do\n \tpop_repo\n done\n \n-test_expect_success 'corrupt tree' '\n+test_expect_success 'too-short tree' '\n \techo abc >malformed-tree &&\n-\ttest_must_fail git hash-object -t tree malformed-tree\n+\ttest_must_fail git hash-object -t tree malformed-tree 2>err &&\n+\tgrep \"too-short tree object\" err\n+'\n+\n+test_expect_success 'malformed mode in tree' '\n+\ttest_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&\n+\tgrep \"malformed mode in tree entry for tree\" err\n+'\n+\n+test_expect_success 'empty filename in tree' '\n+\ttest_must_fail git hash-object -t tree ../t1007/tree-with-empty-filename 2>err &&\n+\tgrep \"empty filename in tree entry for tree\" err\n '\n \n test_expect_success 'corrupt commit' '\ndiff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe\nGIT binary patch\nliteral 28\nkcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3\n\nliteral 0\nHcmV?d00001\n\ndiff --git a/t/t1007/tree-with-malformed-mode b/t/t1007/tree-with-malformed-mode\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..24aa84d60ef8e269fb0b29c67b5208639b9da3ae\nGIT binary patch\nliteral 39\nvcmYewPcJRb%}+^HNXyJg%}dNpWq3CC(d<nZuQ_{nYpyGgx^d`9Pw+$lU*Quk\n\nliteral 0\nHcmV?d00001\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ce27842..ba544cf 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned\n \tconst char *path;\n \tunsigned int mode, len;\n \n-\tif (size < 24 || buf[size - 21])\n-\t\tdie(\"corrupt tree file\");\n+\tif (size < 23 || buf[size - 21])\n+\t\tdie(\"too-short tree object\");\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 for tree\");\n+\tif (!*path)\n+\t\tdie(\"empty filename in tree entry for tree\");\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(\"too-short tree file\");\n \tbuf = end;\n \tsize -= len;\n \tdesc->buffer = buf;\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302622","messageId":"1474918365-10937-3-git-send-email-novalis@novalis.org","threadId":"44161","inReplyTo":"1474918365-10937-1-git-send-email-novalis@novalis.org","subject":"[PATCH 2/2] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-26T19:32:45Z","receivedAt":"2016-09-26T19:33:19Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner <dturner@twosigma.com>\n\nInstead of dying when fsck hits a malformed tree object, log the error\nlike any other and continue.  Now fsck can tell the user which tree is\nbad, too.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n fsck.c                                             |  18 +++--\n t/t1450-fsck.sh                                    |  17 ++++-\n .../307e300745b82417cc1a903f875c7d22e45ef907       |   4 +\n .../f506a346749bb96f52d8605ffba9fb93d46b5ffd       | Bin 0 -> 45 bytes\n tree-walk.c                                        |  83 ++++++++++++++++++---\n tree-walk.h                                        |   8 ++\n 6 files changed, 111 insertions(+), 19 deletions(-)\n create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd\n\ndiff --git a/fsck.c b/fsck.c\nindex c9cf3de..4a3069e 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\treturn -1;\n \n \tname = get_object_name(options, &tree->object);\n-\tinit_tree_desc(&desc, tree->buffer, tree->size);\n-\twhile (tree_entry(&desc, &entry)) {\n+\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\t\treturn -1;\n+\twhile (tree_entry_gently(&desc, &entry)) {\n \t\tstruct object *obj;\n \t\tint result;\n \n@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \n static int fsck_tree(struct tree *item, struct fsck_options *options)\n {\n-\tint retval;\n+\tint retval = 0;\n \tint has_null_sha1 = 0;\n \tint has_full_path = 0;\n \tint has_empty_name = 0;\n@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \tunsigned o_mode;\n \tconst char *o_name;\n \n-\tinit_tree_desc(&desc, item->buffer, item->size);\n+\tif (init_tree_desc_gently(&desc, item->buffer, item->size)) {\n+\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\treturn retval;\n+\t}\n \n \to_mode = 0;\n \to_name = NULL;\n@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\t\t       is_hfs_dotgit(name) ||\n \t\t\t       is_ntfs_dotgit(name));\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n-\t\tupdate_tree_entry(&desc);\n+\t\tif (update_tree_entry_gently(&desc)) {\n+\t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tbreak;\n+\t\t}\n \n \t\tswitch (mode) {\n \t\t/*\n@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\to_name = name;\n \t}\n \n-\tretval = 0;\n \tif (has_null_sha1)\n \t\tretval += report(options, &item->object, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8f52da2..f456963 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '\n \tgrep \"error in commit $new.*unterminated header: NUL at offset\" out\n '\n \n-test_expect_success 'malformatted tree object' '\n-\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+test_expect_success 'tree object with duplicate entries' '\n \ttest_when_finished \"remove_object \\$T\" &&\n \tT=$(\n \t\tGIT_INDEX_FILE=test-index &&\n@@ -208,6 +207,20 @@ test_expect_success 'malformatted tree object' '\n \tgrep \"error in tree .*contains duplicate file entries\" out\n '\n \n+test_expect_success 'unparseable tree object' '\n+\ttest_when_finished \"git update-ref -d refs/heads/wrong\" &&\n+\ttest_when_finished \"remove_object 307e300745b82417cc1a903f875c7d22e45ef907\" &&\n+\ttest_when_finished \"remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd\" &&\n+\tmkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&\n+\tcp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&\n+\tcp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&\n+\tgit update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&\n+\ttest_must_fail git fsck 2>out &&\n+\tgrep \"warning: empty filename in tree entry\" out &&\n+\tgrep \"f506a346749bb96f52d8605ffba9fb93d46b5ffd\" out &&\n+\t! grep \"fatal: empty filename in tree entry\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\ndiff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\nnew file mode 100644\nindex 0000000..6e23d62\n--- /dev/null\n+++ b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n@@ -0,0 +1,4 @@\n+x\u0001ŽA\u000e \u0010E]sŠ¹€f°@Ä\u0018\u0017\u001eÁ\u000b0\u0005Z\u0012[\u0012\n+õú¢é\t\\ýüÅ{ÿ\u000fižc\u0001IòP²÷\u00104\u001aÛ)Ó+b&\u0013ôÙ]\fê\u0010ØR`êœ2ܚ\u0013¶–)exØ-:xÖ¼ø\f×%mö\u0015×ûž§”Ç\u001bHÕd\u0012{-áˆ\n+Q\f¿ÍҀè\u001d‡w,\u0013p\u001aë\n+ßçâ\u0003&ë?Þ\n\\ No newline at end of file\ndiff --git a/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd b/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..9111a7fc3c8578906e13c930a0fbd3cae047762e\nGIT binary patch\nliteral 45\nzcmb=Jqpj)X8)~pA!NA18z}PS_p~CF@#W%j<>n*Fxv)5_&?<#!Z>Hoon;loq@NdS%f\nB6F2|>\n\nliteral 0\nHcmV?d00001\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ba544cf..0fb830b 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)\n \treturn str;\n }\n \n-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)\n+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)\n {\n \tconst char *path;\n \tunsigned int mode, len;\n \n-\tif (size < 23 || buf[size - 21])\n-\t\tdie(\"too-short tree object\");\n+\tif (size < 23 || buf[size - 21]) {\n+\t\tstrbuf_addstr(err, \"too-short tree object\");\n+\t\treturn -1;\n+\t}\n \n \tpath = get_mode(buf, &mode);\n-\tif (!path)\n-\t\tdie(\"malformed mode in tree entry for tree\");\n-\tif (!*path)\n-\t\tdie(\"empty filename in tree entry for tree\");\n+\tif (!path) {\n+\t\tstrbuf_addstr(err, \"malformed mode in tree entry\");\n+\t\treturn -1;\n+\t}\n+\tif (!*path) {\n+\t\tstrbuf_addstr(err, \"empty filename in tree entry\");\n+\t\treturn -1;\n+\t}\n \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n \tdesc->entry.path = path;\n \tdesc->entry.mode = canon_mode(mode);\n \tdesc->entry.oid  = (const struct object_id *)(path + len);\n+\n+\treturn 0;\n }\n \n-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)\n+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)\n {\n \tdesc->buffer = buffer;\n \tdesc->size = size;\n \tif (size)\n-\t\tdecode_tree_entry(desc, buffer, size);\n+\t\treturn decode_tree_entry(desc, buffer, size, err);\n+\treturn 0;\n+}\n+\n+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (init_tree_desc_internal(desc, buffer, size, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+}\n+\n+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint result = init_tree_desc_internal(desc, buffer, size, &err);\n+\tif (result)\n+\t\twarning(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn result;\n }\n \n void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)\n@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)\n \t*a = t->entry;\n }\n \n-void update_tree_entry(struct tree_desc *desc)\n+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)\n {\n \tconst void *buf = desc->buffer;\n \tconst unsigned char *end = desc->entry.oid->hash + 20;\n@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)\n \tdesc->buffer = buf;\n \tdesc->size = size;\n \tif (size)\n-\t\tdecode_tree_entry(desc, buf, size);\n+\t\treturn decode_tree_entry(desc, buf, size, err);\n+\treturn 0;\n+}\n+\n+void update_tree_entry(struct tree_desc *desc)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (update_tree_entry_internal(desc, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+}\n+\n+int update_tree_entry_gently(struct tree_desc *desc)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (update_tree_entry_internal(desc, &err)) {\n+\t\twarning(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\t/* Stop processing this tree after error */\n+\t\tdesc->size = 0;\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&err);\n+\treturn 0;\n }\n \n int tree_entry(struct tree_desc *desc, struct name_entry *entry)\n@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)\n \treturn 1;\n }\n \n+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)\n+{\n+\tif (!desc->size)\n+\t\treturn 0;\n+\n+\t*entry = desc->entry;\n+\tif (update_tree_entry_gently(desc))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n void setup_traverse_info(struct traverse_info *info, const char *base)\n {\n \tint pathlen = strlen(base);\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 97a7d69..68bb78b 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)\n \treturn (const char *)ne->oid - ne->path - 1;\n }\n \n+/*\n+ * The _gently versions of these functions warn and return false on a\n+ * corrupt tree entry rather than dying,\n+ */\n+\n void update_tree_entry(struct tree_desc *);\n+int update_tree_entry_gently(struct tree_desc *);\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);\n \n /*\n  * Helper function that does both tree_entry_extract() and update_tree_entry()\n  * and returns true for success\n  */\n int tree_entry(struct tree_desc *, struct name_entry *);\n+int tree_entry_gently(struct tree_desc *, struct name_entry *);\n \n void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);\n \n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302624","messageId":"CAGZ79kY6W5jDP9YJz4Vr7OK9yFhbBM2h9wpgFLx_Ug+bTocn+g@mail.gmail.com","threadId":"44161","inReplyTo":"1474918365-10937-1-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 0/2] tree-walk improvements","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-26T19:39:40Z","receivedAt":"2016-09-26T19:39:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 26, 2016 at 12:32 PM, David Turner <novalis@novalis.org> wrote:\n> The first patch is a re-roll of Peff's patch from 2014 -- here's\n> the archive message:\n>\n> http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html\n>\n> Not sure why this wasn't applied then, but I thought it looked pretty\n> good, so I added a few tests.\n>\n> Hopefully the encoding works correctly on these patches. If not, you\n> can fetch from\n> https://github.com/novalis/git/\n> on branch dturner/bad-trees\n>\n> Email address note 1: my employer wants me to use my company address,\n> but not my company computer, for patches I write on work time.  This\n> means that I'm going to continue corresponding from\n> novalis@novalis.org, but will send patches with the @twosigma.com\n> address in the author line.\n\nMind sending a patch for .mailmap to reflect that different email\naddresses are still the same person? ;)\n"},{"id":"302625","messageId":"xmqqshsmqwfr.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"1474918365-10937-1-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 0/2] tree-walk improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T19:43:04Z","receivedAt":"2016-09-26T19:43:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> The first patch is a re-roll of Peff's patch from 2014 -- here's\n> the archive message:\n>\n> http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html\n>\n> Not sure why this wasn't applied then, but I thought it looked pretty\n> good, so I added a few tests.\n\nThanks.  Adding tests is very much appreciated.  I however wonder\nwhy you needed to reword a perfectly readable \"truncated\" to\nsomething else, though?\n\n> Email address note 1: my employer wants me to use my company address,\n> but not my company computer, for patches I write on work time.  This\n> means that I'm going to continue corresponding from\n> novalis@novalis.org, but will send patches with the @twosigma.com\n> address in the author line.\n\nThat seems like not an uncommon practice ;-)\n\n> Email address note 2: I'm not subscribed to the mailing list these\n> days, so please CC me (at novalis@novalis.org) on replies.\n\nIt is good to tell others this, but I suspect that it is known by\nthose who are likely to respond to these messages that always CC'ing\nto individual is the norm on this list ;-)\n\nThanks.\n"},{"id":"302627","messageId":"xmqqlgyeqw20.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"1474918365-10937-3-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T19:51:19Z","receivedAt":"2016-09-26T19:51:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> @@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n>  \tunsigned o_mode;\n>  \tconst char *o_name;\n>  \n> -\tinit_tree_desc(&desc, item->buffer, item->size);\n> +\tif (init_tree_desc_gently(&desc, item->buffer, item->size)) {\n> +\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n> +\t\treturn retval;\n> +\t}\n\nGood.  If BAD_TREE is being ignored, this may report a non-error,\nbut we won't descend into the unreadable tree so it is OK.\n\n> @@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n>  \t\t\t       is_hfs_dotgit(name) ||\n>  \t\t\t       is_ntfs_dotgit(name));\n>  \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> -\t\tupdate_tree_entry(&desc);\n> +\t\tif (update_tree_entry_gently(&desc)) {\n> +\t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n> +\t\t\tbreak;\n> +\t\t}\n\nLikewise; breaking out of the loop will stop us from reading further\ninto the corrupted tree data, so this is good.\n\n> @@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n>  \t\to_name = name;\n>  \t}\n>  \n> -\tretval = 0;\n\nGood code hygiene that you moved this to the very top where it is\ndefined, so anybody before this step can set it if it wants to.\n\nReading purely from the text of this function, it was surprising\nthat you can do without a gently variant of tree_entry_extract(),\nbut it merely reads into two variables and does not do any error\ndetection (which happens all in the caller), so it is not at all\nsurprising after all ;-)\n\nI didn't see anything objectionable in this patch.  Thanks for\nworking on this.\n"},{"id":"302629","messageId":"xmqqh992qv9a.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"1474918365-10937-3-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T20:08:33Z","receivedAt":"2016-09-26T20:08:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> From: David Turner <dturner@twosigma.com>\n>\n> Instead of dying when fsck hits a malformed tree object, log the error\n> like any other and continue.  Now fsck can tell the user which tree is\n> bad, too.\n>\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  fsck.c                                             |  18 +++--\n>  t/t1450-fsck.sh                                    |  17 ++++-\n>  .../307e300745b82417cc1a903f875c7d22e45ef907       |   4 +\n\nTo prevent further headaches in this directory, can we have\n.gitattributes that tells us that everything in there are binary\nfiles?  Something like the attached.\n\nThe other object was transferred as a binary patch, but I have no\nfaith in what I applied from your e-mail message for this file that\nwent though latin-1 to utf-8 conversion X-<.\n\n t/t1450/bad-objects/.gitattributes | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes\nnew file mode 100644\nindex 0000000..a173f27\n--- /dev/null\n+++ b/t/t1450/bad-objects/.gitattributes\n@@ -0,0 +1 @@\n+[0-9a-f]*[0-9a-f]\t-diff\n\n\n"},{"id":"302630","messageId":"xmqqd1jqqv3o.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"xmqqh992qv9a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T20:11:55Z","receivedAt":"2016-09-26T20:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> To prevent further headaches in this directory, can we have\n> .gitattributes that tells us that everything in there are binary\n> files?  Something like the attached.\n>\n> The other object was transferred as a binary patch, but I have no\n> faith in what I applied from your e-mail message for this file that\n> went though latin-1 to utf-8 conversion X-<.\n>\n>  t/t1450/bad-objects/.gitattributes | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes\n> new file mode 100644\n> index 0000000..a173f27\n> --- /dev/null\n> +++ b/t/t1450/bad-objects/.gitattributes\n> @@ -0,0 +1 @@\n> +[0-9a-f]*[0-9a-f]\t-diff\n\nI suspect that the t/t1007 directory in 1/2 may deserve a similar\ntreatment.\n"},{"id":"302631","messageId":"1474921343.13374.1.camel@frank","threadId":"44161","inReplyTo":"xmqqshsmqwfr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] tree-walk improvements","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-26T20:22:23Z","receivedAt":"2016-09-26T20:22:30Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2016-09-26 at 12:43 -0700, Junio C Hamano wrote:\n> David Turner <novalis@novalis.org> writes:\n> \n> > The first patch is a re-roll of Peff's patch from 2014 -- here's\n> > the archive message:\n> >\n> > http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html\n> >\n> > Not sure why this wasn't applied then, but I thought it looked pretty\n> > good, so I added a few tests.\n> \n> Thanks.  Adding tests is very much appreciated.  I however wonder\n> why you needed to reword a perfectly readable \"truncated\" to\n> something else, though?\n\nBecause truncated, to me, means \"something that has been cut off\". Here,\nthe recorded length is too short, so it's probably not the case that\nsomething was cut off -- it was never right to begin with.\n\n\n"},{"id":"302637","messageId":"xmqqy42epe3e.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"1474918365-10937-1-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 0/2] tree-walk improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T21:04:37Z","receivedAt":"2016-09-26T21:04:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> Hopefully the encoding works correctly on these patches. If not, you\n> can fetch from\n> https://github.com/novalis/git/\n> on branch dturner/bad-trees\n\nThis does not test cleanly here, unfortunately.  Specifically, tests\n30 and 31 t1007 do fine with 1/2 alone, but they seem to break with\n2/2 applied.\n\nI didn't dug further.  At least not yet.\n\n\n"},{"id":"302664","messageId":"xmqqy42enpri.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"1474921343.13374.1.camel@frank","subject":"Re: [PATCH 0/2] tree-walk improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T00:35:29Z","receivedAt":"2016-09-27T00:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> Because truncated, to me, means \"something that has been cut off\". Here,\n> the recorded length is too short, so it's probably not the case that\n> something was cut off -- it was never right to begin with.\n\nThat's perfectly sensible. Thanks.\n"},{"id":"302674","messageId":"20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net","threadId":"44161","inReplyTo":"1474918365-10937-2-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T05:14:54Z","receivedAt":"2016-09-27T05:15:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 03:32:44PM -0400, David Turner wrote:\n\n> From: Jeff King <peff@peff.net>\n> \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: David Turner <dturner@twosigma.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nYay. This has been on my \"to look at and repost\" list for literally 2\nyears. Thanks for picking it up (see kids, procrastination _does_ pay\noff).\n\n>  t/t1007-hash-object.sh           |  15 +++++++++++++--\n>  t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes\n>  t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes\n\nOoh, and tests. Exciting.\n\n> -test_expect_success 'corrupt tree' '\n> +test_expect_success 'too-short tree' '\n>  \techo abc >malformed-tree &&\n> -\ttest_must_fail git hash-object -t tree malformed-tree\n> +\ttest_must_fail git hash-object -t tree malformed-tree 2>err &&\n> +\tgrep \"too-short tree object\" err\n> +'\n\nShould this be test_i18ngrep? Even if the message is not translated now,\nit seems like a good proactive measure (and probably it _should_ be\ntranslated).\n\n> +test_expect_success 'malformed mode in tree' '\n> +\ttest_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&\n> +\tgrep \"malformed mode in tree entry for tree\" err\n> +'\n\nThis \"..\" will break when the test is run with \"--root\". You should use\n\n  \"$TEST_DIRECTORY\"/t1007/...\n\ninstead. And ditto in the second test, of course.\n\n> diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe\n> GIT binary patch\n> literal 28\n> kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3\n> \n> literal 0\n> HcmV?d00001\n\nThis is rather opaque, of course. :)\n\nI wonder if it would be possible to generate the test vector with\nsomething like:\n\n  # any 20 bytes will do\n  bin_sha1='\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0'\n\n  printf \"100644 \\0$bin_sha1\" >tree-with-empty-filename\n\nI know that is longer and possibly more error-prone to run, but I think\nit makes the test much easier to read and modify later.\n\nI also wonder if $bin_sha1 should actually be more like:\n\n  hex_sha1=$(echo foo | git hash-object --stdin -w)\n  bin_sha1=$(echo $hex_sha1 | perl -ne 'printf \"\\\\%3o\", ord for /./g')\n\nso that it's a real sha1 (or maybe it is in your original, from an\nobject that happens to be in the repo; it's hard to tell). I wouldn't\nexpect the code to actually get to the point of looking at the sha1, but\nit's perhaps a more realistic test.\n\nI also think it would be nice if hash-object had a \"--binary-sha1\"\noption to avoid the perl grossness. :)\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index ce27842..ba544cf 100644\n\nThe code change itself looks brilliant, naturally. :)\n\n-Peff\n"},{"id":"302675","messageId":"20160927052754.bs4frcfy4y7fey62@sigill.intra.peff.net","threadId":"44161","inReplyTo":"1474918365-10937-3-git-send-email-novalis@novalis.org","subject":"Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T05:27:54Z","receivedAt":"2016-09-27T05:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 03:32:45PM -0400, David Turner wrote:\n\n> Instead of dying when fsck hits a malformed tree object, log the error\n> like any other and continue.  Now fsck can tell the user which tree is\n> bad, too.\n\nCool. I think the lack of this is what made me drag my feet on the first\npatch. Thanks for finishing it off.\n\n> diff --git a/fsck.c b/fsck.c\n> index c9cf3de..4a3069e 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n>  \t\treturn -1;\n>  \n>  \tname = get_object_name(options, &tree->object);\n> -\tinit_tree_desc(&desc, tree->buffer, tree->size);\n> -\twhile (tree_entry(&desc, &entry)) {\n> +\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n> +\t\treturn -1;\n> +\twhile (tree_entry_gently(&desc, &entry)) {\n\nI wondered if other callers would be happy with init_tree_desc_gently().\nGrepping for init_tree_desc(), it seems like it would be a fairly\ntrivial conversion for most of them, because they almost invariably run\nunpack_trees() right afterwards, and so have to deal with errors from\nit.\n\nSo perhaps in the long run we can convert them all. But certainly that\ndoes not need to be part of this series.\n\n> +test_expect_success 'unparseable tree object' '\n> +\ttest_when_finished \"git update-ref -d refs/heads/wrong\" &&\n> +\ttest_when_finished \"remove_object 307e300745b82417cc1a903f875c7d22e45ef907\" &&\n> +\ttest_when_finished \"remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd\" &&\n> +\tmkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&\n> +\tcp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&\n> +\tcp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&\n\nThis needs the same $TEST_DIRECTORY treatment as t1007.\n\n> +\tgit update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&\n> +\ttest_must_fail git fsck 2>out &&\n> +\tgrep \"warning: empty filename in tree entry\" out &&\n> +\tgrep \"f506a346749bb96f52d8605ffba9fb93d46b5ffd\" out &&\n> +\t! grep \"fatal: empty filename in tree entry\" out\n> +'\n\nI'd also expect these to be test_i18ngrep, but I see that t1450 is quite\nbad about this in general. I'm OK with adding them as greps and leaving\na conversion of the whole script until later.\n\n> diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n> new file mode 100644\n> index 0000000..6e23d62\n> --- /dev/null\n> +++ b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n> @@ -0,0 +1,4 @@\n> +x\u0001��A\u000e� \u0010E]s���f�@��\u0018\u0017\u001e�\u000b0\u0005Z\u0012[\u0012\n> +����\t\\���{�\u000fi�c\u0001I�P��\u00104\u001a�)�+b&\u0013��]\f�\u0010�R`�2ܚ\u0013��)ex�-:xּ�\f�%m�\u0015������\u001bH�d\u0012{-�\n> +Q\f��Ҁ�\u001d�w,\u0013p\u001a�\n> +���\u0003&�?�\n> \\ No newline at end of file\n\nYikes. :)\n\nI wonder if some printfs, similar to what I showed in the last patch,\ncombined with \"hash-object --literally\", could make these tests more\nreadable and avoid the binary goo.\n\n> -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)\n> +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)\n>  {\n\nI know we used the \"err\" strbuf pattern in the ref code, and it makes\nsense there where we have a lot of different functions with public\ninterfaces. But here, we literally just feed the result to die() or\nwarning(). I wonder if a nicer interface would be:\n\n  typedef void (*err_fn)(const char *, ...);\n\n  static int decode_tree_entry(struct tree_desc *desc,\n                               const char *buf, unsigned long size,\n\t\t\t       err_fn err)\n  {\n         ...\n         if (size < 23 || buf[size - 21]) {\n\t        err(\"too-short tree object\");\n\t\treturn -1;\n\t }\n  }\n\nI dunno. Maybe that is overengineering. I guess we only hit the strbufs\nin the error path (which used to die!), so nobody really cares that much\nabout the extra allocation.\n\n> +int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)\n> +{\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tint result = init_tree_desc_internal(desc, buffer, size, &err);\n> +\tif (result)\n> +\t\twarning(\"%s\", err.buf);\n> +\tstrbuf_release(&err);\n> +\treturn result;\n>  }\n\nI also wonder if this ought to be \"error()\" and not \"warning()\". I think\nit's pretty common for fsck to spit out errors from sub-code but keep going.\n\n-Peff\n"},{"id":"302676","messageId":"xmqqbmz9sy4y.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T05:35:41Z","receivedAt":"2016-09-27T05:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +test_expect_success 'malformed mode in tree' '\n>> +\ttest_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&\n>> +\tgrep \"malformed mode in tree entry for tree\" err\n>> +'\n>\n> This \"..\" will break when the test is run with \"--root\". You should use\n>\n>   \"$TEST_DIRECTORY\"/t1007/...\n>\n> instead. And ditto in the second test, of course.\n\nAhh, that explains the breakage I saw.\n\nThanks.\n"},{"id":"302704","messageId":"1474989574.26902.7.camel@frank","threadId":"44161","inReplyTo":"20160927052754.bs4frcfy4y7fey62@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-27T15:19:34Z","receivedAt":"2016-09-27T15:19:43Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2016-09-27 at 01:27 -0400, Jeff King wrote:\n> > -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)\n> > +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)\n> >  {\n> \n> I know we used the \"err\" strbuf pattern in the ref code, and it makes\n> sense there where we have a lot of different functions with public\n> interfaces. But here, we literally just feed the result to die() or\n> warning(). I wonder if a nicer interface would be:\n> \n>   typedef void (*err_fn)(const char *, ...);\n> \n>   static int decode_tree_entry(struct tree_desc *desc,\n>                                const char *buf, unsigned long size,\n> \t\t\t       err_fn err)\n>   {\n>          ...\n>          if (size < 23 || buf[size - 21]) {\n> \t        err(\"too-short tree object\");\n> \t\treturn -1;\n> \t }\n>   }\n> \n> I dunno. Maybe that is overengineering. I guess we only hit the strbufs\n> in the error path (which used to die!), so nobody really cares that much\n> about the extra allocation.\n\nI don't really like err_fn because:\n(a) without a baton, it's somewhat less general (or less thread-safe)\nthan the strbuf approach and\n(b) with a baton, it's two arguments instead of one.\n\nThanks for all of the rest of the commentary; I've incorporated it and\nwill re-roll shortly.\n\n\n"},{"id":"302705","messageId":"1474989716.26902.9.camel@frank","threadId":"44161","inReplyTo":"20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-27T15:21:56Z","receivedAt":"2016-09-27T15:22:03Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2016-09-27 at 01:14 -0400, Jeff King wrote:\n> I also wonder if $bin_sha1 should actually be more like:\n> \n>   hex_sha1=$(echo foo | git hash-object --stdin -w)\n>   bin_sha1=$(echo $hex_sha1 | perl -ne 'printf \"\\\\%3o\", ord for /./g')\n> \n> so that it's a real sha1 (or maybe it is in your original, from an\n> object that happens to be in the repo; it's hard to tell). I wouldn't\n> expect the code to actually get to the point of looking at the sha1, but\n> it's perhaps a more realistic test.\n\nI'm going to do this for this patch, but for the second patch, I don't\nthink it matters, and it's a bit simpler to avoid it.  I guess it's\nprobably better to test both ways anyway.\n\n"},{"id":"302745","messageId":"20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net","threadId":"44161","inReplyTo":"1474989574.26902.7.camel@frank","subject":"thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T19:19:55Z","receivedAt":"2016-09-27T19:20:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:\n\n> >   typedef void (*err_fn)(const char *, ...);\n> > \n> >   static int decode_tree_entry(struct tree_desc *desc,\n> >                                const char *buf, unsigned long size,\n> > \t\t\t       err_fn err)\n> >   {\n> >          ...\n> >          if (size < 23 || buf[size - 21]) {\n> > \t        err(\"too-short tree object\");\n> > \t\treturn -1;\n> > \t }\n> >   }\n> > \n> > I dunno. Maybe that is overengineering. I guess we only hit the strbufs\n> > in the error path (which used to die!), so nobody really cares that much\n> > about the extra allocation.\n> \n> I don't really like err_fn because:\n> (a) without a baton, it's somewhat less general (or less thread-safe)\n> than the strbuf approach and\n> (b) with a baton, it's two arguments instead of one.\n\nI'm going to ramble for a minute, and I don't think it's worth exploring\nfor this patch series in particular, so feel free to ignore me.\n\nI think this error concept could be extended fairly elegantly with\nsomething like:\n\n  typedef void (*err_fn)(void *, const char *fmt, va_list ap)\n  struct error_context {\n        err_fn fn;\n        void *data;\n  };\n\n  int report_error(struct error_context *err, const char *fmt, ...)\n  {\n        if (err->fn) {\n                va_list ap;\n                va_start(ap, fmt);\n                err->fn(err->data, fmt, ap);\n                va_end(ap);\n        }\n        return -1;\n  }\n\nThen low-level functions just take a context and do:\n\n  return report_error(&err, \"some error: %s\", foo);\n\nAnd then the callers would pick one of a few generic error contexts:\n\n  - passing NULL silences the errors\n\n  - a global for chaining to error, like:\n\n       struct error_context print_errors = {\n          error, /* actually a wrapper to handle va_list and NULL data */\n          NULL\n       };\n\n   - a context that collects errors in a strbuf (or list, etc)\n\n       struct strbuf err_buf = STRBUF_INIT;\n       struct error_context = STRBUF_ERR_CONTEXT(&err_buf);\n\nAnd that error_context can be passed around like a baton through several\nfunctions.\n\nI remember having a big discussion about error-passing patterns around\nthe ref refactoring, but I don't remember this particular thing coming\nup. \n\nAnyway, this is all way outside the scope of what we should consider for\nyour current series. If we were to do something like this, it would make\nsense to start using it consistently. This discussion just made me think\nof it.\n\n-Peff\n"},{"id":"302782","messageId":"1475017054.13103.3.camel@frank","threadId":"44161","inReplyTo":"20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net","subject":"Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-27T22:57:34Z","receivedAt":"2016-09-27T22:57:42Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2016-09-27 at 15:19 -0400, Jeff King wrote:\n> On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:\n> \n> > >   typedef void (*err_fn)(const char *, ...);\n> > > \n> > >   static int decode_tree_entry(struct tree_desc *desc,\n> > >                                const char *buf, unsigned long size,\n> > > \t\t\t       err_fn err)\n> > >   {\n> > >          ...\n> > >          if (size < 23 || buf[size - 21]) {\n> > > \t        err(\"too-short tree object\");\n> > > \t\treturn -1;\n> > > \t }\n> > >   }\n> > > \n> > > I dunno. Maybe that is overengineering. I guess we only hit the strbufs\n> > > in the error path (which used to die!), so nobody really cares that much\n> > > about the extra allocation.\n> > \n> > I don't really like err_fn because:\n> > (a) without a baton, it's somewhat less general (or less thread-safe)\n> > than the strbuf approach and\n> > (b) with a baton, it's two arguments instead of one.\n> \n> I'm going to ramble for a minute, and I don't think it's worth exploring\n> for this patch series in particular, so feel free to ignore me.\n> \n> I think this error concept could be extended fairly elegantly with\n> something like:\n> \n>   typedef void (*err_fn)(void *, const char *fmt, va_list ap)\n>   struct error_context {\n>         err_fn fn;\n>         void *data;\n>   };\n> \n>   int report_error(struct error_context *err, const char *fmt, ...)\n>   {\n>         if (err->fn) {\n>                 va_list ap;\n>                 va_start(ap, fmt);\n>                 err->fn(err->data, fmt, ap);\n>                 va_end(ap);\n>         }\n>         return -1;\n>   }\n> \n> Then low-level functions just take a context and do:\n> \n>   return report_error(&err, \"some error: %s\", foo);\n> \n> And then the callers would pick one of a few generic error contexts:\n> \n>   - passing NULL silences the errors\n\nOverall, +1.\n\nI guess I would rather have a sentinel value for silencing errors,\nbecause I'm worried that someone might read NULL as \"don't handle the\nerrors, just die\".  Of course, code review would hopefully catch this,\nbut even so, it would be easier to read foo(x, y, silence_errors) than\nfoo(x, y, null).\n\n\n\n"},{"id":"302793","messageId":"06b1a3bc-18dc-bd9e-4200-a2eedbec7b97@alum.mit.edu","threadId":"44161","inReplyTo":"20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net","subject":"Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-09-28T05:01:38Z","receivedAt":"2016-09-28T05:01:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/27/2016 09:19 PM, Jeff King wrote:\n> [...]\n> I'm going to ramble for a minute, and I don't think it's worth exploring\n> for this patch series in particular, so feel free to ignore me.\n> \n> I think this error concept could be extended fairly elegantly with\n> something like:\n> \n>   typedef void (*err_fn)(void *, const char *fmt, va_list ap)\n>   struct error_context {\n>         err_fn fn;\n>         void *data;\n>   };\n> \n>   int report_error(struct error_context *err, const char *fmt, ...)\n>   {\n>         if (err->fn) {\n>                 va_list ap;\n>                 va_start(ap, fmt);\n>                 err->fn(err->data, fmt, ap);\n>                 va_end(ap);\n>         }\n>         return -1;\n>   }\n\nI like this idea. It's nicely flexible (more so than the `struct strbuf\n*err` that is currently used for reference transactions) without being\ncumbersome.\n\n> Then low-level functions just take a context and do:\n> \n>   return report_error(&err, \"some error: %s\", foo);\n> \n> And then the callers would pick one of a few generic error contexts:\n> \n>   - passing NULL silences the errors\n> \n>   - a global for chaining to error, like:\n> \n>        struct error_context print_errors = {\n>           error, /* actually a wrapper to handle va_list and NULL data */\n>           NULL\n>        };\n\nThere could also be a global for chaining to `warn()` or `die()`.\n\n> [...]\n\nMichael\n\n"},{"id":"302799","messageId":"20160928065451.klz4okmag3ksz3gu@sigill.intra.peff.net","threadId":"44161","inReplyTo":"1475017054.13103.3.camel@frank","subject":"Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-28T06:54:51Z","receivedAt":"2016-09-28T06:54:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 06:57:34PM -0400, David Turner wrote:\n\n> >   int report_error(struct error_context *err, const char *fmt, ...)\n> >   {\n> >         if (err->fn) {\n> >                 va_list ap;\n> >                 va_start(ap, fmt);\n> >                 err->fn(err->data, fmt, ap);\n> >                 va_end(ap);\n> >         }\n> >         return -1;\n> >   }\n> > \n> > Then low-level functions just take a context and do:\n> > \n> >   return report_error(&err, \"some error: %s\", foo);\n> > \n> > And then the callers would pick one of a few generic error contexts:\n> > \n> >   - passing NULL silences the errors\n> \n> Overall, +1.\n> \n> I guess I would rather have a sentinel value for silencing errors,\n> because I'm worried that someone might read NULL as \"don't handle the\n> errors, just die\".  Of course, code review would hopefully catch this,\n> but even so, it would be easier to read foo(x, y, silence_errors) than\n> foo(x, y, null).\n\nYeah, I waffled on that. If you look carefully, you'll note that\nthe report_error() I showed above would actually require such a\n\"{ NULL, NULL }\" global.\n\nI don't plan to make any patches immediately for this, but I'll let it\npercolate and consider whether it makes sense to try out for a future\nseries.\n\n-Peff\n"},{"id":"302801","messageId":"20160928085841.aoisson3fnuke47q@sigill.intra.peff.net","threadId":"44161","inReplyTo":"06b1a3bc-18dc-bd9e-4200-a2eedbec7b97@alum.mit.edu","subject":"Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-28T08:58:41Z","receivedAt":"2016-09-28T08:58:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 28, 2016 at 07:01:38AM +0200, Michael Haggerty wrote:\n\n> >   - a global for chaining to error, like:\n> > \n> >        struct error_context print_errors = {\n> >           error, /* actually a wrapper to handle va_list and NULL data */\n> >           NULL\n> >        };\n> \n> There could also be a global for chaining to `warn()` or `die()`.\n\nI played around a little with this. The latter actually makes a lot of\ncode cleaner, because we can rely on the functions not returning at all.\nSo for example, you get:\n\ndiff --git a/branch.c b/branch.c\nindex a5a8dcb..53404b8 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -303,17 +303,13 @@ void create_branch(const char *head,\n \n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction ||\n-\t\t    ref_transaction_update(transaction, ref.buf,\n-\t\t\t\t\t   sha1, forcing ? NULL : null_sha1,\n-\t\t\t\t\t   0, msg, &err) ||\n-\t\t    ref_transaction_commit(transaction, &err))\n-\t\t\tdie(\"%s\", err.buf);\n+\n+\t\ttransaction = ref_transaction_begin(&error_die);\n+\t\tref_transaction_update(transaction, ref.buf,\n+\t\t\t\t       sha1, forcing ? NULL : null_sha1,\n+\t\t\t\t       0, msg, &error_die);\n+\t\tref_transaction_commit(transaction, &error_die);\n \t\tref_transaction_free(transaction);\n-\t\tstrbuf_release(&err);\n \t}\n \n \tif (real_ref && track)\n\nwhich is much shorter and to the point (it does rely on the called\nfunctions always calling report_error() and never just returning NULL or\n\"-1\", but that should be the already. If it isn't, we'd be printing\n\"fatal: \" with no message).\n\nCases that call:\n\n  error(\"%s\", err.buf);\n\ncan drop the strbuf handling, but of course still need to retain their\nconditionals. So they're better, but not as much. I did a half-hearted\nconversion of some of the ref code that uses strbufs, and it seems like\nit would save a few hundred lines of boilerplate.\n\nThere are some cases that are _worse_, because they want to prefix the\nerror. E.g., in init-db, we have:\n\n  struct strbuf err = STRBUF_INIT;\n  ...\n  if (refs_init_db(&err))\n\tdie(\"failed to set up refs db: %s\", err.buf);\n\nwhich is fairly clean. Using an error_context adds slightly to the\nboilerplate:\n\n  struct strbuf err_buf = STRBUF_INIT;\n  struct error_context err = STRBUF_ERR(&err_buf);\n  ...\n  if (refs_init_db(&err))\n\tdie(\"failed to set up refs db: %s\", err_buf.buf);\n\nThough if we wanted to get really magical, the err_buf/err pattern could\nbe its own single-line macro.\n\nYou could solve this more generally with something like:\n\n  struct error_prefix_data err;\n\n  error_prefix(&err, &error_die, \"failed to set up refs db\");\n  refs_init_db(&err.err);\n\nwhere error_prefix() basically sets us up to call back a function which\nconcatenates the prefix to the real error, then chains to error_die.\nBut to cover all cases, error_prefix() would actually have to format the\nprefix string. Because some callers would be more like:\n\n  error_prefix(&err, &error_print, \"unable to frob %s\", foo);\n  do_frob(foo, &err);\n\nWe can't just save the va_list passed to error_prefix(), because it's\nnot valid after we return. So you have to format the prefix into a\nbuffer, even though in most cases we won't see an error at all (and\ndoing it completely correctly would involve using a strbuf, which means\nthere needs to be a cleanup step; yuck).\n\n-Peff\n"},{"id":"302822","messageId":"xmqqh98zkimk.fsf@gitster.mtv.corp.google.com","threadId":"44161","inReplyTo":"20160928085841.aoisson3fnuke47q@sigill.intra.peff.net","subject":"Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-28T18:02:27Z","receivedAt":"2016-09-28T18:02:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  \tif (!dont_change_ref) {\n>  \t\tstruct ref_transaction *transaction;\n> -\t\tstruct strbuf err = STRBUF_INIT;\n> -\n> -\t\ttransaction = ref_transaction_begin(&err);\n> -\t\tif (!transaction ||\n> -\t\t    ref_transaction_update(transaction, ref.buf,\n> -\t\t\t\t\t   sha1, forcing ? NULL : null_sha1,\n> -\t\t\t\t\t   0, msg, &err) ||\n> -\t\t    ref_transaction_commit(transaction, &err))\n> -\t\t\tdie(\"%s\", err.buf);\n> +\n> +\t\ttransaction = ref_transaction_begin(&error_die);\n> +\t\tref_transaction_update(transaction, ref.buf,\n> +\t\t\t\t       sha1, forcing ? NULL : null_sha1,\n> +\t\t\t\t       0, msg, &error_die);\n> +\t\tref_transaction_commit(transaction, &error_die);\n>  \t\tref_transaction_free(transaction);\n> -\t\tstrbuf_release(&err);\n>  \t}\n>  \n>  \tif (real_ref && track)\n>\n> which is much shorter and to the point (it does rely on the called\n> functions always calling report_error() and never just returning NULL or\n> \"-1\", but that should be the already. If it isn't, we'd be printing\n> \"fatal: \" with no message).\n\nYes but... grepping for die() got a lot harder, which may not be a\ngood thing.\n\nI do like the flexibility such a mechanism offers, but\nwrapping/hiding die in it is probably an example that the\nflexibility went a bit too far.\n"}]}