{"thread":{"id":"46419","subject":"[PATCH] fsck: remove redundant parse_tree() invocation","startedAt":"2017-07-18T22:29:01Z","lastAt":"2017-07-19T23:26:56Z","messageCount":2,"participants":["Jonathan Tan","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324752","messageId":"20170718222848.1453-1-jonathantanmy@google.com","threadId":"46419","inReplyTo":null,"subject":"[PATCH] fsck: remove redundant parse_tree() invocation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-07-18T22:28:48Z","receivedAt":"2017-07-18T22:29:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If obj->type == OBJ_TREE, an invocation of fsck_walk() will invoke\nparse_tree() and return quickly if that returns nonzero, so it is of no\nuse for traverse_one_object() to invoke parse_tree() in this situation\nbefore invoking fsck_walk(). Remove that code.\n\nThe behavior of traverse_one_object() is changed slightly in that it now\nreturns -1 instead of 1 in the case that parse_tree() fails, but this is\nnot an issue because its only caller (traverse_reachable) does not care\nabout the value as long as it is nonzero.\n\nThis code was introduced in commit 271b8d2 (\"builtin-fsck: move away\nfrom object-refs to fsck_walk\", 2008-02-25). The same issue existed in\nthat commit.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nHere's a code cleanup. I noticed this while looking at modifying fsck.\n---\n builtin/fsck.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 99dea7adf..4ba311cda 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -168,18 +168,7 @@ static void mark_object_reachable(struct object *obj)\n \n static int traverse_one_object(struct object *obj)\n {\n-\tint result;\n-\tstruct tree *tree = NULL;\n-\n-\tif (obj->type == OBJ_TREE) {\n-\t\ttree = (struct tree *)obj;\n-\t\tif (parse_tree(tree) < 0)\n-\t\t\treturn 1; /* error already displayed */\n-\t}\n-\tresult = fsck_walk(obj, obj, &fsck_walk_options);\n-\tif (tree)\n-\t\tfree_tree_buffer(tree);\n-\treturn result;\n+\treturn fsck_walk(obj, obj, &fsck_walk_options);\n }\n \n static int traverse_reachable(void)\n-- \n2.13.2.932.g7449e964c-goog\n\n"},{"id":"324776","messageId":"20170719232541.GA13924@aiede.mtv.corp.google.com","threadId":"46419","inReplyTo":"20170718222848.1453-1-jonathantanmy@google.com","subject":"Re: [PATCH] fsck: remove redundant parse_tree() invocation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-19T23:25:41Z","receivedAt":"2017-07-19T23:26:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> If obj->type == OBJ_TREE, an invocation of fsck_walk() will invoke\n> parse_tree() and return quickly if that returns nonzero, so it is of no\n> use for traverse_one_object() to invoke parse_tree() in this situation\n> before invoking fsck_walk(). Remove that code.\n\nI like the diffstat.  I'm trying to figure out what the old code was\ntrying to do, since that will make it easier to understand this\nchange.\n\nfsck_walk_tree calls parse_tree, so why did we need to parse it in\nadvance?  Was it just about handling the error differently when it\nfails to parse?\n\nIt appears this code comes from\nhttps://public-inbox.org/git/20080214090013.GK24004@spearce.org/:\n\n +\tif (parse_tree(item) < 0)\n +\t\treturn; /* error already displayed */\n +\n +\tinit_tree_desc(&desc, item->buffer, item->size);\n\nAt that point the parse_tree call was not redundant.  Later it must\nhave been amended and cleaned up to use fsck_walk.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for cleaning up.\n"}]}