From 5a3a5f7cec40c70d8c5ceb473f828e1149724962 Mon Sep 17 00:00:00 2001 From: Jason Chang Date: Thu, 17 Aug 2023 11:36:41 -0700 Subject: upload: fix error handling There was a bug in error handeling code that caused an uncaught exception to be raised. Bug: b/296316540 Change-Id: I49c72f29c00f26ba60de552f958bc6eddf841162 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/383254 Reviewed-by: Mike Frysinger Commit-Queue: Jason Chang Tested-by: Jason Chang --- subcmds/upload.py | 250 +++++++++++++++++++++++++++--------------------------- 1 file changed, 125 insertions(+), 125 deletions(-) (limited to 'subcmds/upload.py') 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): - have_errors = False - 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)) + 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 ) - for label in opt.labels: - labels.update(_ExpandCommaList(label)) - - # Handle e-mail notifications. - if opt.notify is False: - notify = "NONE" + print("\n".join(changes)) + print("Continue uploading? (y/N) ", end="", flush=True) + if opt.yes: + print("<--yes>") + a = "yes" else: - key = ( - "review.%s.uploadnotify" % branch.project.remote.review - ) - notify = branch.project.config.GetString(key) + 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 + 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 - ) + 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, + 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 - branch.uploaded = True - except UploadError as e: + def _UploadAndReport(self, opt, todo, people): + have_errors = False + aggregate_errors = [] + for branch in todo: + try: + 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: -- cgit v1.2.3-54-g00ecf