launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32459
Re: [Merge] ~ines-almeida/turnip:add-merge-api into turnip:master
Added some comments
Diff comments:
> 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
> @@ -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
It's atomic but the previous check `original_target_tip != current_target_ref.target` to `create_commit` is not an atomic operation. It's true it's unlikely something can come right in between these calls. I'm not sure if there is a simple solution to this.
> + # 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"],
> + )
Can you add checks that the commiter_name and commuter_email is correct?
> +
> + 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/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).
How does it work with pushing via ssh? Is it the ssh client that goes out to LP?
> + """
> + 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}"
> )
--
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.
References
