Re: [PATCH 04/11] merge-ort: have ll_merge() calls use the attr_index for renormalization

From: Elijah Newren <newren@gmail.com>
Date: Mar 9, 2021, 04:27 UTC
Message-ID: <CABPp-BEMAbeQAcja=MFU3_N7dUOpKWCohtyNfnTj9npep8j1Lw@mail.gmail.com>
In-Reply-To: <87a6rdkdxq.fsf@evledraar.gmail.com>

On Mon, Mar 8, 2021 at 4:49 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 30 quoted lines

>
>
> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > ll_merge() needs an index when renormalization is requested.  Give it
> > the special one we created exactly for that purpose.  This fixes t6418.4
> > and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  merge-ort.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 028d1adcd2c9..87c553c0882c 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -360,7 +360,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
> >       string_list_clear(&opti->paths_to_free, 0);
> >       opti->paths_to_free.strdup_strings = 0;
> >
> > -     if (opti->attr_index.cache_nr)
> > +     if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
> >               discard_index(&opti->attr_index);
>
> Perhaps instead of a comment, in that "if":
>
>     assert(opt->renormalize);

I would, but opt isn't defined here, and passing it in merely for an assertion seems overboard.

Show 23 quoted lines

> >       /* Free memory used by various renames maps */
> > @@ -988,7 +988,6 @@ static int merge_submodule(struct merge_options *opt,
> >       return 0;
> >  }
> >
> > -MAYBE_UNUSED
> >  static void initialize_attr_index(struct merge_options *opt)
> >  {
> >       /*
> > @@ -1063,6 +1062,8 @@ static int merge_3way(struct merge_options *opt,
> >       char *base, *name1, *name2;
> >       int merge_status;
> >
> > +     initialize_attr_index(opt);
>
> Subjective, but I think it's more readable to move the "initialized"
> check in initialize_attr_index() here, so:
>
>     if (!attr_index->initialized)
>         initialize_attr_index(opt);
>
> Saves the reader a trip to the function to see that it doesn't do
> anything except exit early on that flag.

Makes sense; I'll fix this up.

Show 14 quoted lines

>
> >       ll_opts.renormalize = opt->renormalize;
> >       ll_opts.extra_marker_size = extra_marker_size;
> >       ll_opts.xdl_opts = opt->xdl_opts;
> > @@ -1101,7 +1102,7 @@ static int merge_3way(struct merge_options *opt,
> >
> >       merge_status = ll_merge(result_buf, path, &orig, base,
> >                               &src1, name1, &src2, name2,
> > -                             opt->repo->index, &ll_opts);
> > +                             &opt->priv->attr_index, &ll_opts);
> >
> >       free(base);
> >       free(name1);
>