Repository: python/cherry-picker

Issue:
`cherry_picker --continue` does not respect `--no-push` option
If after resolving conflict, I use `cherry_picker --no-push --continue`, `cherry_picker` will push to remote (and open webbrowser/make PR through API) regardless.

Discussion:
jack1142: Right now this adds an `already_committed` to git config store to indicate whether the user is doing `cherry_picker --continue` after `cherry_picker --no-push ...` or after a `cherry_picker ...` that resulted in a conflict BUT I'm considering whether I won't change this to also support people doing manual `git commit` after resolving conflicts as mentioned [here](https://github.com/python/cherry-picker/issues/7#issuecomment-306003172).

I'll check whether I could update the logic to just check whether there was already a commit made or not and post an update here once I do.

If one of the maintainers sees this before I get to it, does cherry-picker currently support commit ranges? [This comment](https://github.com/python/cherry-picker/issues/13#issuecomment-893579735) suggests that it does but this does not appear to be documented anywhere so I'm wondering whether I need to worry about supporting that too.
jack1142: > I'm considering whether I won't change this to also support people doing manual `git commit` after resolving conflicts as mentioned [here](https://github.com/python/cherry-picker/issues/7#issuecomment-306003172).

I updated the logic to support this. Now, if there is exactly one commit in the backport branch, it has its commit message amended and otherwise, cherry_picker makes a new commit on its own.

Patch:
diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py
index bb4b848..73f2794 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):
         """
@@ -317,6 +341,21 @@ Co-authored-by: {get_author_info_from_short_sha(self.commit_sha1)}"""
                 click.echo(cpe.output)
         return updated_commit_message
 
+    def pause_after_committing(self, cherry_pick_branch):
+        click.echo(
+            f"""
+Finished cherry-pick {self.commit_sha1} into {cherry_pick_branch} \U0001F600
+--no-push option used.
+... Stopping here.
+To continue and push the changes:
+$ cherry_picker --continue
+
+To abort the cherry-pick and cleanup:
+$ cherry_picker --abort
+"""
+        )
+        self.set_paused_state()
+
     def push_to_remote(self, base_branch, head_branch, commit_message=""):
         """git push <origin> <branchname>"""
         set_state(WORKFLOW_STATES.PUSHING_TO_REMOTE)
@@ -425,7 +464,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()
@@ -445,19 +490,7 @@ Co-authored-by: {get_author_info_from_short_sha(self.commit_sha1)}"""
                     if not self.is_mirror():
                         self.cleanup_branch(cherry_pick_branch)
                 else:
-                    click.echo(
-                        f"""
-Finished cherry-pick {self.commit_sha1} into {cherry_pick_branch} \U0001F600
---no-push option used.
-... Stopping here.
-To continue and push the changes:
-    $ cherry_picker --continue
-
-To abort the cherry-pick and cleanup:
-    $ cherry_picker --abort
-"""
-                    )
-                    self.set_paused_state()
+                    self.pause_after_committing(cherry_pick_branch)
                     return  # to preserve the correct state
             set_state(WORKFLOW_STATES.BACKPORT_LOOP_END)
         reset_stored_previous_branch()
@@ -470,14 +503,19 @@ To abort the cherry-pick and cleanup:
         if self.initial_state != WORKFLOW_STATES.BACKPORT_PAUSED:
             raise ValueError("One can only abort a paused process.")
 
-        cmd = ["git", "cherry-pick", "--abort"]
         try:
-            set_state(WORKFLOW_STATES.ABORTING)
-            click.echo(self.run_cmd(cmd))
-            set_state(WORKFLOW_STATES.ABORTED)
-        except subprocess.CalledProcessError as cpe:
-            click.echo(cpe.output)
-            set_state(WORKFLOW_STATES.ABORTING_FAILED)
+            validate_sha("CHERRY_PICK_HEAD")
+        except ValueError:
+            pass
+        else:
+            cmd = ["git", "cherry-pick", "--abort"]
+            try:
+                set_state(WORKFLOW_STATES.ABORTING)
+                click.echo(self.run_cmd(cmd))
+                set_state(WORKFLOW_STATES.ABORTED)
+            except subprocess.CalledProcessError as cpe:
+                click.echo(cpe.output)
+                set_state(WORKFLOW_STATES.ABORTING_FAILED)
         # only delete backport branch created by cherry_picker.py
         if get_current_branch().startswith("backport-"):
             self.cleanup_branch(get_current_branch())
@@ -504,30 +542,39 @@ To abort the cherry-pick and cleanup:
                 cherry_pick_branch.index("-") + 1 : cherry_pick_branch.index(base) - 1
             ]
             self.commit_sha1 = get_full_sha_from_short(short_sha)
-            updated_commit_message = self.get_updated_commit_message(cherry_pick_branch)
-            if self.dry_run:
-                click.echo(
-                    f"  dry-run: git commit -a -m '{updated_commit_message}' --allow-empty"
-                )
-            else:
-                cmd = [
-                    "git",
-                    "commit",
-                    "-a",
-                    "-m",
-                    updated_commit_message,
-                    "--allow-empty",
-                ]
-                self.run_cmd(cmd)
-
-            self.push_to_remote(base, cherry_pick_branch)
 
-            if not self.is_mirror():
-                self.cleanup_branch(cherry_pick_branch)
-
-            click.echo("\nBackport PR:\n")
-            click.echo(updated_commit_message)
-            set_state(WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED)
+            commits = get_commits_from_backport_branch(base)
+            if len(commits) == 1:
+                commit_message = self.amend_commit_message(cherry_pick_branch)
+            else:
+                commit_message = self.get_updated_commit_message(cherry_pick_branch)
+                if self.dry_run:
+                    click.echo(
+                        f"  dry-run: git commit -a -m '{commit_message}' --allow-empty"
+                    )
+                else:
+                    cmd = [
+                        "git",
+                        "commit",
+                        "-a",
+                        "-m",
+                        commit_message,
+                        "--allow-empty",
+                    ]
+                    self.run_cmd(cmd)
+
+            if self.push:
+                self.push_to_remote(base, cherry_pick_branch)
+
+                if not self.is_mirror():
+                    self.cleanup_branch(cherry_pick_branch)
+
+                click.echo("\nBackport PR:\n")
+                click.echo(commit_message)
+                set_state(WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED)
+            else:
+                self.pause_after_committing(cherry_pick_branch)
+                return  # to preserve the correct state
 
         else:
             click.echo(
@@ -605,6 +652,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 +716,7 @@ def cherry_pick_cli(
     ctx,
     dry_run,
     pr_remote,
+    upstream_remote,
     abort,
     status,
     push,
@@ -681,6 +736,7 @@ def cherry_pick_cli(
             pr_remote,
             commit_sha1,
             branches,
+            upstream_remote=upstream_remote,
             dry_run=dry_run,
             push=push,
             auto_pr=auto_pr,
@@ -795,6 +851,13 @@ def get_author_info_from_short_sha(short_sha):
     return author
 
 
+def get_commits_from_backport_branch(cherry_pick_branch):
+    cmd = ["git", "log", "--format=%H", f"{cherry_pick_branch}.."]
+    output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+    commits = output.strip().decode("utf-8").splitlines()
+    return commits
+
+
 def normalize_commit_message(commit_message):
     """
     Return a tuple of title and body from the commit message
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..bc5638b 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,
@@ -18,6 +19,7 @@ from .cherry_picker import (
     from_git_rev_read,
     get_author_info_from_short_sha,
     get_base_branch,
+    get_commits_from_backport_branch,
     get_current_branch,
     get_full_sha_from_short,
     get_sha1_from,
@@ -57,6 +59,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"
@@ -101,6 +109,12 @@ def git_cherry_pick():
     )
 
 
+@pytest.fixture
+def git_reset():
+    git_reset_cmd = "git", "reset"
+    return lambda *extra_args: (subprocess.run(git_reset_cmd + extra_args, check=True))
+
+
 @pytest.fixture
 def git_config():
     git_config_cmd = "git", "config"
@@ -238,6 +252,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 +895,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
 ):
@@ -857,8 +959,10 @@ def test_backport_success(
     assert get_state() == WORKFLOW_STATES.UNSET
 
 
+@pytest.mark.parametrize("already_committed", (True, False))
+@pytest.mark.parametrize("push", (True, False))
 def test_backport_pause_and_continue(
-    tmp_git_repo_dir, git_branch, git_add, git_commit, git_checkout
+    tmp_git_repo_dir, git_branch, git_add, git_commit, git_checkout, git_reset, already_committed, push
 ):
     cherry_pick_target_branches = ("3.8",)
     pr_remote = "origin"
@@ -879,16 +983,27 @@ def test_backport_pause_and_continue(
             pr_remote, scm_revision, cherry_pick_target_branches, push=False
         )
 
-    with mock.patch.object(cherry_picker, "checkout_branch"), mock.patch.object(
-        cherry_picker, "fetch_upstream"
-    ), mock.patch.object(
+    with mock.patch.object(cherry_picker, "fetch_upstream"), mock.patch.object(
         cherry_picker, "amend_commit_message", return_value="commit message"
     ):
         cherry_picker.backport()
 
+    assert len(get_commits_from_backport_branch(cherry_pick_target_branches[0])) == 1
     assert get_state() == WORKFLOW_STATES.BACKPORT_PAUSED
 
-    cherry_picker.initial_state = get_state()
+    if not already_committed:
+        git_reset("HEAD~1")
+        assert len(get_commits_from_backport_branch(cherry_pick_target_branches[0])) == 0
+
+    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
+        cherry_picker = CherryPicker(pr_remote, "", [], push=push)
+
+    commit_message = f"""[{cherry_pick_target_branches[0]}] commit message
+(cherry picked from commit xxxxxxyyyyyy)
+
+
+Co-authored-by: Author Name <author@name.email>"""
+
     with mock.patch(
         "cherry_picker.cherry_picker.wipe_cfg_vals_from_git_cfg"
     ), mock.patch(
@@ -900,21 +1015,29 @@ def test_backport_pause_and_continue(
         "cherry_picker.cherry_picker.get_current_branch",
         return_value="backport-xxx-3.8",
     ), mock.patch.object(
-        cherry_picker,
-        "get_updated_commit_message",
-        return_value="""[3.8] commit message
-(cherry picked from commit xxxxxxyyyyyy)
-
-
-Co-authored-by: Author Name <author@name.email>""",
-    ), mock.patch.object(
+        cherry_picker, "amend_commit_message", return_value=commit_message
+    ) as amend_commit_message, mock.patch.object(
+        cherry_picker, "get_updated_commit_message", return_value=commit_message
+    ) as get_updated_commit_message, mock.patch.object(
         cherry_picker, "checkout_branch"
     ), mock.patch.object(
         cherry_picker, "fetch_upstream"
+    ), mock.patch.object(
+        cherry_picker, "cleanup_branch"
     ):
         cherry_picker.continue_cherry_pick()
 
-    assert get_state() == WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED
+    if already_committed:
+        amend_commit_message.assert_called_once()
+        get_updated_commit_message.assert_not_called()
+    else:
+        get_updated_commit_message.assert_called_once()
+        amend_commit_message.assert_not_called()
+
+    if push:
+        assert get_state() == WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED
+    else:
+        assert get_state() == WORKFLOW_STATES.BACKPORT_PAUSED
 
 
 def test_continue_cherry_pick_invalid_state(tmp_git_repo_dir):
@@ -955,18 +1078,6 @@ def test_abort_cherry_pick_invalid_state(tmp_git_repo_dir):
         cherry_picker.abort_cherry_pick()
 
 
-def test_abort_cherry_pick_fail(tmp_git_repo_dir):
-    set_state(WORKFLOW_STATES.BACKPORT_PAUSED)
-
-    with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
-        cherry_picker = CherryPicker("origin", "xxx", [])
-
-    with mock.patch("cherry_picker.cherry_picker.wipe_cfg_vals_from_git_cfg"):
-        cherry_picker.abort_cherry_pick()
-
-    assert get_state() == WORKFLOW_STATES.ABORTING_FAILED
-
-
 def test_abort_cherry_pick_success(
     tmp_git_repo_dir, git_branch, git_add, git_commit, git_checkout, git_cherry_pick
 ):