launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32589
[Merge] ~ines-almeida/turnip:add-cross-repo-merge into turnip:master
Ines Almeida has proposed merging ~ines-almeida/turnip:add-cross-repo-merge into turnip:master.
Commit message:
Add cross-repo merge functionality
When dealing with cross-repo merges, turnip pushes the source branch to the target repo, merges and then cleans up the added ref.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/turnip/+git/turnip/+merge/486519
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/turnip:add-cross-repo-merge into turnip:master.
diff --git a/turnip/api/store.py b/turnip/api/store.py
index 197a4d9..b2c4320 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -22,6 +22,7 @@ from pygit2 import (
IndexEntry,
InvalidSpecError,
Oid,
+ RemoteCallbacks,
Repository,
Signature,
init_repository,
@@ -724,17 +725,98 @@ class RefNotFoundError(Exception):
pass
-def getBranchTip(repo, branch_name):
+def get_branch_tip(repo, branch_ref):
"""Search for a branch within a repo.
Returns the Oid of the branch's last commit"""
+ if not branch_ref.startswith("refs/"):
+ branch_ref = f"refs/heads/{branch_ref}"
+
try:
- ref = repo.lookup_reference(f"refs/heads/{branch_name}")
+ ref = repo.lookup_reference(branch_ref)
return ref.target
except (KeyError, ValueError) as e:
raise RefNotFoundError(
- f"Branch '{branch_name}' not found in repo {repo.path}: {e}"
+ f"Branch '{branch_ref}' not found in repo {repo.path}: {e}"
+ )
+
+
+@contextmanager
+def open_remote(source_repo, target_repo_path, remote_name):
+ """Opens a remote connection from {source_repo} into a {target_repo_path}.
+
+ :param source_repo: repository where we will create the new remote.
+ :param target_repo_path: path to the target repo
+ :param remote_name: name to give to the new remote
+
+ """
+ try:
+ try:
+ remote = source_repo.remotes.create(
+ remote_name, f"file://{target_repo_path}"
+ )
+ except ValueError:
+ # In the case where a remote wasn't removed correctly
+ source_repo.remotes.set_url(
+ remote_name, f"file://{target_repo_path}"
+ )
+ remote = source_repo.remotes[remote_name]
+ yield remote
+ finally:
+ source_repo.remotes.delete(remote_name)
+
+
+def push(repo_store, source_repo_name, target_repo, source_branch):
+ remote_name = f"{source_repo_name}-{source_branch}"
+ source_ref = f"refs/heads/{source_branch}"
+ remote_ref = f"refs/internal/{remote_name}"
+
+ with open_repo(repo_store, source_repo_name) as source_repo, open_remote(
+ source_repo, target_repo.path, remote_name
+ ) as remote:
+ # Specify which ref to push (source_ref:target_ref)
+ refspec = f"{source_ref}:{remote_ref}"
+
+ # No needed credentials for local file URLs
+ callbacks = RemoteCallbacks(credentials=None)
+ remote.push([refspec], callbacks=callbacks)
+
+ return remote_ref
+
+
+def _get_target_commit(repo, target_branch, target_commit_sha1):
+ """Validate that target commit exists within the target branch, and return
+ it"""
+ target_tip = get_branch_tip(repo, target_branch)
+ if not (
+ target_tip.hex == target_commit_sha1
+ or repo.descendant_of(target_tip, target_commit_sha1)
+ ):
+ raise GitError("The target commit is not part of the target branch")
+ return target_tip
+
+
+def _get_source_commit(repo, source_branch, source_commit_sha1):
+ """Validate that source tip matches the requested commit and return it."""
+ source_tip = get_branch_tip(repo, source_branch)
+ if source_tip.hex != source_commit_sha1:
+ raise GitError("The tip of the source branch has changed")
+ return source_tip
+
+
+def _get_remote_source_tip(
+ repo_store, source_repo_name, repo, source_branch, source_commit_sha1
+):
+ """Get source commit from source repo into target repo"""
+ try:
+ # For cross repo, we push a temporary ref to the target repo
+ source_ref_name = push(
+ repo_store, source_repo_name, repo, source_branch
)
+ return _get_source_commit(repo, source_ref_name, source_commit_sha1)
+ finally:
+ # Cleanup temporary refs
+ repo.references.delete(source_ref_name)
def merge(
@@ -764,22 +846,30 @@ def merge(
:param commit_message: [optional] custom commit message
"""
+ source_repo_name = None
+ if len(repo_name.split(":")) == 2:
+ repo_name, source_repo_name = repo_name.split(":")
+ if repo_name == source_repo_name:
+ source_repo_name = None
+
+ is_cross_repo = source_repo_name is not None
+
with open_repo(repo_store, repo_name) as repo:
- target_tip = getBranchTip(repo, target_branch)
- source_tip = getBranchTip(repo, source_branch)
-
- # Check source tip is still the same as when the merge was requested
- if source_tip.hex != source_commit_sha1:
- raise GitError("The tip of the source branch has changed")
-
- # Check target_commit_sha1 exists within the target branch.
- # We fail the merge if the target branch was re-written
- if not (
- target_tip.hex == target_commit_sha1
- or repo.descendant_of(target_tip, target_commit_sha1)
- ):
- raise GitError(
- "The target commit is not part of the target branch"
+ target_tip = _get_target_commit(
+ repo, target_branch, target_commit_sha1
+ )
+
+ if is_cross_repo:
+ source_tip = _get_remote_source_tip(
+ repo_store,
+ source_repo_name,
+ repo,
+ source_branch,
+ source_commit_sha1,
+ )
+ else:
+ source_tip = _get_source_commit(
+ repo, source_branch, source_commit_sha1
)
# Check if source is already included in target
@@ -804,22 +894,17 @@ def merge(
)
# Verify that branch hasn't changed since the start of the merge
- target_ref = f"refs/heads/{target_branch}"
- current_target_ref = repo.lookup_reference(target_ref)
- if target_tip != current_target_ref.target:
+ current_target_tip = get_branch_tip(repo, target_branch)
+ if target_tip != current_target_tip:
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 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 if a new
+ # Note that `create_commit` will raise a GitError if a new
# commit is pushed to the target branch since the start of this
# merge.
+ target_ref = f"refs/heads/{target_branch}"
merge_commit = repo.create_commit(
target_ref,
committer,
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 04330c7..863d62f 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -1492,6 +1492,58 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
self.assertEqual(merge_commit.committer.name, "Test User")
self.assertEqual(merge_commit.committer.email, "test@xxxxxxxxxxx")
+ def test_cross_repo_merge_successful(self):
+ """Test a successful cross-repo merge."""
+ # Create target repo with main branch
+ target_path = os.path.join(self.repo_root, "target")
+ target_factory = RepoFactory(target_path)
+ target_repo = target_factory.build()
+ target_initial = target_factory.add_commit(
+ "target initial", "file.txt"
+ )
+ target_repo.create_branch("main", target_repo.get(target_initial))
+ target_repo.set_head("refs/heads/main")
+
+ # Create source repo with feature branch
+ source_path = os.path.join(self.repo_root, "source")
+ source_factory = RepoFactory(source_path, clone_from=target_factory)
+ source_repo = source_factory.build()
+ source_initial = target_initial
+ source_commit = source_factory.add_commit(
+ "source change", "file.txt", parents=[source_initial]
+ )
+ source_repo.create_branch("feature", source_repo.get(source_commit))
+
+ # Perform cross-repo merge
+ response = self.app.post_json(
+ "/repo/target:source/merge/main:feature",
+ {
+ "target_commit_sha1": target_initial.hex,
+ "source_commit_sha1": source_commit.hex,
+ "committer_name": "Test User",
+ "committer_email": "test@xxxxxxxxxxx",
+ },
+ )
+
+ self.assertEqual(response.status_code, 200)
+ self.assertIn("merge_commit", response.json)
+ self.assertIsNotNone(response.json["merge_commit"])
+
+ # Verify merge commit
+ merge_commit = target_repo.get(response.json["merge_commit"])
+ self.assertEqual(merge_commit.parents[0].hex, target_initial.hex)
+ self.assertEqual(merge_commit.parents[1].hex, source_commit.hex)
+ self.assertEqual(merge_commit.committer.name, "Test User")
+ self.assertEqual(merge_commit.committer.email, "test@xxxxxxxxxxx")
+
+ # Verify temporary ref was cleaned up
+ self.assertNotIn(
+ "refs/internal/source-feature", target_repo.references
+ )
+
+ # Verify temporary remote was cleaned up
+ self.assertEqual(0, len(target_repo.remotes))
+
def test_merge_already_included(self):
"""Test merge when source is already included in target."""
factory = RepoFactory(self.repo_store)
diff --git a/turnip/api/tests/test_store.py b/turnip/api/tests/test_store.py
index 3e3a389..03310aa 100644
--- a/turnip/api/tests/test_store.py
+++ b/turnip/api/tests/test_store.py
@@ -919,7 +919,7 @@ class GetBranchTipTestCase(TestCase):
)
# Get the branch tip
- tip_oid = store.getBranchTip(self.repo, "test_branch")
+ tip_oid = store.get_branch_tip(self.repo, "test_branch")
# Verify the tip matches our commit
self.assertEqual(tip_oid, commit_oid)
@@ -928,9 +928,256 @@ class GetBranchTipTestCase(TestCase):
"""Test getting the tip of a non-existent branch."""
e = self.assertRaises(
store.RefNotFoundError,
- store.getBranchTip,
+ store.get_branch_tip,
self.repo,
"nonexistent_branch",
)
- self.assertIn("Branch 'nonexistent_branch' not found", str(e))
+ self.assertIn(
+ "Branch 'refs/heads/nonexistent_branch' not found", str(e)
+ )
+
+
+class OpenRemoteTestCase(TestCase):
+ def setUp(self):
+ super().setUp()
+ self.repo_store = self.useFixture(TempDir()).path
+ self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))
+
+ # Create source and target repos
+ self.source_path = os.path.join(self.repo_store, "source")
+ self.target_path = os.path.join(self.repo_store, "target")
+ self.source_factory = RepoFactory(self.source_path)
+ self.target_factory = RepoFactory(self.target_path)
+ self.source_repo = self.source_factory.build()
+ self.target_repo = self.target_factory.build()
+
+ def test_open_remote_creates_and_removes_remote(self):
+ """Test that open_remote creates a remote and removes it after use."""
+ remote_name = "test-remote"
+
+ with store.open_remote(
+ self.source_repo, self.target_path, remote_name
+ ):
+ # Verify remote was created
+ remote = self.source_repo.remotes[remote_name]
+ self.assertEqual(remote.url, f"file://{self.target_path}")
+
+ # Verify remote was removed
+ self.assertNotIn(remote_name, self.source_repo.remotes)
+
+ def test_open_remote_existing_remote(self):
+ """Test that if a remote already exists (due to a previous clean up
+ issue), open_remote will still recreate and remove the remote."""
+ remote_name = "test-remote"
+
+ self.source_repo.remotes.create(remote_name, "file://old_target_path")
+
+ with store.open_remote(
+ self.source_repo, self.target_path, remote_name
+ ):
+ remote = self.source_repo.remotes[remote_name]
+ self.assertEqual(remote.url, f"file://{self.target_path}")
+
+ self.assertNotIn(remote_name, self.source_repo.remotes)
+
+ def test_open_remote_handles_errors(self):
+ """Test that open_remote handles errors and still cleans up."""
+ remote_name = "test-remote"
+
+ def _open_remote_error():
+ with store.open_remote(
+ self.source_repo, self.target_path, remote_name
+ ):
+ raise Exception("Test error")
+
+ self.assertRaises(Exception, _open_remote_error)
+
+ # Verify remote was still removed despite error
+ self.assertNotIn(remote_name, self.source_repo.remotes)
+
+
+class PushTestCase(TestCase):
+ def setUp(self):
+ super().setUp()
+ self.repo_store = self.useFixture(TempDir()).path
+ self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))
+
+ # Create source and target repos
+ self.source_path = os.path.join(self.repo_store, "source")
+ self.target_path = os.path.join(self.repo_store, "target")
+ self.source_factory = RepoFactory(self.source_path)
+ self.target_factory = RepoFactory(self.target_path)
+ self.source_repo = self.source_factory.build()
+ self.target_repo = self.target_factory.build()
+
+ # Create a branch in source repo
+ self.branch_name = "feature"
+ self.initial_commit = self.source_factory.add_commit(
+ "initial", "file.txt"
+ )
+ self.source_repo.create_branch(
+ self.branch_name, self.source_repo.get(self.initial_commit)
+ )
+
+ def test_push_creates_internal_ref(self):
+ """Test that push creates an internal ref in target repo."""
+ remote_ref = store.push(
+ self.repo_store, "source", self.target_repo, self.branch_name
+ )
+
+ # Verify ref was created in target repo
+ self.assertIn(remote_ref, self.target_repo.references)
+ target_ref = self.target_repo.references[remote_ref]
+ source_ref = self.source_repo.references[
+ f"refs/heads/{self.branch_name}"
+ ]
+ self.assertEqual(target_ref.target, source_ref.target)
+
+ def test_push_updates_existing_ref(self):
+ """Test that push updates an existing ref."""
+ # First push
+ remote_ref = store.push(
+ self.repo_store, "source", self.target_repo, self.branch_name
+ )
+
+ # Add new commit to source branch
+ new_commit = self.source_factory.add_commit(
+ "new commit", "file.txt", parents=[self.initial_commit]
+ )
+ self.source_repo.references[
+ f"refs/heads/{self.branch_name}"
+ ].set_target(new_commit)
+
+ # Push again
+ store.push(
+ self.repo_store, "source", self.target_repo, self.branch_name
+ )
+
+ # Verify ref was updated
+ target_ref = self.target_repo.references[remote_ref]
+ self.assertEqual(target_ref.target, new_commit)
+
+
+class CrossRepoMergeTestCase(TestCase):
+ def setUp(self):
+ super().setUp()
+ self.repo_store = self.useFixture(TempDir()).path
+ self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))
+
+ # Create target repo
+ self.target_path = os.path.join(self.repo_store, "target")
+ self.target_factory = RepoFactory(self.target_path)
+ self.target_repo = self.target_factory.build()
+
+ self.target_initial = self.target_factory.add_commit(
+ "target initial", "file.txt"
+ )
+ self.target_branch = self.target_repo.create_branch(
+ "main", self.target_repo.get(self.target_initial)
+ )
+ self.target_repo.set_head("refs/heads/main")
+
+ # Create source repo (forked from target)
+ self.source_path = os.path.join(self.repo_store, "source")
+ self.source_factory = RepoFactory(
+ self.source_path, clone_from=self.target_factory
+ )
+ self.source_repo = self.source_factory.build()
+ self.source_initial = self.target_initial
+ self.source_branch = self.source_repo.create_branch(
+ "feature", self.source_repo.get(self.source_initial)
+ )
+
+ def test_cross_repo_merge_successful(self):
+ """Test successful merge from source repo to target repo."""
+ # Add commits to both branches
+ source_commit = self.source_factory.add_commit(
+ "source change",
+ "file.txt",
+ parents=[self.source_initial],
+ )
+ self.source_repo.lookup_reference(self.source_branch.name).set_target(
+ source_commit
+ )
+
+ result = store.merge(
+ self.repo_store,
+ "target:source", # target:source format for cross-repo
+ "main",
+ self.target_initial.hex,
+ "feature",
+ source_commit.hex,
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ # Verify merge was successful
+ self.assertIsNotNone(result["merge_commit"])
+ merge_commit = self.target_repo.get(result["merge_commit"])
+ self.assertEqual(merge_commit.parents[0].hex, self.target_initial.hex)
+ self.assertEqual(merge_commit.parents[1].hex, source_commit.hex)
+
+ # Verify temporary ref was cleaned up
+ self.assertIsNone(
+ self.target_repo.references.get("refs/internal/source-feature")
+ )
+
+ def test_cross_repo_merge_conflicts(self):
+ """Test merge conflicts when merging from source repo."""
+ # Create conflicting changes in both repos
+ source_commit = self.source_factory.add_commit(
+ "source change", "file.txt", parents=[self.source_initial]
+ )
+ self.source_repo.lookup_reference(self.source_branch.name).set_target(
+ source_commit
+ )
+ target_commit = self.target_factory.add_commit(
+ "target change", "file.txt", parents=[self.target_initial]
+ )
+ self.target_repo.lookup_reference(self.target_branch.name).set_target(
+ target_commit
+ )
+
+ # Try to merge
+ self.assertRaises(
+ store.MergeConflicts,
+ store.merge,
+ self.repo_store,
+ "target:source",
+ "main",
+ target_commit.hex,
+ "feature",
+ source_commit.hex,
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
+
+ # Verify temporary ref was cleaned up
+ self.assertIsNone(
+ self.target_repo.references.get("refs/internal/source-feature")
+ )
+
+ def test_cross_repo_merge_source_branch_moved(self):
+ """Test error when source branch tip doesn't match expected commit."""
+ # Add commit to source branch after merge was requested
+ new_source_commit = self.source_factory.add_commit(
+ "new source", "file.txt", parents=[self.source_initial]
+ )
+ self.source_repo.references[self.source_branch.name].set_target(
+ new_source_commit
+ )
+
+ # Try to merge using old source commit
+ self.assertRaises(
+ pygit2.GitError,
+ store.merge,
+ self.repo_store,
+ "target:source",
+ "main",
+ self.target_initial.hex,
+ "feature",
+ self.source_initial.hex,
+ "Test User",
+ "test@xxxxxxxxxxx",
+ )
diff --git a/turnip/api/views.py b/turnip/api/views.py
index 2cd1d67..f3ef8ce 100644
--- a/turnip/api/views.py
+++ b/turnip/api/views.py
@@ -486,13 +486,6 @@ class MergeAPI(BaseAPI):
"committer_name and committer_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,
Follow ups
