Require prompt from users to confirm submodules commit.

Bug: 1481266
Change-Id: Ib840d6e472e3709919518a294e5489ba099a6188
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4875411
Auto-Submit: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Joanna Wang <jojwang@chromium.org>

hooks/pre-commit.py

@@ -17,6 +17,7 @@ import git_common
 from gclient_eval import SYNC
 
 SKIP_VAR = 'SKIP_GITLINK_PRECOMMIT'
+TESTING_ANSWER = 'TESTING_ANSWER'
 
 
 def main():
@@ -55,15 +56,39 @@ def main():
         # DEPS only has to be in sync with gitlinks when state is SYNC.
         exit(0)
 
-    print(f'Found no change to DEPS, unstaging {len(staged_gitlinks)} '
-          f'staged gitlink(s) found in diff:\n{diff}')
-    git_common.run('restore', '--staged', '--', *staged_gitlinks)
+    prompt = (
+        f'Found no change to DEPS, but found staged gitlink(s) in diff:\n{diff}\n'
+        'Press Enter/Return if you intended to include them or "n" to unstage '
+        '(exclude from commit) the gitlink(s): ')
+    print(prompt)
+
+    if os.getenv(TESTING_ANSWER) is not None:
+        answer = os.getenv(TESTING_ANSWER)
+    else:
+        try:
+            sys.stdin = open("/dev/tty", "r")
+        except (FileNotFoundError, OSError):
+            try:
+                sys.stdin = open('CON')
+            except:
+                print(
+                    'Unable to acquire input handle, proceeding without modifications'
+                )
+                exit(0)
+        answer = input()
 
     disable_msg = f'To disable this hook, set {SKIP_VAR}=1'
-    if len(staged_gitlinks) == len(diff.splitlines()):
-        print('Found no changes after unstaging gitlinks, aborting commit.')
-        print(disable_msg)
-        exit(1)
+    if answer.lower() == 'n':
+        print(
+            f'\nUnstaging {len(staged_gitlinks)} staged gitlink(s) found in diff'
+        )
+        git_common.run('restore', '--staged', '--', *staged_gitlinks)
+        if len(staged_gitlinks) == len(diff.splitlines()):
+            print(
+                '\nFound no changes after unstaging gitlinks, aborting commit.')
+            print(disable_msg)
+            exit(1)
+
     print(disable_msg)
 
 

tests/hooks_test.py

@@ -9,6 +9,7 @@ import sys
 import tempfile
 import unittest
 import unittest.mock
+from unittest.mock import patch
 
 ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, ROOT_DIR)
@@ -25,6 +26,7 @@ class HooksTest(unittest.TestCase):
         self.repo = tempfile.mkdtemp()
         self.env = os.environ.copy()
         self.env['SKIP_GITLINK_PRECOMMIT'] = '0'
+        self.env['TESTING_ANSWER'] = 'n'
         self.populate()
 
     def tearDown(self):
@@ -118,12 +120,15 @@ class HooksTest(unittest.TestCase):
         _, stderr = git.run_with_stderr('commit',
                                         '-m',
                                         'regular file and gitlinks',
-                                        cwd=self.repo)
+                                        cwd=self.repo,
+                                        env=self.env)
 
         self.assertIn('dep_a', diff_before_commit)
         self.assertIn('dep_b', diff_before_commit)
         # Gitlinks should be dropped.
-        self.assertIn('unstaging 2 staged gitlink(s)', stderr)
+        self.assertIn(
+            'Found no change to DEPS, but found staged gitlink(s) in diff',
+            stderr)
         diff_after_commit = git.run('diff',
                                     '--name-only',
                                     'HEAD^',
@@ -133,6 +138,46 @@ class HooksTest(unittest.TestCase):
         self.assertNotIn('dep_b', diff_after_commit)
         self.assertIn('foo', diff_after_commit)
 
+    def testPreCommit_IntentionalGitlinkWithoutDEPS(self):
+        # Intentional Gitlink changes staged without a DEPS change.
+        self.write(self.repo, 'foo', 'foo')
+        git.run('add', '.', cwd=self.repo)
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_a',
+                cwd=self.repo)
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"a"*40},dep_b',
+                cwd=self.repo)
+        diff_before_commit = git.run('diff',
+                                     '--cached',
+                                     '--name-only',
+                                     cwd=self.repo)
+        self.env['TESTING_ANSWER'] = ''
+        _, stderr = git.run_with_stderr('commit',
+                                        '-m',
+                                        'regular file and gitlinks',
+                                        cwd=self.repo,
+                                        env=self.env)
+
+        self.assertIn('dep_a', diff_before_commit)
+        self.assertIn('dep_b', diff_before_commit)
+        # Gitlinks should be dropped.
+        self.assertIn(
+            'Found no change to DEPS, but found staged gitlink(s) in diff',
+            stderr)
+        diff_after_commit = git.run('diff',
+                                    '--name-only',
+                                    'HEAD^',
+                                    'HEAD',
+                                    cwd=self.repo)
+        self.assertIn('dep_a', diff_after_commit)
+        self.assertIn('dep_b', diff_after_commit)
+        self.assertIn('foo', diff_after_commit)
+
     def testPreCommit_OnlyGitlinkWithoutDEPS(self):
         # Gitlink changes were staged without a corresponding DEPS change but
         # no other files were included in the commit.
@@ -148,7 +193,8 @@ class HooksTest(unittest.TestCase):
         ret = git.run_with_retcode('commit',
                                    '-m',
                                    'gitlink only',
-                                   cwd=self.repo)
+                                   cwd=self.repo,
+                                   env=self.env)
 
         self.assertIn('dep_a', diff_before_commit)
         # Gitlinks should be droppped and the empty commit should be aborted.
@@ -178,7 +224,8 @@ class HooksTest(unittest.TestCase):
                                    '--all',
                                    '-m',
                                    'commit all',
-                                   cwd=self.repo)
+                                   cwd=self.repo,
+                                   env=self.env)
 
         self.assertIn('dep_a', diff_before_commit)
         self.assertEqual(ret, 0)