Win, gitpython-developers#519: FIX repo TCs.

+ FIX TestRepo.test_submodule_update():
  + submod: del `.git` file prior overwrite; Windows denied otherwise!
+ FIX TestRepo.test_untracked_files():
  + In the `git add <file>` case, it failed with unicode args on PY2.
Had to
encode them with `locale.getpreferredencoding()` AND use SHELL.
+ cmd: add `shell` into `execute()` kwds, for overriding USE_SHELL per
command.
+ repo: replace blocky `communicate()` in `_clone()` with thread-pumps.
+ test_repo.py: unittestize (almost  all) assertions.
+ Replace open --> with open for index (base and TC).
+ test_index.py: Enabled a dormant assertion.

Author: ankostis

git/cmd.py

@@ -45,7 +45,7 @@
 execute_kwargs = set(('istream', 'with_keep_cwd', 'with_extended_output',
                       'with_exceptions', 'as_process', 'stdout_as_string',
                       'output_stream', 'with_stdout', 'kill_after_timeout',
-                      'universal_newlines'))
+                      'universal_newlines', 'shell'))
 
 log = logging.getLogger('git.cmd')
 log.addHandler(logging.NullHandler())
@@ -176,8 +176,8 @@ def __setstate__(self, d):
     GIT_PYTHON_GIT_EXECUTABLE = os.environ.get(_git_exec_env_var, git_exec_name)
 
     # If True, a shell will be used when executing git commands.
-    # This should only be desirable on windows, see https://github.com/gitpython-developers/GitPython/pull/126
-    # for more information
+    # This should only be desirable on Windows, see https://github.com/gitpython-developers/GitPython/pull/126
+    # and check `git/test_repo.py:TestRepo.test_untracked_files()` TC for an example where it is required.
     # Override this value using `Git.USE_SHELL = True`
     USE_SHELL = False
 
@@ -422,6 +422,7 @@ def execute(self, command,
                 kill_after_timeout=None,
                 with_stdout=True,
                 universal_newlines=False,
+                shell=None,
                 **subprocess_kwargs
                 ):
         """Handles executing the command on the shell and consumes and returns
@@ -479,6 +480,9 @@ def execute(self, command,
         :param universal_newlines:
             if True, pipes will be opened as text, and lines are split at
             all known line endings.
+        :param shell:
+            Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
+            It overrides :attr:`USE_SHELL` if it is not `None`.
         :param kill_after_timeout:
             To specify a timeout in seconds for the git command, after which the process
             should be killed. This will have no effect if as_process is set to True. It is
@@ -544,7 +548,7 @@ def execute(self, command,
                          stdin=istream,
                          stderr=PIPE,
                          stdout=PIPE if with_stdout else open(os.devnull, 'wb'),
-                         shell=self.USE_SHELL,
+                         shell=shell is not None and shell or self.USE_SHELL,
                          close_fds=(is_posix),  # unsupported on windows
                          universal_newlines=universal_newlines,
                          creationflags=PROC_CREATIONFLAGS,

git/compat.py

@@ -7,6 +7,7 @@
 """utilities to help provide compatibility with python 3"""
 # flake8: noqa
 
+import locale
 import os
 import sys
 
@@ -15,14 +16,14 @@
     MAXSIZE,
     izip,
 )
-
 from gitdb.utils.encoding import (
     string_types,
     text_type,
     force_bytes,
     force_text
 )
 
+
 PY3 = sys.version_info[0] >= 3
 is_win = (os.name == 'nt')
 is_posix = (os.name == 'posix')
@@ -76,6 +77,16 @@ def safe_encode(s):
         raise TypeError('Expected bytes or text, but got %r' % (s,))
 
 
+def win_encode(s):
+    """Encode unicodes for process arguments on Windows."""
+    if isinstance(s, unicode):
+        return s.encode(locale.getpreferredencoding(False))
+    elif isinstance(s, bytes):
+        return s
+    elif s is not None:
+        raise TypeError('Expected bytes or text, but got %r' % (s,))
+
+
 def with_metaclass(meta, *bases):
     """copied from https://github.com/Byron/bcore/blob/master/src/python/butility/future.py#L15"""
     class metaclass(meta):

git/index/base.py

@@ -214,8 +214,8 @@ def write(self, file_path=None, ignore_extension_data=False):
         self.entries
         lfd = LockedFD(file_path or self._file_path)
         stream = lfd.open(write=True, stream=True)
-        ok = False
 
+        ok = False
         try:
             self._serialize(stream, ignore_extension_data)
             ok = True
@@ -602,14 +602,13 @@ def _store_path(self, filepath, fprogress):
         stream = None
         if S_ISLNK(st.st_mode):
             # in PY3, readlink is string, but we need bytes. In PY2, it's just OS encoded bytes, we assume UTF-8
-            stream = BytesIO(force_bytes(os.readlink(filepath), encoding=defenc))
+            open_stream = lambda: BytesIO(force_bytes(os.readlink(filepath), encoding=defenc))
         else:
-            stream = open(filepath, 'rb')
-        # END handle stream
-        fprogress(filepath, False, filepath)
-        istream = self.repo.odb.store(IStream(Blob.type, st.st_size, stream))
-        fprogress(filepath, True, filepath)
-        stream.close()
+            open_stream = lambda: open(filepath, 'rb')
+        with open_stream() as stream:
+            fprogress(filepath, False, filepath)
+            istream = self.repo.odb.store(IStream(Blob.type, st.st_size, stream))
+            fprogress(filepath, True, filepath)
         return BaseIndexEntry((stat_mode_to_index_mode(st.st_mode),
                                istream.binsha, 0, to_native_path_linux(filepath)))
 

git/objects/submodule/base.py

@@ -29,7 +29,8 @@
 )
 from git.compat import (
     string_types,
-    defenc
+    defenc,
+    is_win,
 )
 
 import stat
@@ -289,14 +290,16 @@ def _write_git_file_and_module_config(cls, working_tree_dir, module_abspath):
         """
         git_file = os.path.join(working_tree_dir, '.git')
         rela_path = os.path.relpath(module_abspath, start=working_tree_dir)
-        fp = open(git_file, 'wb')
-        fp.write(("gitdir: %s" % rela_path).encode(defenc))
-        fp.close()
-
-        writer = GitConfigParser(os.path.join(module_abspath, 'config'), read_only=False, merge_includes=False)
-        writer.set_value('core', 'worktree',
-                         to_native_path_linux(os.path.relpath(working_tree_dir, start=module_abspath)))
-        writer.release()
+        if is_win:
+            if os.path.isfile(git_file):
+                os.remove(git_file)
+        with open(git_file, 'wb') as fp:
+            fp.write(("gitdir: %s" % rela_path).encode(defenc))
+
+        with GitConfigParser(os.path.join(module_abspath, 'config'),
+                             read_only=False, merge_includes=False) as writer:
+            writer.set_value('core', 'worktree',
+                             to_native_path_linux(os.path.relpath(working_tree_dir, start=module_abspath)))
 
     #{ Edit Interface
 

git/repo/base.py

@@ -899,12 +899,8 @@ def _clone(cls, git, url, path, odb_default_type, progress, **kwargs):
         try:
             proc = git.clone(url, path, with_extended_output=True, as_process=True,
                              v=True, **add_progress(kwargs, git, progress))
-            if progress:
-                handle_process_output(proc, None, progress.new_message_handler(), finalize_process)
-            else:
-                (stdout, stderr) = proc.communicate()
-                finalize_process(proc, stderr=stderr)
-            # end handle progress
+            progress_handler = progress and progress.new_message_handler() or None
+            handle_process_output(proc, None, progress_handler, finalize_process)
         finally:
             if prev_cwd is not None:
                 os.chdir(prev_cwd)

git/test/test_index.py

@@ -113,9 +113,8 @@ def test_index_file_base(self):
         # write the data - it must match the original
         tmpfile = tempfile.mktemp()
         index_merge.write(tmpfile)
-        fp = open(tmpfile, 'rb')
-        self.assertEqual(fp.read(), fixture("index_merge"))
-        fp.close()
+        with open(tmpfile, 'rb') as fp:
+            self.assertEqual(fp.read(), fixture("index_merge"))
         os.remove(tmpfile)
 
     def _cmp_tree_index(self, tree, index):
@@ -329,22 +328,19 @@ def test_index_file_diffing(self, rw_repo):
         # reset the working copy as well to current head,to pull 'back' as well
         new_data = b"will be reverted"
         file_path = os.path.join(rw_repo.working_tree_dir, "CHANGES")
-        fp = open(file_path, "wb")
-        fp.write(new_data)
-        fp.close()
+        with open(file_path, "wb") as fp:
+            fp.write(new_data)
         index.reset(rev_head_parent, working_tree=True)
         assert not index.diff(None)
         self.assertEqual(cur_branch, rw_repo.active_branch)
         self.assertEqual(cur_commit, rw_repo.head.commit)
-        fp = open(file_path, 'rb')
-        try:
+        with open(file_path, 'rb') as fp:
             assert fp.read() != new_data
-        finally:
-            fp.close()
 
         # test full checkout
         test_file = os.path.join(rw_repo.working_tree_dir, "CHANGES")
-        open(test_file, 'ab').write(b"some data")
+        with open(test_file, 'ab') as fd:
+            fd.write(b"some data")
         rval = index.checkout(None, force=True, fprogress=self._fprogress)
         assert 'CHANGES' in list(rval)
         self._assert_fprogress([None])
@@ -369,9 +365,8 @@ def test_index_file_diffing(self, rw_repo):
 
         # checkout file with modifications
         append_data = b"hello"
-        fp = open(test_file, "ab")
-        fp.write(append_data)
-        fp.close()
+        with open(test_file, "ab") as fp:
+            fp.write(append_data)
         try:
             index.checkout(test_file)
         except CheckoutError as e:
@@ -380,7 +375,9 @@ def test_index_file_diffing(self, rw_repo):
             self.assertEqual(len(e.failed_files), len(e.failed_reasons))
             self.assertIsInstance(e.failed_reasons[0], string_types)
             self.assertEqual(len(e.valid_files), 0)
-            assert open(test_file, 'rb').read().endswith(append_data)
+            with open(test_file, 'rb') as fd:
+                s = fd.read()
+            self.assertTrue(s.endswith(append_data), s)
         else:
             raise AssertionError("Exception CheckoutError not thrown")
 
@@ -639,9 +636,10 @@ def mixed_iterator():
         if is_win:
             # simlinks should contain the link as text ( which is what a
             # symlink actually is )
-            open(fake_symlink_path, 'rb').read() == link_target
+            with open(fake_symlink_path, 'rt') as fd:
+                self.assertEqual(fd.read(), link_target)
         else:
-            assert S_ISLNK(os.lstat(fake_symlink_path)[ST_MODE])
+            self.assertTrue(S_ISLNK(os.lstat(fake_symlink_path)[ST_MODE]))
 
         # TEST RENAMING
         def assert_mv_rval(rval):
@@ -691,7 +689,8 @@ def make_paths():
 
             for fid in range(3):
                 fname = 'newfile%i' % fid
-                open(fname, 'wb').write(b"abcd")
+                with open(fname, 'wb') as fd:
+                    fd.write(b"abcd")
                 yield Blob(rw_repo, Blob.NULL_BIN_SHA, 0o100644, fname)
             # END for each new file
         # END path producer