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

[Patrick Steinhardt <ps@pks.im>]

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