dvc remove'd folder stays in gitignore

[ammarasmro]

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:

$ 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.

[efiop]

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!

[ammarasmro]

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

[efiop]

@ammarasmro Sorry for the inconvenience 🙁 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.

[ammarasmro]

It's all good! No harm done there.
Looking forward to try it out!

[nik123]

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?

[efiop]

@nik123 That looks like a pretty good approach! Let's do that for now. Thanks! 🙏