Merge pull request #2071 from pre-commit/submodule-recurse-bug

make sure to not discard changes even if submodule.recurse=1

pre_commit/staged_files_only.py

@@ -13,6 +13,12 @@ from pre_commit.xargs import xargs
 
 logger = logging.getLogger('pre_commit')
 
+# without forcing submodule.recurse=0, changes in nested submodules will be
+# discarded if `submodule.recurse=1` is configured
+# we choose this instead of `--no-recurse-submodules` because it works on
+# versions of git before that option was added to `git checkout`
+_CHECKOUT_CMD = ('git', '-c', 'submodule.recurse=0', 'checkout', '--', '.')
+
 
 def _git_apply(patch: str) -> None:
     args = ('apply', '--whitespace=nowarn', patch)
@@ -58,7 +64,7 @@ def _unstaged_changes_cleared(patch_dir: str) -> Generator[None, None, None]:
 
         # prevent recursive post-checkout hooks (#1418)
         no_checkout_env = dict(os.environ, _PRE_COMMIT_SKIP_POST_CHECKOUT='1')
-        cmd_output_b('git', 'checkout', '--', '.', env=no_checkout_env)
+        cmd_output_b(*_CHECKOUT_CMD, env=no_checkout_env)
 
         try:
             yield
@@ -74,7 +80,7 @@ def _unstaged_changes_cleared(patch_dir: str) -> Generator[None, None, None]:
                 # We failed to apply the patch, presumably due to fixes made
                 # by hooks.
                 # Roll back the changes made by hooks.
-                cmd_output_b('git', 'checkout', '--', '.', env=no_checkout_env)
+                cmd_output_b(*_CHECKOUT_CMD, env=no_checkout_env)
                 _git_apply(patch_filename)
 
             logger.info(f'Restored changes from {patch_filename}.')

tests/staged_files_only_test.py

@@ -181,9 +181,11 @@ def test_img_conflict(img_staged, patch_dir):
 
 
 @pytest.fixture
-def submodule_with_commits(tempdir_factory):
+def repo_with_commits(tempdir_factory):
     path = git_dir(tempdir_factory)
     with cwd(path):
+        open('foo', 'a+').close()
+        cmd_output('git', 'add', 'foo')
         git_commit()
         rev1 = cmd_output('git', 'rev-parse', 'HEAD')[1].strip()
         git_commit()
@@ -196,18 +198,21 @@ def checkout_submodule(rev):
 
 
 @pytest.fixture
-def sub_staged(submodule_with_commits, tempdir_factory):
+def sub_staged(repo_with_commits, tempdir_factory):
     path = git_dir(tempdir_factory)
     with cwd(path):
+        open('bar', 'a+').close()
+        cmd_output('git', 'add', 'bar')
+        git_commit()
         cmd_output(
-            'git', 'submodule', 'add', submodule_with_commits.path, 'sub',
+            'git', 'submodule', 'add', repo_with_commits.path, 'sub',
         )
-        checkout_submodule(submodule_with_commits.rev1)
+        checkout_submodule(repo_with_commits.rev1)
         cmd_output('git', 'add', 'sub')
         yield auto_namedtuple(
             path=path,
             sub_path=os.path.join(path, 'sub'),
-            submodule=submodule_with_commits,
+            submodule=repo_with_commits,
         )
 
 
@@ -242,6 +247,34 @@ def test_sub_something_unstaged(sub_staged, patch_dir):
     _test_sub_state(sub_staged, 'rev2', 'AM')
 
 
+def test_submodule_does_not_discard_changes(sub_staged, patch_dir):
+    with open('bar', 'w') as f:
+        f.write('unstaged changes')
+
+    foo_path = os.path.join(sub_staged.sub_path, 'foo')
+    with open(foo_path, 'w') as f:
+        f.write('foo contents')
+
+    with staged_files_only(patch_dir):
+        with open('bar') as f:
+            assert f.read() == ''
+
+        with open(foo_path) as f:
+            assert f.read() == 'foo contents'
+
+    with open('bar') as f:
+        assert f.read() == 'unstaged changes'
+
+    with open(foo_path) as f:
+        assert f.read() == 'foo contents'
+
+
+def test_submodule_does_not_discard_changes_recurse(sub_staged, patch_dir):
+    cmd_output('git', 'config', 'submodule.recurse', '1', cwd=sub_staged.path)
+
+    test_submodule_does_not_discard_changes(sub_staged, patch_dir)
+
+
 def test_stage_utf8_changes(foo_staged, patch_dir):
     contents = '\u2603'
     with open('foo', 'w', encoding='UTF-8') as foo_file: