Skip to content

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

Merged
efiop merged 4 commits into
treeverse:masterfrom
dudarev:3738-error-when-ignored-by-git
Nov 30, 2020
Merged

dvc: more explict error during init when .dvc is ignored by git#4978
efiop merged 4 commits into
treeverse:masterfrom
dudarev:3738-error-when-ignored-by-git

Conversation

@dudarev
Copy link
Copy Markdown
Contributor

@dudarev dudarev commented Nov 26, 2020

Fixes #3738

Comment thread dvc/scm/git.py
return entry, gitignore

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

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.

Copy link
Copy Markdown
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @dudarev! It looks good so far, just a few small suggestions

Comment thread .gitignore
*.pyc
.env/
.env2.7/
.python-version
Copy link
Copy Markdown
Contributor

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

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.

Comment thread dvc/repo/init.py Outdated
Comment thread tests/func/test_scm.py Outdated
Comment thread tests/func/test_init.py Outdated
Comment thread tests/func/test_init.py
Comment on lines +119 to +120
with caplog.at_level(logging.ERROR, logger="dvc"):
assert main(["init"]) == 1
Copy link
Copy Markdown
Contributor

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

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.

@pmrowla pmrowla requested review from efiop, pared and skshetry November 30, 2020 09:16
@efiop efiop merged commit 38cbd99 into treeverse:master Nov 30, 2020
@efiop efiop added the ui user interface / interaction label Dec 1, 2020
Comment thread dvc/repo/init.py
Comment on lines +56 to +58
f"{dvc_dir} is ignored by your SCM tool. \n"
"Make sure that it's tracked, "
"for example, by adding '!.dvc' to .gitignore."
Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui user interface / interaction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dvc init fails when .dvc directory is git-ignored

5 participants