Repository: iterative/dvc
Pull request: remove: clean gitignore content
Fixes #4013 

* [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.

PR doesn't require documentation updates

Problem statement:
dvc remove'd folder stays in gitignore
## Bug Report

### Please provide information about your setup
The removed folder's name is still in `.gitignore`. Not sure if that's intentional or not
**Output of `dvc version`:**

```console
$ dvc version

DVC version: 0.93.0
Python version: 3.7.5
Platform: Darwin-19.5.0-x86_64-i386-64bit
Binary: True
Package: osxpkg
Supported remotes: azure, gdrive, gs, hdfs, http, https, s3, ssh, oss
Cache: reflink - supported, hardlink - supported, symlink - supported
Filesystem type (cache directory): ('apfs', '/dev/disk1s5')
Repo: dvc, git
Filesystem type (workspace): ('apfs', '/dev/disk1s5')
```

**Additional Information (if any):**

If applicable, please also provide a `--verbose` output of the command, eg: `dvc add --verbose`.

Hints from issue discussion:
Hi @ammarasmro !

Indeed, this is a bug on our side. The issue is that we just unprotect the outputs in https://github.com/iterative/dvc/blob/1.0.0a10/dvc/stage/__init__.py#L288 by default, but we really should also remove the output from the gitignore.

Thanks for the feedback!
Just a small suggestion. The first time I ran `dvc remove foldername.dvc` I lost the data that I had. It would've been cool if it just removed the tracker and left the data for me to decide what to do with it.

I got the data back easily so not a huge problem there
@ammarasmro Sorry for the inconvenience :slightly_frowning_face: dvc 1.0 (of which we have a few pre-releases available already https://dvc.org/doc/install/pre-release#install-pre-release-version) doesn't remove the outputs by default.
It's all good! No harm done there.
Looking forward to try it out!
I'd like to pick it up. The naive way to fix it is to add `ignore_remove` into `BaseOutput` and add `ignore_remove_outs` then modify the stage.remove:

```
def remove(self, force=False, remove_outs=True, purge=True):
    if remove_outs:
        self.remove_outs(ignore_remove=True, force=force)
    else:
        self.unprotect_outs()
        self.ignore_remove_outs()
        if purge:
            self.dvcfile.remove_stage(self)
```

But adding these methods seems like an overkill for such a simple task. Am I missing something?
@nik123 That looks like a pretty good approach! Let's do that for now. Thanks! :pray:

Commit message: remove: refactor: using ignore_remove in remove

Patch to review:
diff --git a/dvc/output/base.py b/dvc/output/base.py
index 37d30a210c..20f97842c5 100644
--- a/dvc/output/base.py
+++ b/dvc/output/base.py
@@ -239,6 +239,12 @@ def ignore(self):
 
         self.repo.scm.ignore(self.fspath)
 
+    def ignore_remove(self):
+        if not self.use_scm_ignore:
+            return
+
+        self.repo.scm.ignore_remove(self.fspath)
+
     def save(self):
         if not self.exists:
             raise self.DoesNotExistError(self)
@@ -337,7 +343,7 @@ def remove(self, ignore_remove=False):
             return
 
         if ignore_remove and self.use_scm_ignore:
-            self.repo.scm.ignore_remove(self.fspath)
+            self.ignore_remove()
 
     def move(self, out):
         if self.scheme == "local" and self.use_scm_ignore:
diff --git a/dvc/stage/__init__.py b/dvc/stage/__init__.py
index 85615c43f8..c39969babf 100644
--- a/dvc/stage/__init__.py
+++ b/dvc/stage/__init__.py
@@ -286,12 +286,17 @@ def unprotect_outs(self):
         for out in self.outs:
             out.unprotect()
 
+    def ignore_remove_outs(self):
+        for out in self.outs:
+            out.ignore_remove()
+
     @rwlocked(write=["outs"])
     def remove(self, force=False, remove_outs=True, purge=True):
         if remove_outs:
             self.remove_outs(ignore_remove=True, force=force)
         else:
             self.unprotect_outs()
+            self.ignore_remove_outs()
         if purge:
             self.dvcfile.remove_stage(self)
 
diff --git a/tests/func/test_remove.py b/tests/func/test_remove.py
index ac6057e65e..e5f3ef4ff7 100644
--- a/tests/func/test_remove.py
+++ b/tests/func/test_remove.py
@@ -7,14 +7,19 @@
 from dvc.stage.exceptions import StageFileDoesNotExistError
 from dvc.system import System
 from dvc.utils.fs import remove
+from tests.utils import get_gitignore_content
 
 
 @pytest.mark.parametrize("remove_outs", [True, False])
-def test_remove(tmp_dir, dvc, run_copy, remove_outs):
+def test_remove(tmp_dir, scm, dvc, run_copy, remove_outs):
     (stage1,) = tmp_dir.dvc_gen("foo", "foo")
     stage2 = run_copy("foo", "bar", single_stage=True)
     stage3 = run_copy("bar", "foobar", name="copy-bar-foobar")
 
+    assert "/foo" in get_gitignore_content()
+    assert "/bar" in get_gitignore_content()
+    assert "/foobar" in get_gitignore_content()
+
     for stage in [stage1, stage2, stage3]:
         dvc.remove(stage.addressing, outs=remove_outs)
         out_exists = (out.exists for out in stage.outs)
@@ -24,6 +29,8 @@ def test_remove(tmp_dir, dvc, run_copy, remove_outs):
         else:
             assert all(out_exists)
 
+        assert not any(out in get_gitignore_content() for out in stage.outs)
+
 
 def test_remove_non_existent_file(tmp_dir, dvc):
     with pytest.raises(StageFileDoesNotExistError):


Code review comments:
diff_hunk: @@ -337,7 +343,7 @@ def remove(self, ignore_remove=False):
             return
 
         if ignore_remove and self.use_scm_ignore:
path: dvc/output/base.py
text: @user1:
```suggestion
        if ignore_remove:
```