Re: [PATCH 10/11] bisect: check get_terms return at all call sites

Hi Patrick,

On Wed, 15 Jul 2026, Patrick Steinhardt wrote:

> On Tue, Jul 14, 2026 at 10:48:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[email protected]>
> > 
> > Six callers of get_terms() silently discard its return value. When
> > get_terms fails (missing or truncated BISECT_TERMS file), the term
> > strings remain NULL or empty, causing confusing downstream
> > behavior: commands like "bisect next" or "bisect run" proceed with
> > empty term strings, producing nonsensical ref names (refs/bisect/
> > with no suffix) and misleading error messages.
> > 
> > Add checks at each call site so that a failed get_terms produces a
> > clear "no terms defined" error, matching the pattern already used
> > in bisect_terms() at line 512. The check tests the term pointers
> > rather than the return value because some callers (bisect skip,
> > legacy bad/good) call set_terms before get_terms, and the
> > set_terms values should survive a get_terms failure.
> 
> Hm. Are there any callers that accept the case where either `term->bad`
> or `term->good` are `NULL`?

As far as I can tell, no, the case where either `term->bad` or
`term->good` are `NULL` is not permissible.

> If not, should we maybe adapt the function itself to return an error if
> so and then have all callers only ever check for the return value of
> `get_term()` instead of also having to check the result? That might also
> allow us to deduplicate the error messages.

It's a good point that we should not look at `term->bad` and `term->good`,
but at the return value of `get_term()` instead. That's incidentally what
`bisect_terms()` does, and we should do the same here (including the same,
already-translated error message).

Thanks,
Johannes