From f7e85d34707e685410ce2a61cdb281d27d29d144 Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Fri, 14 Feb 2025 09:53:47 -0800 Subject: [PATCH] Remove redundant try/catch for better error reporting During testing, I discovered that the try/catch block surrounding most of SplitCl is broken: subprocess errors seem to return bytes more often than strings. When I fixed that, however, I found that the errors were much more comprehensible if we simply re-raised the initial error and got all the information it contained; this both gives us a stack trace and makes it very obvious an error occurred. Specifically, we also had the problem that stderr was sometimes empty even if the process failed (because it wrote to stdout instead), and if we just wrote the output then it wasn't clear that an error happened until you actually read the output, as opposed to the exception printing where the stack trace makes it obvious. Since re-raising without additional work is pointless, I just removed the block entirely. Bug: 389069356 Change-Id: I1d1bec765469d0b70b463f7214f47a25316a37c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6258516 Reviewed-by: Josip Sokcevic Commit-Queue: Devon Loehr --- split_cl.py | 144 ++++++++++++++++++++++++---------------------------- 1 file changed, 67 insertions(+), 77 deletions(-) diff --git a/split_cl.py b/split_cl.py index 557781d28f..f544805f0e 100644 --- a/split_cl.py +++ b/split_cl.py @@ -8,8 +8,6 @@ import collections import dataclasses import os import re -import subprocess2 -import sys import tempfile from typing import List, Set, Tuple, Dict, Any @@ -372,90 +370,82 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, comment = gclient_utils.FileRead(comment_file) if comment_file else None - try: - EnsureInGitRepository() + EnsureInGitRepository() - cl = changelist() - upstream = cl.GetCommonAncestorWithUpstream() - files = [ - (action.strip(), f) - for action, f in scm.GIT.CaptureStatus(repository_root, upstream) - ] + cl = changelist() + upstream = cl.GetCommonAncestorWithUpstream() + files = [(action.strip(), f) + for action, f in scm.GIT.CaptureStatus(repository_root, upstream)] - if not files: - print('Cannot split an empty CL.') - return 1 - - author = git.run('config', 'user.email').strip() or None - refactor_branch = git.current_branch() - assert refactor_branch, "Can't run from detached branch." - refactor_branch_upstream = git.upstream(refactor_branch) - assert refactor_branch_upstream, \ - "Branch %s must have an upstream." % refactor_branch + if not files: + print('Cannot split an empty CL.') + return 1 - if not dry_run and not CheckDescriptionBugLink(description): - return 0 + author = git.run('config', 'user.email').strip() or None + refactor_branch = git.current_branch() + assert refactor_branch, "Can't run from detached branch." + refactor_branch_upstream = git.upstream(refactor_branch) + assert refactor_branch_upstream, \ + "Branch %s must have an upstream." % refactor_branch - if from_file: - cl_infos = LoadSplittingFromFile(from_file, files_on_disk=files) - else: - files_split_by_reviewers = SelectReviewersForFiles( - cl, author, files, max_depth) + if not dry_run and not CheckDescriptionBugLink(description): + return 0 - cl_infos = CLInfoFromFilesAndOwnersDirectoriesDict( - files_split_by_reviewers) + if from_file: + cl_infos = LoadSplittingFromFile(from_file, files_on_disk=files) + else: + files_split_by_reviewers = SelectReviewersForFiles( + cl, author, files, max_depth) - if not dry_run: - PrintSummary(cl_infos, refactor_branch) - answer = gclient_utils.AskForData( - 'Proceed? (y/N, or i to edit interactively): ') - if answer.lower() == 'i': - cl_infos = EditSplittingInteractively(cl_infos, - files_on_disk=files) - else: - # Save even if we're continuing, so the user can safely resume an - # aborted upload with the same splitting - SaveSplittingToTempFile(cl_infos) - if answer.lower() != 'y': - return 0 - - cls_per_reviewer = collections.defaultdict(int) - for cl_index, cl_info in enumerate(cl_infos, 1): - # Convert reviewers from tuple to set. - if dry_run: - file_paths = [f for _, f in cl_info.files] - PrintClInfo(cl_index, len(cl_infos), cl_info.directories, - file_paths, description, cl_info.reviewers, - cq_dry_run, enable_auto_submit, topic) - else: - UploadCl(refactor_branch, refactor_branch_upstream, - cl_info.directories, cl_info.files, description, - comment, cl_info.reviewers, changelist, cmd_upload, - cq_dry_run, enable_auto_submit, topic, repository_root) - - for reviewer in cl_info.reviewers: - cls_per_reviewer[reviewer] += 1 - - # List the top reviewers that will be sent the most CLs as a result of - # the split. - reviewer_rankings = sorted(cls_per_reviewer.items(), - key=lambda item: item[1], - reverse=True) - print('The top reviewers are:') - for reviewer, count in reviewer_rankings[:CL_SPLIT_TOP_REVIEWERS]: - print(f' {reviewer}: {count} CLs') + cl_infos = CLInfoFromFilesAndOwnersDirectoriesDict( + files_split_by_reviewers) - if dry_run: - # Wait until now to save the splitting so the file name doesn't get - # washed away by the flood of dry-run printing. + if not dry_run: + PrintSummary(cl_infos, refactor_branch) + answer = gclient_utils.AskForData( + 'Proceed? (y/N, or i to edit interactively): ') + if answer.lower() == 'i': + cl_infos = EditSplittingInteractively(cl_infos, files_on_disk=files) + else: + # Save even if we're continuing, so the user can safely resume an + # aborted upload with the same splitting SaveSplittingToTempFile(cl_infos) + if answer.lower() != 'y': + return 0 - # Go back to the original branch. - git.run('checkout', refactor_branch) - - except subprocess2.CalledProcessError as cpe: - sys.stderr.write(cpe.stderr) - return 1 + cls_per_reviewer = collections.defaultdict(int) + for cl_index, cl_info in enumerate(cl_infos, 1): + # Convert reviewers from tuple to set. + if dry_run: + file_paths = [f for _, f in cl_info.files] + PrintClInfo(cl_index, len(cl_infos), cl_info.directories, + file_paths, description, cl_info.reviewers, cq_dry_run, + enable_auto_submit, topic) + else: + UploadCl(refactor_branch, refactor_branch_upstream, + cl_info.directories, cl_info.files, description, comment, + cl_info.reviewers, changelist, cmd_upload, cq_dry_run, + enable_auto_submit, topic, repository_root) + + for reviewer in cl_info.reviewers: + cls_per_reviewer[reviewer] += 1 + + # List the top reviewers that will be sent the most CLs as a result of + # the split. + reviewer_rankings = sorted(cls_per_reviewer.items(), + key=lambda item: item[1], + reverse=True) + print('The top reviewers are:') + for reviewer, count in reviewer_rankings[:CL_SPLIT_TOP_REVIEWERS]: + print(f' {reviewer}: {count} CLs') + + if dry_run: + # Wait until now to save the splitting so the file name doesn't get + # washed away by the flood of dry-run printing. + SaveSplittingToTempFile(cl_infos) + + # Go back to the original branch. + git.run('checkout', refactor_branch) return 0