From 6f7b85f2675c228309d400fbe01359f136a0b80b Mon Sep 17 00:00:00 2001 From: Gavin Mak Date: Wed, 10 Apr 2024 19:52:15 +0000 Subject: [PATCH] Hide git log by default if rolling from a non-public host Always displaying the commit message/git log makes it easier to upload a manual roll with private details. Omit the git log if rolling from a non-public host. The `--always-log` flag can be used to include it anyway. Bug: 332331835 Change-Id: Ic9201d5323fa04a55dcf4a451a9f76c48eebfd5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5444190 Commit-Queue: Gavin Mak Reviewed-by: Scott Lee --- roll_dep.py | 59 ++++++++++++++++++++++++++++++++++-------- tests/roll_dep_test.py | 42 ++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/roll_dep.py b/roll_dep.py index 5b64a7928a..d727f84f00 100755 --- a/roll_dep.py +++ b/roll_dep.py @@ -38,6 +38,24 @@ _ROLL_SUBJECT = re.compile( r'Roll recipe dependencies \(trivial\)\.' r')$') +_PUBLIC_GERRIT_HOSTS = { + 'android', + 'aomedia', + 'boringssl', + 'chromium', + 'dart', + 'dawn', + 'fuchsia', + 'gn', + 'go', + 'llvm', + 'pdfium', + 'quiche', + 'skia', + 'swiftshader', + 'webrtc', +} + class Error(Exception): pass @@ -77,10 +95,15 @@ def is_pristine(root): and not check_output(diff_cmd + ['--cached'], cwd=root).strip()) +def get_gerrit_host(url): + """Returns the host for a given Gitiles URL.""" + m = re.match(r'https://([^/]*)\.googlesource\.com/', url) + return m and m.group(1) + + def get_log_url(upstream_url, head, tot): """Returns an URL to read logs via a Web UI if applicable.""" - if re.match(r'https://[^/]*\.googlesource\.com/', upstream_url): - # gitiles + if get_gerrit_host(upstream_url): return '%s/+log/%s..%s' % (upstream_url, head[:12], tot[:12]) if upstream_url.startswith('https://github.com/'): upstream_url = upstream_url.rstrip('/') @@ -97,7 +120,7 @@ def should_show_log(upstream_url): return False if 'webrtc' in upstream_url: return False - return True + return get_gerrit_host(upstream_url) in _PUBLIC_GERRIT_HOSTS def gclient(args): @@ -105,14 +128,11 @@ def gclient(args): return check_output([sys.executable, GCLIENT_PATH] + args).strip() -def generate_commit_message(full_dir, dependency, head, roll_to, no_log, - log_limit): +def generate_commit_message(full_dir, dependency, head, roll_to, upstream_url, + show_log, log_limit): """Creates the commit message for this specific roll.""" commit_range = '%s..%s' % (head, roll_to) commit_range_for_header = '%s..%s' % (head[:9], roll_to[:9]) - upstream_url = check_output(['git', 'config', 'remote.origin.url'], - cwd=full_dir).strip() - log_url = get_log_url(upstream_url, head, roll_to) cmd = ['git', 'log', commit_range, '--date=short', '--no-merges'] logs = check_output( # Args with '=' are automatically quoted. @@ -130,11 +150,11 @@ def generate_commit_message(full_dir, dependency, head, roll_to, no_log, 's' if nb_commits > 1 else '', ('; %s trivial rolls' % rolls) if rolls else '') log_section = '' - if log_url: + if log_url := get_log_url(upstream_url, head, roll_to): log_section = log_url + '\n\n' # It is important that --no-log continues to work, as it is used by # internal -> external rollers. Please do not remove or break it. - if not no_log and should_show_log(upstream_url): + if show_log: log_section += '$ %s ' % ' '.join(cmd) log_section += '--format=\'%ad %ae %s\'\n' log_section = log_section.replace(commit_range, commit_range_for_header) @@ -247,6 +267,10 @@ def main(): '--no-log', action='store_true', help='Do not include the short log in the commit message') + parser.add_argument( + '--always-log', + action='store_true', + help='Always include the short log in the commit message') parser.add_argument('--log-limit', type=int, default=100, @@ -270,6 +294,10 @@ def main(): if args.key: parser.error( 'Can\'t use multiple paths to roll simultaneously and --key') + + if args.no_log and args.always_log: + parser.error('Can\'t use both --no-log and --always-log') + reviewers = None if args.reviewer: reviewers = list(itertools.chain(*[r.split(',') @@ -315,8 +343,17 @@ def main(): logs = [] setdep_args = [] for dependency, (head, roll_to, full_dir) in sorted(rolls.items()): + upstream_url = check_output(['git', 'config', 'remote.origin.url'], + cwd=full_dir).strip() + show_log = args.always_log or \ + (not args.no_log and should_show_log(upstream_url)) + if not show_log: + print( + f'{dependency}: Omitting git log from the commit message. ' + 'Use the `--always-log` flag to include it.') log = generate_commit_message(full_dir, dependency, head, roll_to, - args.no_log, args.log_limit) + upstream_url, show_log, + args.log_limit) logs.append(log) setdep_args.extend(['-r', '{}@{}'.format(dependency, roll_to)]) diff --git a/tests/roll_dep_test.py b/tests/roll_dep_test.py index 86fcb38dd9..a798e40f16 100755 --- a/tests/roll_dep_test.py +++ b/tests/roll_dep_test.py @@ -8,10 +8,12 @@ import os import sys import subprocess import unittest +from unittest import mock ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) +import roll_dep from testing_support import fake_repos ROLL_DEP = os.path.join(ROOT_DIR, 'roll-dep') @@ -204,6 +206,46 @@ class RollDepTest(fake_repos.FakeReposTestBase): self.assertIn(expected_message, commit_message) +class CommitMessageTest(unittest.TestCase): + + def setUp(self): + self.logs = '\n'.join([ + '2024-04-05 alice Goodbye', + '2024-04-03 bob Hello World', + ]) + + # Mock the `git log` call. + mock.patch('roll_dep.check_output', return_value=self.logs).start() + self.addCleanup(mock.patch.stopall) + + def testShowShortLog(self): + message = roll_dep.generate_commit_message( + '/path/to/dir', 'dep', 'abc', 'def', + 'https://chromium.googlesource.com', True, 10) + + self.assertIn('Roll dep/ abc..def (2 commits)', message) + self.assertIn('$ git log', message) + self.assertIn(self.logs, message) + + def testHideShortLog(self): + message = roll_dep.generate_commit_message( + '/path/to/dir', 'dep', 'abc', 'def', + 'https://chromium.googlesource.com', False, 10) + + self.assertNotIn('$ git log', message) + self.assertNotIn(self.logs, message) + + def testShouldShowLogWithPublicHost(self): + self.assertTrue( + roll_dep.should_show_log( + 'https://chromium.googlesource.com/project')) + + def testShouldNotShowLogWithPrivateHost(self): + self.assertFalse( + roll_dep.should_show_log( + 'https://private.googlesource.com/project')) + + if __name__ == '__main__': level = logging.DEBUG if '-v' in sys.argv else logging.FATAL logging.basicConfig(level=level,