Re: [PATCH 1/3] merge-ort: add new merge_ort_generic() function

>
> On Fri, Mar 07, 2025 at 03:48:40PM +0000, Elijah Newren via GitGitGadget wrote:
> > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
> > index d6f61359965..62834c30e9e 100644
> > --- a/merge-ort-wrappers.c
> > +++ b/merge-ort-wrappers.c
> > @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt,
> >
> >       return tmp.clean;
> >  }
> > +
> > +static struct commit *get_ref(struct repository *repo,
> > +                           const struct object_id *oid,
> > +                           const char *name)
> > +{
> > +     struct object *object;
> > +
> > +     object = deref_tag(repo, parse_object(repo, oid),
> > +                        name, strlen(name));
> > +     if (!object)
> > +             return NULL;
> > +     if (object->type == OBJ_TREE)
> > +             return make_virtual_commit(repo, (struct tree*)object, name);
> > +     if (object->type != OBJ_COMMIT)
> > +             return NULL;
> > +     if (repo_parse_commit(repo, (struct commit *)object))
> > +             return NULL;
> > +     return (struct commit *)object;
> > +}
>
> This is an exact copy of the same function in "merge-recursive.c".
>
> > +int merge_ort_generic(struct merge_options *opt,
> > +                   const struct object_id *head,
> > +                   const struct object_id *merge,
> > +                   int num_merge_bases,
> > +                   const struct object_id *merge_bases,
> > +                   struct commit **result)
> > +{
> > +     int clean;
> > +     struct lock_file lock = LOCK_INIT;
> > +     struct commit *head_commit = get_ref(opt->repo, head, opt->branch1);
> > +     struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2);
> > +     struct commit_list *ca = NULL;
> > +
> > +     if (merge_bases) {
> > +             int i;
> > +             for (i = 0; i < num_merge_bases; ++i) {
> > +                     struct commit *base;
> > +                     if (!(base = get_ref(opt->repo, &merge_bases[i],
> > +                                          oid_to_hex(&merge_bases[i]))))
> > +                             return error(_("Could not parse object '%s'"),
> > +                                          oid_to_hex(&merge_bases[i]));
> > +                     commit_list_insert(base, &ca);
> > +             }
> > +     }
> > +
> > +     repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);
> > +     clean = merge_ort_recursive(opt, head_commit, next_commit, ca,
> > +                                 result);
> > +     free_commit_list(ca);
> > +     if (clean < 0) {
> > +             rollback_lock_file(&lock);
> > +             return clean;
> > +     }
> > +
> > +     if (write_locked_index(opt->repo->index, &lock,
> > +                            COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > +             return error(_("Unable to write index."));
> > +
> > +     return clean ? 0 : 1;
> > +}
>
> There are two differences here:
>
>   - We use `error()` instead of the custom `err()` function that
>     "merge-recursive.c" uses. I'm happy to see us using standard error
>     reporting.
>
>   - We don't have the check for `num_merge_bases == 1`. I have no idea
>     why we don't have it, and it's likely that other readers may be
>     puzzled in the same way. So this is something I'd expect to see
>     explained in the commit message.

>
> Other than that this function looks identical.
>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 46e78c3ffa6..b4ff24403a1 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt,
> >               ancestor_name = "empty tree";
> >       } else if (merge_bases) {
> >               ancestor_name = "merged common ancestors";
> > +     } else if (opt->ancestor) {
> > +             ancestor_name = opt->ancestor;
> >       } else {
> >               strbuf_add_unique_abbrev(&merge_base_abbrev,
> >                                        &merged_merge_bases->object.oid,
> > @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt,
> >  {
> >       trace2_region_enter("merge", "incore_recursive", opt->repo);
> >
> > -     /* We set the ancestor label based on the merge_bases */
> > -     assert(opt->ancestor == NULL);
> > +     /*
> > +      * We set the ancestor label based on the merge_bases...but we
> > +      * allow one exception through so that builtin/am can override
> > +      * with its constructed fake ancestor.
> > +      */
> > +     assert(opt->ancestor == NULL ||
> > +            (merge_bases && !merge_bases->next));
> >
> >       trace2_region_enter("merge", "merge_start", opt->repo);
> >       merge_start(opt, result);
>
> These two hunks look related to my above observation that we don't have
> the check for `num_merge_bases == 1`, as in "merge-recursive.c" we used
> to set `opt->ancestor = "constructed merge base" if so.
>
> Patrick