Skip to content

Commit

Permalink
Fix --no-push related issues in --continue and --abort (#43)
Browse files Browse the repository at this point in the history 
* Fix `--no-push` related issues in `--continue` and `--abort`

* Fix existing test

* Add tests

* Use different logic that should also work with manual use of git commit

* Update tests

* Remove test_abort_cherry_pick_fail

I'm not entirely convinced there is no way for `cherry-pick --abort`
to still fail but I can't think of a test case I could use to replace
this.
Due to newly added logic in `abort_cherry_pick()`, aborting is simply
skipped if there is no CHERRY_PICK_HEAD which is why in this case
it no longer fails.
  • Loading branch information
@Jackenmen
Jackenmen authored Oct 3, 2022
1 parent 7e333ac commit b1647e8
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 69 deletions.
110 changes: 67 additions & 43 deletions cherry_picker/cherry_picker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,21 @@ def amend_commit_message(self, cherry_pick_branch):
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)
Expand Down Expand Up @@ -475,19 +490,7 @@ def backport(self):
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()
Expand All @@ -500,14 +503,19 @@ def abort_cherry_pick(self):
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())
Expand All @@ -534,30 +542,39 @@ def continue_cherry_pick(self):
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(
Expand Down Expand Up @@ -834,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
Expand Down
68 changes: 42 additions & 26 deletions cherry_picker/test_cherry_picker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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,
Expand Down Expand Up @@ -108,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"
Expand Down Expand Up @@ -952,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"
Expand All @@ -974,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 <[email protected]>"""

with mock.patch(
"cherry_picker.cherry_picker.wipe_cfg_vals_from_git_cfg"
), mock.patch(
Expand All @@ -995,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 <[email protected]>""",
), 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):
Expand Down Expand Up @@ -1050,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
):
Expand Down

0 comments on commit b1647e8

Please sign in to comment.