[05/23] fsck: stop checking commit->tree value
diff mbox series

Message ID 20191018044820.GE17879@sigill.intra.peff.net
State New
Headers show
Series
  • parsing and fsck cleanups
Related show

Commit Message

Jeff King Oct. 18, 2019, 4:48 a.m. UTC
We check in fsck_commit_buffer() that commit->tree isn't NULL, which in
turn generally comes from a previous parse by parse_commit(). But this
isn't really accomplishing anything. The two things we might care about
are:

  - was there a syntactically valid "tree <oid>" line in the object? But
    we've just done our own parse in fsck_commit_buffer() to check this.

  - does it point to a valid tree object? But checking the "tree"
    pointer here doesn't actually accomplish that; it just shows that
    lookup_tree() didn't return NULL, which only means that we haven't
    yet seen that oid as a non-tree in this process.

    A real connectivity check would exhaustively walk all graph links,
    and we do that already in a separate function.

So this code isn't helping anything. And it makes the fsck code slightly
more confusing and rigid (e.g., it requires that any commit structs have
already been parsed). Let's drop it.

As a bit of history, the presence of this code looks like a leftover
from early fsck code (which did rely on parse_commit() to do most of the
parsing). The check comes from ff5ebe39b0 (Port fsck-cache to use
parsing functions, 2005-04-18), but we later added an explicit walk in
355885d531 (add generic, type aware object chain walker, 2008-02-25).

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Junio C Hamano Oct. 24, 2019, 3:57 a.m. UTC | #1
Jeff King <peff@peff.net> writes:

> We check in fsck_commit_buffer() that commit->tree isn't NULL, which in
> ...
> So this code isn't helping anything. And it makes the fsck code slightly
> more confusing and rigid (e.g., it requires that any commit structs have
> already been parsed). Let's drop it.

Thanks; I recall we discussed this during a review of a topic.

Patch
diff mbox series

diff --git a/fsck.c b/fsck.c
index cdb7d8db03..6dfc533fb0 100644
--- a/fsck.c
+++ b/fsck.c
@@ -800,11 +800,6 @@  static int fsck_commit_buffer(struct commit *commit, const char *buffer,
 	err = fsck_ident(&buffer, &commit->object, options);
 	if (err)
 		return err;
-	if (!get_commit_tree(commit)) {
-		err = report(options, &commit->object, FSCK_MSG_BAD_TREE, "could not load commit's tree %s", oid_to_hex(&tree_oid));
-		if (err)
-			return err;
-	}
 	if (memchr(buffer_begin, '\0', size)) {
 		err = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,
 			     "NUL byte in the commit object body");