launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32457
[Merge] ~ines-almeida/turnip:add-merge-api into turnip:master
Ines Almeida has proposed merging ~ines-almeida/turnip:add-merge-api into turnip:master.
Commit message:
Add new merge API
The new API allows for doing regular merges with a merge commit. Other merge strategies would need to be implemented separately
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/turnip/+git/turnip/+merge/485401
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/turnip:add-merge-api into turnip:master.
diff --git a/turnip/api/store.py b/turnip/api/store.py
index 16d324b..a24b5aa 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -23,6 +23,7 @@ from pygit2 import (
InvalidSpecError,
Oid,
Repository,
+ Signature,
init_repository,
)
from twisted.web import xmlrpc
@@ -711,6 +712,96 @@ def get_merge_diff(
}
+class MergeConflicts(Exception):
+ pass
+
+
+class BranchNotFoundError(Exception):
+ pass
+
+
+def merge(
+ repo_store,
+ repo_name,
+ target_branch,
+ source_branch,
+ committer_name,
+ committer_email,
+ commit_message=None,
+):
+ """Do a regular merge from source branch into target branch.
+
+ This currently only supports a regular merge with a merge commit, other
+ merge strategies still need to be implemented.
+
+ :param repo_store: path to the repository store
+ :param repo_name: name of the repository
+ :param target_branch: target branch to merge into
+ :param source_branch: source branch to merge from
+ :param committer_name: name of the committer
+ :param committer_email: email of the committer
+ :param commit_message: [optional] custom commit message
+ """
+
+ with open_repo(repo_store, repo_name) as repo:
+ try:
+ target_ref = repo.lookup_reference(f"refs/heads/{target_branch}")
+ source_ref = repo.lookup_reference(f"refs/heads/{source_branch}")
+ target_tip = repo[target_ref.target]
+ source_tip = repo[source_ref.target]
+ except (KeyError, ValueError) as e:
+ raise BranchNotFoundError(f"Branch not found: {str(e)}")
+
+ original_target_tip = target_ref.target
+
+ # Check if source is already included in target
+ common_ancestor_id = repo.merge_base(target_tip.oid, source_tip.oid)
+ if common_ancestor_id == source_ref.target:
+ return {"merge_commit": None}
+
+ # Create an in-memory index for the merge
+ index = repo.merge_commits(target_tip, source_tip)
+ if index.conflicts is not None:
+ raise MergeConflicts("Merge conflicts detected")
+
+ tree_id = index.write_tree(repo)
+
+ committer = Signature(committer_name, committer_email)
+ if commit_message is None:
+ commit_message = (
+ f"Merge branch '{source_branch}' into {target_branch}"
+ )
+
+ # Verify that branch hasn't changed since the start of the merge
+ current_target_ref = repo.lookup_reference(
+ f"refs/heads/{target_branch}"
+ )
+ if original_target_tip != current_target_ref.target:
+ raise GitError("Target branch was modified during operation")
+
+ # Create a merge commit that has both branch tips as parents to
+ # preserve the commit history.
+ #
+ # This is the only write operation in this function. Since it's
+ # a single atomic operation, we don't need additional safety
+ # mechanisms: if the operation fails, no changes are made; if it
+ # succeeds, the merge is complete.
+ #
+ # Note also that `create_commit` will raise a GitError a new
+ # commit is pushed to the target branch since the start of this
+ # merge.
+ merge_commit = repo.create_commit(
+ target_ref.name,
+ committer,
+ committer,
+ commit_message,
+ tree_id,
+ [target_ref.target, source_ref.target],
+ )
+
+ return {"merge_commit": merge_commit.hex}
+
+
def get_diff(repo_store, repo_name, sha1_from, sha1_to, context_lines=3):
"""Get patch and associated commits of two sha1s.
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 9bcfda0..fed5546 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -1456,6 +1456,197 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
)
self.assertEqual(404, resp.status_code)
+ def test_merge_successful(self):
+ """Test a successful merge between two branches."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ feature_commit = factory.add_commit(
+ "feature", "file.txt", parents=[initial_commit]
+ )
+ repo.create_branch("feature", repo.get(feature_commit))
+
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ )
+
+ self.assertEqual(200, resp.status_code)
+ self.assertIsNotNone(resp.json["merge_commit"])
+
+ merge_commit = repo.get(resp.json["merge_commit"])
+ self.assertEqual(merge_commit.parents[0].hex, initial_commit.hex)
+ self.assertEqual(merge_commit.parents[1].hex, feature_commit.hex)
+
+ self.assertEqual(
+ repo.references["refs/heads/main"].target.hex,
+ resp.json["merge_commit"],
+ )
+
+ def test_merge_already_included(self):
+ """Test merge when source is already included in target."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ feature_commit = factory.add_commit(
+ "feature", "file.txt", parents=[initial_commit]
+ )
+ repo.create_branch("feature", repo.get(feature_commit))
+
+ self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ )
+
+ # Try to merge again
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ )
+
+ self.assertEqual(200, resp.status_code)
+ self.assertIsNone(resp.json["merge_commit"])
+
+ def test_merge_conflicts(self):
+ """Test merge with conflicts."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ # Create conflicting changes
+ main_commit = factory.add_commit(
+ "main change", "file.txt", parents=[initial_commit]
+ )
+ repo.references["refs/heads/main"].set_target(main_commit)
+ feature_commit = factory.add_commit(
+ "feature change", "file.txt", parents=[initial_commit]
+ )
+ repo.create_branch("feature", repo.get(feature_commit))
+
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ expect_errors=True,
+ )
+
+ self.assertEqual(400, resp.status_code)
+ self.assertIn(
+ "Found conflicts between target and source branches",
+ resp.text,
+ )
+
+ def test_merge_missing_branches(self):
+ """Test merge with missing branches."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:nonexisting",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ expect_errors=True,
+ )
+
+ self.assertEqual(404, resp.status_code)
+
+ def test_merge_custom_message(self):
+ """Test merge with custom commit message."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ feature_commit = factory.add_commit(
+ "feature", "file.txt", parents=[initial_commit]
+ )
+ repo.create_branch("feature", repo.get(feature_commit))
+
+ custom_message = "Custom merge message"
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ "commit_message": custom_message,
+ },
+ )
+
+ self.assertEqual(200, resp.status_code)
+ merge_commit = repo.get(resp.json["merge_commit"])
+ self.assertEqual(merge_commit.message, custom_message)
+
+ def test_merge_no_commit_message(self):
+ """Test merge without a custom commit message."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ feature_commit = factory.add_commit(
+ "feature", "file.txt", parents=[initial_commit]
+ )
+ repo.create_branch("feature", repo.get(feature_commit))
+
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ )
+
+ self.assertEqual(200, resp.status_code)
+ merge_commit = repo.get(resp.json["merge_commit"])
+ self.assertEqual(
+ merge_commit.message, "Merge branch 'feature' into main"
+ )
+
+ def test_merge_invalid_input(self):
+ """Test merge with invalid input."""
+ factory = RepoFactory(self.repo_store)
+ initial_commit = factory.add_commit("initial", "file.txt")
+ repo = factory.build()
+ repo.create_branch("main", repo.get(initial_commit))
+ repo.set_head("refs/heads/main")
+
+ resp = self.app.post_json(
+ f"/repo/{self.repo_path}/merge/main:feature",
+ {
+ # Missing committer_email
+ "committer_name": "Test User",
+ },
+ expect_errors=True,
+ )
+
+ self.assertEqual(400, resp.status_code)
+
class AsyncRepoCreationAPI(TestCase, ApiRepoStoreMixin):
def setUp(self):
diff --git a/turnip/api/tests/test_store.py b/turnip/api/tests/test_store.py
index 647c48b..900fd14 100644
--- a/turnip/api/tests/test_store.py
+++ b/turnip/api/tests/test_store.py
@@ -670,3 +670,130 @@ class InitTestCase(TestCase):
celery_fixture.waitUntil(
10, lambda: self.hasZeroLooseObjects(orig_path)
)
+
+
+class MergeTestCase(TestCase):
+ def setUp(self):
+ super().setUp()
+ self.repo_store = self.useFixture(TempDir()).path
+ self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))
+ self.repo_path = os.path.join(self.repo_store, "repo")
+ self.factory = RepoFactory(self.repo_path)
+ self.repo = self.factory.build()
+
+ self.initial_commit = self.factory.add_commit("initial", "file.txt")
+ self.repo.create_branch("main", self.repo.get(self.initial_commit))
+ self.repo.set_head("refs/heads/main")
+
+ self.feature_commit = self.factory.add_commit(
+ "feature", "file.txt", parents=[self.initial_commit]
+ )
+ self.repo.create_branch("feature", self.repo.get(self.feature_commit))
+
+ def test_merge_successful(self):
+ """Test a successful merge between two branches."""
+ result = store.merge(
+ self.repo_store,
+ "repo",
+ "main",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ self.assertIsNotNone(result["merge_commit"])
+ merge_commit = self.repo.get(result["merge_commit"])
+ self.assertEqual(merge_commit.parents[0].hex, self.initial_commit.hex)
+ self.assertEqual(merge_commit.parents[1].hex, self.feature_commit.hex)
+
+ self.assertEqual(
+ self.repo.references["refs/heads/main"].target.hex,
+ result["merge_commit"],
+ )
+
+ def test_merge_already_included(self):
+ """Test merge when source is already included in target."""
+ store.merge(
+ self.repo_store,
+ "repo",
+ "main",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ # Try to merge again
+ result = store.merge(
+ self.repo_store,
+ "repo",
+ "main",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+ self.assertIsNone(result["merge_commit"])
+
+ def test_merge_conflicts(self):
+ """Test merge with conflicts."""
+ main_commit = self.factory.add_commit(
+ "main content", "file.txt", parents=[self.initial_commit]
+ )
+ self.repo.references["refs/heads/main"].set_target(main_commit)
+
+ feature_commit = self.factory.add_commit(
+ "feature content", "file.txt", parents=[self.initial_commit]
+ )
+ self.repo.references["refs/heads/feature"].set_target(feature_commit)
+
+ self.assertRaises(
+ store.MergeConflicts,
+ store.merge,
+ self.repo_store,
+ "repo",
+ "main",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ def test_merge_custom_message(self):
+ """Test merge with custom commit message."""
+ custom_message = "Custom merge message"
+ result = store.merge(
+ self.repo_store,
+ "repo",
+ "main",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ commit_message=custom_message,
+ )
+
+ merge_commit = self.repo.get(result["merge_commit"])
+ self.assertEqual(merge_commit.message, custom_message)
+
+ def test_merge_with_invalid_branch_names(self):
+ """Test error handling for invalid branch names."""
+ self.assertRaises(
+ store.BranchNotFoundError,
+ store.merge,
+ self.repo_store,
+ "repo",
+ "main",
+ "invalid/branch/name",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ def test_merge_with_nonexistent_branches(self):
+ """Test error handling when branches don't exist."""
+ self.assertRaises(
+ store.BranchNotFoundError,
+ store.merge,
+ self.repo_store,
+ "repo",
+ "nonexistent",
+ "feature",
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
diff --git a/turnip/api/views.py b/turnip/api/views.py
index bd5b9fd..84c3a94 100644
--- a/turnip/api/views.py
+++ b/turnip/api/views.py
@@ -446,6 +446,61 @@ class DiffMergeAPI(BaseAPI):
return patch
+@resource(path="/repo/{name}/merge/{target_branch}:{source_branch}")
+class MergeAPI(BaseAPI):
+ """Provides an HTTP API for merging.
+
+ {source_branch} will be merged into {target_branch} within
+ repo {name} (currently we don't support cross-repo)
+ """
+
+ def __init__(self, request, context=None):
+ super().__init__()
+ self.request = request
+
+ @validate_path
+ def post(self, repo_store, repo_name):
+ """Merge a source branch into a target branch.
+
+ This endpoint assumes that the committer is authorized to perform this
+ merge, i.e., it does not check for permissions. The authorization will
+ lie on the system that makes the request (e.g. Launchpad).
+ """
+ committer_name = self.request.json.get("committer_name")
+ committer_email = self.request.json.get("committer_email")
+ commit_message = self.request.json.get("commit_message")
+
+ if not committer_name or not committer_email:
+ return exc.HTTPBadRequest("Committer name and email are required")
+
+ # TODO ines-almeida 2025-04-30 we are starting with only allowing
+ # merging branches within the same repo. In Launchpad, it's very common
+ # that users what to merge cross-repo, so this is something we would
+ # need to implement soon for this to be a useful feature.
+ if len(repo_name.split(":")) > 1:
+ return exc.HTTPBadRequest("We don't yet allow cross-repo merges")
+
+ try:
+ response = store.merge(
+ repo_store,
+ repo_name,
+ self.request.matchdict["target_branch"],
+ self.request.matchdict["source_branch"],
+ committer_name,
+ committer_email,
+ commit_message,
+ )
+ except store.MergeConflicts:
+ return exc.HTTPBadRequest(
+ "Found conflicts between target and source branches"
+ )
+ except store.BranchNotFoundError:
+ return exc.HTTPNotFound()
+ except GitError:
+ return exc.HTTPBadRequest()
+ return response
+
+
@resource(
collection_path="/repo/{name}/commits", path="/repo/{name}/commits/{sha1}"
)
Follow ups
