From 500d3c203be0f91aabd8263d6f6e0b32c3c9093d Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 7 Aug 2021 02:36:32 +0200 Subject: [PATCH 01/10] Allow specifying custom upstream remote name --- cherry_picker/cherry_picker.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index 5c6effc..3ef4b79 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -93,6 +93,7 @@ def __init__( commit_sha1, branches, *, + upstream_remote=None, dry_run=False, push=True, prefix_commit=True, @@ -122,6 +123,7 @@ def __init__( 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 @@ -138,14 +140,20 @@ def set_paused_state(self): @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. """ + if self.upstream_remote is not None: + return self.upstream_remote cmd = ["git", "remote", "get-url", "upstream"] try: self.run_cmd(cmd) except subprocess.CalledProcessError: - return "origin" - return "upstream" + self.upstream_remote = "origin" + else: + self.upstream_remote = "upstream" + return self.upstream_remote @property def sorted_branches(self): @@ -553,6 +561,13 @@ class state: 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", @@ -607,7 +622,7 @@ class state: @click.argument("branches", nargs=-1) @click.pass_context def cherry_pick_cli( - ctx, dry_run, pr_remote, abort, status, push, auto_pr, config_path, commit_sha1, branches + ctx, dry_run, pr_remote, upstream_remote, abort, status, push, auto_pr, config_path, commit_sha1, branches ): """cherry-pick COMMIT_SHA1 into target BRANCHES.""" @@ -620,6 +635,7 @@ def cherry_pick_cli( pr_remote, commit_sha1, branches, + upstream_remote=upstream_remote, dry_run=dry_run, push=push, auto_pr=auto_pr, From c9172a14e1a8ccd9f30aef456dec6b16ac56b0ea Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 7 Aug 2021 02:48:55 +0200 Subject: [PATCH 02/10] Add test --- cherry_picker/test.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/cherry_picker/test.py b/cherry_picker/test.py index c01ba26..e5885fa 100644 --- a/cherry_picker/test.py +++ b/cherry_picker/test.py @@ -56,6 +56,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" @@ -217,6 +223,27 @@ 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", "origin", "python")) +def test_upstream_name(remote_name, config, tmp_git_repo_dir, git_remote): + upstream_remote = None + if remote_name == "python": + upstream_remote = "python" + 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 + + def test_get_pr_url(config): branches = ["3.6"] From 3a569a1a3d3bc7a906c6810a83f7fc5647f9a3b1 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sun, 8 Aug 2021 21:02:30 +0200 Subject: [PATCH 03/10] Update readme --- readme.rst | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/readme.rst b/readme.rst index 2aa63e1..0d0b4fa 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] + cherry_picker [--pr-remote REMOTE] [--upstream-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--status] [--abort/--continue] [--push/--no-push] |pyversion status| |pypi status| @@ -41,6 +41,9 @@ Requires Python 3.6. 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. +If this is incorrect, then the correct remote will need be specified +using 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:: @@ -69,7 +72,7 @@ From the cloned CPython directory: :: - (venv) $ cherry_picker [--pr-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--abort/--continue] [--status] [--push/--no-push] + (venv) $ cherry_picker [--pr-remote REMOTE] [--upstream-remote REMOTE] [--dry-run] [--config-path CONFIG-PATH] [--abort/--continue] [--status] [--push/--no-push] Commit sha1 @@ -90,9 +93,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:: @@ -242,6 +247,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 ----------------- From ad6cc0d502c4897026c262d2e4f0ed2b25a2af67 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 9 Jul 2022 04:43:25 +0200 Subject: [PATCH 04/10] Parametrize upstream_remote instead of weird ifs --- cherry_picker/test_cherry_picker.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index a39c993..4b8029e 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -233,11 +233,15 @@ 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", "origin", "python")) -def test_upstream_name(remote_name, config, tmp_git_repo_dir, git_remote): - upstream_remote = None - if remote_name == "python": - upstream_remote = "python" +@pytest.mark.parametrize( + "remote_name,upstream_remote", + ( + ("upstream", None), + ("origin", None), + ("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") From 6fd172da7494b0fda099c20e6f16bf61530f66e4 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 9 Jul 2022 04:43:49 +0200 Subject: [PATCH 05/10] Add two more parametrization variants --- cherry_picker/test_cherry_picker.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 4b8029e..f6aab36 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -237,7 +237,9 @@ def test_get_cherry_pick_branch(os_path_exists, config): "remote_name,upstream_remote", ( ("upstream", None), + ("upstream", "upstream"), ("origin", None), + ("origin", "origin"), ("python", "python"), ), ) From 2ba3e1860614691bf7bd6291583f81f562d2be4a Mon Sep 17 00:00:00 2001 From: Jakub Kuczys <6032823+jack1142@users.noreply.github.com> Date: Mon, 15 Aug 2022 15:58:22 +0200 Subject: [PATCH 06/10] Rephrase mention of `--upstream-remote` in README Co-authored-by: Ezio Melotti --- readme.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/readme.rst b/readme.rst index d19bd27..6028338 100644 --- a/readme.rst +++ b/readme.rst @@ -41,9 +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. -If this is incorrect, then the correct remote will need be specified -using the ``--upstream-remote`` option (e.g. -``--upstream-remote python`` to use a remote named ``python``). +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: From a00318f5115f05b29361cafc1d5a2eadfad22f36 Mon Sep 17 00:00:00 2001 From: Jakub Kuczys <6032823+jack1142@users.noreply.github.com> Date: Mon, 15 Aug 2022 15:49:28 +0200 Subject: [PATCH 07/10] Error out early if remote does not exist --- cherry_picker/cherry_picker.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index 98a4832..68fa008 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -2,6 +2,7 @@ import collections import enum +import functools import os import re import subprocess @@ -153,22 +154,31 @@ def remember_previous_branch(self): save_cfg_vals_to_git_cfg(previous_branch=current_branch) @property + @functools.lru_cache(maxsize=None) def upstream(self): """Get the remote name to use for upstream branches Uses the remote passed to `--upstream-remote`. If this flag wasn't passed, it uses "upstream" if it exists or "origin" otherwise. """ - if self.upstream_remote is not None: - return self.upstream_remote 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: - self.upstream_remote = "origin" - else: - self.upstream_remote = "upstream" - return self.upstream_remote + 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( + f"There are no remotes with name 'upstream' or 'origin'." + ) + + return cmd[-1] @property def sorted_branches(self): From 16bd65480b97de01945c81cb88daaa2abfe2dc26 Mon Sep 17 00:00:00 2001 From: Jakub Kuczys <6032823+jack1142@users.noreply.github.com> Date: Mon, 15 Aug 2022 15:57:40 +0200 Subject: [PATCH 08/10] Test that cp.upstream errors on missing remote --- cherry_picker/test_cherry_picker.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index d485010..00caa5c 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -271,6 +271,35 @@ def test_upstream_name(remote_name, upstream_remote, config, tmp_git_repo_dir, g 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"] From 5c7266eafd29fd4d10b81d7e21b36a8c932ef574 Mon Sep 17 00:00:00 2001 From: Jakub Kuczys <6032823+jack1142@users.noreply.github.com> Date: Mon, 15 Aug 2022 16:01:49 +0200 Subject: [PATCH 09/10] Use good-old private attribute instead of the fancy lru_cache --- cherry_picker/cherry_picker.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index 68fa008..e111bec 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -2,7 +2,6 @@ import collections import enum -import functools import os import re import subprocess @@ -138,6 +137,9 @@ def __init__( 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 @@ -154,13 +156,16 @@ def remember_previous_branch(self): save_cfg_vals_to_git_cfg(previous_branch=current_branch) @property - @functools.lru_cache(maxsize=None) def upstream(self): """Get the remote name to use for upstream branches 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 @@ -178,7 +183,8 @@ def upstream(self): f"There are no remotes with name 'upstream' or 'origin'." ) - return cmd[-1] + self._upstream = cmd[-1] + return self._upstream @property def sorted_branches(self): From 935085be6b7c1d68ffeb6c9c58f2c8c417b2f69b Mon Sep 17 00:00:00 2001 From: Jakub Kuczys <6032823+jack1142@users.noreply.github.com> Date: Mon, 15 Aug 2022 16:02:27 +0200 Subject: [PATCH 10/10] Drop unnecessary f from f-string --- cherry_picker/cherry_picker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index e111bec..25da7ad 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -180,7 +180,7 @@ def upstream(self): self.run_cmd(cmd) except subprocess.CalledProcessError: raise ValueError( - f"There are no remotes with name 'upstream' or 'origin'." + "There are no remotes with name 'upstream' or 'origin'." ) self._upstream = cmd[-1]