Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`

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

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Some functions in Git's source code follow the convention that returning
> a negative value indicates a fatal error, e.g. repository corruption.
>
> Let's use this convention in `repo_in_merge_bases()` to report when one
> of the specified commits is missing (i.e. when `repo_parse_commit()`
> reports an error).
>
> Also adjust the callers of `repo_in_merge_bases()` to handle such
> negative return values.

All of the above makes sense, but I have to wonder if this hunk
should rather want to be part of the previous step:

> @@ -486,10 +488,10 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
>  	timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
>  
>  	if (repo_parse_commit(r, commit))
> -		return ret;
> +		return ignore_missing_commits ? 0 : -1;
>  	for (i = 0; i < nr_reference; i++) {
>  		if (repo_parse_commit(r, reference[i]))
> -			return ret;
> +			return ignore_missing_commits ? 0 : -1;
>  
>  		generation = commit_graph_generation(reference[i]);
>  		if (generation > max_generation)

as this hunk is not about many callers of repo_in_merge_bases() that
ignored the return values, which are all fixed by this patch, but
about returning that error signal back to the caller.

Yes, I know you wrote in [02/11] that it does not change the
behaviour, and if you move this hunk to [02/11], it might change the
behaviour, but that is changing for the better.  Besides, adding a
parameter "ignore_missing" to the function only to be ignored until
the next patch feels rather incomplete.

The other changes in this patch about its primary theme, fixing the
callers that used to ignore return values of repo_in_merge_bases(),
all looked sensible.  This hunk somehow stood out like a sore thumb
to me.