diff --git a/project.py b/project.py index 6e6a605e..a3b6312e 100644 --- a/project.py +++ b/project.py @@ -1116,7 +1116,8 @@ class Project(object): if not re.match(r"^.+[+-][0-9]+$", label): raise UploadError( f'invalid label syntax "{label}": labels use forms like ' - "CodeReview+1 or Verified-1" + "CodeReview+1 or Verified-1", + project=self.name, ) if dest_branch is None: @@ -1132,7 +1133,7 @@ class Project(object): url = branch.remote.ReviewUrl(self.UserEmail, validate_certs) if url is None: - raise UploadError("review not configured") + raise UploadError("review not configured", project=self.name) cmd = ["push"] if dryrun: cmd.append("-n") @@ -1177,8 +1178,9 @@ class Project(object): ref_spec = ref_spec + "%" + ",".join(opts) cmd.append(ref_spec) - if GitCommand(self, cmd, bare=True).Wait() != 0: - raise UploadError("Upload failed") + GitCommand( + self, cmd, bare=True, capture_stderr=True, verify_command=True + ).Wait() if not dryrun: msg = "posted to %s for %s" % (branch.remote.review, dest_branch) diff --git a/subcmds/upload.py b/subcmds/upload.py index d0c028b9..040eaeb5 100644 --- a/subcmds/upload.py +++ b/subcmds/upload.py @@ -21,7 +21,7 @@ from typing import List from command import DEFAULT_LOCAL_JOBS, InteractiveCommand from editor import Editor -from error import UploadError, RepoExitError +from error import UploadError, SilentRepoExitError, GitError from git_command import GitCommand from git_refs import R_HEADS from hooks import RepoHook @@ -31,7 +31,7 @@ from project import ReviewableBranch _DEFAULT_UNUSUAL_COMMIT_THRESHOLD = 5 -class UploadExitError(RepoExitError): +class UploadExitError(SilentRepoExitError): """Indicates that there is an upload command error requiring a sys exit.""" @@ -532,137 +532,137 @@ Gerrit Code Review: https://www.gerritcodereview.com/ except (AttributeError, IndexError): return "" - def _UploadAndReport(self, opt, todo, original_people): + def _UploadBranch(self, opt, branch, original_people): + """Upload Branch.""" + people = copy.deepcopy(original_people) + self._AppendAutoList(branch, people) + + # Check if there are local changes that may have been forgotten. + changes = branch.project.UncommitedFiles() + if opt.ignore_untracked_files: + untracked = set(branch.project.UntrackedFiles()) + changes = [x for x in changes if x not in untracked] + + if changes: + key = "review.%s.autoupload" % branch.project.remote.review + answer = branch.project.config.GetBoolean(key) + + # If they want to auto upload, let's not ask because it + # could be automated. + if answer is None: + print() + print( + "Uncommitted changes in %s (did you forget to " + "amend?):" % branch.project.name + ) + print("\n".join(changes)) + print("Continue uploading? (y/N) ", end="", flush=True) + if opt.yes: + print("<--yes>") + a = "yes" + else: + a = sys.stdin.readline().strip().lower() + if a not in ("y", "yes", "t", "true", "on"): + print("skipping upload", file=sys.stderr) + branch.uploaded = False + branch.error = "User aborted" + return + + # Check if topic branches should be sent to the server during + # upload. + if opt.auto_topic is not True: + key = "review.%s.uploadtopic" % branch.project.remote.review + opt.auto_topic = branch.project.config.GetBoolean(key) + + def _ExpandCommaList(value): + """Split |value| up into comma delimited entries.""" + if not value: + return + for ret in value.split(","): + ret = ret.strip() + if ret: + yield ret + + # Check if hashtags should be included. + key = "review.%s.uploadhashtags" % branch.project.remote.review + hashtags = set(_ExpandCommaList(branch.project.config.GetString(key))) + for tag in opt.hashtags: + hashtags.update(_ExpandCommaList(tag)) + if opt.hashtag_branch: + hashtags.add(branch.name) + + # Check if labels should be included. + key = "review.%s.uploadlabels" % branch.project.remote.review + labels = set(_ExpandCommaList(branch.project.config.GetString(key))) + for label in opt.labels: + labels.update(_ExpandCommaList(label)) + + # Handle e-mail notifications. + if opt.notify is False: + notify = "NONE" + else: + key = "review.%s.uploadnotify" % branch.project.remote.review + notify = branch.project.config.GetString(key) + + destination = opt.dest_branch or branch.project.dest_branch + + if branch.project.dest_branch and not opt.dest_branch: + merge_branch = self._GetMergeBranch( + branch.project, local_branch=branch.name + ) + + full_dest = destination + if not full_dest.startswith(R_HEADS): + full_dest = R_HEADS + full_dest + + # If the merge branch of the local branch is different from + # the project's revision AND destination, this might not be + # intentional. + if ( + merge_branch + and merge_branch != branch.project.revisionExpr + and merge_branch != full_dest + ): + print( + f"For local branch {branch.name}: merge branch " + f"{merge_branch} does not match destination branch " + f"{destination}" + ) + print("skipping upload.") + print( + f"Please use `--destination {destination}` if this " + "is intentional" + ) + branch.uploaded = False + return + + branch.UploadForReview( + people, + dryrun=opt.dryrun, + auto_topic=opt.auto_topic, + hashtags=hashtags, + labels=labels, + private=opt.private, + notify=notify, + wip=opt.wip, + ready=opt.ready, + dest_branch=destination, + validate_certs=opt.validate_certs, + push_options=opt.push_options, + ) + + branch.uploaded = True + + def _UploadAndReport(self, opt, todo, people): have_errors = False + aggregate_errors = [] for branch in todo: try: - people = copy.deepcopy(original_people) - self._AppendAutoList(branch, people) - - # Check if there are local changes that may have been forgotten. - changes = branch.project.UncommitedFiles() - if opt.ignore_untracked_files: - untracked = set(branch.project.UntrackedFiles()) - changes = [x for x in changes if x not in untracked] - - if changes: - key = "review.%s.autoupload" % branch.project.remote.review - answer = branch.project.config.GetBoolean(key) - - # If they want to auto upload, let's not ask because it - # could be automated. - if answer is None: - print() - print( - "Uncommitted changes in %s (did you forget to " - "amend?):" % branch.project.name - ) - print("\n".join(changes)) - print("Continue uploading? (y/N) ", end="", flush=True) - if opt.yes: - print("<--yes>") - a = "yes" - else: - a = sys.stdin.readline().strip().lower() - if a not in ("y", "yes", "t", "true", "on"): - print("skipping upload", file=sys.stderr) - branch.uploaded = False - branch.error = "User aborted" - continue - - # Check if topic branches should be sent to the server during - # upload. - if opt.auto_topic is not True: - key = "review.%s.uploadtopic" % branch.project.remote.review - opt.auto_topic = branch.project.config.GetBoolean(key) - - def _ExpandCommaList(value): - """Split |value| up into comma delimited entries.""" - if not value: - return - for ret in value.split(","): - ret = ret.strip() - if ret: - yield ret - - # Check if hashtags should be included. - key = "review.%s.uploadhashtags" % branch.project.remote.review - hashtags = set( - _ExpandCommaList(branch.project.config.GetString(key)) - ) - for tag in opt.hashtags: - hashtags.update(_ExpandCommaList(tag)) - if opt.hashtag_branch: - hashtags.add(branch.name) - - # Check if labels should be included. - key = "review.%s.uploadlabels" % branch.project.remote.review - labels = set( - _ExpandCommaList(branch.project.config.GetString(key)) - ) - for label in opt.labels: - labels.update(_ExpandCommaList(label)) - - # Handle e-mail notifications. - if opt.notify is False: - notify = "NONE" - else: - key = ( - "review.%s.uploadnotify" % branch.project.remote.review - ) - notify = branch.project.config.GetString(key) - - destination = opt.dest_branch or branch.project.dest_branch - - if branch.project.dest_branch and not opt.dest_branch: - merge_branch = self._GetMergeBranch( - branch.project, local_branch=branch.name - ) - - full_dest = destination - if not full_dest.startswith(R_HEADS): - full_dest = R_HEADS + full_dest - - # If the merge branch of the local branch is different from - # the project's revision AND destination, this might not be - # intentional. - if ( - merge_branch - and merge_branch != branch.project.revisionExpr - and merge_branch != full_dest - ): - print( - f"For local branch {branch.name}: merge branch " - f"{merge_branch} does not match destination branch " - f"{destination}" - ) - print("skipping upload.") - print( - f"Please use `--destination {destination}` if this " - "is intentional" - ) - branch.uploaded = False - continue - - branch.UploadForReview( - people, - dryrun=opt.dryrun, - auto_topic=opt.auto_topic, - hashtags=hashtags, - labels=labels, - private=opt.private, - notify=notify, - wip=opt.wip, - ready=opt.ready, - dest_branch=destination, - validate_certs=opt.validate_certs, - push_options=opt.push_options, - ) - - branch.uploaded = True - except UploadError as e: + self._UploadBranch(opt, branch, people) + except (UploadError, GitError) as e: self.git_event_log.ErrorEvent(f"upload error: {e}") branch.error = e + aggregate_errors.append(e) branch.uploaded = False have_errors = True @@ -701,7 +701,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/ ) if have_errors: - raise branch.error + raise UploadExitError(aggregate_errors=aggregate_errors) def _GetMergeBranch(self, project, local_branch=None): if local_branch is None: diff --git a/tests/test_subcmds_upload.py b/tests/test_subcmds_upload.py new file mode 100644 index 00000000..75811996 --- /dev/null +++ b/tests/test_subcmds_upload.py @@ -0,0 +1,69 @@ +# Copyright (C) 2023 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unittests for the subcmds/upload.py module.""" + +import unittest +from unittest import mock + +from subcmds import upload +from error import UploadError, GitError + + +class UnexpectedError(Exception): + """An exception not expected by upload command.""" + + +class UploadCommand(unittest.TestCase): + """Check registered all_commands.""" + + def setUp(self): + self.cmd = upload.Upload() + self.branch = mock.MagicMock() + self.people = mock.MagicMock() + self.opt, _ = self.cmd.OptionParser.parse_args([]) + mock.patch.object( + self.cmd, "_AppendAutoList", return_value=None + ).start() + mock.patch.object(self.cmd, "git_event_log").start() + + def tearDown(self): + mock.patch.stopall() + + def test_UploadAndReport_UploadError(self): + """Check UploadExitError raised when UploadError encountered.""" + side_effect = UploadError("upload error") + with mock.patch.object( + self.cmd, "_UploadBranch", side_effect=side_effect + ): + with self.assertRaises(upload.UploadExitError): + self.cmd._UploadAndReport(self.opt, [self.branch], self.people) + + def test_UploadAndReport_GitError(self): + """Check UploadExitError raised when GitError encountered.""" + side_effect = GitError("some git error") + with mock.patch.object( + self.cmd, "_UploadBranch", side_effect=side_effect + ): + with self.assertRaises(upload.UploadExitError): + self.cmd._UploadAndReport(self.opt, [self.branch], self.people) + + def test_UploadAndReport_UnhandledError(self): + """Check UnexpectedError passed through.""" + side_effect = UnexpectedError("some os error") + with mock.patch.object( + self.cmd, "_UploadBranch", side_effect=side_effect + ): + with self.assertRaises(type(side_effect)): + self.cmd._UploadAndReport(self.opt, [self.branch], self.people)