Respect existing trailers (including co-author lines) when backporting (#46)

Author: Jackenmen

cherry_picker/cherry_picker.py

@@ -317,14 +317,48 @@ def get_exit_message(self, branch):
 """
 
     def get_updated_commit_message(self, cherry_pick_branch):
+        """
+        Get updated commit message for the cherry-picked commit.
+        """
+        # Get the original commit message and prefix it with the branch name
+        # if that's enabled.
         commit_prefix = ""
         if self.prefix_commit:
             commit_prefix = f"[{get_base_branch(cherry_pick_branch)}] "
-        return f"""{commit_prefix}{self.get_commit_message(self.commit_sha1)}
-(cherry picked from commit {self.commit_sha1})
+        updated_commit_message = f"{commit_prefix}{self.get_commit_message(self.commit_sha1)}"
+
+        # Add '(cherry picked from commit ...)' to the message
+        # and add new Co-authored-by trailer if necessary.
+        cherry_pick_information = f"(cherry picked from commit {self.commit_sha1})\n:"
+        # Here, we're inserting new Co-authored-by trailer and we *somewhat*
+        # abuse interpret-trailers by also adding cherry_pick_information which
+        # is not an actual trailer.
+        # `--where start` makes it so we insert new trailers *before* the existing
+        # trailers so cherry-pick information gets added before any of the trailers
+        # which prevents us from breaking the trailers.
+        cmd = [
+            "git",
+            "interpret-trailers",
+            "--where",
+            "start",
+            "--trailer",
+            f"Co-authored-by: {get_author_info_from_short_sha(self.commit_sha1)}",
+            "--trailer",
+            cherry_pick_information,
+        ]
+        output = subprocess.check_output(cmd, input=updated_commit_message.encode())
+        # Replace the right most-occurence of the "cherry picked from commit" string.
+        #
+        # This needs to be done because `git interpret-trailers` required us to add `:`
+        # to `cherry_pick_information` when we don't actually want it.
+        before, after = output.strip().decode().rsplit(f"\n{cherry_pick_information}", 1)
+        if not before.endswith("\n"):
+            # ensure that we still have a newline between cherry pick information
+            # and commit headline
+            cherry_pick_information = f"\n{cherry_pick_information}"
+        updated_commit_message = cherry_pick_information[:-1].join((before, after))
 
-
-Co-authored-by: {get_author_info_from_short_sha(self.commit_sha1)}"""
+        return updated_commit_message
 
     def amend_commit_message(self, cherry_pick_branch):
         """ prefix the commit message with (X.Y) """

cherry_picker/test_cherry_picker.py

@@ -538,6 +538,96 @@ def test_normalize_short_commit_message():
     )
 
 
+@pytest.mark.parametrize(
+    "commit_message,expected_commit_message",
+    (
+        # ensure existing co-author is retained
+        (
+            """Fix broken `Show Source` links on documentation pages (GH-3113)
+
+Co-authored-by: PR Co-Author <another@author.com>""",
+            """[3.6] Fix broken `Show Source` links on documentation pages (GH-3113)
+(cherry picked from commit b9ff498793611d1c6a9b99df464812931a1e2d69)
+
+Co-authored-by: PR Author <author@name.email>
+Co-authored-by: PR Co-Author <another@author.com>""",
+        ),
+        # ensure co-author trailer is not duplicated
+        (
+            """Fix broken `Show Source` links on documentation pages (GH-3113)
+
+Co-authored-by: PR Author <author@name.email>""",
+            """[3.6] Fix broken `Show Source` links on documentation pages (GH-3113)
+(cherry picked from commit b9ff498793611d1c6a9b99df464812931a1e2d69)
+
+Co-authored-by: PR Author <author@name.email>""",
+        ),
+        # ensure message is formatted properly when original commit is short
+        (
+            "Fix broken `Show Source` links on documentation pages (GH-3113)",
+            """[3.6] Fix broken `Show Source` links on documentation pages (GH-3113)
+(cherry picked from commit b9ff498793611d1c6a9b99df464812931a1e2d69)
+
+Co-authored-by: PR Author <author@name.email>""",
+        ),
+        # ensure message is formatted properly when original commit is long
+        (
+            """Fix broken `Show Source` links on documentation pages (GH-3113)
+
+The `Show Source` was broken because of a change made in sphinx 1.5.1
+In Sphinx 1.4.9, the sourcename was "index.txt".
+In Sphinx 1.5.1+, it is now "index.rst.txt".""",
+            """[3.6] Fix broken `Show Source` links on documentation pages (GH-3113)
+
+The `Show Source` was broken because of a change made in sphinx 1.5.1
+In Sphinx 1.4.9, the sourcename was "index.txt".
+In Sphinx 1.5.1+, it is now "index.rst.txt".
+(cherry picked from commit b9ff498793611d1c6a9b99df464812931a1e2d69)
+
+Co-authored-by: PR Author <author@name.email>""",
+        ),
+        # ensure message is formatted properly when original commit is long
+        # and it has a co-author
+        (
+            """Fix broken `Show Source` links on documentation pages (GH-3113)
+
+The `Show Source` was broken because of a change made in sphinx 1.5.1
+In Sphinx 1.4.9, the sourcename was "index.txt".
+In Sphinx 1.5.1+, it is now "index.rst.txt".
+
+Co-authored-by: PR Co-Author <another@author.com>""",
+            """[3.6] Fix broken `Show Source` links on documentation pages (GH-3113)
+
+The `Show Source` was broken because of a change made in sphinx 1.5.1
+In Sphinx 1.4.9, the sourcename was "index.txt".
+In Sphinx 1.5.1+, it is now "index.rst.txt".
+(cherry picked from commit b9ff498793611d1c6a9b99df464812931a1e2d69)
+
+Co-authored-by: PR Author <author@name.email>
+Co-authored-by: PR Co-Author <another@author.com>""",
+        ),
+    ),
+)
+def test_get_updated_commit_message_with_trailers(commit_message, expected_commit_message):
+    cherry_pick_branch = "backport-22a594a-3.6"
+    commit = "b9ff498793611d1c6a9b99df464812931a1e2d69"
+
+    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
+        cherry_picker = CherryPicker("origin", commit, [])
+
+    with mock.patch(
+        "cherry_picker.cherry_picker.validate_sha", return_value=True
+    ), mock.patch.object(
+        cherry_picker, "get_commit_message", return_value=commit_message
+    ), mock.patch(
+        "cherry_picker.cherry_picker.get_author_info_from_short_sha",
+        return_value="PR Author <author@name.email>",
+    ):
+        updated_commit_message = cherry_picker.get_updated_commit_message(cherry_pick_branch)
+
+    assert updated_commit_message == expected_commit_message
+
+
 @pytest.mark.parametrize(
     "input_path", ("/some/path/without/revision", "HEAD:some/non-existent/path")
 )