All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Patrick Steinhardt <ps@pks.im>, Taylor Blau <me@ttaylorr.com>,
	Elijah Newren <newren@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: [PATCH v2 1/6] merge-ort: add new merge_ort_generic() function
Date: Thu, 13 Mar 2025 02:46:36 +0000	[thread overview]
Message-ID: <c2f2e3e0cd646eae2801bf8fb079266f43219921.1741834001.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1875.v2.git.1741834001.gitgitgadget@gmail.com>

From: Elijah Newren <newren@gmail.com>

merge-recursive.[ch] have three entry points:
  * merge_trees()
  * merge_recursive()
  * merge_recursive_generic()
merge-ort*.[ch] only has equivalents for the first two.  Add an
equivalent for the final entry point, so we can switch callers to
use it and remove merge-recursive.[ch].

While porting it over, finally fix the issue with the label for the
ancestor (used when merge.conflictStyle=diff3 as a conflict label).
merge-recursive.c has traditionally not allowed callers to set that
label, but I have found that problematic for years.

(Side note: This function was initially part of the merge-ort rewrite,
but reviewers questioned the ancestor label funnyness which I was
never really happy with anyway.  It resulted in me jettisoning it and
hoping at the time that I would eventually be able to force the existing
callers to use some other API.  That worked with `git stash`, as per
874cf2a60444 (stash: apply stash using 'merge_ort_nonrecursive()',
2022-05-10), but this API is the most reasonable one for `git am` and
`git merge-recursive`, if we can just allow them some freedom over the
ancestor label.)

The merge_recursive_generic() function did not know whether it was being
invoked by `git stash`, `git merge-recursive`, or `git am`, and the
choice of meaningful ancestor label, when there is a unique ancestor,
varies for these different callers:

  * git am: ancestor is a constructed "fake ancestor" that user knows
            nothing about and has no access to.  (And is different than
            the normal thing we mean by a "virtual merge base" which is
            the merging of merge bases.)
  * git merge-recursive: ancestor might be a tree, but at least it
                         was one specified by the user (if they invoked
                         merge-recursive directly)
  * git stash: ancestor was the commit serving as the stash base

Thus, using a label like "constructed merge base" (as
merge_recursive_generic() does) presupposes that `git am` is the only
caller; it is incorrect for other callers.  This label has thrown me off
more than once.  Allow the caller to override when there is a unique
merge base.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort-wrappers.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
 merge-ort-wrappers.h | 12 +++++++++
 merge-ort.c          | 17 ++++++++----
 merge-ort.h          |  5 ++++
 4 files changed, 93 insertions(+), 5 deletions(-)

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
@@ -1,9 +1,13 @@
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "hash.h"
+#include "hex.h"
+#include "lockfile.h"
 #include "merge-ort.h"
 #include "merge-ort-wrappers.h"
 #include "read-cache-ll.h"
+#include "repository.h"
+#include "tag.h"
 #include "tree.h"
 
 #include "commit.h"
@@ -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;
+}
+
+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;
+}
diff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h
index 90af1f69c55..aeffa1c87b4 100644
--- a/merge-ort-wrappers.h
+++ b/merge-ort-wrappers.h
@@ -22,4 +22,16 @@ int merge_ort_recursive(struct merge_options *opt,
 			const struct commit_list *ancestors,
 			struct commit **result);
 
+/*
+ * rename-detecting three-way merge.  num_merge_bases must be at least 1.
+ * Recursive ancestor consolidation will be performed if num_merge_bases > 1.
+ * Wrapper mimicking the old merge_recursive_generic() function.
+ */
+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);
+
 #endif
diff --git a/merge-ort.c b/merge-ort.c
index 46e78c3ffa6..b4ff24403a1 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4878,9 +4878,9 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)
 	c->maybe_tree = t;
 }
 
-static struct commit *make_virtual_commit(struct repository *repo,
-					  struct tree *tree,
-					  const char *comment)
+struct commit *make_virtual_commit(struct repository *repo,
+				   struct tree *tree,
+				   const char *comment)
 {
 	struct commit *commit = alloc_commit_node(repo);
 
@@ -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);
diff --git a/merge-ort.h b/merge-ort.h
index 82f2b3222d2..b63bc5424e7 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -44,6 +44,11 @@ struct merge_result {
 	unsigned _properly_initialized;
 };
 
+/* Mostly internal function also used by merge-ort-wrappers.c */
+struct commit *make_virtual_commit(struct repository *repo,
+				   struct tree *tree,
+				   const char *comment);
+
 /*
  * rename-detecting three-way merge with recursive ancestor consolidation.
  * working tree and index are untouched.
-- 
gitgitgadget


  reply	other threads:[~2025-03-13  2:46 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-07 15:48 [PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch] Elijah Newren via GitGitGadget
2025-03-07 15:48 ` [PATCH 1/3] merge-ort: add new merge_ort_generic() function Elijah Newren via GitGitGadget
2025-03-12  8:06   ` Patrick Steinhardt
2025-03-12 20:00     ` Taylor Blau
2025-03-12 21:39     ` Elijah Newren
2025-03-07 15:48 ` [PATCH 2/3] merge-ort: allow rename detection to be disabled Elijah Newren via GitGitGadget
2025-03-12  8:06   ` Patrick Steinhardt
2025-03-12 20:02     ` Taylor Blau
2025-03-12 21:40       ` Elijah Newren
2025-03-12 21:50         ` Taylor Blau
2025-03-13  5:25         ` Jeff King
2025-03-07 15:48 ` [PATCH 3/3] merge-ort: support having merge verbosity be set to 0 Elijah Newren via GitGitGadget
2025-03-12 20:03   ` Taylor Blau
2025-03-12 21:44     ` Elijah Newren
2025-03-12 21:50       ` Taylor Blau
2025-03-12  8:06 ` [PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch] Patrick Steinhardt
2025-03-12 20:05   ` Taylor Blau
2025-03-13  2:46 ` [PATCH v2 0/6] " Elijah Newren via GitGitGadget
2025-03-13  2:46   ` Elijah Newren via GitGitGadget [this message]
2025-03-13  2:46   ` [PATCH v2 2/6] merge-ort: allow rename detection to be disabled Elijah Newren via GitGitGadget
2025-03-13  2:46   ` [PATCH v2 3/6] merge-ort: support having merge verbosity be set to 0 Elijah Newren via GitGitGadget
2025-03-13  2:46   ` [PATCH v2 4/6] t3650: document bug when directory renames are turned off Johannes Schindelin via GitGitGadget
2025-03-13  2:46   ` [PATCH v2 5/6] merge-ort: fix merge.directoryRenames=false Elijah Newren via GitGitGadget
2025-03-13  2:46   ` [PATCH v2 6/6] am: switch from merge_recursive_generic() to merge_ort_generic() Elijah Newren via GitGitGadget
2025-03-17 21:25   ` [PATCH v2 0/6] Small new merge-ort features, prepping for deletion of merge-recursive.[ch] Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c2f2e3e0cd646eae2801bf8fb079266f43219921.1741834001.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.