Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled

[Elijah Newren <newren@gmail.com>]

On Wed, Mar 12, 2025 at 1:02 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Mar 12, 2025 at 09:06:35AM +0100, Patrick Steinhardt wrote:
> > On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > [...] Also, there has been at least one request to
> > > allow rename detection to be disabled for behavioral rather than
> > > performance reasons, so let's start heeding the config and command line
> > > settings.
> >
> > It might be nice to provide a link to that request for more context.

Will add.

> I don't know if a link exists; I suspect the request referred to here is
> an email that Johannes Schindelin wrote to Elijah privately).

It exists: https://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/

...which wasn't Johannes' request.

> But I am almost certain that the behavior requested here is to disable
> rename detection to match the behavior of GitHub's prior use of libgit2
> to perform merges, where we also had rename detection disabled (for
> reasons that are unclear to me, but Peff might know).

No, if that were the sole reason, I'd say it probably only belongs in
our internal fork.  Disabling of rename detection within GitHub was a
temporary internal migration measure, not a desired end state -- at
least that's the way Johannes portrayed it to me.  I know that
"temporary" sometimes lasts longer than we want, but now that I've
become internal to GitHub, one of the things I want to do is add some
weight to that "temporary" modifier.

> > > diff --git a/merge-ort.c b/merge-ort.c
> > > index b4ff24403a1..a6960b6a1b4 100644
> > > --- a/merge-ort.c
> > > +++ b/merge-ort.c
> > > @@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)
> > >
> > >     if (!possible_renames(renames))
> > >             goto cleanup;
> > > +   if (opt->detect_renames == 0) {
>
>     if (!opt->detect_renames)
>
> ?

Yeah, I wanted an opt->detect_renames == DIFF_DETECT_NONE, but we
never defined that and only defined DIFF_DETECT_RENAME and
DIFF_DETECT_COPY.  I'll switch it over.

> > > +           renames->redo_after_renames = 0;
> > > +           renames->cached_pairs_valid_side = 0;
> > > +           goto cleanup;
> > > +   }
> > >
> > >     trace2_region_enter("merge", "regular renames", opt->repo);
> > >     detection_run |= detect_regular_renames(opt, MERGE_SIDE1);
> >
> > Do we want to add a test that demonstrates that the option works as
> > expected?
>
> Yeah, having a test here would be nice.

Will add.