Skip to content

dvc: more explict error during init when .dvc is ignored by git #4978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
efiop merged 4 commits into from
Nov 30, 2020
Merged

dvc: more explict error during init when .dvc is ignored by git #4978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82715d5
dvc: more explict error during init when .dvc is ignored by git
dudarev Nov 26, 2020
e365f34
dvc: address PR feedback
dudarev Nov 27, 2020
39a0f67
styling
dudarev Nov 27, 2020
c336fe1
styling
dudarev Nov 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ neatlynx/__pycache__
*.pyc
.env/
.env2.7/
.python-version
Copy link
Copy Markdown
Contributor

@pmrowla pmrowla Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pyenv file, right? I'm not a pyenv user (and we recommend using virtualenvs for anyone doing development on DVC), is it normal to gitignore this?

Copy link
Copy Markdown
Contributor Author

@dudarev dudarev Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, .python-version is used by pyenv. It's typical to ignore it in projects intended to be ran in multiple environments:

https://github.com/github/gitignore/blob/master/Python.gitignore#L88

Note that pyenv with pyenv-virtualenv plugin is just a convenience wrapper on top of virtualenv. This is a very common way to use it, so virtualenv is still being used. .python-version is a convenient feature that switches local shell env to what's mentioned in that file. It's typically a single line of text there.


.dvc.conf.lock
.DS_Store
Expand Down
17 changes: 10 additions & 7 deletions dvc/repo/init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,22 @@ def init(root_dir=os.curdir, no_scm=False, force=False, subdir=False):
scm = SCM(root_dir, search_parent_directories=subdir, no_scm=no_scm)
except SCMError:
raise InitError(
"{repo} is not tracked by any supported SCM tool (e.g. Git). "
f"{root_dir} is not tracked by any supported SCM tool (e.g. Git). "
"Use `--no-scm` if you don't want to use any SCM or "
"`--subdir` if initializing inside a subdirectory of a parent SCM "
"repository.".format(repo=root_dir)
"repository."
)

if scm.is_ignored(dvc_dir):
raise InitError(
f"{dvc_dir} is ignored by your SCM tool. \n"
"Make sure that it's tracked, "
"for example, by adding '!.dvc' to .gitignore."
Comment on lines +56 to +58
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks fot this update!

BTW what do you guys think about just saying "Git" for 2.x +?

I don't know of any plans to support other SCMs soon anyway... Docs already only ever mention Git.

)

if os.path.isdir(dvc_dir):
if not force:
raise InitError(
"'{repo}' exists. Use `-f` to force.".format(
repo=relpath(dvc_dir)
)
)
raise InitError(f"'{relpath(dvc_dir)}' exists. Use `-f` to force.")

remove(dvc_dir)

Expand Down
4 changes: 4 additions & 0 deletions dvc/scm/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ def is_submodule(root_dir): # pylint: disable=unused-argument
"""
return True

def is_ignored(self, path): # pylint: disable=unused-argument
"""Returns whether or not path is ignored by SCM."""
return False

def ignore(self, path): # pylint: disable=unused-argument
"""Makes SCM ignore a specified path."""

Expand Down
4 changes: 2 additions & 2 deletions dvc/scm/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def _get_gitignore(self, path):

return entry, gitignore

def _ignored(self, path):
def is_ignored(self, path):
Copy link
Copy Markdown
Contributor Author

@dudarev dudarev Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it because it's used as a public method in the PR.

from dulwich import ignore
from dulwich.repo import Repo

Expand All @@ -194,7 +194,7 @@ def _ignored(self, path):
def ignore(self, path):
entry, gitignore = self._get_gitignore(path)

if self._ignored(path):
if self.is_ignored(path):
return

msg = "Adding '{}' to '{}'.".format(relpath(path), relpath(gitignore))
Expand Down
14 changes: 14 additions & 0 deletions tests/func/test_init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,17 @@ def test_gen_dvcignore(tmp_dir):
"# https://dvc.org/doc/user-guide/dvcignore\n"
)
assert text == (tmp_dir / ".dvcignore").read_text()


def test_init_when_ignored_by_git(tmp_dir, scm, caplog):
# https://github.com/iterative/dvc/issues/3738
tmp_dir.gen({".gitignore": ".*"})
with caplog.at_level(logging.ERROR, logger="dvc"):
assert main(["init"]) == 1
Comment on lines +119 to +120
Copy link
Copy Markdown
Contributor

@pmrowla pmrowla Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to test Repo.init() here rather than using main (since this is an issue with the init internals, not necessarily with only the command line dvc init).

So something like

    with pytest.raises(InitError):
        # test using DvcRepo.init() instead of main()
        ...

Copy link
Copy Markdown
Contributor Author

@dudarev dudarev Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to leave testing with main and checking caplog in this functional test. If only an exception is checked when DvcRepo.init() is called it may not test the full scenario that happened to users.

In issues

#3738
#3803

the users did not see a clear message about the issue. Even though the exception when calling the init method has that message, it could happen in the future that it gets suppressed somehow and is not displayed to users. Testing it with main would make it more robust.

assert (
"{dvc_dir} is ignored by your SCM tool. \n"
"Make sure that it's tracked, "
"for example, by adding '!.dvc' to .gitignore.".format(
dvc_dir=tmp_dir / DvcRepo.DVC_DIR
)
) in caplog.text
4 changes: 2 additions & 2 deletions tests/func/test_scm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ def test_ignored(tmp_dir, scm):
tmp_dir.gen({"dir1": {"file1.jpg": "cont", "file2.txt": "cont"}})
tmp_dir.gen({".gitignore": "dir1/*.jpg"})

assert scm._ignored(os.fspath(tmp_dir / "dir1" / "file1.jpg"))
assert not scm._ignored(os.fspath(tmp_dir / "dir1" / "file2.txt"))
assert scm.is_ignored(tmp_dir / "dir1" / "file1.jpg")
assert not scm.is_ignored(tmp_dir / "dir1" / "file2.txt")


def test_get_gitignore(tmp_dir, scm):
Expand Down