[PATCH 08/11] revision: avoid dereferencing NULL in `add_parents_only()`

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This function 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, but get_reference() can return NULL through three
distinct paths:

  1. revs->ignore_missing: the caller asked to silently skip missing
     objects.

  2. revs->exclude_promisor_objects: the object is a lazy promisor
     object that should be excluded from the walk.

  3. revs->do_not_die_on_missing_objects: the caller wants to record
     missing OIDs for later reporting (used by `git rev-list
     --missing=print`) rather than dying.

In the latter two instances, the code falls through to dereference the
NULL pointer.

Handle all three cases explicitly:

  - ignore_missing: return 0, matching the existing behavior and
    the pattern in `handle_revision_arg()`.

  - do_not_die_on_missing_objects: return 0. The missing OID has already
    been recorded in `revs->missing_commits` by `get_reference()`.
    Returning 0 is consistent with `handle_revision_arg()` and
    `process_parents()`, 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()` 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"
(`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) _would be_
strictly worse.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 revision.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index e91d7e1f11..7f3999b551 100644
--- a/revision.c
+++ b/revision.c
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 		return 0;
 	while (1) {
 		it = get_reference(revs, arg, &oid, 0);
-		if (!it && revs->ignore_missing)
-			return 0;
+		if (!it) {
+			if (revs->ignore_missing)
+				return 0;
+			if (revs->do_not_die_on_missing_objects)
+				return 0;
+			return -1;
+		}
 		if (it->type != OBJ_TAG)
 			break;
 		if (!((struct tag*)it)->tagged)
-- 
gitgitgadget