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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2024, 00:33 UTC
Message-ID: <xmqqle7asnu9.fsf@gitster.g>
In-Reply-To: <4dd214f91d4783f29b03908cc0034156253889a7.1708608110.git.gitgitgadget@gmail.com>

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

Show 11 quoted lines

> 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:

Show 13 quoted lines

> @@ -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.