Re: [PATCH 06/11] bisect: handle NULL commit in `bisect_successful()`

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index e7c2d2f3bb..6ff600c856 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms)
>  
>  	refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
>  	commit = lookup_commit_reference_by_name(bad_ref);
> +	if (!commit) {
> +		res = error(_("could not find commit for '%s'"), bad_ref);
> +		free(bad_ref);
> +		return res;
> +	}

Catching this case as an error is the right thing to do, but there is
a bit of an impedance mismatch between the return value from error()
and the status passed around in the bisect codebase.

The bisect.h header defines an enum bisect_error type, and I think
the sole caller of this function, bisect_next(), expects to see
BISECT_FAILED.  It may happen to be the same -1 that error()
returns, but for longer term maintainability, I would prefer to see
it done more like:

	error(_("..."));
	free(bad_ref);
	return BISECT_FAILED;

or something along those lines.

Thanks.

>  	repo_format_commit_message(the_repository, commit, "%s", &commit_name,
>  				   &pp);