launchpad-reviewers team mailing list archive

[Ines Almeida <mp+488417@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/turnip:return-merge-commit-already-merged into turnip:master.

Commit message:
Return the merge_commit even if commit was already merged previously
    
This will allow marking a proposal as merged from the Launchpad side if the merge was somehow missed


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/turnip/+git/turnip/+merge/488417

Relevant to handle timeouts from the Launchpad side
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/turnip:return-merge-commit-already-merged into turnip:master.

diff --git a/turnip/api/store.py b/turnip/api/store.py
index b2c4320..6b22b61 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -819,6 +819,21 @@ def _get_remote_source_tip(
         repo.references.delete(source_ref_name)
 
 
+def _find_merge_commit(repo, target_tip, source_tip):
+    """Find the merge commit that has source_tip as one of its parents"""
+    walker = repo.walk(target_tip, GIT_SORT_TOPOLOGICAL)
+
+    for i, commit in enumerate(walker):
+        # Limit search depth
+        if i >= 100:
+            break
+
+        if len(commit.parents) > 1 and source_tip in commit.parent_ids:
+            return commit.hex
+
+    return None
+
+
 def merge(
     repo_store,
     repo_name,
@@ -875,7 +890,11 @@ def merge(
         # Check if source is already included in target
         common_ancestor_id = repo.merge_base(target_tip, source_tip)
         if common_ancestor_id == source_tip:
-            return {"merge_commit": None}
+            merge_commit = _find_merge_commit(repo, target_tip, source_tip)
+            return {
+                "merge_commit": merge_commit,
+                "previously_merged": True,
+            }
 
         # Create an in-memory index for the merge
         index = repo.merge_commits(target_tip, source_tip)
@@ -914,7 +933,10 @@ def merge(
             [target_tip, source_tip],
         )
 
-        return {"merge_commit": merge_commit.hex}
+        return {
+            "merge_commit": merge_commit.hex,
+            "previously_merged": False,
+        }
 
 
 def get_diff(repo_store, repo_name, sha1_from, sha1_to, context_lines=3):
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 863d62f..9d6d4c7 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -1557,7 +1557,7 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
         )
         repo.create_branch("feature", repo.get(feature_commit))
 
-        self.app.post_json(
+        initial_resp = self.app.post_json(
             f"/repo/{self.repo_path}/merge/main:feature",
             {
                 "committer_name": "Test User",
@@ -1579,7 +1579,10 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
         )
 
         self.assertEqual(200, resp.status_code)
-        self.assertIsNone(resp.json["merge_commit"])
+        self.assertEqual(
+            initial_resp.json["merge_commit"], resp.json["merge_commit"]
+        )
+        self.assertTrue(resp.json["previously_merged"])
 
     def test_merge_conflicts(self):
         """Test merge with conflicts."""
diff --git a/turnip/api/tests/test_store.py b/turnip/api/tests/test_store.py
index 70ad89c..5d7ea19 100644
--- a/turnip/api/tests/test_store.py
+++ b/turnip/api/tests/test_store.py
@@ -709,6 +709,7 @@ class MergeTestCase(TestCase):
             self.repo.references["refs/heads/main"].target.hex,
             result["merge_commit"],
         )
+        self.assertFalse(result["previously_merged"])
 
         merge_commit = self.repo.get(result["merge_commit"])
         self.assertEqual(merge_commit.parents[0].hex, self.initial_commit.hex)
@@ -718,7 +719,7 @@ class MergeTestCase(TestCase):
 
     def test_merge_already_included(self):
         """Test merge when source is already included in target."""
-        store.merge(
+        initial_result = store.merge(
             self.repo_store,
             "repo",
             "main",
@@ -740,6 +741,44 @@ class MergeTestCase(TestCase):
             "Test User",
             "test@xxxxxxxxxxx",
         )
+        self.assertEqual(
+            initial_result["merge_commit"],
+            result["merge_commit"],
+        )
+        self.assertTrue(result["previously_merged"])
+
+    def test_merge_already_included_old_commit(self):
+        """Test merge when source is already included in target."""
+
+        commit = self.initial_commit
+        for i in range(110):
+            commit = self.factory.add_commit(
+                f"initial {i}", "file.txt", parents=[commit]
+            )
+
+        store.merge(
+            self.repo_store,
+            "repo",
+            "main",
+            commit.hex,
+            "feature",
+            self.feature_commit.hex,
+            "Test User",
+            "test@xxxxxxxxxxx",
+        )
+
+        # Try to merge again
+        result = store.merge(
+            self.repo_store,
+            "repo",
+            "main",
+            self.initial_commit.hex,
+            "feature",
+            self.feature_commit.hex,
+            "Test User",
+            "test@xxxxxxxxxxx",
+        )
+        self.assertTrue(result["previously_merged"])
         self.assertIsNone(result["merge_commit"])
 
     def test_merge_conflicts(self):