Allow passing a base branch that doesn't have version info (#70)

Co-authored-by: Ezio Melotti <[email protected]> Co-authored-by: jack1142 <[email protected]> Co-authored-by: Mariatta Wijaya <[email protected]>
python · Nov 16, 2024 · 940787a · 940787a
1 parent a1552fb
commit 940787a
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 41 deletions.
diff --git a/README.md b/README.md
@@ -137,33 +137,38 @@ repo = "aiohttp"
 check_sha = "f382b5ffc445e45a110734f5396728da7914aeb6"
 fix_commit_msg = false
 default_branch = "devel"
+require_version_in_branch_name = false
 ```
 
 Available config options:
 
 ```
-team            github organization or individual nick,
-                e.g "aio-libs" for https://github.com/aio-libs/aiohttp
-                ("python" by default)
-
-repo            github project name,
-                e.g "aiohttp" for https://github.com/aio-libs/aiohttp
-                ("cpython" by default)
-
-check_sha       A long hash for any commit from the repo,
-                e.g. a sha1 hash from the very first initial commit
-                ("7f777ed95a19224294949e1b4ce56bbffcb1fe9f" by default)
-
-fix_commit_msg  Replace # with GH- in cherry-picked commit message.
-                It is the default behavior for CPython because of external
-                Roundup bug tracker (https://bugs.python.org) behavior:
-                #xxxx should point on issue xxxx but GH-xxxx points
-                on pull-request xxxx.
-                For projects using GitHub Issues, this option can be disabled.
-
-default_branch  Project's default branch name,
-                e.g "devel" for https://github.com/ansible/ansible
-                ("main" by default)
+team                            github organization or individual nick,
+                                e.g "aio-libs" for https://github.com/aio-libs/aiohttp
+                                ("python" by default)
+
+repo                            github project name,
+                                e.g "aiohttp" for https://github.com/aio-libs/aiohttp
+                                ("cpython" by default)
+
+check_sha                       A long hash for any commit from the repo,
+                                e.g. a sha1 hash from the very first initial commit
+                                ("7f777ed95a19224294949e1b4ce56bbffcb1fe9f" by default)
+
+fix_commit_msg                  Replace # with GH- in cherry-picked commit message.
+                                It is the default behavior for CPython because of external
+                                Roundup bug tracker (https://bugs.python.org) behavior:
+                                #xxxx should point on issue xxxx but GH-xxxx points
+                                on pull-request xxxx.
+                                For projects using GitHub Issues, this option can be disabled.
+
+default_branch                  Project's default branch name,
+                                e.g "devel" for https://github.com/ansible/ansible
+                                ("main" by default)
+
+require_version_in_branch_name  Allow backporting to branches whose names don't contain
+                                something that resembles a version number
+                                (i.e. at least two dot-separated numbers).
 ```
 
 To customize the tool for used by other project:

diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py
@@ -4,6 +4,7 @@
 
 import collections
 import enum
+import functools
 import os
 import re
 import subprocess
@@ -31,6 +32,7 @@
         "check_sha": "7f777ed95a19224294949e1b4ce56bbffcb1fe9f",
         "fix_commit_msg": True,
         "default_branch": "main",
+        "require_version_in_branch_name": True,
     }
 )
 
@@ -191,7 +193,9 @@ def upstream(self):
     @property
     def sorted_branches(self):
         """Return the branches to cherry-pick to, sorted by version."""
-        return sorted(self.branches, reverse=True, key=version_from_branch)
+        return sorted(
+            self.branches, key=functools.partial(compute_version_sort_key, self.config)
+        )
 
     @property
     def username(self):
@@ -333,7 +337,7 @@ def get_updated_commit_message(self, cherry_pick_branch):
         updated_commit_message = self.get_commit_message(self.commit_sha1)
         if self.prefix_commit:
             updated_commit_message = remove_commit_prefix(updated_commit_message)
-            base_branch = get_base_branch(cherry_pick_branch)
+            base_branch = get_base_branch(cherry_pick_branch, config=self.config)
             updated_commit_message = f"[{base_branch}] {updated_commit_message}"
 
         # Add '(cherry picked from commit ...)' to the message
@@ -600,7 +604,7 @@ def continue_cherry_pick(self):
         if cherry_pick_branch.startswith("backport-"):
             set_state(WORKFLOW_STATES.CONTINUATION_STARTED)
             # amend the commit message, prefix with [X.Y]
-            base = get_base_branch(cherry_pick_branch)
+            base = get_base_branch(cherry_pick_branch, config=self.config)
             short_sha = cherry_pick_branch[
                 cherry_pick_branch.index("-") + 1 : cherry_pick_branch.index(base) - 1
             ]
@@ -831,7 +835,7 @@ def cherry_pick_cli(
             sys.exit(-1)
 
 
-def get_base_branch(cherry_pick_branch):
+def get_base_branch(cherry_pick_branch, *, config):
     """
     return '2.7' from 'backport-sha-2.7'
 
@@ -855,7 +859,7 @@ def get_base_branch(cherry_pick_branch):
 
     # Subject the parsed base_branch to the same tests as when we generated it
     # This throws a ValueError if the base_branch doesn't meet our requirements
-    version_from_branch(base_branch)
+    compute_version_sort_key(config, base_branch)
 
     return base_branch
 
@@ -876,14 +880,31 @@ def validate_sha(sha):
         )
 
 
-def version_from_branch(branch):
+def compute_version_sort_key(config, branch):
     """
-    return version information from a git branch name
+    Get sort key based on version information from the given git branch name.
+
+    This function can be used as a sort key in list.sort()/sorted() provided that
+    you additionally pass config as a first argument by e.g. wrapping it with
+    functools.partial().
+
+    Branches with version information come first and are sorted from latest
+    to oldest version.
+    Branches without version information come second and are sorted alphabetically.
     """
     m = re.search(r"\d+(?:\.\d+)+", branch)
-    if not m:
+    if m:
+        raw_version = m[0].split(".")
+        # Use 0 to sort version numbers *before* regular branch names
+        return (0, *(-int(x) for x in raw_version))
+
+    if not branch:
+        raise ValueError("Branch name is an empty string.")
+    if config["require_version_in_branch_name"]:
         raise ValueError(f"Branch {branch} seems to not have a version in its name.")
-    return tuple(map(int, m[0].split(".")))
+
+    # Use 1 to sort regular branch names *after* version numbers
+    return (1, branch)
 
 
 def get_current_branch():

diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py
@@ -149,20 +149,20 @@ def tmp_git_repo_dir(tmpdir, cd, git_init, git_commit, git_config):
 
 
 @mock.patch("subprocess.check_output")
-def test_get_base_branch(subprocess_check_output):
+def test_get_base_branch(subprocess_check_output, config):
     # The format of cherry-pick branches we create are::
     #     backport-{SHA}-{base_branch}
     subprocess_check_output.return_value = b"22a594a0047d7706537ff2ac676cdc0f1dcb329c"
     cherry_pick_branch = "backport-22a594a-2.7"
-    result = get_base_branch(cherry_pick_branch)
+    result = get_base_branch(cherry_pick_branch, config=config)
     assert result == "2.7"
 
 
 @mock.patch("subprocess.check_output")
-def test_get_base_branch_which_has_dashes(subprocess_check_output):
+def test_get_base_branch_which_has_dashes(subprocess_check_output, config):
     subprocess_check_output.return_value = b"22a594a0047d7706537ff2ac676cdc0f1dcb329c"
     cherry_pick_branch = "backport-22a594a-baseprefix-2.7-basesuffix"
-    result = get_base_branch(cherry_pick_branch)
+    result = get_base_branch(cherry_pick_branch, config=config)
     assert result == "baseprefix-2.7-basesuffix"
 
 
@@ -171,14 +171,14 @@ def test_get_base_branch_which_has_dashes(subprocess_check_output):
     [
         "backport-22a594a",  # Not enough fields
         "prefix-22a594a-2.7",  # Not the prefix we were expecting
-        "backport-22a594a-base",  # No version info in the base branch
+        "backport-22a594a-",  # No base branch
     ],
 )
 @mock.patch("subprocess.check_output")
-def test_get_base_branch_invalid(subprocess_check_output, cherry_pick_branch):
+def test_get_base_branch_invalid(subprocess_check_output, cherry_pick_branch, config):
     subprocess_check_output.return_value = b"22a594a0047d7706537ff2ac676cdc0f1dcb329c"
     with pytest.raises(ValueError):
-        get_base_branch(cherry_pick_branch)
+        get_base_branch(cherry_pick_branch, config=config)
 
 
 @mock.patch("subprocess.check_output")
@@ -206,18 +206,33 @@ def test_get_author_info_from_short_sha(subprocess_check_output):
 
 
 @pytest.mark.parametrize(
-    "input_branches,sorted_branches",
+    "input_branches,sorted_branches,require_version",
     [
-        (["3.1", "2.7", "3.10", "3.6"], ["3.10", "3.6", "3.1", "2.7"]),
+        (["3.1", "2.7", "3.10", "3.6"], ["3.10", "3.6", "3.1", "2.7"], True),
         (
             ["stable-3.1", "lts-2.7", "3.10-other", "smth3.6else"],
             ["3.10-other", "smth3.6else", "stable-3.1", "lts-2.7"],
+            True,
+        ),
+        (["3.1", "2.7", "3.10", "3.6"], ["3.10", "3.6", "3.1", "2.7"], False),
+        (
+            ["stable-3.1", "lts-2.7", "3.10-other", "smth3.6else"],
+            ["3.10-other", "smth3.6else", "stable-3.1", "lts-2.7"],
+            False,
+        ),
+        (
+            ["3.7", "3.10", "2.7", "foo", "stable", "branch"],
+            ["3.10", "3.7", "2.7", "branch", "foo", "stable"],
+            False,
         ),
     ],
 )
 @mock.patch("os.path.exists")
-def test_sorted_branch(os_path_exists, config, input_branches, sorted_branches):
+def test_sorted_branch(
+    os_path_exists, config, input_branches, sorted_branches, require_version
+):
     os_path_exists.return_value = True
+    config["require_version_in_branch_name"] = require_version
     cp = CherryPicker(
         "origin",
         "22a594a0047d7706537ff2ac676cdc0f1dcb329c",
@@ -227,6 +242,21 @@ def test_sorted_branch(os_path_exists, config, input_branches, sorted_branches):
     assert cp.sorted_branches == sorted_branches
 
 
+@mock.patch("os.path.exists")
+def test_invalid_branch_empty_string(os_path_exists, config):
+    os_path_exists.return_value = True
+    # already tested for require_version_in_branch_name=True below
+    config["require_version_in_branch_name"] = False
+    cp = CherryPicker(
+        "origin",
+        "22a594a0047d7706537ff2ac676cdc0f1dcb329c",
+        ["3.1", "2.7", "3.10", "3.6", ""],
+        config=config,
+    )
+    with pytest.raises(ValueError, match=r"^Branch name is an empty string\.$"):
+        cp.sorted_branches
+
+
 @pytest.mark.parametrize(
     "input_branches",
     [
@@ -460,6 +490,7 @@ def test_load_full_config(tmp_git_repo_dir, git_add, git_commit):
             "team": "python",
             "fix_commit_msg": True,
             "default_branch": "devel",
+            "require_version_in_branch_name": True,
         },
     )
 
@@ -483,6 +514,7 @@ def test_load_partial_config(tmp_git_repo_dir, git_add, git_commit):
             "team": "python",
             "fix_commit_msg": True,
             "default_branch": "main",
+            "require_version_in_branch_name": True,
         },
     )
 
@@ -511,6 +543,7 @@ def test_load_config_no_head_sha(tmp_git_repo_dir, git_add, git_commit):
             "team": "python",
             "fix_commit_msg": True,
             "default_branch": "devel",
+            "require_version_in_branch_name": True,
         },
     )