# [PATCH] fsck: remove redundant parse_tree() invocation 2 messages from 2017-07-18 to 2017-07-19. Participants: Jonathan Tan, Jonathan Nieder. Thread: https://gitlist.dev/t/46419 ## Jonathan Tan, 2017-07-18 22:28 Subject: [PATCH] fsck: remove redundant parse_tree() invocation Message-ID: <20170718222848.1453-1-jonathantanmy@google.com> URL: https://gitlist.dev/e/20170718222848.1453-1-jonathantanmy%40google.com ``` If obj->type == OBJ_TREE, an invocation of fsck_walk() will invoke parse_tree() and return quickly if that returns nonzero, so it is of no use for traverse_one_object() to invoke parse_tree() in this situation before invoking fsck_walk(). Remove that code. The behavior of traverse_one_object() is changed slightly in that it now returns -1 instead of 1 in the case that parse_tree() fails, but this is not an issue because its only caller (traverse_reachable) does not care about the value as long as it is nonzero. This code was introduced in commit 271b8d2 ("builtin-fsck: move away from object-refs to fsck_walk", 2008-02-25). The same issue existed in that commit. Signed-off-by: Jonathan Tan --- Here's a code cleanup. I noticed this while looking at modifying fsck. --- builtin/fsck.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index 99dea7adf..4ba311cda 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -168,18 +168,7 @@ static void mark_object_reachable(struct object *obj) static int traverse_one_object(struct object *obj) { - int result; - struct tree *tree = NULL; - - if (obj->type == OBJ_TREE) { - tree = (struct tree *)obj; - if (parse_tree(tree) < 0) - return 1; /* error already displayed */ - } - result = fsck_walk(obj, obj, &fsck_walk_options); - if (tree) - free_tree_buffer(tree); - return result; + return fsck_walk(obj, obj, &fsck_walk_options); } static int traverse_reachable(void) -- 2.13.2.932.g7449e964c-goog ``` ## Jonathan Nieder, 2017-07-19 23:25 Subject: Re: [PATCH] fsck: remove redundant parse_tree() invocation Message-ID: <20170719232541.GA13924@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20170719232541.GA13924%40aiede.mtv.corp.google.com In-Reply-To: <20170718222848.1453-1-jonathantanmy@google.com> ``` Hi, Jonathan Tan wrote: > If obj->type == OBJ_TREE, an invocation of fsck_walk() will invoke > parse_tree() and return quickly if that returns nonzero, so it is of no > use for traverse_one_object() to invoke parse_tree() in this situation > before invoking fsck_walk(). Remove that code. I like the diffstat. I'm trying to figure out what the old code was trying to do, since that will make it easier to understand this change. fsck_walk_tree calls parse_tree, so why did we need to parse it in advance? Was it just about handling the error differently when it fails to parse? It appears this code comes from https://public-inbox.org/git/20080214090013.GK24004@spearce.org/: + if (parse_tree(item) < 0) + return; /* error already displayed */ + + init_tree_desc(&desc, item->buffer, item->size); At that point the parse_tree call was not redundant. Later it must have been amended and cleaned up to use fsck_walk. Reviewed-by: Jonathan Nieder Thanks for cleaning up. ```