On Wed, Mar 12, 2025 at 1:06 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> 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.
Yeah, it looks like when I was splitting commits, I had more of this
explanation in a commit which was explaining differences in testcases.
I'll copy the relevant bits here. Thanks for reading carefully.
>
> 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
