Fix pre-commit#322 by only removing git environment variables while cloning

asottile · asottile · commit 495fefd316e8 · 2015-12-22T19:10:08.000-08:00
diff --git a/appveyor.yml b/appveyor.yml
@@ -15,6 +15,8 @@ build: false
 before_test:
   - git config --global user.name "AppVeyor CI"
   - git config --global user.email "user@example.com"
+  # Shut up CRLF messages
+  - git config --global core.safecrlf false
 
 test_script: tox
 
diff --git a/pre_commit/main.py b/pre_commit/main.py
@@ -22,16 +22,6 @@
 # to install packages to the wrong place.  We don't want anything to deal with
 # pyvenv
 os.environ.pop('__PYVENV_LAUNCHER__', None)
-# https://github.com/pre-commit/pre-commit/issues/300
-# In git 2.6.3 (maybe others), git exports this while running pre-commit hooks
-os.environ.pop('GIT_WORK_TREE', None)
-# In git 1.9.1 (maybe others), git exports these while running pre-commit hooks
-# in submodules.  In the general case this causes problems.
-# These are covered by test_install_in_submodule_and_run
-# Causes git clone to clone wrong thing
-os.environ.pop('GIT_DIR', None)
-# Causes 'error invalid object ...' during commit
-os.environ.pop('GIT_INDEX_FILE', None)
 
 
 def main(argv=None):
diff --git a/pre_commit/store.py b/pre_commit/store.py
@@ -14,6 +14,7 @@
 from pre_commit.util import clean_path_on_failure
 from pre_commit.util import cmd_output
 from pre_commit.util import cwd
+from pre_commit.util import no_git_env
 
 
 logger = logging.getLogger('pre_commit')
@@ -114,9 +115,11 @@ def clone(self, url, sha):
 
         dir = tempfile.mkdtemp(prefix='repo', dir=self.directory)
         with clean_path_on_failure(dir):
-            cmd_output('git', 'clone', '--no-checkout', url, dir)
+            cmd_output(
+                'git', 'clone', '--no-checkout', url, dir, env=no_git_env(),
+            )
             with cwd(dir):
-                cmd_output('git', 'reset', sha, '--hard')
+                cmd_output('git', 'reset', sha, '--hard', env=no_git_env())
 
         # Update our db with the created repo
         with sqlite3.connect(self.db_path) as db:
diff --git a/pre_commit/util.py b/pre_commit/util.py
@@ -71,6 +71,20 @@ def shell_escape(arg):
     return "'" + arg.replace("'", "'\"'\"'".strip()) + "'"
 
 
+def no_git_env():
+    # Too many bugs dealing with environment variables and GIT:
+    # https://github.com/pre-commit/pre-commit/issues/300
+    # In git 2.6.3 (maybe others), git exports GIT_WORK_TREE while running
+    # pre-commit hooks
+    # In git 1.9.1 (maybe others), git exports GIT_DIR and GIT_INDEX_FILE
+    # while running pre-commit hooks in submodules.
+    # GIT_DIR: Causes git clone to clone wrong thing
+    # GIT_INDEX_FILE: Causes 'error invalid object ...' during commit
+    return dict(
+        (k, v) for k, v in os.environ.items() if not k.startswith('GIT_')
+    )
+
+
 @contextlib.contextmanager
 def tarfile_open(*args, **kwargs):
     """Compatibility layer because python2.6"""
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
@@ -133,7 +133,7 @@ def _get_commit_output(
     home = home or tempdir_factory.get()
     env = dict(env_base, PRE_COMMIT_HOME=home)
     return cmd_output(
-        'git', 'commit', '-m', 'Commit!', '--allow-empty',
+        'git', 'commit', '-am', 'Commit!', '--allow-empty',
         # git commit puts pre-commit to stderr
         stderr=subprocess.STDOUT,
         env=env,
@@ -175,7 +175,7 @@ def test_install_in_submodule_and_run(tempdir_factory):
     parent_path = git_dir(tempdir_factory)
     with cwd(parent_path):
         cmd_output('git', 'submodule', 'add', src_path, 'sub')
-        cmd_output('git', 'commit', '-am', 'foo')
+        cmd_output('git', 'commit', '-m', 'foo')
 
     sub_pth = os.path.join(parent_path, 'sub')
     with cwd(sub_pth):
@@ -185,6 +185,23 @@ def test_install_in_submodule_and_run(tempdir_factory):
         assert NORMAL_PRE_COMMIT_RUN.match(output)
 
 
+def test_commit_am(tempdir_factory):
+    """Regression test for #322."""
+    path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
+    with cwd(path):
+        # Make an unstaged change
+        open('unstaged', 'w').close()
+        cmd_output('git', 'add', '.')
+        cmd_output('git', 'commit', '-m', 'foo')
+        with io.open('unstaged', 'w') as foo_file:
+            foo_file.write('Oh hai')
+
+        assert install(Runner(path)) == 0
+
+        ret, output = _get_commit_output(tempdir_factory)
+        assert ret == 0
+
+
 def test_install_idempotent(tempdir_factory):
     path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
     with cwd(path):
diff --git a/tests/repository_test.py b/tests/repository_test.py
@@ -377,6 +377,7 @@ def test_additional_dependencies_roll_forward(tempdir_factory, store):
         assert 'mccabe' in output
 
 
+@skipif_slowtests_false
 @xfailif_windows_no_ruby
 @pytest.mark.integration
 def test_additional_ruby_dependencies_installed(
@@ -392,6 +393,7 @@ def test_additional_ruby_dependencies_installed(
         assert 'thread_safe' in output
 
 
+@skipif_slowtests_false
 @xfailif_windows_no_node
 @pytest.mark.integration
 def test_additional_node_dependencies_installed(
