From 2f85c32d1fa7d81ceffdeafd5ae1d588005c0803 Mon Sep 17 00:00:00 2001 From: Jakub Kuczys Date: Tue, 4 Oct 2022 01:18:29 +0200 Subject: [PATCH] Allow specifying custom upstream remote name (#35) * Allow specifying custom upstream remote name * Add test * Update readme * Parametrize upstream_remote instead of weird ifs * Add two more parametrization variants * Rephrase mention of `--upstream-remote` in README Co-authored-by: Ezio Melotti * Error out early if remote does not exist * Test that cp.upstream errors on missing remote * Use good-old private attribute instead of the fancy lru_cache * Drop unnecessary f from f-string Co-authored-by: Ezio Melotti --- cherry_picker/cherry_picker.py | 39 ++++++++++++++++-- cherry_picker/test_cherry_picker.py | 62 +++++++++++++++++++++++++++++ readme.rst | 19 ++++++--- 3 files changed, 112 insertions(+), 8 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index bb4b848..25da7ad 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -99,6 +99,7 @@ def __init__( commit_sha1, branches, *, + upstream_remote=None, dry_run=False, push=True, prefix_commit=True, @@ -128,6 +129,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 @@ -135,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 @@ -153,14 +158,33 @@ def remember_previous_branch(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. """ + # 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): @@ -605,6 +629,13 @@ def is_mirror(self) -> bool: 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 +693,7 @@ def cherry_pick_cli( ctx, dry_run, pr_remote, + upstream_remote, abort, status, push, @@ -681,6 +713,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/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 6f733e3..00caa5c 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -57,6 +57,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 +244,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"] 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] + 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] |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] + (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 @@ -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 -----------------