[PATCH] fsck: remove redundant parse_tree() invocation

Jonathan Tan· Jul 18, 2017, 22:28 UTC · lore

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 <jonathantanmy@google.com>
---
Here's a code cleanup. I noticed this while looking at modifying fsck.
---
 builtin/fsck.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Show changes to builtin/fsck.c +1 −12

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· Jul 19, 2017, 23:25 UTC · re: Jonathan Tan · lore

Re: [PATCH] fsck: remove redundant parse_tree() invocation

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 <jrnieder@gmail.com>

Thanks for cleaning up.

patchfsck: remove redundant parse_tree() invocation