From d98f3935247818eb5f5ff2f3816d67c680915161 Mon Sep 17 00:00:00 2001 From: David Greenaway Date: Fri, 9 Dec 2022 09:38:24 +1100 Subject: [PATCH] upload: Allow user to configure unusual commit threshold Add a per-remote option `uploadwarningthreshold` allowing the user to override how many commits can be uploaded prior to a warning being displayed. Change-Id: Ia7e1b2c7de89a0bf9ca1c24cc83dc595b3667437 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/354375 Tested-by: David Greenaway Reviewed-by: Mike Frysinger --- docs/internal-fs-layout.md | 35 +++++++-------- subcmds/upload.py | 88 ++++++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 44 deletions(-) diff --git a/docs/internal-fs-layout.md b/docs/internal-fs-layout.md index a9bd1d26..8be61782 100644 --- a/docs/internal-fs-layout.md +++ b/docs/internal-fs-layout.md @@ -222,23 +222,24 @@ The `[remote]` settings are automatically populated/updated from the manifest. The `[branch]` settings are updated by `repo start` and `git branch`. -| Setting | Subcommands | Use/Meaning | -|-------------------------------|---------------|-------------| -| review.\.autocopy | upload | Automatically add to `--cc=` | -| review.\.autoreviewer | upload | Automatically add to `--reviewers=` | -| review.\.autoupload | upload | Automatically answer "yes" or "no" to all prompts | -| review.\.uploadhashtags | upload | Automatically add to `--hashtag=` | -| review.\.uploadlabels | upload | Automatically add to `--label=` | -| review.\.uploadnotify | upload | [Notify setting][upload-notify] to use | -| review.\.uploadtopic | upload | Default [topic] to use | -| review.\.username | upload | Override username with `ssh://` review URIs | -| remote.\.fetch | sync | Set of refs to fetch | -| remote.\.projectname | \ | The name of the project as it exists in Gerrit review | -| remote.\.pushurl | upload | The base URI for pushing CLs | -| remote.\.review | upload | The URI of the Gerrit review server | -| remote.\.url | sync & upload | The URI of the git project to fetch | -| branch.\.merge | sync & upload | The branch to merge & upload & track | -| branch.\.remote | sync & upload | The remote to track | +| Setting | Subcommands | Use/Meaning | +|---------------------------------------|---------------|-------------| +| review.\.autocopy | upload | Automatically add to `--cc=` | +| review.\.autoreviewer | upload | Automatically add to `--reviewers=` | +| review.\.autoupload | upload | Automatically answer "yes" or "no" to all prompts | +| review.\.uploadhashtags | upload | Automatically add to `--hashtag=` | +| review.\.uploadlabels | upload | Automatically add to `--label=` | +| review.\.uploadnotify | upload | [Notify setting][upload-notify] to use | +| review.\.uploadtopic | upload | Default [topic] to use | +| review.\.uploadwarningthreshold | upload | Warn when attempting to upload more than this many CLs | +| review.\.username | upload | Override username with `ssh://` review URIs | +| remote.\.fetch | sync | Set of refs to fetch | +| remote.\.projectname | \ | The name of the project as it exists in Gerrit review | +| remote.\.pushurl | upload | The base URI for pushing CLs | +| remote.\.review | upload | The URI of the Gerrit review server | +| remote.\.url | sync & upload | The URI of the git project to fetch | +| branch.\.merge | sync & upload | The branch to merge & upload & track | +| branch.\.remote | sync & upload | The remote to track | ## ~/ dotconfig layout diff --git a/subcmds/upload.py b/subcmds/upload.py index 0ad3ce26..ceab6463 100644 --- a/subcmds/upload.py +++ b/subcmds/upload.py @@ -17,6 +17,7 @@ import functools import optparse import re import sys +from typing import List from command import DEFAULT_LOCAL_JOBS, InteractiveCommand from editor import Editor @@ -24,21 +25,54 @@ from error import UploadError from git_command import GitCommand from git_refs import R_HEADS from hooks import RepoHook +from project import ReviewableBranch -UNUSUAL_COMMIT_THRESHOLD = 5 +_DEFAULT_UNUSUAL_COMMIT_THRESHOLD = 5 -def _ConfirmManyUploads(multiple_branches=False): - if multiple_branches: - print('ATTENTION: One or more branches has an unusually high number ' - 'of commits.') - else: - print('ATTENTION: You are uploading an unusually high number of commits.') - print('YOU PROBABLY DO NOT MEAN TO DO THIS. (Did you rebase across ' - 'branches?)') - answer = input("If you are sure you intend to do this, type 'yes': ").strip() - return answer == "yes" +def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool: + """Perform basic safety checks on the given set of branches. + + Ensures that each branch does not have a "large" number of commits + and, if so, prompts the user to confirm they want to proceed with + the upload. + + Returns true if all branches pass the safety check or the user + confirmed. Returns false if the upload should be aborted. + """ + + # Determine if any branch has a suspicious number of commits. + many_commits = False + for branch in branches: + # Get the user's unusual threshold for the branch. + # + # Each branch may be configured to have a different threshold. + remote = branch.project.GetBranch(branch.name).remote + key = f'review.{remote.review}.uploadwarningthreshold' + threshold = branch.project.config.GetInt(key) + if threshold is None: + threshold = _DEFAULT_UNUSUAL_COMMIT_THRESHOLD + + # If the branch has more commits than the threshold, show a warning. + if len(branch.commits) > threshold: + many_commits = True + break + + # If any branch has many commits, prompt the user. + if many_commits: + if len(branches) > 1: + print('ATTENTION: One or more branches has an unusually high number ' + 'of commits.') + else: + print('ATTENTION: You are uploading an unusually high number of commits.') + print('YOU PROBABLY DO NOT MEAN TO DO THIS. (Did you rebase across ' + 'branches?)') + answer = input( + "If you are sure you intend to do this, type 'yes': ").strip() + return answer == 'yes' + + return True def _die(fmt, *args): @@ -149,6 +183,13 @@ review.URL.uploadnotify: Control e-mail notifications when uploading. https://gerrit-review.googlesource.com/Documentation/user-upload.html#notify +review.URL.uploadwarningthreshold: + +Repo will warn you if you are attempting to upload a large number +of commits in one or more branches. By default, the threshold +is five commits. This option allows you to override the warning +threshold to a different value. + # References Gerrit Code Review: https://www.gerritcodereview.com/ @@ -261,16 +302,15 @@ Gerrit Code Review: https://www.gerritcodereview.com/ else: answer = sys.stdin.readline().strip().lower() answer = answer in ('y', 'yes', '1', 'true', 't') + if not answer: + _die("upload aborted by user") - if not opt.yes and answer: - if len(branch.commits) > UNUSUAL_COMMIT_THRESHOLD: - answer = _ConfirmManyUploads() - - if answer: - self._UploadAndReport(opt, [branch], people) - else: + # Perform some basic safety checks prior to uploading. + if not opt.yes and not _VerifyPendingCommits([branch]): _die("upload aborted by user") + self._UploadAndReport(opt, [branch], people) + def _MultipleBranches(self, opt, pending, people): projects = {} branches = {} @@ -337,15 +377,9 @@ Gerrit Code Review: https://www.gerritcodereview.com/ if not todo: _die("nothing uncommented for upload") - if not opt.yes: - many_commits = False - for branch in todo: - if len(branch.commits) > UNUSUAL_COMMIT_THRESHOLD: - many_commits = True - break - if many_commits: - if not _ConfirmManyUploads(multiple_branches=True): - _die("upload aborted by user") + # Perform some basic safety checks prior to uploading. + if not opt.yes and not _VerifyPendingCommits(todo): + _die("upload aborted by user") self._UploadAndReport(opt, todo, people)