parse_commit_buffer(): treat lookup_commit() failure as parse error

While parsing the parents of a commit, if we are able to parse an actual oid but lookup_commit() fails on it (because we previously saw it in this process as a different object type), we silently omit the parent and do not report any error to the caller. The caller has no way of knowing this happened, because even an empty parent list is a valid parse result. As a result, it's possible to fool our "rev-list" connectivity check into accepting a corrupted set of objects. There's a test for this case already in t6102, but unfortunately it has a slight error. It creates a broken commit with a parent line pointing to a blob, and then checks that rev-list notices the problem in two cases: 1. the "lone" case: we traverse the broken commit by itself (here we try to actually load the blob from disk and find out that it's not a commit) 2. the "seen" case: we parse the blob earlier in the process, and then when calling lookup_commit() we realize immediately that it's not a commit The "seen" variant for this test mistakenly parsed another commit instead of the blob, meaning that we were actually just testing the "lone" case again. Changing that reveals the breakage (and shows that this fixes it). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 18, 2019 at 00:42 UTC c78fe00459d49cd57cbfabc5c564af0cb9a934f1

2 files changed +9 -4

commit.c

+8 -3

@@ -432,8 +432,11 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b

432 if (graft && (graft->nr_parent < 0 || grafts_replace_parents))

433 continue;

434 new_parent = lookup_commit(r, &parent);

435 - if (new_parent)

436 - pptr = &commit_list_insert(new_parent, pptr)->next;

435 + if (!new_parent)

436 + return error("bad parent %s in commit %s",

437 + oid_to_hex(&parent),

438 + oid_to_hex(&item->object.oid));

439 + pptr = &commit_list_insert(new_parent, pptr)->next;

440 }

441 if (graft) {

442 int i;

@@ -442,7 +445,9 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b

445 new_parent = lookup_commit(r,

446 &graft->parent[i]);

447 if (!new_parent)

445 - continue;

448 + return error("bad graft parent %s in commit %s",

449 + oid_to_hex(&graft->parent[i]),

450 + oid_to_hex(&item->object.oid));

451 pptr = &commit_list_insert(new_parent, pptr)->next;

452 }

453 }

t/t6102-rev-list-unexpected-objects.sh

+1 -1

@@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '

52 '

54 test_expect_success 'traverse unexpected non-commit parent (seen)' '

55 - test_must_fail git rev-list --objects $commit $broken_commit \

55 + test_must_fail git rev-list --objects $blob $broken_commit \

56 >output 2>&1 &&

57 test_i18ngrep "not a commit" output

58 '