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 <gavinmak@google.com>
Reviewed-by: Scott Lee <ddoman@chromium.org>

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)])
 

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,