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

From: Elijah Newren <newren@gmail.com>
Date: Mar 12, 2025, 21:40 UTC
Message-ID: <CABPp-BFj08mpXGiSoZ3xZ4KamwTJ4k5wPLTCJh6hUtKSxWE52Q@mail.gmail.com>
In-Reply-To: <Z9Hobtp+9esKkY/O@nand.local>

On Wed, Mar 12, 2025 at 1:02 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 11 quoted lines

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

Show 13 quoted lines

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

Show 12 quoted lines

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