Hi Junio,
On Fri, 23 Feb 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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.
I wanted 2/11 to be trivial to review, and therefore specifically wanted
behavior not to change just yet: At least when I review patches, this
information helps me assess the correctness of the patch because I have a
different pair of glasses on, so to say.
> Besides, adding a parameter "ignore_missing" to the function only to be
> ignored until the next patch feels rather incomplete.
By that reasoning, the entire patch series should be squashed into a
single patch, as the missing commits will only be handled properly if all
11 patches are applied ;-)
Seriously again, I designed this patch series in a way where it builds up
incrementally, adding preparations here and there, in as easily reviewable
a shape as I could, until the final patch wraps everything in a bow.
I did this mostly to be able to convince myself of the correctness of the
patches because I sense such a vast opportunity for bugs to creep in.
> 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.
I have an idea. How about pulling out this hunk into its own patch? And
insert it between 2/11 and 3/11? That would probably make most sense, as
it would make the patch series still (relatively) easy to review, and it
would not conflate the purpose of this hunk with the rest of 3/11's hunks.
What do you think?
Ciao,
Johannes
