forbid-new-submodules: fix triggering failure when only a submodule is committed (without any other file); support --from-ref and --to-ref; fixes #609

.pre-commit-hooks.yaml

@@ -154,6 +154,7 @@
     language: python
     entry: forbid-new-submodules
     description: Prevent addition of new git submodules
+    types: [directory]
 -   id: mixed-line-ending
     name: Mixed line ending
     description: Replaces or checks mixed line ending

pre_commit_hooks/forbid_new_submodules.py

@@ -1,3 +1,5 @@
+import argparse
+import os
 from typing import Optional
 from typing import Sequence
 
@@ -5,10 +7,23 @@ from pre_commit_hooks.util import cmd_output
 
 
 def main(argv: Optional[Sequence[str]] = None) -> int:
-    # `argv` is ignored, pre-commit will send us a list of files that we
-    # don't care about
+    parser = argparse.ArgumentParser()
+    parser.add_argument('filenames', nargs='*')
+    args = parser.parse_args(argv)
+
+    if (
+        'PRE_COMMIT_FROM_REF' in os.environ and
+        'PRE_COMMIT_TO_REF' in os.environ
+    ):
+        diff_arg = '...'.join((
+            os.environ['PRE_COMMIT_FROM_REF'],
+            os.environ['PRE_COMMIT_TO_REF'],
+        ))
+    else:
+        diff_arg = '--staged'
     added_diff = cmd_output(
-        'git', 'diff', '--staged', '--diff-filter=A', '--raw',
+        'git', 'diff', '--diff-filter=A', '--raw', diff_arg, '--',
+        *args.filenames,
     )
     retv = 0
     for line in added_diff.splitlines():

testing/util.py

@@ -1,4 +1,5 @@
 import os.path
+import subprocess
 
 
 TESTING_DIR = os.path.abspath(os.path.dirname(__file__))
@@ -6,3 +7,8 @@ TESTING_DIR = os.path.abspath(os.path.dirname(__file__))
 
 def get_resource_path(path):
     return os.path.join(TESTING_DIR, 'resources', path)
+
+
+def git_commit(*args, **kwargs):
+    cmd = ('git', 'commit', '--no-gpg-sign', '--no-verify', '--no-edit', *args)
+    subprocess.check_call(cmd, **kwargs)

tests/check_added_large_files_test.py

@@ -5,6 +5,7 @@ import pytest
 from pre_commit_hooks.check_added_large_files import find_large_added_files
 from pre_commit_hooks.check_added_large_files import main
 from pre_commit_hooks.util import cmd_output
+from testing.util import git_commit
 
 
 def test_nothing_added(temp_git_dir):
@@ -104,7 +105,7 @@ def test_moves_with_gitlfs(temp_git_dir, monkeypatch):  # pragma: no cover
         # First add the file we're going to move
         temp_git_dir.join('a.bin').write('a' * 10000)
         cmd_output('git', 'add', '--', '.')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-am', 'foo')
+        git_commit('-am', 'foo')
         # Now move it and make sure the hook still succeeds
         cmd_output('git', 'mv', 'a.bin', 'b.bin')
         assert main(('--maxkb', '9', 'b.bin')) == 0

tests/check_case_conflict_test.py

@@ -6,6 +6,7 @@ from pre_commit_hooks.check_case_conflict import find_conflicting_filenames
 from pre_commit_hooks.check_case_conflict import main
 from pre_commit_hooks.check_case_conflict import parents
 from pre_commit_hooks.util import cmd_output
+from testing.util import git_commit
 
 skip_win32 = pytest.mark.skipif(
     sys.platform == 'win32',
@@ -85,7 +86,7 @@ def test_file_conflicts_with_committed_file(temp_git_dir):
     with temp_git_dir.as_cwd():
         temp_git_dir.join('f.py').write("print('hello world')")
         cmd_output('git', 'add', 'f.py')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-n', '-m', 'Add f.py')
+        git_commit('-m', 'Add f.py')
 
         temp_git_dir.join('F.py').write("print('hello world')")
         cmd_output('git', 'add', 'F.py')
@@ -98,7 +99,7 @@ def test_file_conflicts_with_committed_dir(temp_git_dir):
     with temp_git_dir.as_cwd():
         temp_git_dir.mkdir('dir').join('x').write('foo')
         cmd_output('git', 'add', '-A')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-n', '-m', 'Add f.py')
+        git_commit('-m', 'Add f.py')
 
         temp_git_dir.join('DIR').write('foo')
         cmd_output('git', 'add', '-A')

tests/check_merge_conflict_test.py

@@ -6,6 +6,7 @@ import pytest
 from pre_commit_hooks.check_merge_conflict import main
 from pre_commit_hooks.util import cmd_output
 from testing.util import get_resource_path
+from testing.util import git_commit
 
 
 @pytest.fixture
@@ -20,19 +21,19 @@ def f1_is_a_conflict_file(tmpdir):
     with repo1.as_cwd():
         repo1_f1.ensure()
         cmd_output('git', 'add', '.')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-m', 'commit1')
+        git_commit('-m', 'commit1')
 
     cmd_output('git', 'clone', str(repo1), str(repo2))
 
     # Commit in master
     with repo1.as_cwd():
         repo1_f1.write('parent\n')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-am', 'master commit2')
+        git_commit('-am', 'master commit2')
 
     # Commit in clone and pull
     with repo2.as_cwd():
         repo2_f1.write('child\n')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-am', 'clone commit2')
+        git_commit('-am', 'clone commit2')
         cmd_output('git', 'pull', '--no-rebase', retcode=None)
         # We should end up in a merge conflict!
         f1 = repo2_f1.read()
@@ -75,20 +76,20 @@ def repository_pending_merge(tmpdir):
     with repo1.as_cwd():
         repo1_f1.ensure()
         cmd_output('git', 'add', '.')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-m', 'commit1')
+        git_commit('-m', 'commit1')
 
     cmd_output('git', 'clone', str(repo1), str(repo2))
 
     # Commit in master
     with repo1.as_cwd():
         repo1_f1.write('parent\n')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-am', 'master commit2')
+        git_commit('-am', 'master commit2')
 
     # Commit in clone and pull without committing
     with repo2.as_cwd():
         repo2_f2.write('child\n')
         cmd_output('git', 'add', '.')
-        cmd_output('git', 'commit', '--no-gpg-sign', '-m', 'clone commit2')
+        git_commit('-m', 'clone commit2')
         cmd_output('git', 'pull', '--no-commit', '--no-rebase')
         # We should end up in a pending merge
         assert repo2_f1.read() == 'parent\n'

tests/destroyed_symlinks_test.py

@@ -5,6 +5,7 @@ import pytest
 
 from pre_commit_hooks.destroyed_symlinks import find_destroyed_symlinks
 from pre_commit_hooks.destroyed_symlinks import main
+from testing.util import git_commit
 
 TEST_SYMLINK = 'test_symlink'
 TEST_SYMLINK_TARGET = '/doesnt/really/matters'
@@ -23,9 +24,7 @@ def repo_with_destroyed_symlink(tmpdir):
         with open(TEST_FILE, 'w') as f:
             print('some random content', file=f)
         subprocess.check_call(('git', 'add', '.'))
-        subprocess.check_call(
-            ('git', 'commit', '--no-gpg-sign', '-m', 'initial'),
-        )
+        git_commit('-m', 'initial')
         assert b'120000 ' in subprocess.check_output(
             ('git', 'cat-file', '-p', 'HEAD^{tree}'),
         )

tests/forbid_new_submodules_test.py

@@ -1,22 +1,20 @@
+import os
 import subprocess
+from unittest import mock
 
 import pytest
 
 from pre_commit_hooks.forbid_new_submodules import main
+from testing.util import git_commit
 
 
 @pytest.fixture
 def git_dir_with_git_dir(tmpdir):
     with tmpdir.as_cwd():
         subprocess.check_call(('git', 'init', '.'))
-        subprocess.check_call((
-            'git', 'commit', '-m', 'init', '--allow-empty', '--no-gpg-sign',
-        ))
+        git_commit('--allow-empty', '-m', 'init')
         subprocess.check_call(('git', 'init', 'foo'))
-        subprocess.check_call(
-            ('git', 'commit', '-m', 'init', '--allow-empty', '--no-gpg-sign'),
-            cwd=str(tmpdir.join('foo')),
-        )
+        git_commit('--allow-empty', '-m', 'init', cwd=str(tmpdir.join('foo')))
         yield
 
 
@@ -31,7 +29,24 @@ def git_dir_with_git_dir(tmpdir):
 )
 def test_main_new_submodule(git_dir_with_git_dir, capsys, cmd):
     subprocess.check_call(cmd)
-    assert main() == 1
+    assert main(('random_non-related_file',)) == 0
+    assert main(('foo',)) == 1
+    out, _ = capsys.readouterr()
+    assert out.startswith('foo: new submodule introduced\n')
+
+
+def test_main_new_submodule_committed(git_dir_with_git_dir, capsys):
+    rev_parse_cmd = ('git', 'rev-parse', 'HEAD')
+    from_ref = subprocess.check_output(rev_parse_cmd).decode().strip()
+    subprocess.check_call(('git', 'submodule', 'add', './foo'))
+    git_commit('-m', 'new submodule')
+    to_ref = subprocess.check_output(rev_parse_cmd).decode().strip()
+    with mock.patch.dict(
+        os.environ,
+        {'PRE_COMMIT_FROM_REF': from_ref, 'PRE_COMMIT_TO_REF': to_ref},
+    ):
+        assert main(('random_non-related_file',)) == 0
+        assert main(('foo',)) == 1
     out, _ = capsys.readouterr()
     assert out.startswith('foo: new submodule introduced\n')
 
@@ -39,4 +54,4 @@ def test_main_new_submodule(git_dir_with_git_dir, capsys, cmd):
 def test_main_no_new_submodule(git_dir_with_git_dir):
     open('test.py', 'a+').close()
     subprocess.check_call(('git', 'add', 'test.py'))
-    assert main() == 0
+    assert main(('test.py',)) == 0

tests/no_commit_to_branch_test.py

@@ -3,6 +3,7 @@ import pytest
 from pre_commit_hooks.no_commit_to_branch import is_on_branch
 from pre_commit_hooks.no_commit_to_branch import main
 from pre_commit_hooks.util import cmd_output
+from testing.util import git_commit
 
 
 def test_other_branch(temp_git_dir):
@@ -62,7 +63,7 @@ def test_main_default_call(temp_git_dir):
 
 def test_not_on_a_branch(temp_git_dir):
     with temp_git_dir.as_cwd():
-        cmd_output('git', 'commit', '--no-gpg-sign', '--allow-empty', '-m1')
+        git_commit('--allow-empty', '-m1')
         head = cmd_output('git', 'rev-parse', 'HEAD').strip()
         cmd_output('git', 'checkout', head)
         # we're not on a branch!