Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion dvc/command/imp.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ def add_parser(subparsers, parent_parser):
"-o", "--out", nargs="?", help="Destination path to download files to"
)
import_parser.add_argument(
"--rev", nargs="?", help="Git revision (e.g. branch, tag, SHA)"
"--rev", nargs="?", help="Git revision in repository to update from."
Copy link

@ghost ghost Jan 15, 2020

Choose a reason for hiding this comment

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

@ilgooz , why removing the examples? Not everyone is familiar with the term revision)

)
import_parser.set_defaults(func=CmdImport)
5 changes: 4 additions & 1 deletion dvc/command/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def run(self):
ret = 0
for target in self.args.targets:
try:
self.repo.update(target)
self.repo.update(target, rev=self.args.rev)
except DvcException:
logger.exception("failed to update '{}'.".format(target))
ret = 1
Expand All @@ -33,4 +33,7 @@ def add_parser(subparsers, parent_parser):
update_parser.add_argument(
"targets", nargs="+", help="DVC-files to update."
)
update_parser.add_argument(
"--rev", nargs="?", help="Git revision in repository to update from."
)
update_parser.set_defaults(func=CmdUpdate)
6 changes: 4 additions & 2 deletions dvc/dependency/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from dvc.exceptions import DvcException
from dvc.exceptions import UpdateWithRevNotPossibleError


class DependencyDoesNotExistError(DvcException):
Expand Down Expand Up @@ -27,5 +28,6 @@ class DependencyBase(object):
IsNotFileOrDirError = DependencyIsNotFileOrDirError
IsStageFileError = DependencyIsStageFileError

def update(self):
pass
def update(self, rev=None):
if rev:
raise UpdateWithRevNotPossibleError()
7 changes: 5 additions & 2 deletions dvc/dependency/repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ def download(self, to):
self.def_path, self.def_repo[self.PARAM_URL]
)

def update(self):
with self._make_repo(rev_lock=None) as repo:
def update(self, rev=None):
if not rev:
rev = self.def_repo.get("rev")
Comment on lines +126 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do that. See _make_rev, where it will merge your args with def_repo anyways.


with self._make_repo(rev=rev, rev_lock=None) as repo:
self.def_repo[self.PARAM_REV_LOCK] = repo.scm.get_rev()
7 changes: 7 additions & 0 deletions dvc/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -309,3 +309,10 @@ def __init__(self, path, repo):
" neighther as an output nor a git-handled file."
)
super().__init__(msg.format(path, repo))


class UpdateWithRevNotPossibleError(DvcException):
def __init__(self):
super(UpdateWithRevNotPossibleError, self).__init__(
"Revision option (`--rev`) is not a feature of non-Git sources."
Copy link

Choose a reason for hiding this comment

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

Usually it is easier to read without double negations:
not a feature of non-Git sources -> only a feature of Git sources

Suggested change
"Revision option (`--rev`) is not a feature of non-Git sources."
"Revision option (`--rev`) is supported only for Git sources."

)
5 changes: 3 additions & 2 deletions dvc/repo/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@


@locked
def update(self, target):
def update(self, target, rev=None):
from dvc.stage import Stage

stage = Stage.load(self, target)
stage.update()

stage.update(rev=rev)

stage.dump()
4 changes: 2 additions & 2 deletions dvc/stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,11 @@ def reproduce(self, interactive=False, **kwargs):

return self

def update(self):
def update(self, rev=None):
if not self.is_repo_import and not self.is_import:
raise StageUpdateError(self.relpath)

self.deps[0].update()
self.deps[0].update(rev)
locked = self.locked
self.locked = False
try:
Expand Down
36 changes: 34 additions & 2 deletions tests/func/test_update.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import shutil
import pytest

from dvc.stage import Stage
from dvc.compat import fspath
from dvc.exceptions import UpdateWithRevNotPossibleError
from dvc.external_repo import clean_repos
from dvc.compat import fspath
from dvc.stage import Stage


@pytest.mark.parametrize("cached", [True, False])
Expand Down Expand Up @@ -71,3 +73,33 @@ def test_update_import_url(tmp_dir, dvc, tmp_path_factory):

assert dst.is_file()
assert dst.read_text() == "updated file content"


def test_update_rev(tmp_dir, dvc, erepo_dir, monkeypatch):
with monkeypatch.context() as m:
Copy link

Choose a reason for hiding this comment

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

@ilgooz , if the reason for using monkeypatch is to chdir to erepo_dir, you can use instead:

Suggested change
with monkeypatch.context() as m:
with erepo_dir.chdir():

m.chdir(fspath(erepo_dir))
erepo_dir.scm.checkout("new_branch", create_new=True)
Copy link

@ghost ghost Jan 15, 2020

Choose a reason for hiding this comment

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

Another useful helper to change to a branch temporarily:

Suggested change
erepo_dir.scm.checkout("new_branch", create_new=True)
with erepo_dir.branch("new_branch", new=True):
# ...

erepo_dir.dvc_gen("foo", "foo content", commit="create foo on branch")
erepo_dir.scm.checkout("master")
erepo_dir.scm.checkout("new_branch_2", create_new=True)
Copy link

@ghost ghost Jan 15, 2020

Choose a reason for hiding this comment

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

Same here:

Suggested change
erepo_dir.scm.checkout("new_branch_2", create_new=True)
with erepo_dir.branch("new_branch_2", new=True):

erepo_dir.dvc_gen(
"foo", "foo content 2", commit="create foo on branch"
)
erepo_dir.scm.checkout("master")

stage = dvc.imp(fspath(erepo_dir), "foo", "foo_imported", rev="new_branch")
dvc.update(stage.path, rev="new_branch_2")

assert (tmp_dir / "foo_imported").read_text() == "foo content 2"


def test_update_rev_non_git_failure(repo_dir, dvc_repo):
Copy link

Choose a reason for hiding this comment

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

We are not using repo_dir and dvc_repo anymore, instead, we use tmp_dir or dvc.
This is not crucial, but it would be nice if you could read the docstring from tests/dir_helpers.py.
Sorry for the extra work, @ilgooz 😞

Copy link

Choose a reason for hiding this comment

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

Here's a suggestion, to save you some time:

def test_update_rev_non_git_failure(tmp_dir, dvc):
    tmp_dir.gen("file", "text")
    stage = dvc.imp_url("file", "imported")

    with pytest.raises(UpdateWithRevNotPossibleError):
        dvc_repo.update(stage.path, rev="dev")

Looks neat)

src = "file"
dst = src + "_imported"

shutil.copyfile(repo_dir.FOO, src)

stage = dvc_repo.imp_url(src, dst)

with pytest.raises(UpdateWithRevNotPossibleError):
dvc_repo.update(stage.path, rev="dev")
4 changes: 2 additions & 2 deletions tests/unit/command/test_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@

def test_update(dvc, mocker):
targets = ["target1", "target2", "target3"]
cli_args = parse_args(["update"] + targets)
cli_args = parse_args(["update", "--rev", "develop"] + targets)
assert cli_args.func == CmdUpdate
cmd = cli_args.func(cli_args)
m = mocker.patch("dvc.repo.Repo.update")

assert cmd.run() == 0

calls = [mocker.call(target) for target in targets]
calls = [mocker.call(target, rev="develop") for target in targets]
m.assert_has_calls(calls)