Reland "support for --fix and automated fixup notices" This reverts commit 7f9c6f84c8d6ba534db0070e07479231c3ea8dc8. Reason for reland: the companion change in git-repo (https://gerrit-review.googlesource.com/c/git-repo/+/619281) has now been released in v2.67 / stable. Bug: 546510319 Test: python3 -m pytest Change-Id: I6fd5d2eb0b24b671bcddc11b3a054a03021f3b61 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repohooks/+/630821 Reviewed-by: Gavin Mak <gavinmak@google.com> Reviewed-by: Rahul Yadav <yadavrah@google.com> Commit-Queue: Rahul Yadav <yadavrah@google.com> Tested-by: Rahul Yadav <yadavrah@google.com>
diff --git a/pre-upload.py b/pre-upload.py index 0f589c2..f167f39 100755 --- a/pre-upload.py +++ b/pre-upload.py
@@ -285,7 +285,9 @@ def _attempt_fixes( - projects_results: List[rh.results.ProjectResults], yes: bool = False + projects_results: List[rh.results.ProjectResults], + fix: bool = False, + yes: bool = False, ) -> None: """Attempts to fix fixable results.""" # Filter out any result that has a fixup. @@ -301,14 +303,22 @@ banner = f"Multiple fixups ({len(fixups)}) are available." else: banner = "Automated fixups are available." + + # Non-interactive without explicit --fix: do not prompt, do not mutate + # files. + if not fix and (yes or not sys.stdin.isatty()): + banner += ( + "\nTo apply them, run:\n" + " repo upload --fix\n" + "Then amend/rebase and upload again.\n" + ) + print(Output.COLOR.color(Output.COLOR.MAGENTA, banner), file=sys.stderr) + return + print(Output.COLOR.color(Output.COLOR.MAGENTA, banner), file=sys.stderr) - # If there's more than one fixup available, ask if they want to blindly run - # them all, or prompt for them one-by-one. - mode = "some" - if yes: - mode = "all" - elif len(fixups) > 1: + mode = "all" if fix else "some" + if not fix and len(fixups) > 1: while True: response = rh.terminal.str_prompt( "What would you like to do", @@ -570,6 +580,7 @@ jobs: Optional[int] = None, from_git: bool = False, commit_list: Optional[List[str]] = None, + fix: bool = False, yes: bool = False, ) -> bool: """Run all the hooks @@ -583,6 +594,7 @@ commit_list: A list of commits to run hooks against. If None or empty list then we'll automatically get the list of commits that would be uploaded. + fix: Automatically apply all automated fixup prompts. yes: Answer yes to all safe prompts. Returns: @@ -607,14 +619,14 @@ # very minimal, so we don't add it then. print("", file=sys.stderr) - _attempt_fixes(results, yes=yes) + _attempt_fixes(results, fix=fix, yes=yes) ret = not any(results) finally: rh.trace.exit_session(0 if ret else 1) return ret -def main(project_list, worktree_list=None, yes=False, **_kwargs): +def main(project_list, worktree_list=None, fix=False, yes=False, **_kwargs): """Main function invoked directly by repo. We must use the name "main" as that is what repo requires. @@ -628,12 +640,13 @@ project_list, so that each entry in project_list matches with a directory in worktree_list. If None, we will attempt to calculate the directories automatically. + fix: Automatically apply all automated fixup prompts. yes: Answer yes to all safe prompts. kwargs: Leave this here for forward-compatibility. """ if not worktree_list: worktree_list = [None] * len(project_list) - if not _run_projects_hooks(project_list, worktree_list, yes=yes): + if not _run_projects_hooks(project_list, worktree_list, fix=fix, yes=yes): color = rh.terminal.Color() print( color.color(color.RED, "FATAL") @@ -720,6 +733,11 @@ "current system.", ) parser.add_argument( + "--fix", + action="store_true", + help="Automatically apply all automated fixups without prompting", + ) + parser.add_argument( "-y", "--yes", action="store_true", @@ -755,6 +773,7 @@ jobs=opts.jobs, from_git=opts.git, commit_list=opts.commits, + fix=opts.fix, yes=opts.yes, ): return 0
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py index 6591e5f..0271cb0 100755 --- a/pre-upload_unittest.py +++ b/pre-upload_unittest.py
@@ -16,6 +16,7 @@ """Unittests for pre-upload.py.""" import importlib.util +import io import os from pathlib import Path import sys @@ -29,6 +30,7 @@ sys.path.insert(0, str(THIS_DIR)) # pylint: disable=wrong-import-position +import rh.results # noqa: E402 import rh.utils # noqa: E402 @@ -49,16 +51,18 @@ """Test pre-upload.py main/direct_main parameter propagation.""" @mock.patch("pre_upload._run_projects_hooks", return_value=True) - def test_main_yes_propagation(self, mock_run): - """Verify main(..., yes=True) passes yes=True to _run_projects_hooks.""" - pre_upload.main(["project"], yes=True) - mock_run.assert_called_once_with(["project"], [None], yes=True) + def test_main_fix_propagation(self, mock_run): + """Verify main(..., fix=True, yes=True) passes flags to _run_projects_hooks.""" + pre_upload.main(["project"], fix=True, yes=True) + mock_run.assert_called_once_with( + ["project"], [None], fix=True, yes=True + ) @mock.patch("pre_upload._run_project_hooks", return_value=True) @mock.patch("pre_upload._attempt_fixes") - def test_run_projects_hooks_yes_propagation(self, mock_attempt, mock_run): - """Verify _run_projects_hooks passes yes=True to _attempt_fixes.""" - pre_upload._run_projects_hooks(["p1"], [None], yes=True) + def test_run_projects_hooks_fix_propagation(self, mock_attempt, mock_run): + """Verify _run_projects_hooks passes fix=True to _attempt_fixes.""" + pre_upload._run_projects_hooks(["p1"], [None], fix=True, yes=True) mock_run.assert_called_once_with( "p1", proj_dir=None, @@ -66,31 +70,76 @@ from_git=False, commit_list=None, ) - mock_attempt.assert_called_once_with([True], yes=True) + mock_attempt.assert_called_once_with([True], fix=True, yes=True) @mock.patch("pre_upload._run_projects_hooks", return_value=True) @mock.patch("pre_upload._identify_project", return_value="project") @mock.patch("rh.git.is_git_repository", return_value=True) @mock.patch("rh.utils.run") - def test_direct_main_yes( + def test_direct_main_fix_yes( self, mock_run_cmd, _mock_is_git, _mock_identify, mock_run ): - """Verify direct_main with --yes passes yes=True to - _run_projects_hooks. - """ + """Verify direct_main with --fix and --yes passes flags to _run_projects_hooks.""" mock_run_cmd.return_value = rh.utils.CompletedProcess( stdout="/path/to/repo/.git\n" ) - pre_upload.direct_main(["--yes", "commit_hash"]) + pre_upload.direct_main(["--fix", "--yes", "commit_hash"]) mock_run.assert_called_once_with( ["project"], ["/path/to/repo"], jobs=None, from_git=False, commit_list=["commit_hash"], + fix=True, yes=True, ) + @mock.patch("sys.stdin.isatty", return_value=True) + @mock.patch("rh.terminal.boolean_prompt") + @mock.patch("rh.utils.run") + def test_attempt_fixes_with_fix_flag( + self, mock_run, mock_prompt, _mock_isatty + ): + """Verify _attempt_fixes with fix=True executes fixups without prompting.""" + hook_result = rh.results.HookResult( + "test_hook", + "test_project", + "test_commit", + error="error", + fixup_cmd=["fix_cmd"], + ) + proj_result = rh.results.ProjectResults( + "test_project", "/workdir", [hook_result] + ) + + pre_upload._attempt_fixes([proj_result], fix=True) + + mock_prompt.assert_not_called() + mock_run.assert_called_once() + + @mock.patch("sys.stderr", new_callable=io.StringIO) + @mock.patch("sys.stdin.isatty", return_value=False) + @mock.patch("rh.utils.run") + def test_attempt_fixes_non_interactive_no_fix( + self, mock_run, _mock_isatty, mock_stderr + ): + """Verify _attempt_fixes without fix in non-interactive mode does not run fixups.""" + hook_result = rh.results.HookResult( + "test_hook", + "test_project", + "test_commit", + error="error", + fixup_cmd=["fix_cmd"], + ) + proj_result = rh.results.ProjectResults( + "test_project", "/workdir", [hook_result] + ) + + pre_upload._attempt_fixes([proj_result], fix=False, yes=True) + + mock_run.assert_not_called() + self.assertIn("repo upload --fix", mock_stderr.getvalue()) + if __name__ == "__main__": unittest.main()