Repository: iterative/dvc
Pull request: remove: delete .gitignore file if empty
modified behavior to unlink empty .gitignore files.
added corresponding unit & func test cases.
updated failing test cases.
no failing tests

Fixes #4962

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
remove: delete .gitignore file if empty
Just like `dvc remove` deletes dvc.yaml and dvc.lock if they're empty after the removal, should it do the same to .gitignore? Currently it's left there even if empty, which shows up as a change in git (so it's pretty obvious).

Hints from issue discussion:
I am exploring DVC. Can I be assigned this one ?
@devramx Sure! Thanks for looking into it! :pray:

Commit message: fix failing lint checks

Patch to review:
diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py
index d01e675d1b..158a1545e2 100644
--- a/dvc/scm/git/__init__.py
+++ b/dvc/scm/git/__init__.py
@@ -165,6 +165,10 @@ def ignore_remove(self, path):
 
         filtered = list(filter(lambda x: x.strip() != entry.strip(), lines))
 
+        if not filtered:
+            os.unlink(gitignore)
+            return
+
         with open(gitignore, "w") as fobj:
             fobj.writelines(filtered)
 
diff --git a/tests/func/test_remove.py b/tests/func/test_remove.py
index 0faeb7dbee..90f164feab 100644
--- a/tests/func/test_remove.py
+++ b/tests/func/test_remove.py
@@ -7,7 +7,7 @@
 from dvc.stage.exceptions import StageFileDoesNotExistError
 from dvc.system import System
 from dvc.utils.fs import remove
-from tests.utils import get_gitignore_content
+from tests.utils import get_gitignore_content, gitignore_exists
 
 
 @pytest.mark.parametrize("remove_outs", [True, False])
@@ -29,7 +29,7 @@ def test_remove(tmp_dir, scm, dvc, run_copy, remove_outs):
         else:
             assert all(out_exists)
 
-        assert not any(out in get_gitignore_content() for out in stage.outs)
+    assert not gitignore_exists()
 
 
 def test_remove_non_existent_file(tmp_dir, dvc):
@@ -69,3 +69,35 @@ def test_cmd_remove(tmp_dir, dvc):
     assert main(["remove", stage.addressing, "--outs"]) == 0
     assert not (tmp_dir / stage.relpath).exists()
     assert not (tmp_dir / "foo").exists()
+
+
+def test_cmd_remove_gitignore_single_stage(tmp_dir, scm, dvc, run_copy):
+    stage = dvc.run(
+        name="my", cmd='echo "hello" > out', deps=[], outs=["out"],
+    )
+
+    assert (tmp_dir / ".gitignore").exists(), ".gitignore does not exist"
+
+    assert main(["remove", stage.addressing]) == 0
+    assert not (tmp_dir / stage.relpath).exists(), "Pipeline file does exists"
+    assert not (
+        stage.dvcfile._lockfile
+    ).exists(), "Pipeline lock file does exists"
+    assert not (
+        tmp_dir / ".gitignore"
+    ).exists(), ".gitignore exist when not expected to"
+
+
+def test_cmd_remove_gitignore_multistage(tmp_dir, scm, dvc, run_copy):
+    (stage,) = tmp_dir.dvc_gen("foo", "foo")
+    stage1 = run_copy("foo", "foo1", single_stage=True)
+    stage2 = run_copy("foo1", "foo2", name="copy-foo1-foo2")
+
+    assert (tmp_dir / ".gitignore").exists(), ".gitignore does not exist"
+
+    assert main(["remove", stage2.addressing]) == 0
+    assert main(["remove", stage1.addressing]) == 0
+    assert main(["remove", stage.addressing]) == 0
+    assert not (
+        tmp_dir / ".gitignore"
+    ).exists(), ".gitignore file exist when not expected to"
diff --git a/tests/func/test_scm.py b/tests/func/test_scm.py
index f0ef1fb1cd..d138859208 100644
--- a/tests/func/test_scm.py
+++ b/tests/func/test_scm.py
@@ -7,7 +7,7 @@
 from dvc.scm.base import SCMError
 from dvc.system import System
 from tests.basic_env import TestGit, TestGitSubmodule
-from tests.utils import get_gitignore_content
+from tests.utils import get_gitignore_content, gitignore_exists
 
 
 def test_init_none(tmp_dir):
@@ -85,7 +85,7 @@ def test_ignore(tmp_dir, scm):
     assert _count_gitignore_entries(target) == 1
 
     scm.ignore_remove(foo)
-    assert _count_gitignore_entries(target) == 0
+    assert not gitignore_exists()
 
 
 def test_ignored(tmp_dir, scm):
diff --git a/tests/unit/scm/test_git.py b/tests/unit/scm/test_git.py
index 12fe775198..5677e53a7f 100644
--- a/tests/unit/scm/test_git.py
+++ b/tests/unit/scm/test_git.py
@@ -279,3 +279,36 @@ def test_list_all_commits(tmp_dir, scm):
     scm.set_ref("refs/foo/bar", rev_c)
 
     assert {rev_a, rev_b} == set(scm.list_all_commits())
+
+
+def test_ignore_remove_empty(tmp_dir, scm):
+    from dvc.scm.git import Git
+
+    git_ = Git(os.fspath(tmp_dir))
+
+    test_entries = [
+        {"entry": "/foo1", "path": f"{tmp_dir}/foo1"},
+        {"entry": "/foo2", "path": f"{tmp_dir}/foo2"},
+    ]
+
+    path_to_gitignore = tmp_dir / ".gitignore"
+
+    with open(path_to_gitignore, "a") as f:
+        for entry in test_entries:
+            f.write(entry["entry"] + "\n")
+
+    assert (
+        path_to_gitignore.exists()
+    ), "gitignore not found. expected to be found."
+
+    git_.ignore_remove(test_entries[0]["path"])
+    assert (
+        path_to_gitignore.exists()
+    ), "gitignore not found. expected to be found."
+
+    git_.ignore_remove(test_entries[1]["path"])
+    assert (
+        not path_to_gitignore.exists()
+    ), "gitignore found, expected to not be found."
+
+    git_.close()
diff --git a/tests/utils/__init__.py b/tests/utils/__init__.py
index 91f63c6fb4..d932e3a3bd 100644
--- a/tests/utils/__init__.py
+++ b/tests/utils/__init__.py
@@ -9,6 +9,10 @@ def get_gitignore_content():
         return gitignore.read().splitlines()
 
 
+def gitignore_exists():
+    return os.path.exists(Git.GITIGNORE)
+
+
 @contextmanager
 def cd(newdir):
     prevdir = os.getcwd()


Code review comments:
diff_hunk: @@ -279,3 +279,36 @@ def test_list_all_commits(tmp_dir, scm):
     scm.set_ref("refs/foo/bar", rev_c)
 
     assert {rev_a, rev_b} == set(scm.list_all_commits())
+
+
+def test_ignore_remove_empty(tmp_dir, scm):
+    from dvc.scm.git import Git
+
+    git_ = Git(os.fspath(tmp_dir))
path: tests/unit/scm/test_git.py
text: @user1:
Seems like it is unused?

diff_hunk: @@ -69,3 +69,35 @@ def test_cmd_remove(tmp_dir, dvc):
     assert main(["remove", stage.addressing, "--outs"]) == 0
     assert not (tmp_dir / stage.relpath).exists()
     assert not (tmp_dir / "foo").exists()
+
+
+def test_cmd_remove_gitignore_single_stage(tmp_dir, scm, dvc, run_copy):
+    stage = dvc.run(
+        name="my", cmd='echo "hello" > out', deps=[], outs=["out"],
+    )
+
+    assert (tmp_dir / ".gitignore").exists(), ".gitignore does not exist"
+
+    assert main(["remove", stage.addressing]) == 0
+    assert not (tmp_dir / stage.relpath).exists(), "Pipeline file does exists"
path: tests/func/test_remove.py
text: @user1:
It is pretty self explanatory, we tend to not use assert messages instead trying to make the code self-explanatory, so for consistency let's just:
```suggestion
    assert not (tmp_dir / stage.relpath).exists()
```
and in other places where it is not necessary

diff_hunk: @@ -9,6 +9,10 @@ def get_gitignore_content():
         return gitignore.read().splitlines()
 
 
+def gitignore_exists():
+    return os.path.exists(Git.GITIGNORE)
+
path: tests/utils/__init__.py
text: @user1:
Let's just inline it, it is not saving us any code or readablility.