test_repo works

13 files changed, 106 insertions, 45 deletions

diff --git a/doc/source/changes.rst b/doc/source/changes.rst
index 06a73f41..3b62ddc8 100644
--- a/doc/source/changes.rst
+++ b/doc/source/changes.rst
@@ -6,6 +6,7 @@ Changelog
 ========================
 * Internally, hexadecimal SHA1 are treated as ascii encoded strings. Binary SHA1 are treated as bytes.
 * Id attribute of Commit objects is now `hexsha`, instead of `binsha`. The latter makes no sense in python 3 and I see no application of it anyway besides its artificial usage in test cases.
+* **IMPORTANT**: If you were using the config_writer(), you implicitly relied on __del__ to work as expected to flush changes. To be sure changes are flushed under PY3, you will have to call the new `release()` method to trigger a flush. For some reason, __del__ is not called necessarily anymore when a symbol goes out of scope.
 
 0.3.3
 =====
diff --git a/git/cmd.py b/git/cmd.py
index aac7ffb7..9b85b14a 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -22,7 +22,8 @@ from .exc import GitCommandError
 from git.compat import (
     text_type,
     string_types,
-    defenc
+    defenc,
+    PY3
 )
 
 execute_kwargs = ('istream', 'with_keep_cwd', 'with_extended_output',
@@ -372,8 +373,8 @@ class Git(LazyMixin):
 
         # Wait for the process to return
         status = 0
-        stdout_value = ''
-        stderr_value = ''
+        stdout_value = b''
+        stderr_value = b''
         try:
             if output_stream is None:
                 stdout_value, stderr_value = proc.communicate()
@@ -388,7 +389,7 @@ class Git(LazyMixin):
                 stdout_value = output_stream
                 stderr_value = proc.stderr.read()
                 # strip trailing "\n"
-                if stderr_value.endswith("\n"):
+                if stderr_value.endswith(b"\n"):
                     stderr_value = stderr_value[:-1]
                 status = proc.wait()
             # END stdout handling
@@ -444,7 +445,9 @@ class Git(LazyMixin):
     @classmethod
     def __unpack_args(cls, arg_list):
         if not isinstance(arg_list, (list, tuple)):
-            if isinstance(arg_list, text_type):
+            # This is just required for unicode conversion, as subprocess can't handle it
+            # However, in any other case, passing strings (usually utf-8 encoded) is totally fine
+            if not PY3 and isinstance(arg_list, unicode):
                 return [arg_list.encode(defenc)]
             return [str(arg_list)]
 
@@ -452,7 +455,7 @@ class Git(LazyMixin):
         for arg in arg_list:
             if isinstance(arg_list, (list, tuple)):
                 outlist.extend(cls.__unpack_args(arg))
-            elif isinstance(arg_list, text_type):
+            elif not PY3 and isinstance(arg_list, unicode):
                 outlist.append(arg_list.encode(defenc))
             # END recursion
             else:
@@ -509,8 +512,8 @@ class Git(LazyMixin):
 
         # Prepare the argument list
         opt_args = self.transform_kwargs(**kwargs)
-
         ext_args = self.__unpack_args([a for a in args if a is not None])
+
         args = opt_args + ext_args
 
         def make_call():
diff --git a/git/config.py b/git/config.py
index 96991b84..c7e8b7ac 100644
--- a/git/config.py
+++ b/git/config.py
@@ -103,8 +103,7 @@ class SectionConstraint(object):
         # Yes, for some reason, we have to call it explicitly for it to work in PY3 !
         # Apparently __del__ doesn't get call anymore if refcount becomes 0
         # Ridiculous ... .
-        self._config.__del__()
-        # del self._config
+        self._config.release()
 
     def __getattr__(self, attr):
         if attr in self._valid_attrs_:
@@ -121,6 +120,10 @@ class SectionConstraint(object):
         """return: Configparser instance we constrain"""
         return self._config
 
+    def release(self):
+        """Equivalent to GitConfigParser.release(), which is called on our underlying parser instance"""
+        return self._config.release()
+
 
 class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, object)):
 
@@ -198,6 +201,13 @@ class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, obje
 
     def __del__(self):
         """Write pending changes if required and release locks"""
+        # NOTE: only consistent in PY2
+        self.release()
+
+    def release(self):
+        """Flush changes and release the configuration write lock. This instance must not be used anymore afterwards.
+        In Python 3, it's required to explicitly release locks and flush changes, as __del__ is not called 
+        deterministically anymore."""
         # checking for the lock here makes sure we do not raise during write()
         # in case an invalid parser was created who could not get a lock
         if self.read_only or (self._lock and not self._lock._has_lock()):
diff --git a/git/exc.py b/git/exc.py
index 67fb9894..42191c62 100644
--- a/git/exc.py
+++ b/git/exc.py
@@ -7,6 +7,8 @@
 
 from gitdb.exc import *     # NOQA
 
+from git.compat import defenc
+
 
 class InvalidGitRepositoryError(Exception):
 
@@ -32,9 +34,9 @@ class GitCommandError(Exception):
         ret = "'%s' returned with exit code %i" % \
               (' '.join(str(i) for i in self.command), self.status)
         if self.stderr:
-            ret += "\nstderr: '%s'" % self.stderr
+            ret += "\nstderr: '%s'" % self.stderr.decode(defenc)
         if self.stdout:
-            ret += "\nstdout: '%s'" % self.stdout
+            ret += "\nstdout: '%s'" % self.stdout.decode(defenc)
         return ret
 
 
diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index 0fb3f35f..0ec6f656 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -17,7 +17,10 @@ from git.util import (
     rmtree
 )
 
-from git.config import SectionConstraint
+from git.config import (
+    SectionConstraint,
+    cp
+)
 from git.exc import (
     InvalidGitRepositoryError,
     NoSuchPathError
@@ -302,6 +305,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             writer.set_value(cls.k_head_option, br.path)
             sm._branch_path = br.path
         # END handle path
+        writer.release()
         del(writer)
 
         # we deliberatly assume that our head matches our index !
@@ -419,7 +423,9 @@ class Submodule(util.IndexObject, Iterable, Traversable):
                 # the default implementation will be offended and not update the repository
                 # Maybe this is a good way to assure it doesn't get into our way, but
                 # we want to stay backwards compatible too ... . Its so redundant !
-                self.repo.config_writer().set_value(sm_section(self.name), 'url', self.url)
+                writer = self.repo.config_writer()
+                writer.set_value(sm_section(self.name), 'url', self.url)
+                writer.release()
             # END handle dry_run
         # END handle initalization
 
@@ -576,6 +582,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
                 writer = self.config_writer(index=index)        # auto-write
                 writer.set_value('path', module_path)
                 self.path = module_path
+                writer.release()
                 del(writer)
             # END handle configuration flag
         except Exception:
@@ -700,8 +707,12 @@ class Submodule(util.IndexObject, Iterable, Traversable):
 
             # now git config - need the config intact, otherwise we can't query
             # inforamtion anymore
-            self.repo.config_writer().remove_section(sm_section(self.name))
-            self.config_writer().remove_section()
+            writer = self.repo.config_writer()
+            writer.remove_section(sm_section(self.name))
+            writer.release()
+            writer = self.config_writer()
+            writer.remove_section()
+            writer.release()
         # END delete configuration
 
         # void our data not to delay invalid access
@@ -800,14 +811,18 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         """
         :return: True if the submodule exists, False otherwise. Please note that
             a submodule may exist (in the .gitmodules file) even though its module
-            doesn't exist"""
+            doesn't exist on disk"""
         # keep attributes for later, and restore them if we have no valid data
         # this way we do not actually alter the state of the object
         loc = locals()
         for attr in self._cache_attrs:
-            if hasattr(self, attr):
-                loc[attr] = getattr(self, attr)
-            # END if we have the attribute cache
+            try:
+                if hasattr(self, attr):
+                    loc[attr] = getattr(self, attr)
+                # END if we have the attribute cache
+            except cp.NoSectionError:
+                # on PY3, this can happen apparently ... don't know why this doesn't happen on PY2
+                pass
         # END for each attr
         self._clear_cache()
 
diff --git a/git/refs/head.py b/git/refs/head.py
index 0a14158c..750d15b6 100644
--- a/git/refs/head.py
+++ b/git/refs/head.py
@@ -148,6 +148,7 @@ class Head(Reference):
             writer.set_value(self.k_config_remote, remote_reference.remote_name)
             writer.set_value(self.k_config_remote_ref, Head.to_full_path(remote_reference.remote_head))
         # END handle ref value
+        writer.release()
 
         return self
 
diff --git a/git/repo/base.py b/git/repo/base.py
index 3e0e51cc..2a63492b 100644
--- a/git/repo/base.py
+++ b/git/repo/base.py
@@ -551,6 +551,7 @@ class Repo(object):
         prefix = "?? "
         untracked_files = list()
         for line in proc.stdout:
+            line = line.decode(defenc)
             if not line.startswith(prefix):
                 continue
             filename = line[len(prefix):].rstrip('\n')
@@ -735,7 +736,7 @@ class Repo(object):
             writer = repo.remotes[0].config_writer
             writer.set_value('url', repo.remotes[0].url.replace("\\\\", "\\").replace("\\", "/"))
             # PY3: be sure cleanup is performed and lock is released
-            del writer
+            writer.release()
         # END handle remote repo
         return repo
 
@@ -767,7 +768,7 @@ class Repo(object):
 
     def archive(self, ostream, treeish=None, prefix=None, **kwargs):
         """Archive the tree at the given revision.
-        :parm ostream: file compatible stream object to which the archive will be written
+        :parm ostream: file compatible stream object to which the archive will be written as bytes
         :parm treeish: is the treeish name/id, defaults to active branch
         :parm prefix: is the optional prefix to prepend to each filename in the archive
         :parm kwargs:
diff --git a/git/repo/fun.py b/git/repo/fun.py
index ac0fa6f1..233666c9 100644
--- a/git/repo/fun.py
+++ b/git/repo/fun.py
@@ -150,7 +150,7 @@ def to_commit(obj):
 def rev_parse(repo, rev):
     """
     :return: Object at the given revision, either Commit, Tag, Tree or Blob
-    :param rev: git-rev-parse compatible revision specification, please see
+    :param rev: git-rev-parse compatible revision specification as string, please see
         http://www.kernel.org/pub/software/scm/git/docs/git-rev-parse.html
         for details
     :note: Currently there is no access to the rev-log, rev-specs may only contain
diff --git a/git/test/lib/helper.py b/git/test/lib/helper.py
index eea594e7..bd679512 100644
--- a/git/test/lib/helper.py
+++ b/git/test/lib/helper.py
@@ -179,6 +179,7 @@ def with_rw_and_rw_remote_repo(working_tree_ref):
                 pass
             crw.set(section, "receivepack", True)
             # release lock
+            crw.release()
             del(crw)
 
             # initialize the remote - first do it as local remote and pull, then
diff --git a/git/test/test_index.py b/git/test/test_index.py
index d81d08ef..f7504b32 100644
--- a/git/test/test_index.py
+++ b/git/test/test_index.py
@@ -382,6 +382,7 @@ class TestIndex(TestBase):
         writer = rw_repo.config_writer()
         writer.set_value("user", "name", uname)
         writer.set_value("user", "email", umail)
+        writer.release()
 
         # remove all of the files, provide a wild mix of paths, BaseIndexEntries,
         # IndexEntries
diff --git a/git/test/test_refs.py b/git/test/test_refs.py
index af33765a..14b91cfe 100644
--- a/git/test/test_refs.py
+++ b/git/test/test_refs.py
@@ -105,9 +105,11 @@ class TestRefs(TestBase):
             tv = "testopt"
             writer.set_value(tv, 1)
             assert writer.get_value(tv) == 1
-            del(writer)
+            writer.release()
             assert head.config_reader().get_value(tv) == 1
-            head.config_writer().remove_option(tv)
+            writer = head.config_writer()
+            writer.remove_option(tv)
+            writer.release()
 
             # after the clone, we might still have a tracking branch setup
             head.set_tracking_branch(None)
diff --git a/git/test/test_repo.py b/git/test/test_repo.py
index ae824086..f216039e 100644
--- a/git/test/test_repo.py
+++ b/git/test/test_repo.py
@@ -30,7 +30,10 @@ from git import (
 from git.util import join_path_native
 from git.exc import BadObject
 from gitdb.util import bin_to_hex
-from git.compat import string_types
+from git.compat import (
+    string_types,
+    defenc
+)
 
 import os
 import sys
@@ -259,13 +262,16 @@ class TestRepo(TestBase):
         assert self.rorepo.tag('refs/tags/0.1.5').commit
 
     def test_archive(self):
-        tmpfile = os.tmpfile()
-        self.rorepo.archive(tmpfile, '0.1.5')
-        assert tmpfile.tell()
+        tmpfile = tempfile.mktemp(suffix='archive-test')
+        stream = open(tmpfile, 'wb')
+        self.rorepo.archive(stream, '0.1.5')
+        assert stream.tell()
+        stream.close()
+        os.remove(tmpfile)
 
     @patch.object(Git, '_call_process')
     def test_should_display_blame_information(self, git):
-        git.return_value = fixture('blame')
+        git.return_value = fixture('blame').decode(defenc)
         b = self.rorepo.blame('master', 'lib/git.py')
         assert_equal(13, len(b))
         assert_equal(2, len(b[0]))
@@ -336,6 +342,7 @@ class TestRepo(TestBase):
             try:
                 writer = self.rorepo.config_writer(config_level)
                 assert not writer.read_only
+                writer.release()
             except IOError:
                 # its okay not to get a writer for some configuration files if we
                 # have no permissions
@@ -350,7 +357,8 @@ class TestRepo(TestBase):
 
         tag = self.rorepo.create_tag("new_tag", "HEAD~2")
         self.rorepo.delete_tag(tag)
-        self.rorepo.config_writer()
+        writer = self.rorepo.config_writer()
+        writer.release()
         remote = self.rorepo.create_remote("new_remote", "git@server:repo.git")
         self.rorepo.delete_remote(remote)
 
@@ -365,7 +373,7 @@ class TestRepo(TestBase):
         l1 = b"0123456789\n"
         l2 = b"abcdefghijklmnopqrstxy\n"
         l3 = b"z\n"
-        d = b"%s%s%s\n" % (l1, l2, l3)
+        d = l1 + l2 + l3 + b"\n"
 
         l1p = l1[:5]
 
@@ -382,7 +390,7 @@ class TestRepo(TestBase):
         # readlines no limit
         s = mkfull()
         lines = s.readlines()
-        assert len(lines) == 3 and lines[-1].endswith('\n')
+        assert len(lines) == 3 and lines[-1].endswith(b'\n')
         assert s._stream.tell() == len(d)   # must have scrubbed to the end
 
         # realines line limit
@@ -566,7 +574,7 @@ class TestRepo(TestBase):
         # try partial parsing
         max_items = 40
         for i, binsha in enumerate(self.rorepo.odb.sha_iter()):
-            assert rev_parse(bin_to_hex(binsha)[:8 - (i % 2)]).binsha == binsha
+            assert rev_parse(bin_to_hex(binsha)[:8 - (i % 2)].decode('ascii')).binsha == binsha
             if i > max_items:
                 # this is rather slow currently, as rev_parse returns an object
                 # which requires accessing packs, it has some additional overhead
@@ -645,6 +653,6 @@ class TestRepo(TestBase):
         assert os.path.abspath(git_file_repo.git_dir) == real_path_abs
 
         # Test using an absolute gitdir path in the .git file.
-        open(git_file_path, 'wb').write('gitdir: %s\n' % real_path_abs)
+        open(git_file_path, 'wb').write(('gitdir: %s\n' % real_path_abs).encode('ascii'))
         git_file_repo = Repo(rwrepo.working_tree_dir)
         assert os.path.abspath(git_file_repo.git_dir) == real_path_abs
diff --git a/git/test/test_submodule.py b/git/test/test_submodule.py
index 8c1580fe..499d6bac 100644
--- a/git/test/test_submodule.py
+++ b/git/test/test_submodule.py
@@ -99,7 +99,7 @@ class TestSubmodule(TestBase):
             # for faster checkout, set the url to the local path
             new_smclone_path = to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, sm.path))
             writer.set_value('url', new_smclone_path)
-            del(writer)
+            writer.release()
             assert sm.config_reader().get_value('url') == new_smclone_path
             assert sm.url == new_smclone_path
         # END handle bare repo
@@ -196,7 +196,9 @@ class TestSubmodule(TestBase):
 
             # adjust the path of the submodules module to point to the local destination
             new_csmclone_path = to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, sm.path, csm.path))
-            csm.config_writer().set_value('url', new_csmclone_path)
+            writer = csm.config_writer()
+            writer.set_value('url', new_csmclone_path)
+            writer.release()
             assert csm.url == new_csmclone_path
 
             # dry-run does nothing
@@ -257,8 +259,12 @@ class TestSubmodule(TestBase):
             # NOTE: As we did  a few updates in the meanwhile, the indices were reset
             # Hence we create some changes
             csm.set_parent_commit(csm.repo.head.commit)
-            sm.config_writer().set_value("somekey", "somevalue")
-            csm.config_writer().set_value("okey", "ovalue")
+            writer = sm.config_writer()
+            writer.set_value("somekey", "somevalue")
+            writer.release()
+            writer = csm.config_writer()
+            writer.set_value("okey", "ovalue")
+            writer.release()
             self.failUnlessRaises(InvalidGitRepositoryError, sm.remove)
             # if we remove the dirty index, it would work
             sm.module().index.reset()
@@ -406,7 +412,8 @@ class TestSubmodule(TestBase):
 
         assert len(rm.list_items(rm.module())) == 1
         rm.config_reader()
-        rm.config_writer()
+        w = rm.config_writer()
+        w.release()
 
         # deep traversal gitdb / async
         rsmsp = [sm.path for sm in rm.traverse()]
@@ -431,8 +438,9 @@ class TestSubmodule(TestBase):
         assert not sm.module_exists()               # was never updated after rwrepo's clone
 
         # assure we clone from a local source
-        sm.config_writer().set_value(
-            'url', to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, sm.path)))
+        writer = sm.config_writer()
+        writer.set_value('url', to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, sm.path)))
+        writer.release()
 
         # dry-run does nothing
         sm.update(recursive=False, dry_run=True, progress=prog)
@@ -440,7 +448,9 @@ class TestSubmodule(TestBase):
 
         sm.update(recursive=False)
         assert sm.module_exists()
-        sm.config_writer().set_value('path', fp)    # change path to something with prefix AFTER url change
+        writer = sm.config_writer()
+        writer.set_value('path', fp)    # change path to something with prefix AFTER url change
+        writer.release()
 
         # update fails as list_items in such a situations cannot work, as it cannot
         # find the entry at the changed path
@@ -504,7 +514,9 @@ class TestSubmodule(TestBase):
         # repository at the different url
         nsm.set_parent_commit(csmremoved)
         nsmurl = to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, rsmsp[0]))
-        nsm.config_writer().set_value('url', nsmurl)
+        writer = nsm.config_writer()
+        writer.set_value('url', nsmurl)
+        writer.release()
         csmpathchange = rwrepo.index.commit("changed url")
         nsm.set_parent_commit(csmpathchange)
 
@@ -532,7 +544,9 @@ class TestSubmodule(TestBase):
         nsmm = nsm.module()
         prev_commit = nsmm.head.commit
         for branch in ("some_virtual_branch", cur_branch.name):
-            nsm.config_writer().set_value(Submodule.k_head_option, git.Head.to_full_path(branch))
+            writer = nsm.config_writer()
+            writer.set_value(Submodule.k_head_option, git.Head.to_full_path(branch))
+            writer.release()
             csmbranchchange = rwrepo.index.commit("changed branch to %s" % branch)
             nsm.set_parent_commit(csmbranchchange)
         # END for each branch to change
@@ -560,7 +574,9 @@ class TestSubmodule(TestBase):
         assert nsm.exists() and nsm.module_exists() and len(nsm.children()) >= 1
         # assure we pull locally only
         nsmc = nsm.children()[0]
-        nsmc.config_writer().set_value('url', async_url)
+        writer = nsmc.config_writer()
+        writer.set_value('url', async_url)
+        writer.release()
         rm.update(recursive=True, progress=prog, dry_run=True)      # just to run the code
         rm.update(recursive=True, progress=prog)