config, gitpython-developers#525: polish more config-urls

+ FIXED also `TestSubmodule.test_root_module()`!

Author: ankostis

Diff for: git/objects/submodule/base.py

@@ -41,6 +41,7 @@
 from unittest.case import SkipTest
 from git.util import HIDE_WINDOWS_KNOWN_ERRORS
 from git.objects.base import IndexObject, Object
+from git.cmd import Git
 
 __all__ = ["Submodule", "UpdateProgress"]
 
@@ -394,6 +395,9 @@ def add(cls, repo, name, path, url=None, branch=None, no_checkout=False):
             mrepo = cls._clone_repo(repo, url, path, name, **kwargs)
         # END verify url
 
+        ## See #525 for ensuring git urls in config-files valid under Windows.
+        url = Git.polish_url(url)
+
         # It's important to add the URL to the parent config, to let `git submodule` know.
         # otherwise there is a '-' character in front of the submodule listing
         #  a38efa84daef914e4de58d1905a500d8d14aaf45 mymodule (v0.9.0-1-ga38efa8)

Diff for: git/test/lib/helper.py

@@ -214,7 +214,7 @@ def case(self, rw_repo, rw_remote_repo)
     See working dir info in with_rw_repo
     :note: We attempt to launch our own invocation of git-daemon, which will be shutdown at the end of the test.
     """
-    from git import Git, Remote
+    from git import Git, Remote  # To avoid circular deps.
 
     assert isinstance(working_tree_ref, string_types), "Decorator requires ref name for working tree checkout"
 
@@ -250,7 +250,7 @@ def remote_repo_creator(self):
 
             base_path, rel_repo_dir = osp.split(remote_repo_dir)
 
-            remote_repo_url = "git://localhost:%s/%s" % (GIT_DAEMON_PORT, rel_repo_dir)
+            remote_repo_url = Git.polish_url("git://localhost:%s/%s" % (GIT_DAEMON_PORT, rel_repo_dir))
             with d_remote.config_writer as cw:
                 cw.set('url', remote_repo_url)
 
@@ -342,7 +342,8 @@ class TestBase(TestCase):
 
     def _small_repo_url(self):
         """:return" a path to a small, clonable repository"""
-        return osp.join(self.rorepo.working_tree_dir, 'git/ext/gitdb/gitdb/ext/smmap')
+        from git.cmd import Git
+        return Git.polish_url(osp.join(self.rorepo.working_tree_dir, 'git/ext/gitdb/gitdb/ext/smmap'))
 
     @classmethod
     def setUpClass(cls):

Diff for: git/test/test_submodule.py

@@ -5,6 +5,7 @@
 from unittest.case import skipIf
 
 import git
+from git.cmd import Git
 from git.compat import string_types, is_win
 from git.exc import (
     InvalidGitRepositoryError,
@@ -23,6 +24,7 @@
 from git.test.lib import with_rw_directory
 from git.util import HIDE_WINDOWS_KNOWN_ERRORS
 from git.util import to_native_path_linux, join_path_native
+import os.path as osp
 
 
 # Change the configuration if possible to prevent the underlying memory manager
@@ -111,7 +113,7 @@ def _do_base_tests(self, rwrepo):
         else:
             with sm.config_writer() as writer:
                 # 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))
+                new_smclone_path = Git.polish_url(osp.join(self.rorepo.working_tree_dir, sm.path))
                 writer.set_value('url', new_smclone_path)
                 writer.release()
                 assert sm.config_reader().get_value('url') == new_smclone_path
@@ -168,7 +170,7 @@ def _do_base_tests(self, rwrepo):
             #################
 
             # lets update it - its a recursive one too
-            newdir = os.path.join(sm.abspath, 'dir')
+            newdir = osp.join(sm.abspath, 'dir')
             os.makedirs(newdir)
 
             # update fails if the path already exists non-empty
@@ -213,7 +215,7 @@ def _do_base_tests(self, rwrepo):
             csm_repopath = csm.path
 
             # 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))
+            new_csmclone_path = Git.polish_url(osp.join(self.rorepo.working_tree_dir, sm.path, csm.path))
             with csm.config_writer() as writer:
                 writer.set_value('url', new_csmclone_path)
             assert csm.url == new_csmclone_path
@@ -301,7 +303,7 @@ def _do_base_tests(self, rwrepo):
             csm.update()
             assert csm.module_exists()
             assert csm.exists()
-            assert os.path.isdir(csm.module().working_tree_dir)
+            assert osp.isdir(csm.module().working_tree_dir)
 
             # this would work
             assert sm.remove(force=True, dry_run=True) is sm
@@ -354,15 +356,15 @@ def _do_base_tests(self, rwrepo):
             assert nsm.module_exists()
             assert nsm.exists()
             # its not checked out
-            assert not os.path.isfile(join_path_native(nsm.module().working_tree_dir, Submodule.k_modules_file))
+            assert not osp.isfile(join_path_native(nsm.module().working_tree_dir, Submodule.k_modules_file))
             assert len(rwrepo.submodules) == 1
 
             # add another submodule, but into the root, not as submodule
             osm = Submodule.add(rwrepo, osmid, csm_repopath, new_csmclone_path, Submodule.k_head_default)
             assert osm != nsm
             assert osm.module_exists()
             assert osm.exists()
-            assert os.path.isfile(join_path_native(osm.module().working_tree_dir, 'setup.py'))
+            assert osp.isfile(join_path_native(osm.module().working_tree_dir, 'setup.py'))
 
             assert len(rwrepo.submodules) == 2
 
@@ -430,11 +432,6 @@ def test_base_rw(self, rwrepo):
     def test_base_bare(self, rwrepo):
         self._do_base_tests(rwrepo)
 
-    @skipIf(HIDE_WINDOWS_KNOWN_ERRORS and sys.version_info[:2] == (3, 5), """
-        File "C:\projects\gitpython\git\cmd.py", line 559, in execute
-        raise GitCommandNotFound(command, err)
-        git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid')
-        cmdline: git clone -n --shared -v C:\projects\gitpython\.git Users\appveyor\AppData\Local\Temp\1\tmplyp6kr_rnon_bare_test_root_module""")  # noqa E501
     @with_rw_repo(k_subm_current, bare=False)
     def test_root_module(self, rwrepo):
         # Can query everything without problems
@@ -479,7 +476,7 @@ def test_root_module(self, rwrepo):
 
         # assure we clone from a local source
         with sm.config_writer() as writer:
-            writer.set_value('url', to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, sm.path)))
+            writer.set_value('url', Git.polish_url(osp.join(self.rorepo.working_tree_dir, sm.path)))
 
         # dry-run does nothing
         sm.update(recursive=False, dry_run=True, progress=prog)
@@ -513,7 +510,7 @@ def test_root_module(self, rwrepo):
         #================
         nsmn = "newsubmodule"
         nsmp = "submrepo"
-        subrepo_url = to_native_path_linux(join_path_native(self.rorepo.working_tree_dir, rsmsp[0], rsmsp[1]))
+        subrepo_url = Git.polish_url(osp.join(self.rorepo.working_tree_dir, rsmsp[0], rsmsp[1]))
         nsm = Submodule.add(rwrepo, nsmn, nsmp, url=subrepo_url)
         csmadded = rwrepo.index.commit("Added submodule").hexsha    # make sure we don't keep the repo reference
         nsm.set_parent_commit(csmadded)
@@ -535,24 +532,24 @@ def test_root_module(self, rwrepo):
         sm.set_parent_commit(csmadded)
         smp = sm.abspath
         assert not sm.remove(module=False).exists()
-        assert os.path.isdir(smp)           # module still exists
+        assert osp.isdir(smp)           # module still exists
         csmremoved = rwrepo.index.commit("Removed submodule")
 
         # an update will remove the module
         # not in dry_run
         rm.update(recursive=False, dry_run=True, force_remove=True)
-        assert os.path.isdir(smp)
+        assert osp.isdir(smp)
 
         # when removing submodules, we may get new commits as nested submodules are auto-committing changes
         # to allow deletions without force, as the index would be dirty otherwise.
         # QUESTION: Why does this seem to work in test_git_submodule_compatibility() ?
         self.failUnlessRaises(InvalidGitRepositoryError, rm.update, recursive=False, force_remove=False)
         rm.update(recursive=False, force_remove=True)
-        assert not os.path.isdir(smp)
+        assert not osp.isdir(smp)
 
         # 'apply work' to the nested submodule and assure this is not removed/altered during updates
         # Need to commit first, otherwise submodule.update wouldn't have a reason to change the head
-        touch(os.path.join(nsm.module().working_tree_dir, 'new-file'))
+        touch(osp.join(nsm.module().working_tree_dir, 'new-file'))
         # We cannot expect is_dirty to even run as we wouldn't reset a head to the same location
         assert nsm.module().head.commit.hexsha == nsm.hexsha
         nsm.module().index.add([nsm])
@@ -574,7 +571,7 @@ def test_root_module(self, rwrepo):
         # ... to the first repository, this way we have a fast checkout, and a completely different
         # 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]))
+        nsmurl = Git.polish_url(osp.join(self.rorepo.working_tree_dir, rsmsp[0]))
         with nsm.config_writer() as writer:
             writer.set_value('url', nsmurl)
         csmpathchange = rwrepo.index.commit("changed url")
@@ -648,21 +645,21 @@ def test_first_submodule(self, rwrepo):
         assert len(list(rwrepo.iter_submodules())) == 0
 
         for sm_name, sm_path in (('first', 'submodules/first'),
-                                 ('second', os.path.join(rwrepo.working_tree_dir, 'submodules/second'))):
+                                 ('second', osp.join(rwrepo.working_tree_dir, 'submodules/second'))):
             sm = rwrepo.create_submodule(sm_name, sm_path, rwrepo.git_dir, no_checkout=True)
             assert sm.exists() and sm.module_exists()
             rwrepo.index.commit("Added submodule " + sm_name)
         # end for each submodule path to add
 
-        self.failUnlessRaises(ValueError, rwrepo.create_submodule, 'fail', os.path.expanduser('~'))
+        self.failUnlessRaises(ValueError, rwrepo.create_submodule, 'fail', osp.expanduser('~'))
         self.failUnlessRaises(ValueError, rwrepo.create_submodule, 'fail-too',
-                              rwrepo.working_tree_dir + os.path.sep)
+                              rwrepo.working_tree_dir + osp.sep)
 
     @with_rw_directory
     def test_add_empty_repo(self, rwdir):
-        empty_repo_dir = os.path.join(rwdir, 'empty-repo')
+        empty_repo_dir = osp.join(rwdir, 'empty-repo')
 
-        parent = git.Repo.init(os.path.join(rwdir, 'parent'))
+        parent = git.Repo.init(osp.join(rwdir, 'parent'))
         git.Repo.init(empty_repo_dir)
 
         for checkout_mode in range(2):
@@ -673,7 +670,7 @@ def test_add_empty_repo(self, rwdir):
 
     @with_rw_directory
     def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
-        parent = git.Repo.init(os.path.join(rwdir, 'parent'))
+        parent = git.Repo.init(osp.join(rwdir, 'parent'))
         parent.git.submodule('add', self._small_repo_url(), 'module')
         parent.index.commit("added submodule")
 
@@ -683,7 +680,7 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
         assert sm.exists() and sm.module_exists()
 
         clone = git.Repo.clone_from(self._small_repo_url(),
-                                    os.path.join(parent.working_tree_dir, 'existing-subrepository'))
+                                    osp.join(parent.working_tree_dir, 'existing-subrepository'))
         sm2 = parent.create_submodule('nongit-file-submodule', clone.working_tree_dir)
         assert len(parent.submodules) == 2
 
@@ -700,7 +697,7 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
         parent.index.commit("moved submodules")
 
         smm = sm.module()
-        fp = os.path.join(smm.working_tree_dir, 'empty-file')
+        fp = osp.join(smm.working_tree_dir, 'empty-file')
         with open(fp, 'w'):
             pass
         smm.git.add(fp)
@@ -733,7 +730,7 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
     #         "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx")  # noqa E501
     @with_rw_directory
     def test_git_submodule_compatibility(self, rwdir):
-        parent = git.Repo.init(os.path.join(rwdir, 'parent'))
+        parent = git.Repo.init(osp.join(rwdir, 'parent'))
         sm_path = join_path_native('submodules', 'intermediate', 'one')
         sm = parent.create_submodule('mymodules/myname', sm_path, url=self._small_repo_url())
         parent.index.commit("added submodule")
@@ -747,13 +744,13 @@ def assert_exists(sm, value=True):
         # muss it up. That's the only reason why the test is still here ... .
         assert len(parent.git.submodule().splitlines()) == 1
 
-        module_repo_path = os.path.join(sm.module().working_tree_dir, '.git')
-        assert module_repo_path.startswith(os.path.join(parent.working_tree_dir, sm_path))
+        module_repo_path = osp.join(sm.module().working_tree_dir, '.git')
+        assert module_repo_path.startswith(osp.join(parent.working_tree_dir, sm_path))
         if not sm._need_gitfile_submodules(parent.git):
-            assert os.path.isdir(module_repo_path)
+            assert osp.isdir(module_repo_path)
             assert not sm.module().has_separate_working_tree()
         else:
-            assert os.path.isfile(module_repo_path)
+            assert osp.isfile(module_repo_path)
             assert sm.module().has_separate_working_tree()
             assert find_git_dir(module_repo_path) is not None, "module pointed to by .git file must be valid"
         # end verify submodule 'style'
@@ -803,12 +800,12 @@ def assert_exists(sm, value=True):
         for dry_run in (True, False):
             sm.remove(dry_run=dry_run, force=True)
             assert_exists(sm, value=dry_run)
-            assert os.path.isdir(sm_module_path) == dry_run
+            assert osp.isdir(sm_module_path) == dry_run
         # end for each dry-run mode
 
     @with_rw_directory
     def test_remove_norefs(self, rwdir):
-        parent = git.Repo.init(os.path.join(rwdir, 'parent'))
+        parent = git.Repo.init(osp.join(rwdir, 'parent'))
         sm_name = 'mymodules/myname'
         sm = parent.create_submodule(sm_name, sm_name, url=self._small_repo_url())
         assert sm.exists()
@@ -817,7 +814,7 @@ def test_remove_norefs(self, rwdir):
 
         assert sm.repo is parent  # yoh was surprised since expected sm repo!!
         # so created a new instance for submodule
-        smrepo = git.Repo(os.path.join(rwdir, 'parent', sm.path))
+        smrepo = git.Repo(osp.join(rwdir, 'parent', sm.path))
         # Adding a remote without fetching so would have no references
         smrepo.create_remote('special', 'git@server-shouldnotmatter:repo.git')
         # And we should be able to remove it just fine
@@ -826,7 +823,7 @@ def test_remove_norefs(self, rwdir):
 
     @with_rw_directory
     def test_rename(self, rwdir):
-        parent = git.Repo.init(os.path.join(rwdir, 'parent'))
+        parent = git.Repo.init(osp.join(rwdir, 'parent'))
         sm_name = 'mymodules/myname'
         sm = parent.create_submodule(sm_name, sm_name, url=self._small_repo_url())
         parent.index.commit("Added submodule")
@@ -843,7 +840,7 @@ def test_rename(self, rwdir):
         assert sm.exists()
 
         sm_mod = sm.module()
-        if os.path.isfile(os.path.join(sm_mod.working_tree_dir, '.git')) == sm._need_gitfile_submodules(parent.git):
+        if osp.isfile(osp.join(sm_mod.working_tree_dir, '.git')) == sm._need_gitfile_submodules(parent.git):
             assert sm_mod.git_dir.endswith(join_path_native('.git', 'modules', new_sm_name))
         # end
 
@@ -852,8 +849,8 @@ def test_branch_renames(self, rw_dir):
         # Setup initial sandbox:
         # parent repo has one submodule, which has all the latest changes
         source_url = self._small_repo_url()
-        sm_source_repo = git.Repo.clone_from(source_url, os.path.join(rw_dir, 'sm-source'), b='master')
-        parent_repo = git.Repo.init(os.path.join(rw_dir, 'parent'))
+        sm_source_repo = git.Repo.clone_from(source_url, osp.join(rw_dir, 'sm-source'), b='master')
+        parent_repo = git.Repo.init(osp.join(rw_dir, 'parent'))
         sm = parent_repo.create_submodule('mysubmodule', 'subdir/submodule',
                                           sm_source_repo.working_tree_dir, branch='master')
         parent_repo.index.commit('added submodule')
@@ -862,7 +859,7 @@ def test_branch_renames(self, rw_dir):
         # Create feature branch with one new commit in submodule source
         sm_fb = sm_source_repo.create_head('feature')
         sm_fb.checkout()
-        new_file = touch(os.path.join(sm_source_repo.working_tree_dir, 'new-file'))
+        new_file = touch(osp.join(sm_source_repo.working_tree_dir, 'new-file'))
         sm_source_repo.index.add([new_file])
         sm.repo.index.commit("added new file")
 
@@ -888,7 +885,7 @@ def test_branch_renames(self, rw_dir):
         # To make it even 'harder', we shall fork and create a new commit
         sm_pfb = sm_source_repo.create_head('past-feature', commit='HEAD~20')
         sm_pfb.checkout()
-        sm_source_repo.index.add([touch(os.path.join(sm_source_repo.working_tree_dir, 'new-file'))])
+        sm_source_repo.index.add([touch(osp.join(sm_source_repo.working_tree_dir, 'new-file'))])
         sm_source_repo.index.commit("new file added, to past of '%r'" % sm_fb)
 
         # Change designated submodule checkout branch to a new commit in its own past
@@ -897,7 +894,7 @@ def test_branch_renames(self, rw_dir):
         sm.repo.index.commit("changed submodule branch to '%s'" % sm_pfb)
 
         # Test submodule updates - must fail if submodule is dirty
-        touch(os.path.join(sm_mod.working_tree_dir, 'unstaged file'))
+        touch(osp.join(sm_mod.working_tree_dir, 'unstaged file'))
         # This doesn't fail as our own submodule binsha didn't change, and the reset is only triggered if
         # to latest revision is True.
         parent_repo.submodule_update(to_latest_revision=False)