scm: git: end gitignore with newline

dvc/scm/git/__init__.py

@@ -104,6 +104,16 @@ class Git(Base):
 
         return entry, gitignore
 
+    @staticmethod
+    def _ignored(entry, gitignore_path):
+        if os.path.exists(gitignore_path):
+            with open(gitignore_path, "r") as fobj:
+                ignore_list = fobj.readlines()
+            return any(
+                filter(lambda x: x.strip() == entry.strip(), ignore_list)
+            )
+        return False
+
     def ignore(self, path, in_curr_dir=False):
         base_dir = (
             os.path.realpath(os.curdir)
@@ -112,31 +122,32 @@ class Git(Base):
         )
         entry, gitignore = self._get_gitignore(path, base_dir)
 
-        ignore_list = []
-        if os.path.exists(gitignore):
-            with open(gitignore, "r") as f:
-                ignore_list = f.readlines()
-            if any(filter(lambda x: x.strip() == entry.strip(), ignore_list)):
-                return
+        if self._ignored(entry, gitignore):
+            return
 
         msg = "Adding '{}' to '{}'.".format(
             os.path.relpath(path), os.path.relpath(gitignore)
         )
         logger.info(msg)
 
-        self._add_entry_to_gitignore(entry, gitignore, ignore_list)
+        self._add_entry_to_gitignore(entry, gitignore)
 
         self.track_file(os.path.relpath(gitignore))
 
         self.ignored_paths.append(path)
 
     @staticmethod
-    def _add_entry_to_gitignore(entry, gitignore, ignore_list):
-        content = entry
-        if ignore_list:
-            content = "\n" + content
-        with open(gitignore, "a", encoding="utf-8") as fobj:
-            fobj.write(content)
+    def _add_entry_to_gitignore(entry, gitignore):
+        with open(gitignore, "a+", encoding="utf-8") as fobj:
+            fobj.seek(0, os.SEEK_END)
+            if fobj.tell() == 0:
+                # Empty file
+                prefix = ""
+            else:
+                fobj.seek(fobj.tell() - 1, os.SEEK_SET)
+                last = fobj.read(1)
+                prefix = "" if last == "\n" else "\n"
+            fobj.write("{}{}\n".format(prefix, entry))
 
     def ignore_remove(self, path):
         entry, gitignore = self._get_gitignore(path)

tests/func/test_scm.py

@@ -2,6 +2,7 @@ from __future__ import unicode_literals
 
 import os
 
+import pytest
 from git import Repo
 
 from dvc.utils.compat import str  # noqa: F401
@@ -61,80 +62,133 @@ class TestSCMGitSubmodule(TestGitSubmodule):
         self.assertTrue("foo" in self.git.git.ls_files())
 
 
-class TestIgnore(TestGit):
-    def _count_gitignore(self):
+class TestIgnore(object):
+    @staticmethod
+    def _count_gitignore_entries(string):
         lines = get_gitignore_content()
-        return len([i for i in lines if i == "/" + self.FOO])
+        return len([i for i in lines if i == string])
 
-    def test_ignore(self):
-        git = Git(self._root_dir)
-        foo = os.path.join(self._root_dir, self.FOO)
+    def test_ignore(self, git, repo_dir):
+        git = Git(repo_dir._root_dir)
+        foo = os.path.join(repo_dir._root_dir, repo_dir.FOO)
+
+        target = "/" + repo_dir.FOO
 
         git.ignore(foo)
-        self.assertTrue(os.path.isfile(Git.GITIGNORE))
-        self.assertEqual(self._count_gitignore(), 1)
+        assert os.path.isfile(Git.GITIGNORE)
+        assert self._count_gitignore_entries(target) == 1
 
         git.ignore(foo)
-        self.assertEqual(self._count_gitignore(), 1)
+        assert os.path.isfile(Git.GITIGNORE)
+        assert self._count_gitignore_entries(target) == 1
 
         git.ignore_remove(foo)
-        self.assertEqual(self._count_gitignore(), 0)
+        assert self._count_gitignore_entries(target) == 0
 
-    def test_get_gitignore(self):
-        data_dir = os.path.join(self._root_dir, "file1")
-        entry, gitignore = Git(self._root_dir)._get_gitignore(data_dir)
-        self.assertEqual(entry, "/file1")
-        self.assertEqual(
-            gitignore, os.path.join(self._root_dir, Git.GITIGNORE)
+    def test_get_gitignore(self, git, repo_dir):
+        data_dir = os.path.join(repo_dir._root_dir, "file1")
+        entry, gitignore = Git(repo_dir._root_dir)._get_gitignore(data_dir)
+        assert entry == "/file1"
+        assert gitignore == os.path.join(repo_dir._root_dir, Git.GITIGNORE)
+
+        data_dir = os.path.join(repo_dir._root_dir, "dir")
+        entry, gitignore = Git(repo_dir._root_dir)._get_gitignore(data_dir)
+
+        assert entry == "/dir"
+        assert gitignore == os.path.join(repo_dir._root_dir, Git.GITIGNORE)
+
+    def test_get_gitignore_subdir(self, git, repo_dir):
+        data_dir = os.path.join(
+            repo_dir._root_dir, os.path.join("dir1", "file1")
+        )
+        entry, gitignore = Git(repo_dir._root_dir)._get_gitignore(data_dir)
+        assert entry == "/file1"
+        assert gitignore == os.path.join(
+            repo_dir._root_dir, "dir1", Git.GITIGNORE
         )
 
-        data_dir = os.path.join(self._root_dir, "dir")
-        entry, gitignore = Git(self._root_dir)._get_gitignore(data_dir)
-        self.assertEqual(entry, "/dir")
-        self.assertEqual(
-            gitignore, os.path.join(self._root_dir, Git.GITIGNORE)
+        data_dir = os.path.join(
+            repo_dir._root_dir, os.path.join("dir1", "dir2")
+        )
+        entry, gitignore = Git(repo_dir._root_dir)._get_gitignore(data_dir)
+        assert entry == "/dir2"
+        assert gitignore == os.path.join(
+            repo_dir._root_dir, "dir1", Git.GITIGNORE
         )
 
-    def test_get_gitignore_subdir(self):
-        data_dir = os.path.join(self._root_dir, os.path.join("dir1", "file1"))
-        entry, gitignore = Git(self._root_dir)._get_gitignore(data_dir)
-        self.assertEqual(entry, "/file1")
-        self.assertEqual(
-            gitignore, os.path.join(self._root_dir, "dir1", Git.GITIGNORE)
-        )
-
-        data_dir = os.path.join(self._root_dir, os.path.join("dir1", "dir2"))
-        entry, gitignore = Git(self._root_dir)._get_gitignore(data_dir)
-        self.assertEqual(entry, "/dir2")
-        self.assertEqual(
-            gitignore, os.path.join(self._root_dir, "dir1", Git.GITIGNORE)
-        )
-
-    def test_get_gitignore_ignorefile_dir(self):
-        git = Git(self._root_dir)
+    def test_get_gitignore_ignorefile_dir(self, git, repo_dir):
+        git = Git(repo_dir._root_dir)
 
         file_double_dir = os.path.join("dir1", "dir2", "file1")
-        data_dir1 = os.path.join(self._root_dir, file_double_dir)
+        data_dir1 = os.path.join(repo_dir._root_dir, file_double_dir)
         dir1_real1 = os.path.realpath("dir1")
         entry, gitignore = git._get_gitignore(data_dir1, dir1_real1)
-        self.assertEqual(entry, "/dir2/file1")
-        gitignore1 = os.path.join(self._root_dir, "dir1", Git.GITIGNORE)
-        self.assertEqual(gitignore, gitignore1)
+        assert entry == "/dir2/file1"
+        gitignore1 = os.path.join(repo_dir._root_dir, "dir1", Git.GITIGNORE)
+        assert gitignore == gitignore1
 
         triple_dir = os.path.join("dir1", "dir2", "dir3")
-        data_dir2 = os.path.join(self._root_dir, triple_dir)
+        data_dir2 = os.path.join(repo_dir._root_dir, triple_dir)
         dir1_real2 = os.path.realpath("dir1")
         entry, gitignore = git._get_gitignore(data_dir2, dir1_real2)
-        self.assertEqual(entry, "/dir2/dir3")
-        gitignore2 = os.path.join(self._root_dir, "dir1", Git.GITIGNORE)
-        self.assertEqual(gitignore, gitignore2)
+        assert entry == "/dir2/dir3"
+        gitignore2 = os.path.join(repo_dir._root_dir, "dir1", Git.GITIGNORE)
+        assert gitignore == gitignore2
 
-    def test_get_gitignore_ignorefile_dir_upper_level(self):
-        git = Git(self._root_dir)
+    def test_get_gitignore_ignorefile_dir_upper_level(self, git, repo_dir):
+        git = Git(repo_dir._root_dir)
 
         file_double_dir = os.path.join("dir1", "dir2", "file1")
-        data_dir1 = os.path.join(self._root_dir, file_double_dir)
+        data_dir1 = os.path.join(repo_dir._root_dir, file_double_dir)
         ignore_file_dir = os.path.realpath(os.path.join("aa", "bb"))
 
-        with self.assertRaises(FileNotInTargetSubdirError):
+        with pytest.raises(FileNotInTargetSubdirError):
             git._get_gitignore(data_dir1, ignore_file_dir)
+
+    def test_gitignore_should_end_with_newline(self, git, repo_dir):
+        git = Git(repo_dir._root_dir)
+
+        foo = os.path.join(repo_dir._root_dir, repo_dir.FOO)
+        bar = os.path.join(repo_dir._root_dir, repo_dir.BAR)
+        gitignore = os.path.join(repo_dir._root_dir, Git.GITIGNORE)
+
+        git.ignore(foo)
+
+        with open(gitignore, "r") as fobj:
+            last = fobj.readlines()[-1]
+
+        assert last.endswith("\n")
+
+        git.ignore(bar)
+
+        with open(gitignore, "r") as fobj:
+            last = fobj.readlines()[-1]
+
+        assert last.endswith("\n")
+
+    def test_gitignore_should_append_newline_to_gitignore(self, git, repo_dir):
+        git = Git(repo_dir._root_dir)
+
+        foo_ignore_pattern = "/foo"
+        bar_ignore_pattern = "/bar"
+        bar_path = os.path.join(repo_dir._root_dir, repo_dir.BAR)
+        gitignore = os.path.join(repo_dir._root_dir, Git.GITIGNORE)
+
+        with open(gitignore, "w") as fobj:
+            fobj.write(foo_ignore_pattern)
+
+        with open(gitignore, "r") as fobj:
+            last = fobj.readlines()[-1]
+        assert not last.endswith("\n")
+
+        git.ignore(bar_path)
+
+        with open(gitignore, "r") as fobj:
+            lines = list(fobj.readlines())
+
+        assert len(lines) == 2
+        for l in lines:
+            assert l.endswith("\n")
+
+        assert lines[0].strip() == foo_ignore_pattern
+        assert lines[1].strip() == bar_ignore_pattern