Repository: python/cherry-picker

Issue:
Consider laxing the requirements for maintenance branch names?
I know that this is a tool primarily for CPython but I found myself in a use case where I could benefit from cherry-picker's help but the branch name I want to backport to isn't version-like. To be specific, I want to use it with a branch named `stable` because it allows me to replace ReadTheDocs's stable version without going through the release process:
https://docs.readthedocs.io/en/stable/versions.html#versioned-documentation

I can't seem to find a good reason for this limitation, the only thing the version in the branch appears to be used for is sorting - it wouldn't be too hard to primarily sort by version but fallback to lexicographic order if the branch doesn't contain a version. If the concern is that user passes the program something that is a commit rather than a branch name then we could enforce that it is in a different way (though you already do this indirectly when creating a backport branch using `upstream/MAINTENANCE_BRANCH_NAME` as a base which doesn't work with commits).

I'd be happy to implement this if accepted by the way.

Discussion:
Mariatta: I think this change is reasonable.
jack1142: Tests are currently failing on main and so this PR is affected, I fixed them in #76.
Mariatta: I think allowing it to be configured via the toml file is a good idea as well. Would you be able to add that in a separate PR?
jack1142: I can do it here or in a separate PR, whatever you prefer. Can probably get to it tomorrow.

I assume you would want this to be a true/false setting determining whether branches without version info are allowed (defaulting to false to keep the current behavior intact)?
Mariatta: I think it should support the original behavior, if not supplied it should use the `upstream`
jack1142: Alright, I added a `require_version_in_branch_name` option. Quite a mouthful but I couldn't really come up with anything else that was still clear about what the option is. At most I could maybe manage to make the name 4-5 characters shorter but the names I came up with were, in my opinion, a lot more vague than the current name.

---

While looking into this, I also realized that *I* suggested adding such an option in the PR's description which I didn't really realize when you asked me to add it :smile: 
Jackenmen: I created https://github.com/python/miss-islington/pull/639 to make miss-islington work with this PR since I realized that this is going to be necessary now that this also adds a new configuration option.
Jackenmen: I addressed the review and resolved the merge conflicts. Seems like I may have missed the merge window for the release though :smile: 

If by the time this is looked at by a maintainer new conflicts arise, just let me know and I'll resolve them - maybe I'll finally be able to use upstream directly :)
webknjaz: @hugovk mind taking a look? 

Patch:
diff --git a/README.md b/README.md
index 28be5ae..07e32bf 100644
--- a/README.md
+++ b/README.md
@@ -50,15 +50,13 @@ Write tests using [pytest](https://docs.pytest.org/).
 
 ## Setup info
 
-Requires Python 3.8+.
-
 ```console
 $ python3 -m venv venv
 $ source venv/bin/activate
 (venv) $ python -m pip install cherry_picker
 ```
 
-The cherry picking script assumes that if an `upstream` remote is defined, then
+The cherry-picking script assumes that if an `upstream` remote is defined, then
 it should be used as the source of upstream changes and as the base for
 cherry-pick branches. Otherwise, `origin` is used for that purpose.
 You can override this behavior with the `--upstream-remote` option
@@ -139,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
index 95229b1..2bc5a81 100755
--- a/cherry_picker/cherry_picker.py
+++ b/cherry_picker/cherry_picker.py
@@ -4,6 +4,7 @@ from __future__ import annotations
 
 import collections
 import enum
+import functools
 import os
 import re
 import subprocess
@@ -31,6 +32,7 @@ DEFAULT_CONFIG = collections.ChainMap(
         "check_sha": "7f777ed95a19224294949e1b4ce56bbffcb1fe9f",
         "fix_commit_msg": True,
         "default_branch": "main",
+        "require_version_in_branch_name": True,
     }
 )
 
@@ -191,7 +193,9 @@ class CherryPicker:
     @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 @@ To abort the cherry-pick and cleanup:
         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 @@ $ cherry_picker --abort
         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():


Test patch:
diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py
index 88c3cde..7b77d18 100644
--- 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,
         },
     )