| Message ID | cb035ac5fe4ab18b697eff42afedcab62880ceec.1614905738.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Complete merge-ort implementation...almost | expand |
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); > /* 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. > 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);
On Mon, Mar 8, 2021 at 4:49 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > 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. > > /* 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. > > > 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); >
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); /* 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); + 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);