Repository: python/cherry-picker

Issue:
cherry-picker ends up in bad state if a `backport-...` branch already exists
If, for whatever reason, the `backport-...` branch already exists, cherry-picker ends up in a bad state:
```
λ python -m cherry_picker e5ee6a87a3b6fbf1a7e3856248885811f04d1f89 3.4
🐍 🍒 ⛏

Now backporting 'e5ee6a87a3b6fbf1a7e3856248885811f04d1f89' into '3.4'
Error checking out the branch backport-e5ee6a8-3.4.
fatal: A branch named 'backport-e5ee6a8-3.4' already exists.

λ python -m cherry_picker e5ee6a87a3b6fbf1a7e3856248885811f04d1f89 3.4
🐍 🍒 ⛏
Usage: __main__.py [OPTIONS] [COMMIT_SHA1] [BRANCHES]...
Try '__main__.py -h' for help.

Error: Run state cherry-picker.state=BACKPORT_LOOP_START in Git config is not known.
Perhaps it has been set by a newer version of cherry-picker. Try upgrading.
Valid states are: BACKPORT_PAUSED, UNSET. If this looks suspicious, raise an issue at https://github.com/python/core-workflow/issues/new.
As the last resort you can reset the runtime state stored in Git config using the following command: `git config --local --remove-section cherry-picker`
```

Discussion:
ambv: Your change is still worth integrating but note that when a backport branch already exists, now cherry-picker will force-push to it (with --force-with-lease so only if there are no other changes on that remote branch):

https://github.com/python/cherry-picker/commit/7122a15bec80326f3fbfa44a2dafa059749c9963
jack1142: Does that mean I should change this to overwrite the existing local branch or should I leave it as it is right now? I went for the more cautious way but I don't really mind changing it if you think it would be better the other way.
jack1142: I resolved the conflict caused by #56. Since I'm already commenting, I just want to gently remind you that I haven't received a response to my question here. I realize it's probably a terrible time for a reminder since it's Christmas, it just so happened that I found about these conflicts while working on something else today 😄 

Happy holidays!

Patch:
diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py
index bb4b848..4c050d6 100755
--- a/cherry_picker/cherry_picker.py
+++ b/cherry_picker/cherry_picker.py
@@ -77,7 +77,9 @@ WORKFLOW_STATES = enum.Enum(
 
 
 class BranchCheckoutException(Exception):
-    pass
+    def __init__(self, branch_name):
+        self.branch_name = branch_name
+        super().__init__(f"Error checking out the branch {branch_name!r}.")
 
 
 class CherryPickException(Exception):
@@ -99,6 +101,7 @@ class CherryPicker:
         commit_sha1,
         branches,
         *,
+        upstream_remote=None,
         dry_run=False,
         push=True,
         prefix_commit=True,
@@ -128,6 +131,7 @@ class CherryPicker:
             click.echo("Dry run requested, listing expected command sequence")
 
         self.pr_remote = pr_remote
+        self.upstream_remote = upstream_remote
         self.commit_sha1 = commit_sha1
         self.branches = branches
         self.dry_run = dry_run
@@ -135,6 +139,9 @@ class CherryPicker:
         self.auto_pr = auto_pr
         self.prefix_commit = prefix_commit
 
+        # the cached calculated value of self.upstream property
+        self._upstream = None
+
         # This is set to the PR number when cherry-picker successfully
         # creates a PR through API.
         self.pr_number = None
@@ -153,14 +160,33 @@ class CherryPicker:
     @property
     def upstream(self):
         """Get the remote name to use for upstream branches
-        Uses "upstream" if it exists, "origin" otherwise
+
+        Uses the remote passed to `--upstream-remote`.
+        If this flag wasn't passed, it uses "upstream" if it exists or "origin" otherwise.
         """
+        # the cached calculated value of the property
+        if self._upstream is not None:
+            return self._upstream
+
         cmd = ["git", "remote", "get-url", "upstream"]
+        if self.upstream_remote is not None:
+            cmd[-1] = self.upstream_remote
+
         try:
             self.run_cmd(cmd)
         except subprocess.CalledProcessError:
-            return "origin"
-        return "upstream"
+            if self.upstream_remote is not None:
+                raise ValueError(f"There is no remote with name {cmd[-1]!r}.")
+            cmd[-1] = "origin"
+            try:
+                self.run_cmd(cmd)
+            except subprocess.CalledProcessError:
+                raise ValueError(
+                    "There are no remotes with name 'upstream' or 'origin'."
+                )
+
+        self._upstream = cmd[-1]
+        return self._upstream
 
     @property
     def sorted_branches(self):
@@ -214,12 +240,10 @@ class CherryPicker:
             self.run_cmd(cmd)
         except subprocess.CalledProcessError as err:
             click.echo(
-                f"Error checking out the branch {branch_name}."
+                f"Error checking out the branch {checked_out_branch!r}."
             )
             click.echo(err.output)
-            raise BranchCheckoutException(
-                f"Error checking out the branch {branch_name}."
-            )
+            raise BranchCheckoutException(checked_out_branch)
 
     def get_commit_message(self, commit_sha):
         """
@@ -425,7 +449,13 @@ Co-authored-by: {get_author_info_from_short_sha(self.commit_sha1)}"""
             click.echo(f"Now backporting '{self.commit_sha1}' into '{maint_branch}'")
 
             cherry_pick_branch = self.get_cherry_pick_branch(maint_branch)
-            self.checkout_branch(maint_branch, create_branch=True)
+            try:
+                self.checkout_branch(maint_branch, create_branch=True)
+            except BranchCheckoutException:
+                self.checkout_default_branch()
+                reset_stored_config_ref()
+                reset_state()
+                raise
             commit_message = ""
             try:
                 self.cherry_pick()
@@ -605,6 +635,13 @@ CONTEXT_SETTINGS = dict(help_option_names=["-h", "--help"])
     help="git remote to use for PR branches",
     default="origin",
 )
+@click.option(
+    "--upstream-remote",
+    "upstream_remote",
+    metavar="REMOTE",
+    help="git remote to use for upstream branches",
+    default=None,
+)
 @click.option(
     "--abort",
     "abort",
@@ -662,6 +699,7 @@ def cherry_pick_cli(
     ctx,
     dry_run,
     pr_remote,
+    upstream_remote,
     abort,
     status,
     push,
@@ -681,6 +719,7 @@ def cherry_pick_cli(
             pr_remote,
             commit_sha1,
             branches,
+            upstream_remote=upstream_remote,
             dry_run=dry_run,
             push=push,
             auto_pr=auto_pr,
diff --git a/readme.rst b/readme.rst
index 13c5f5c..6028338 100644
--- a/readme.rst
+++ b/readme.rst
@@ -1,6 +1,6 @@
 Usage (from a cloned CPython directory) ::
 
-   cherry_picker [--pr-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--status] [--abort/--continue] [--push/--no-push] [--auto-pr/--no-auto-pr] <commit_sha1> <branches>
+   cherry_picker [--pr-remote REMOTE] [--upstream-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--status] [--abort/--continue] [--push/--no-push] [--auto-pr/--no-auto-pr] <commit_sha1> <branches>
 
 |pyversion status|
 |pypi status|
@@ -41,6 +41,8 @@ Requires Python 3.7.
 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
+(e.g. ``--upstream-remote python`` to use a remote named ``python``).
 
 Verify that an ``upstream`` remote is set to the CPython repository:
 
@@ -73,7 +75,7 @@ From the cloned CPython directory:
 
 .. code-block:: console
 
-    (venv) $ cherry_picker [--pr-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--abort/--continue] [--status] [--push/--no-push] [--auto-pr/--no-auto-pr] <commit_sha1> <branches>
+    (venv) $ cherry_picker [--pr-remote REMOTE] [--upstream-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--abort/--continue] [--status] [--push/--no-push] [--auto-pr/--no-auto-pr] <commit_sha1> <branches>
 
 
 Commit sha1
@@ -94,9 +96,11 @@ Options
 
 ::
 
-    --dry-run           Dry Run Mode.  Prints out the commands, but not executed.
-    --pr-remote REMOTE  Specify the git remote to push into.  Default is 'origin'.
-    --status            Do `git status` in cpython directory.
+    --dry-run                 Dry Run Mode.  Prints out the commands, but not executed.
+    --pr-remote REMOTE        Specify the git remote to push into.  Default is 'origin'.
+    --upstream-remote REMOTE  Specify the git remote to use for upstream branches.
+                              Default is 'upstream' or 'origin' if the former doesn't exist.
+    --status                  Do `git status` in cpython directory.
 
 
 Additional options::
@@ -252,6 +256,11 @@ steps it would execute without actually executing any of them. For example:
 This will generate pull requests through a remote other than ``origin``
 (e.g. ``pr``)
 
+`--upstream-remote` option
+--------------------------
+
+This will generate branches from a remote other than ``upstream``/``origin``
+(e.g. ``python``)
 
 `--status` option
 -----------------


Test patch:
diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py
index 6f733e3..1b6d7b0 100644
--- a/cherry_picker/test_cherry_picker.py
+++ b/cherry_picker/test_cherry_picker.py
@@ -11,6 +11,7 @@ import pytest
 from .cherry_picker import (
     DEFAULT_CONFIG,
     WORKFLOW_STATES,
+    BranchCheckoutException,
     CherryPicker,
     CherryPickException,
     InvalidRepoException,
@@ -57,6 +58,12 @@ def git_init():
     return lambda: subprocess.run(git_init_cmd, check=True)
 
 
+@pytest.fixture
+def git_remote():
+    git_remote_cmd = "git", "remote"
+    return lambda *extra_args: (subprocess.run(git_remote_cmd + extra_args, check=True))
+
+
 @pytest.fixture
 def git_add():
     git_add_cmd = "git", "add"
@@ -238,6 +245,62 @@ def test_get_cherry_pick_branch(os_path_exists, config):
     assert cp.get_cherry_pick_branch("3.6") == "backport-22a594a-3.6"
 
 
+@pytest.mark.parametrize(
+    "remote_name,upstream_remote",
+    (
+        ("upstream", None),
+        ("upstream", "upstream"),
+        ("origin", None),
+        ("origin", "origin"),
+        ("python", "python"),
+    ),
+)
+def test_upstream_name(remote_name, upstream_remote, config, tmp_git_repo_dir, git_remote):
+    git_remote("add", remote_name, "https://github.com/python/cpython.git")
+    if remote_name != "origin":
+        git_remote("add", "origin", "https://github.com/miss-islington/cpython.git")
+
+    branches = ["3.6"]
+    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
+        cp = CherryPicker(
+            "origin",
+            "22a594a0047d7706537ff2ac676cdc0f1dcb329c",
+            branches,
+            config=config,
+            upstream_remote=upstream_remote,
+        )
+    assert cp.upstream == remote_name
+
+
+@pytest.mark.parametrize(
+    "remote_to_add,remote_name,upstream_remote",
+    (
+        (None, "upstream", None),
+        ("origin", "upstream", "upstream"),
+        (None, "origin", None),
+        ("upstream", "origin", "origin"),
+        ("origin", "python", "python"),
+        (None, "python", None),
+    ),
+)
+def test_error_on_missing_remote(remote_to_add, remote_name, upstream_remote, config, tmp_git_repo_dir, git_remote):
+    git_remote("add", "some-remote-name", "https://github.com/python/cpython.git")
+    if remote_to_add is not None:
+        git_remote("add", remote_to_add, "https://github.com/miss-islington/cpython.git")
+
+    branches = ["3.6"]
+    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
+        cp = CherryPicker(
+            "origin",
+            "22a594a0047d7706537ff2ac676cdc0f1dcb329c",
+            branches,
+            config=config,
+            upstream_remote=upstream_remote,
+        )
+    with pytest.raises(ValueError):
+        cp.upstream
+
+
 def test_get_pr_url(config):
     branches = ["3.6"]
 
@@ -825,6 +888,38 @@ def test_backport_cherry_pick_crash_ignored(
     assert get_state() == WORKFLOW_STATES.UNSET
 
 
+def test_backport_cherry_pick_branch_already_exists(
+    tmp_git_repo_dir, git_branch, git_add, git_commit, git_checkout
+):
+    cherry_pick_target_branches = ("3.8",)
+    pr_remote = "origin"
+    test_file = "some.file"
+    tmp_git_repo_dir.join(test_file).write("some contents")
+    git_branch(cherry_pick_target_branches[0])
+    git_branch(
+        f"{pr_remote}/{cherry_pick_target_branches[0]}", cherry_pick_target_branches[0]
+    )
+    git_add(test_file)
+    git_commit("Add a test file")
+    scm_revision = get_sha1_from("HEAD")
+
+    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
+        cherry_picker = CherryPicker(
+            pr_remote, scm_revision, cherry_pick_target_branches
+        )
+
+    backport_branch_name = cherry_picker.get_cherry_pick_branch(cherry_pick_target_branches[0])
+    git_branch(backport_branch_name)
+
+    with mock.patch.object(cherry_picker, "fetch_upstream"), pytest.raises(
+        BranchCheckoutException
+    ) as exc_info:
+        cherry_picker.backport()
+
+    assert exc_info.value.branch_name == backport_branch_name
+    assert get_state() == WORKFLOW_STATES.UNSET
+
+
 def test_backport_success(
     tmp_git_repo_dir, git_branch, git_add, git_commit, git_checkout
 ):