Repository: iterative/dvc

Issue:
rename `git` property (a Repo object) in class `dvc.scm.Git`?
`class Git(Base)` property [`self.git`](https://github.com/iterative/dvc/blob/master/dvc/scm/git/__init__.py#L45), while named `git`, is not a **Git** object but rather a **Repo** object, which comes with a `git` property itself (which is the actual Git object); So using the latter (which we do a lot in tests) makes the code a little confusing, like: `self.git.git` and `dvc.scm.git.git`.
> It could even be `self.git.git.git` [in `class GitTree`](https://github.com/iterative/dvc/blob/master/dvc/scm/git/tree.py#L25)!

I think ideally the property name should be `self.repo`, except that name its already in use [by `class Base`](https://github.com/iterative/dvc/blob/master/dvc/scm/base.py#L35) (parent of `class Git`) so renaming `dvc.scm.Git.git` to `dvc.scm.Git.repo` would overwrite `dvc.scm.Base.repo`. I don't know whether this is an immediate problem since I don't see the current `dvc.scm.Git.repo` property being used anywhere in the code, but we probably want to avoid that overwrite anyway.

The options I see are:
a) Don't mind the overwrite and just rename `dvc.scm.Git.git` to `dvc.scm.Git.repo`
b) Rename `dvc.scm.Base.repo` to something else (e.g. `dvc.scm.Base._repo`) and then rename `dvc.scm.Git.git` to `dvc.scm.Git.repo`
c) Rename `dvc.scm.Git.git` to something else, e.g. `dvc.scm.Git.repository`

Discussion:

Patch:
diff --git a/dvc/external_repo.py b/dvc/external_repo.py
index c29863465..cb5f38620 100644
--- a/dvc/external_repo.py
+++ b/dvc/external_repo.py
@@ -46,7 +46,7 @@ def _clone(cache_dir=None, **kwargs):
         repo = Repo(_path)
         cache_config = CacheConfig(repo.config)
         cache_config.set_dir(cache_dir, level=Config.LEVEL_LOCAL)
-        repo.scm.git.close()
+        repo.scm.close()
 
     return Repo(_path)
 
@@ -70,7 +70,7 @@ def _clone_git_repo(to_path, url=None, rev=None, rev_lock=None):
 
 
 def _remove(repo):
-    repo.scm.git.close()
+    repo.scm.close()
 
     if os.name == "nt":
         # git.exe may hang for a while not permitting to remove temp dir
diff --git a/dvc/scm/base.py b/dvc/scm/base.py
index e52fafe99..e7cf61fa5 100644
--- a/dvc/scm/base.py
+++ b/dvc/scm/base.py
@@ -144,3 +144,6 @@ class Base(object):
 
     def belongs_to_scm(self, path):
         """Return boolean whether file belongs to scm"""
+
+    def close(self):
+        """ Method to close the files """
diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py
index 883c75afd..290ca5802 100644
--- a/dvc/scm/git/__init__.py
+++ b/dvc/scm/git/__init__.py
@@ -42,7 +42,7 @@ class Git(Base):
         from git.exc import InvalidGitRepositoryError
 
         try:
-            self.git = git.Repo(self.root_dir)
+            self.repo = git.Repo(self.root_dir)
         except InvalidGitRepositoryError:
             msg = "{} is not a git repository"
             raise SCMError(msg.format(self.root_dir))
@@ -51,7 +51,7 @@ class Git(Base):
         # http://pyinstaller.readthedocs.io/en/stable/runtime-information.html
         env = fix_env(None)
         libpath = env.get("LD_LIBRARY_PATH", None)
-        self.git.git.update_environment(LD_LIBRARY_PATH=libpath)
+        self.repo.git.update_environment(LD_LIBRARY_PATH=libpath)
 
         self.ignored_paths = []
         self.files_to_track = set()
@@ -70,7 +70,7 @@ class Git(Base):
 
     @property
     def dir(self):
-        return self.git.git_dir
+        return self.repo.git_dir
 
     @property
     def ignore_file(self):
@@ -167,7 +167,7 @@ class Git(Base):
         # NOTE: GitPython is not currently able to handle index version >= 3.
         # See https://github.com/iterative/dvc/issues/610 for more details.
         try:
-            self.git.index.add(paths)
+            self.repo.index.add(paths)
         except AssertionError:
             msg = (
                 "failed to add '{}' to git. You can add those files"
@@ -179,41 +179,41 @@ class Git(Base):
             logger.exception(msg)
 
     def commit(self, msg):
-        self.git.index.commit(msg)
+        self.repo.index.commit(msg)
 
     def checkout(self, branch, create_new=False):
         if create_new:
-            self.git.git.checkout("HEAD", b=branch)
+            self.repo.git.checkout("HEAD", b=branch)
         else:
-            self.git.git.checkout(branch)
+            self.repo.git.checkout(branch)
 
     def branch(self, branch):
-        self.git.git.branch(branch)
+        self.repo.git.branch(branch)
 
     def tag(self, tag):
-        self.git.git.tag(tag)
+        self.repo.git.tag(tag)
 
     def untracked_files(self):
-        files = self.git.untracked_files
-        return [os.path.join(self.git.working_dir, fname) for fname in files]
+        files = self.repo.untracked_files
+        return [os.path.join(self.repo.working_dir, fname) for fname in files]
 
     def is_tracked(self, path):
-        # it is equivalent to `bool(self.git.git.ls_files(path))` by
+        # it is equivalent to `bool(self.repo.git.ls_files(path))` by
         # functionality, but ls_files fails on unicode filenames
         path = relpath(path, self.root_dir)
-        return path in [i[0] for i in self.git.index.entries]
+        return path in [i[0] for i in self.repo.index.entries]
 
     def is_dirty(self):
-        return self.git.is_dirty()
+        return self.repo.is_dirty()
 
     def active_branch(self):
-        return self.git.active_branch.name
+        return self.repo.active_branch.name
 
     def list_branches(self):
-        return [h.name for h in self.git.heads]
+        return [h.name for h in self.repo.heads]
 
     def list_tags(self):
-        return [t.name for t in self.git.tags]
+        return [t.name for t in self.repo.tags]
 
     def _install_hook(self, name, cmd):
         command = '[ -z "$(git ls-files .dvc)" ] || exec dvc {}'.format(cmd)
@@ -266,7 +266,7 @@ class Git(Base):
         return basename == self.ignore_file or Git.GIT_DIR in path_parts
 
     def get_tree(self, rev):
-        return GitTree(self.git, rev)
+        return GitTree(self.repo, rev)
 
     def _get_diff_trees(self, a_ref, b_ref):
         """Private method for getting the trees and commit hashes of 2 git
@@ -284,10 +284,10 @@ class Git(Base):
         trees = {DIFF_A_TREE: None, DIFF_B_TREE: None}
         commits = []
         if b_ref is None:
-            b_ref = self.git.head.commit
+            b_ref = self.repo.head.commit
         try:
-            a_commit = self.git.git.rev_parse(a_ref, short=True)
-            b_commit = self.git.git.rev_parse(b_ref, short=True)
+            a_commit = self.repo.git.rev_parse(a_ref, short=True)
+            b_commit = self.repo.git.rev_parse(b_ref, short=True)
             # See https://gitpython.readthedocs.io
             # /en/2.1.11/reference.html#git.objects.base.Object.__str__
             commits.append(a_commit)
@@ -322,4 +322,7 @@ class Git(Base):
         return diff_dct
 
     def get_rev(self):
-        return self.git.git.rev_parse("HEAD")
+        return self.repo.git.rev_parse("HEAD")
+
+    def close(self):
+        self.repo.close()


Test patch:
diff --git a/tests/basic_env.py b/tests/basic_env.py
index 5de08bc0f..5afba2c3c 100644
--- a/tests/basic_env.py
+++ b/tests/basic_env.py
@@ -166,7 +166,7 @@ class TestDvcGitFixture(TestGitFixture):
         self.dvc.scm.commit("init dvc")
 
     def tearDown(self):
-        self.dvc.scm.git.close()
+        self.dvc.scm.close()
         super(TestDvcGitFixture, self).tearDown()
 
 
diff --git a/tests/conftest.py b/tests/conftest.py
index 8010ff18f..3356a4a0b 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -162,7 +162,7 @@ def erepo(repo_dir):
 
         repo.dvc.scm.checkout("master")
 
-        repo.dvc.scm.git.close()
+        repo.dvc.scm.close()
         repo.git.close()
 
         os.chdir(repo._saved_dir)
diff --git a/tests/func/test_diff.py b/tests/func/test_diff.py
index f4b148cfa..92910a3d0 100644
--- a/tests/func/test_diff.py
+++ b/tests/func/test_diff.py
@@ -96,7 +96,7 @@ class TestDiffDir(TestDvcGit):
         self.git.index.add([self.DATA_DIR + ".dvc"])
         self.git.index.commit("adds data_dir")
         self.a_ref = self.git.git.rev_parse(
-            self.dvc.scm.git.head.commit, short=True
+            self.dvc.scm.repo.head.commit, short=True
         )
         self.old_checksum = _get_checksum(self.dvc, self.DATA_DIR)
         self.new_file = os.path.join(self.DATA_SUB_DIR, diff.DIFF_NEW_FILE)
@@ -154,7 +154,7 @@ class TestDiffDirRepoDeletedFile(TestDiffDir):
 
         self.b_ref = self.a_ref
         self.new_checksum = self.old_checksum
-        self.a_ref = str(self.dvc.scm.git.head.commit)
+        self.a_ref = str(self.dvc.scm.repo.head.commit)
         self.old_checksum = _get_checksum(self.dvc, self.DATA_DIR)
 
     def test(self):
diff --git a/tests/func/test_install.py b/tests/func/test_install.py
index 1e3c7b8ba..140e0b5d5 100644
--- a/tests/func/test_install.py
+++ b/tests/func/test_install.py
@@ -88,12 +88,12 @@ class TestInstall(object):
         assert ret == 0
 
         stage_file = repo_dir.FOO + Stage.STAGE_FILE_SUFFIX
-        dvc_repo.scm.git.index.add([stage_file, ".gitignore"])
-        dvc_repo.scm.git.index.commit("commit message")
+        dvc_repo.scm.repo.index.add([stage_file, ".gitignore"])
+        dvc_repo.scm.repo.index.commit("commit message")
 
-        dvc_repo.scm.git.clone(git_remote)
-        dvc_repo.scm.git.create_remote("origin", git_remote)
+        dvc_repo.scm.repo.clone(git_remote)
+        dvc_repo.scm.repo.create_remote("origin", git_remote)
 
-        dvc_repo.scm.git.git.push("origin", "master")
+        dvc_repo.scm.repo.git.push("origin", "master")
 
         assert os.path.isfile(expected_cache_path)
diff --git a/tests/func/test_metrics.py b/tests/func/test_metrics.py
index 4159c6461..e7892f506 100644
--- a/tests/func/test_metrics.py
+++ b/tests/func/test_metrics.py
@@ -22,7 +22,7 @@ class TestMetricsBase(TestDvcGit):
         branches = ["foo", "bar", "baz"]
 
         for branch in branches:
-            self.dvc.scm.git.create_head(branch)
+            self.dvc.scm.repo.create_head(branch)
 
         for branch in branches:
             self.dvc.scm.checkout(branch)
@@ -692,7 +692,7 @@ class TestCachedMetrics(TestDvcGit):
 
         # TestDvc currently is based on TestGit, so it is safe to use
         # scm.git for now
-        self.dvc.scm.git.git.clean("-fd")
+        self.dvc.scm.repo.git.clean("-fd")
 
         self.dvc = DvcRepo(".")
 
diff --git a/tests/func/test_repo.py b/tests/func/test_repo.py
index 8da5082dd..8184721c6 100644
--- a/tests/func/test_repo.py
+++ b/tests/func/test_repo.py
@@ -31,7 +31,7 @@ class TestCollect(TestDvcGit):
 
     def _check(self, branch, target, with_deps, expected):
         if branch:
-            self.dvc.tree = GitTree(self.dvc.scm.git, branch)
+            self.dvc.tree = GitTree(self.dvc.scm.repo, branch)
         else:
             self.dvc.tree = WorkingTree()
         result = self.dvc.collect(target + ".dvc", with_deps=with_deps)
diff --git a/tests/func/test_scm.py b/tests/func/test_scm.py
index 8135f6606..ada5b4759 100644
--- a/tests/func/test_scm.py
+++ b/tests/func/test_scm.py
@@ -42,7 +42,7 @@ class TestSCMGit(TestGit):
         G.commit("add")
         self.assertTrue(G.is_tracked(foo))
         self.assertTrue(G.is_tracked(self.FOO))
-        G.git.index.remove([self.FOO], working_tree=True)
+        G.repo.index.remove([self.FOO], working_tree=True)
         self.assertFalse(G.is_tracked(foo))
         self.assertFalse(G.is_tracked(self.FOO))
         self.assertFalse(G.is_tracked("not-existing-file"))
diff --git a/tests/func/test_update.py b/tests/func/test_update.py
index 7345627aa..454501504 100644
--- a/tests/func/test_update.py
+++ b/tests/func/test_update.py
@@ -30,7 +30,7 @@ def test_update_import(dvc_repo, erepo):
     repo.scm.commit("updated")
     repo.scm.checkout("master")
 
-    repo.scm.git.close()
+    repo.scm.close()
 
     os.chdir(saved_dir)