Skip to content

Commit 6d4740c

Browse files
committed
revision: handle all NULL returns from get_reference() in add_parents_only
add_parents_only() resolves revision suffixes like commit^@ (all parents), commit^! (commit minus parents), and commit^-N (exclude Nth parent). It calls get_reference() in a loop to peel through tag objects until it reaches a commit. The existing NULL check after get_reference() only handles the ignore_missing case: if (!it && revs->ignore_missing) return 0; if (it->type != OBJ_TAG) /* crashes when it == NULL */ But get_reference() can return NULL through three distinct paths: 1. revs->ignore_missing (line 360): the user asked to silently skip missing objects. 2. revs->exclude_promisor_objects (line 363): the object is a lazy promisor object that should be excluded from the walk. 3. revs->do_not_die_on_missing_objects (line 364): the caller wants to record missing OIDs for later reporting (used by "git rev-list --missing=print") rather than dying. When NULL comes from paths 2 or 3, the code falls through to it->type and dereferences the NULL pointer. Handle all three cases explicitly: - ignore_missing: return 0, matching the existing behavior and the pattern in handle_revision_arg() (line 2236). - do_not_die_on_missing_objects: return 0. The missing OID has already been recorded in revs->missing_commits by get_reference() (line 365). Returning 0 is consistent with handle_revision_arg() (line 2236) and process_parents() (line 1197), both of which continue without error when this flag is set. The broader codebase pattern for this flag is "record and continue": list-objects.c, builtin/rev-list.c, and process_parents all skip the die/error and keep walking. - everything else (only the exclude_promisor_objects case in practice): return -1, consistent with handle_revision_arg() at line 2236 where the condition only matches ignore_missing or do_not_die_on_missing_objects, falling through to ret = -1 for the promisor case. Note: the callers of add_parents_only() in handle_revision_pseudo_opt() treat any nonzero return as "handled" (line 2186: "if (add_parents_only(...)) { ret = 0; }"), so the -1 for the promisor case is indistinguishable from success there. This means a promisor-excluded tag target referenced via commit^@ would be silently skipped rather than producing an error. This is a pre-existing limitation of the caller's return value handling and not made worse by this change; the alternative (a NULL dereference crash) was strictly worse. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
1 parent a204bdd commit 6d4740c

1 file changed

Lines changed: 7 additions & 2 deletions

File tree

revision.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
19031903
return 0;
19041904
while (1) {
19051905
it = get_reference(revs, arg, &oid, 0);
1906-
if (!it && revs->ignore_missing)
1907-
return 0;
1906+
if (!it) {
1907+
if (revs->ignore_missing)
1908+
return 0;
1909+
if (revs->do_not_die_on_missing_objects)
1910+
return 0;
1911+
return -1;
1912+
}
19081913
if (it->type != OBJ_TAG)
19091914
break;
19101915
if (!((struct tag*)it)->tagged)

0 commit comments

Comments
 (0)