From f303fd2aeb072a23b67d251c388b4f349ea6853d Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 7 Aug 2021 20:41:18 +0200 Subject: [PATCH 1/6] Fix `--no-push` related issues in `--continue` and `--abort` --- cherry_picker/cherry_picker.py | 121 +++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 44 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index bb4b848..235465a 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -317,6 +317,22 @@ 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 +""" + ) + set_is_already_committed() + self.set_paused_state() + def push_to_remote(self, base_branch, head_branch, commit_message=""): """git push """ set_state(WORKFLOW_STATES.PUSHING_TO_REMOTE) @@ -445,19 +461,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() @@ -470,18 +474,20 @@ 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) + if not get_is_already_committed(): + 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()) + reset_is_already_committed() reset_stored_previous_branch() reset_stored_config_ref() reset_state() @@ -504,30 +510,38 @@ 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) + if get_is_already_committed(): + commit_message = self.get_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( @@ -535,6 +549,7 @@ def continue_cherry_pick(self): ) set_state(WORKFLOW_STATES.CONTINUATION_FAILED) + reset_is_already_committed() reset_stored_previous_branch() reset_stored_config_ref() reset_state() @@ -880,6 +895,24 @@ def reset_stored_previous_branch(): wipe_cfg_vals_from_git_cfg("previous_branch") +def get_is_already_committed(): + """Get information from Git config about the cherry-pick being already committed.""" + return bool(int(load_val_from_git_cfg("already_committed") or "0")) + + +def set_is_already_committed(): + """Remove information from Git config about the cherry-pick being already committed.""" + save_cfg_vals_to_git_cfg(already_committed="1") + + +def reset_is_already_committed(): + """Remove information from Git config about the cherry-pick being already committed.""" + try: + wipe_cfg_vals_from_git_cfg("already_committed") + except subprocess.CalledProcessError: + """Information was not stored in Git config.""" + + def reset_state(): """Remove the progress state from Git config.""" wipe_cfg_vals_from_git_cfg("state") From 3d361f292c861d642eb568072dc4ac25aadc703e Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 7 Aug 2021 20:12:01 +0200 Subject: [PATCH 2/6] Fix existing test --- cherry_picker/test_cherry_picker.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 6f733e3..1ba2284 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -888,7 +888,9 @@ def test_backport_pause_and_continue( assert get_state() == WORKFLOW_STATES.BACKPORT_PAUSED - cherry_picker.initial_state = get_state() + with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True): + cherry_picker = CherryPicker(pr_remote, "", []) + with mock.patch( "cherry_picker.cherry_picker.wipe_cfg_vals_from_git_cfg" ), mock.patch( From 8113c7e22307e9f42ee598e41282851f3d676d99 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 7 Aug 2021 22:44:09 +0200 Subject: [PATCH 3/6] Add tests --- cherry_picker/test_cherry_picker.py | 42 +++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 1ba2284..2fa8e29 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -20,11 +20,13 @@ get_base_branch, get_current_branch, get_full_sha_from_short, + get_is_already_committed, get_sha1_from, get_state, load_config, load_val_from_git_cfg, normalize_commit_message, + reset_is_already_committed, reset_state, reset_stored_config_ref, set_state, @@ -857,8 +859,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, already_committed, push ): cherry_pick_target_branches = ("3.8",) pr_remote = "origin" @@ -886,10 +890,20 @@ def test_backport_pause_and_continue( ): cherry_picker.backport() + assert get_is_already_committed() assert get_state() == WORKFLOW_STATES.BACKPORT_PAUSED + if not already_committed: + reset_is_already_committed() + with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True): - cherry_picker = CherryPicker(pr_remote, "", []) + 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 """ with mock.patch( "cherry_picker.cherry_picker.wipe_cfg_vals_from_git_cfg" @@ -902,21 +916,27 @@ 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 """, - ), mock.patch.object( + cherry_picker, "get_commit_message", return_value=commit_message + ) as get_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" ): cherry_picker.continue_cherry_pick() - assert get_state() == WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED + if already_committed: + get_commit_message.assert_called_once() + get_updated_commit_message.assert_not_called() + else: + get_updated_commit_message.assert_called_once() + get_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): From a107630da897a474d189b6e04c2c89f90a036ebd Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sun, 8 Aug 2021 01:33:16 +0200 Subject: [PATCH 4/6] Use different logic that should also work with manual use of git commit --- cherry_picker/cherry_picker.py | 39 +++++++++++++--------------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker.py index 235465a..dd9cd9e 100755 --- a/cherry_picker/cherry_picker.py +++ b/cherry_picker/cherry_picker.py @@ -330,7 +330,6 @@ def pause_after_committing(self, cherry_pick_branch): $ cherry_picker --abort """ ) - set_is_already_committed() self.set_paused_state() def push_to_remote(self, base_branch, head_branch, commit_message=""): @@ -474,7 +473,11 @@ def abort_cherry_pick(self): if self.initial_state != WORKFLOW_STATES.BACKPORT_PAUSED: raise ValueError("One can only abort a paused process.") - if not get_is_already_committed(): + try: + validate_sha("CHERRY_PICK_HEAD") + except ValueError: + pass + else: cmd = ["git", "cherry-pick", "--abort"] try: set_state(WORKFLOW_STATES.ABORTING) @@ -487,7 +490,6 @@ def abort_cherry_pick(self): if get_current_branch().startswith("backport-"): self.cleanup_branch(get_current_branch()) - reset_is_already_committed() reset_stored_previous_branch() reset_stored_config_ref() reset_state() @@ -511,8 +513,9 @@ def continue_cherry_pick(self): ] self.commit_sha1 = get_full_sha_from_short(short_sha) - if get_is_already_committed(): - commit_message = self.get_commit_message(cherry_pick_branch) + 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: @@ -549,7 +552,6 @@ def continue_cherry_pick(self): ) set_state(WORKFLOW_STATES.CONTINUATION_FAILED) - reset_is_already_committed() reset_stored_previous_branch() reset_stored_config_ref() reset_state() @@ -810,6 +812,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 @@ -895,24 +904,6 @@ def reset_stored_previous_branch(): wipe_cfg_vals_from_git_cfg("previous_branch") -def get_is_already_committed(): - """Get information from Git config about the cherry-pick being already committed.""" - return bool(int(load_val_from_git_cfg("already_committed") or "0")) - - -def set_is_already_committed(): - """Remove information from Git config about the cherry-pick being already committed.""" - save_cfg_vals_to_git_cfg(already_committed="1") - - -def reset_is_already_committed(): - """Remove information from Git config about the cherry-pick being already committed.""" - try: - wipe_cfg_vals_from_git_cfg("already_committed") - except subprocess.CalledProcessError: - """Information was not stored in Git config.""" - - def reset_state(): """Remove the progress state from Git config.""" wipe_cfg_vals_from_git_cfg("state") From 79a95a8b7257fa239772a5765097980e9a654964 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sat, 25 Dec 2021 01:46:06 +0100 Subject: [PATCH 5/6] Update tests --- cherry_picker/test_cherry_picker.py | 30 +++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 2fa8e29..46359f9 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -18,15 +18,14 @@ 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_is_already_committed, get_sha1_from, get_state, load_config, load_val_from_git_cfg, normalize_commit_message, - reset_is_already_committed, reset_state, reset_stored_config_ref, set_state, @@ -103,6 +102,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" @@ -862,7 +867,7 @@ def test_backport_success( @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, already_committed, push + 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" @@ -883,18 +888,17 @@ 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 get_is_already_committed() + assert len(get_commits_from_backport_branch(cherry_pick_target_branches[0])) == 1 assert get_state() == WORKFLOW_STATES.BACKPORT_PAUSED if not already_committed: - reset_is_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) @@ -916,22 +920,24 @@ 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_commit_message", return_value=commit_message - ) as get_commit_message, 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() if already_committed: - get_commit_message.assert_called_once() + amend_commit_message.assert_called_once() get_updated_commit_message.assert_not_called() else: get_updated_commit_message.assert_called_once() - get_commit_message.assert_not_called() + amend_commit_message.assert_not_called() if push: assert get_state() == WORKFLOW_STATES.BACKPORTING_CONTINUATION_SUCCEED From 9873e90d9e5e784429979ec3423d5b3edead1ecc Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Sun, 8 Aug 2021 02:27:12 +0200 Subject: [PATCH 6/6] 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. --- cherry_picker/test_cherry_picker.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/cherry_picker/test_cherry_picker.py b/cherry_picker/test_cherry_picker.py index 46359f9..c2ef1f5 100644 --- a/cherry_picker/test_cherry_picker.py +++ b/cherry_picker/test_cherry_picker.py @@ -983,18 +983,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 ):