Add gclient installhooks to add pre-commit hook

With submodules, users can accidentally stage and commit gitlink
changes. Add a new gclient command to install a pre-commit hook to
automatically drop gitlink changes that don't correspond to a DEPS
change.

Dropping gitlinks can be bypassed by setting
SKIP_GITLINK_PRECOMMIT=1.

Bug: 1481266
Change-Id: Idd8b273e7d8e37d52627964e8ed6004d068b6b7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4863221
Reviewed-by: Joanna Wang <jojwang@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

gclient.py

@@ -128,6 +128,8 @@ PREVIOUS_SYNC_COMMITS = 'GCLIENT_PREVIOUS_SYNC_COMMITS'
 
 NO_SYNC_EXPERIMENT = 'no-sync'
 
+PRECOMMIT_HOOK_VAR = 'GCLIENT_PRECOMMIT'
+
 
 class GNException(Exception):
     pass
@@ -1982,6 +1984,46 @@ it or fix the checkout.
             patch_refs[patch_repo] = patch_ref
         return patch_refs, target_branches
 
+    def _InstallPreCommitHook(self):
+        # On Windows, this path is written to the file as
+        # "dir\hooks\pre-commit.py" but it gets interpreted as
+        # "dirhookspre-commit.py".
+        gclient_hook_path = os.path.join(DEPOT_TOOLS_DIR, 'hooks',
+                                         'pre-commit.py').replace('\\', '\\\\')
+        gclient_hook_content = '\n'.join((
+            f'{PRECOMMIT_HOOK_VAR}={gclient_hook_path}',
+            f'if [ -f "${PRECOMMIT_HOOK_VAR}" ]; then',
+            f'    python3 "${PRECOMMIT_HOOK_VAR}" || exit 1',
+            'fi',
+        ))
+
+        soln = gclient_paths.GetPrimarySolutionPath()
+        if not soln:
+            print('Could not find gclient solution.')
+            return
+
+        git_dir = os.path.join(soln, '.git')
+        if not os.path.exists(git_dir):
+            return
+
+        hook = os.path.join(git_dir, 'hooks', 'pre-commit')
+        if os.path.exists(hook):
+            with open(hook, 'r') as f:
+                content = f.read()
+            if PRECOMMIT_HOOK_VAR in content:
+                print(f'{hook} already contains the gclient pre-commit hook.')
+            else:
+                print(f'A pre-commit hook already exists at {hook}.\n'
+                      f'Please append the following lines to the hook:\n\n'
+                      f'{gclient_hook_content}')
+            return
+
+        print(f'Creating a pre-commit hook at {hook}')
+        with open(hook, 'w') as f:
+            f.write('#!/bin/sh\n')
+            f.write(f'{gclient_hook_content}\n')
+        os.chmod(hook, 0o755)
+
     def _RemoveUnversionedGitDirs(self):
         """Remove directories that are no longer part of the checkout.
 
@@ -3552,6 +3594,23 @@ def CMDrunhooks(parser, args):
     return client.RunOnDeps('runhooks', args)
 
 
+# TODO(crbug.com/1481266): Collect merics for installhooks.
+def CMDinstallhooks(parser, args):
+    """Installs gclient git hooks.
+
+    Currently only installs a pre-commit hook to drop staged gitlinks. To
+    bypass this pre-commit hook once it's installed, set the environment
+    variable SKIP_GITLINK_PRECOMMIT=1.
+    """
+    (options, args) = parser.parse_args(args)
+    client = GClient.LoadCurrentConfig(options)
+    if not client:
+        raise gclient_utils.Error(
+            'client not configured; see \'gclient config\'')
+    client._InstallPreCommitHook()
+    return 0
+
+
 @metrics.collector.collect_metrics('gclient revinfo')
 def CMDrevinfo(parser, args):
     """Outputs revision info mapping for the client and its dependencies.

hooks/pre-commit.py

@@ -0,0 +1,71 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""A git pre-commit hook to drop staged gitlink changes.
+
+To bypass this hook, set SKIP_GITLINK_PRECOMMIT=1.
+"""
+
+import os
+import sys
+
+ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+sys.path.insert(0, ROOT_DIR)
+
+import git_common
+from gclient_eval import SYNC
+
+SKIP_VAR = 'SKIP_GITLINK_PRECOMMIT'
+
+
+def main():
+    if os.getenv(SKIP_VAR) == '1':
+        print(f'{SKIP_VAR} is set. Committing gitlinks, if any.')
+        exit(0)
+
+    has_deps_diff = False
+    staged_gitlinks = []
+    diff = git_common.run('diff-index', '--cached', 'HEAD')
+    for line in diff.splitlines():
+        path = line.split()[-1]
+        if path == 'DEPS':
+            has_deps_diff = True
+            continue
+        if line.startswith(':160000 160000'):
+            staged_gitlinks.append(path)
+
+    if not staged_gitlinks or has_deps_diff:
+        exit(0)
+
+    # There are staged gitlinks and DEPS wasn't changed. Get git_dependencies
+    # migration state in DEPS.
+    state = None
+    try:
+        with open('DEPS', 'r') as f:
+            for l in f.readlines():
+                if l.startswith('git_dependencies'):
+                    state = l.split()[-1].strip(' "\'')
+                    break
+    except OSError:
+        # Don't abort the commit if DEPS wasn't found.
+        exit(0)
+
+    if state != SYNC:
+        # 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)
+
+    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)
+    print(disable_msg)
+
+
+if __name__ == "__main__":
+    main()

tests/gclient_git_smoketest.py

@@ -387,6 +387,41 @@ class GClientSmokeGIT(gclient_smoketest_base.GClientSmokeBase):
             self.githash('repo_2', 1),
             self.gitrevparse(os.path.join(self.root_dir, 'src/repo2')))
 
+    def testInstallHooks_no_existing_hook(self):
+        repo = os.path.join(self.git_base, 'repo_18')
+        self.gclient(['config', repo, '--name', 'src'], cwd=repo)
+        self.gclient(['sync'], cwd=repo)
+        self.gclient(['installhooks'], cwd=repo)
+
+        hook = os.path.join(repo, 'src', '.git', 'hooks', 'pre-commit')
+        with open(hook) as f:
+            contents = f.read()
+            self.assertIn('python3 "$GCLIENT_PRECOMMIT"', contents)
+
+        # Later runs should only inform that the hook was already installed.
+        stdout, _, _ = self.gclient(['installhooks'], cwd=repo)
+        self.assertIn(f'{hook} already contains the gclient pre-commit hook.',
+                      stdout)
+
+    def testInstallHooks_existing_hook(self):
+        repo = os.path.join(self.git_base, 'repo_19')
+        self.gclient(['config', repo, '--name', 'src'], cwd=repo)
+        self.gclient(['sync'], cwd=repo)
+
+        # Create an existing pre-commit hook.
+        hook = os.path.join(repo, 'src', '.git', 'hooks', 'pre-commit')
+        hook_contents = ['#!/bin/sh', 'echo hook']
+        with open(hook, 'w') as f:
+            f.write('\n'.join(hook_contents))
+
+        stdout, _, _ = self.gclient(['installhooks'], cwd=repo)
+        self.assertIn('Please append the following lines to the hook', stdout)
+
+        # The orignal hook is left unchanged.
+        with open(hook) as f:
+            contents = f.read().splitlines()
+            self.assertEqual(hook_contents, contents)
+
     def testRunHooks(self):
         self.gclient(['config', self.git_base + 'repo_1', '--name', 'src'])
         self.gclient(['sync', '--deps', 'mac'])

tests/hooks_test.py

@@ -0,0 +1,275 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os
+import os.path
+import sys
+import tempfile
+import unittest
+import unittest.mock
+
+ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+sys.path.insert(0, ROOT_DIR)
+
+from gclient import PRECOMMIT_HOOK_VAR
+import gclient_utils
+from gclient_eval import SYNC, SUBMODULES
+import git_common as git
+
+
+class HooksTest(unittest.TestCase):
+    def setUp(self):
+        super(HooksTest, self).setUp()
+        self.repo = tempfile.mkdtemp()
+        self.env = os.environ.copy()
+        self.env['SKIP_GITLINK_PRECOMMIT'] = '0'
+        self.populate()
+
+    def tearDown(self):
+        gclient_utils.rmtree(self.repo)
+
+    def write(self, repo, path, content):
+        with open(os.path.join(repo, path), 'w') as f:
+            f.write(content)
+
+    def populate(self):
+        git.run('init', cwd=self.repo)
+        deps_content = '\n'.join((
+            f'git_dependencies = "{SYNC}"',
+            'deps = {',
+            f'    "dep_a": "host://dep_a@{"a"*40}",',
+            f'    "dep_b": "host://dep_b@{"b"*40}",',
+            '}',
+        ))
+        self.write(self.repo, 'DEPS', deps_content)
+
+        self.dep_a_repo = os.path.join(self.repo, 'dep_a')
+        os.mkdir(self.dep_a_repo)
+        git.run('init', cwd=self.dep_a_repo)
+        os.mkdir(os.path.join(self.repo, 'dep_b'))
+        gitmodules_content = '\n'.join((
+            '[submodule "dep_a"]'
+            '\tpath = dep_a',
+            '\turl = host://dep_a',
+            '[submodule "dep_b"]'
+            '\tpath = dep_b',
+            '\turl = host://dep_b',
+        ))
+        self.write(self.repo, '.gitmodules', gitmodules_content)
+        git.run('update-index',
+                '--add',
+                '--cacheinfo',
+                f'160000,{"a"*40},dep_a',
+                cwd=self.repo)
+        git.run('update-index',
+                '--add',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_b',
+                cwd=self.repo)
+
+        git.run('add', '.', cwd=self.repo)
+        git.run('commit', '-m', 'init', cwd=self.repo)
+
+        # On Windows, this path is written to the file as
+        # "root_dir\hooks\pre-commit.py", but it gets interpreted as
+        # "root_dirhookspre-commit.py".
+        precommit_path = os.path.join(ROOT_DIR, 'hooks',
+                                      'pre-commit.py').replace('\\', '\\\\')
+        precommit_content = '\n'.join((
+            '#!/bin/sh',
+            f'{PRECOMMIT_HOOK_VAR}={precommit_path}',
+            f'if [ -f "${PRECOMMIT_HOOK_VAR}" ]; then',
+            f'    python3 "${PRECOMMIT_HOOK_VAR}" || exit 1',
+            'fi',
+        ))
+        self.write(self.repo, os.path.join('.git', 'hooks', 'pre-commit'),
+                   precommit_content)
+        os.chmod(os.path.join(self.repo, '.git', 'hooks', 'pre-commit'), 0o755)
+
+    def testPreCommit_NoGitlinkOrDEPS(self):
+        # Sanity check. Neither gitlinks nor DEPS are touched.
+        self.write(self.repo, 'foo', 'foo')
+        git.run('add', '.', cwd=self.repo)
+        expected_diff = git.run('diff', '--cached', cwd=self.repo)
+        git.run('commit', '-m', 'foo', cwd=self.repo)
+        self.assertEqual(expected_diff,
+                         git.run('diff', 'HEAD^', 'HEAD', cwd=self.repo))
+
+    def testPreCommit_GitlinkWithoutDEPS(self):
+        # Gitlink changes were staged without a corresponding 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)
+        _, stderr = git.run_with_stderr('commit',
+                                        '-m',
+                                        'regular file and gitlinks',
+                                        cwd=self.repo)
+
+        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)
+        diff_after_commit = git.run('diff',
+                                    '--name-only',
+                                    'HEAD^',
+                                    'HEAD',
+                                    cwd=self.repo)
+        self.assertNotIn('dep_a', diff_after_commit)
+        self.assertNotIn('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.
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_a',
+                cwd=self.repo)
+        diff_before_commit = git.run('diff',
+                                     '--cached',
+                                     '--name-only',
+                                     cwd=self.repo)
+        ret = git.run_with_retcode('commit',
+                                   '-m',
+                                   'gitlink only',
+                                   cwd=self.repo)
+
+        self.assertIn('dep_a', diff_before_commit)
+        # Gitlinks should be droppped and the empty commit should be aborted.
+        self.assertEqual(ret, 1)
+        diff_after_commit = git.run('diff',
+                                    '--cached',
+                                    '--name-only',
+                                    cwd=self.repo)
+        self.assertNotIn('dep_a', diff_after_commit)
+
+    def testPreCommit_CommitAll(self):
+        self.write(self.repo, 'foo', 'foo')
+        git.run('add', '.', cwd=self.repo)
+        git.run('commit', '-m', 'add foo', cwd=self.repo)
+        self.write(self.repo, 'foo', 'foo2')
+
+        # Create a new commit in dep_a.
+        self.write(self.dep_a_repo, 'sub_foo', 'sub_foo')
+        git.run('add', '.', cwd=self.dep_a_repo)
+        git.run('commit', '-m', 'sub_foo', cwd=self.dep_a_repo)
+
+        diff_before_commit = git.run('status',
+                                     cwd=self.repo)
+        self.assertIn('foo', diff_before_commit)
+        self.assertIn('dep_a', diff_before_commit)
+        ret = git.run_with_retcode('commit',
+                                   '--all',
+                                   '-m',
+                                   'commit all',
+                                   cwd=self.repo)
+
+        self.assertIn('dep_a', diff_before_commit)
+        self.assertEqual(ret, 0)
+        diff_after_commit = git.run('diff',
+                                    '--cached',
+                                    '--name-only',
+                                    cwd=self.repo)
+        self.assertNotIn('dep_a', diff_after_commit)
+        diff_from_commit = git.run('diff',
+                                    '--name-only',
+                                    'HEAD^',
+                                    'HEAD',
+                                    cwd=self.repo)
+        self.assertIn('foo', diff_from_commit)
+
+    def testPreCommit_GitlinkWithDEPS(self):
+        # A gitlink was staged with a corresponding DEPS change.
+        updated_deps = '\n'.join((
+            f'git_dependencies = "{SYNC}"',
+            'deps = {',
+            f'    "dep_a": "host://dep_a@{"b"*40}",',
+            f'    "dep_b": "host://dep_b@{"b"*40}",',
+            '}',
+        ))
+        self.write(self.repo, 'DEPS', updated_deps)
+        git.run('add', '.', cwd=self.repo)
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_a',
+                cwd=self.repo)
+        diff_before_commit = git.run('diff', '--cached', cwd=self.repo)
+        git.run('commit', '-m', 'gitlink and DEPS', cwd=self.repo)
+
+        # There should be no changes to the commit.
+        diff_after_commit = git.run('diff', 'HEAD^', 'HEAD', cwd=self.repo)
+        self.assertEqual(diff_before_commit, diff_after_commit)
+
+    def testPreCommit_SkipPrecommit(self):
+        # A gitlink was staged without a corresponding DEPS change but the
+        # SKIP_GITLINK_PRECOMMIT envvar was set.
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_a',
+                cwd=self.repo)
+        diff_before_commit = git.run('diff',
+                                     '--cached',
+                                     '--name-only',
+                                     cwd=self.repo)
+        self.env['SKIP_GITLINK_PRECOMMIT'] = '1'
+        git.run('commit',
+                '-m',
+                'gitlink only, skipping precommit',
+                cwd=self.repo,
+                env=self.env)
+
+        # Gitlink should be kept.
+        self.assertIn('dep_a', diff_before_commit)
+        diff_after_commit = git.run('diff',
+                                    '--name-only',
+                                    'HEAD^',
+                                    'HEAD',
+                                    cwd=self.repo)
+        self.assertIn('dep_a', diff_after_commit)
+
+    def testPreCommit_OtherDEPSState(self):
+        # DEPS is set to a git_dependencies state other than SYNC.
+        deps_content = '\n'.join((
+            f'git_dependencies = \'{SUBMODULES}\'',
+            'deps = {',
+            f'    "dep_a": "host://dep_a@{"a"*40}",',
+            f'    "dep_b": "host://dep_b@{"b"*40}",',
+            '}',
+        ))
+        self.write(self.repo, 'DEPS', deps_content)
+        git.run('add', '.', cwd=self.repo)
+        git.run('commit', '-m', 'change git_dependencies', cwd=self.repo)
+
+        git.run('update-index',
+                '--replace',
+                '--cacheinfo',
+                f'160000,{"b"*40},dep_a',
+                cwd=self.repo)
+        diff_before_commit = git.run('diff', '--cached', cwd=self.repo)
+        git.run('commit', '-m', 'update dep_a', cwd=self.repo)
+
+        # There should be no changes to the commit.
+        diff_after_commit = git.run('diff', 'HEAD^', 'HEAD', cwd=self.repo)
+        self.assertEqual(diff_before_commit, diff_after_commit)
+
+
+if __name__ == '__main__':
+    unittest.main()