diff --git a/dvc/dvcfile.py b/dvc/dvcfile.py index 79e5a3024..65cbf4d98 100644 --- a/dvc/dvcfile.py +++ b/dvc/dvcfile.py @@ -199,7 +199,7 @@ class SingleStageFile(FileMixin): check_dvcfile_path(self.repo, self.path) logger.debug(f"Saving information to '{relpath(self.path)}'.") dump_yaml(self.path, serialize.to_single_stage_file(stage)) - self.repo.scm.track_file(self.relpath) + self.repo.scm_context.track_file(self.relpath) def remove_stage(self, stage): # pylint: disable=unused-argument self.remove() @@ -268,7 +268,7 @@ class PipelineFile(FileMixin): else: data["stages"].update(stage_data) - self.repo.scm.track_file(self.relpath) + self.repo.scm_context.track_file(self.relpath) @property def stage(self): @@ -388,7 +388,7 @@ class Lockfile(FileMixin): data["stages"].update(stage_data) if modified: - self.repo.scm.track_file(self.relpath) + self.repo.scm_context.track_file(self.relpath) def remove_stage(self, stage): if not self.exists(): diff --git a/dvc/output.py b/dvc/output.py index 4abb1c8c4..a0c433cb2 100644 --- a/dvc/output.py +++ b/dvc/output.py @@ -513,13 +513,13 @@ class Output: if self.repo.scm.is_tracked(self.fspath): raise OutputAlreadyTrackedError(self) - self.repo.scm.ignore(self.fspath) + self.repo.scm_context.ignore(self.fspath) def ignore_remove(self): if not self.use_scm_ignore: return - self.repo.scm.ignore_remove(self.fspath) + self.repo.scm_context.ignore_remove(self.fspath) # pylint: enable=no-member @@ -782,7 +782,7 @@ class Output: def move(self, out): # pylint: disable=no-member if self.scheme == "local" and self.use_scm_ignore: - self.repo.scm.ignore_remove(self.fspath) + self.repo.scm_context.ignore_remove(self.fspath) self.fs.move(self.fs_path, out.fs_path) self.def_path = out.def_path @@ -791,7 +791,7 @@ class Output: self.commit() if self.scheme == "local" and self.use_scm_ignore: - self.repo.scm.ignore(self.fspath) + self.repo.scm_context.ignore(self.fspath) def transfer( self, source, odb=None, jobs=None, update=False, no_progress_bar=False diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index c69238c94..22e459818 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -17,6 +17,7 @@ from dvc.utils.fs import path_isin if TYPE_CHECKING: from dvc.fs.base import BaseFileSystem from dvc.objects.file import HashFile + from dvc.repo.scm_context import SCMContext from dvc.scm import Base @@ -269,6 +270,12 @@ class Repo: return SCM(self.root_dir, no_scm=True) raise + @cached_property + def scm_context(self) -> "SCMContext": + from dvc.repo.scm_context import SCMContext + + return SCMContext(self.scm, self.config) + @cached_property def dvcignore(self) -> DvcIgnoreFilter: diff --git a/dvc/repo/experiments/executor/base.py b/dvc/repo/experiments/executor/base.py index 43ca2e46c..9d8111d75 100644 --- a/dvc/repo/experiments/executor/base.py +++ b/dvc/repo/experiments/executor/base.py @@ -392,7 +392,7 @@ class BaseExecutor(ABC): dvc = Repo(dvc_dir) if cls.QUIET: - dvc.scm.quiet = cls.QUIET + dvc.scm_context.quiet = cls.QUIET if dvc_dir is not None: old_cwd: Optional[str] = os.getcwd() if rel_cwd: diff --git a/dvc/repo/experiments/init.py b/dvc/repo/experiments/init.py index 2d3d30326..bef9f96e3 100644 --- a/dvc/repo/experiments/init.py +++ b/dvc/repo/experiments/init.py @@ -264,12 +264,11 @@ def init( console=ui.error_console, default=True, ): - scm = repo.scm - with _disable_logging(), scm.track_file_changes(autostage=True): + with repo.scm_context(autostage=True, quiet=True), _disable_logging(): stage.dump(update_lock=False) stage.ignore_outs() if params: - scm.track_file(params) + repo.scm_context.track_file(params) else: raise DvcException("Aborting ...") return stage diff --git a/dvc/repo/reproduce.py b/dvc/repo/reproduce.py index 9943ecbe3..3b2116ec9 100644 --- a/dvc/repo/reproduce.py +++ b/dvc/repo/reproduce.py @@ -8,12 +8,14 @@ from dvc.repo.scm_context import scm_context from . import locked if typing.TYPE_CHECKING: + from dvc.stage import Stage + from . import Repo logger = logging.getLogger(__name__) -def _reproduce_stage(stage, **kwargs): +def _reproduce_stage(stage: "Stage", **kwargs): def _run_callback(repro_callback): _dump_stage(stage) _track_stage(stage) @@ -56,27 +58,33 @@ def _dump_stage(stage): dvcfile.dump(stage, update_pipeline=False) -def _track_stage(stage): - from dvc.utils import relpath - - stage.repo.scm.track_file(stage.dvcfile.relpath) +def _get_stage_files(stage: "Stage") -> typing.Iterator[str]: + yield stage.dvcfile.relpath for dep in stage.deps: if ( not dep.use_scm_ignore and dep.is_in_repo and not stage.repo.repo_fs.isdvc(dep.fs_path) ): - stage.repo.scm.track_file(relpath(dep.fs_path)) + yield dep.fs_path for out in stage.outs: if not out.use_scm_ignore and out.is_in_repo: - stage.repo.scm.track_file(relpath(out.fs_path)) + yield out.fs_path if out.live: from dvc.repo.live import summary_fs_path summary = summary_fs_path(out) if summary: - stage.repo.scm.track_file(relpath(summary)) - stage.repo.scm.track_changed_files() + yield summary + + +def _track_stage(stage: "Stage") -> None: + from dvc.utils import relpath + + context = stage.repo.scm_context + for path in _get_stage_files(stage): + context.track_file(relpath(path)) + return context.track_changed_files() @locked diff --git a/dvc/repo/scm_context.py b/dvc/repo/scm_context.py index b0ecc2df5..5043710c0 100644 --- a/dvc/repo/scm_context.py +++ b/dvc/repo/scm_context.py @@ -1,16 +1,106 @@ +import logging +import shlex +from contextlib import contextmanager from functools import wraps -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any, Dict, Iterator, List, Set if TYPE_CHECKING: from dvc.repo import Repo + from dvc.scm.base import Base -def scm_context(method): +logger = logging.getLogger(__name__) + + +class SCMContext: + def __init__(self, scm: "Base", config: Dict[str, Any] = None) -> None: + from funcy import get_in + + self.scm: "Base" = scm + self.autostage: bool = get_in( + config or {}, ["core", "autostage"], default=False + ) + self.ignored_paths: List[str] = [] + self.files_to_track: Set[str] = set() + self.quiet: bool = False + + def track_file(self, path: str) -> None: + """Track file to remind user to track new files or autostage later.""" + return self.files_to_track.add(path) + + def track_changed_files(self) -> None: + """Stage files that have changed.""" + if not self.files_to_track: + return + + logger.debug("Staging files: %s", self.files_to_track) + return self.scm.add(self.files_to_track) + + def ignore(self, path: str) -> None: + logger.debug("Adding '%s' to gitignore file.", path) + gitignore_file = self.scm.ignore(path) + if gitignore_file: + self.track_file(gitignore_file) + return self.ignored_paths.append(path) + + def ignore_remove(self, path: str) -> None: + logger.debug("Removing '%s' from gitignore file.", path) + gitignore_file = self.scm.ignore_remove(path) + if gitignore_file: + return self.track_file(gitignore_file) + + @contextmanager + def __call__( + self, autostage: bool = None, quiet: bool = None + ) -> Iterator["SCMContext"]: + from dvc.scm import NoSCM + + try: + yield self + except Exception: + for path in self.ignored_paths: + self.ignore_remove(path) + raise + finally: + self.ignored_paths = [] + + if not self.files_to_track: + return + + if autostage is None: + autostage = self.autostage + if quiet is None: + quiet = self.quiet + + if autostage: + self.track_changed_files() + elif not quiet and not isinstance(self.scm, NoSCM): + files = " ".join(map(shlex.quote, self.files_to_track)) + + logger.info( + "\nTo track the changes with git, run:\n\n" + f"\tgit add {files}" + ) + logger.info( + "\nTo enable auto staging, run:\n\n" + "\tdvc config core.autostage true" + ) + + self.files_to_track = set() + + def __enter__(self) -> "SCMContext": + self._cm = self() # pylint: disable=attribute-defined-outside-init + return self._cm.__enter__() # pylint: disable=no-member + + def __exit__(self, *exc_args) -> None: + assert self._cm + self._cm.__exit__(*exc_args) # pylint: disable=no-member + + +def scm_context(method, autostage: bool = None, quiet: bool = None): @wraps(method) def run(repo: "Repo", *args, **kw): - scm = repo.scm - - with scm.track_file_changes(config=repo.config): + with repo.scm_context(autostage=autostage, quiet=quiet): return method(repo, *args, **kw) return run diff --git a/dvc/repo/stage.py b/dvc/repo/stage.py index 243698ea1..7d4430129 100644 --- a/dvc/repo/stage.py +++ b/dvc/repo/stage.py @@ -130,8 +130,7 @@ class StageLoad: force=force, **stage_data, ) - scm = self.repo.scm - with scm.track_file_changes(config=self.repo.config): + with self.repo.scm_context: stage.dump(update_lock=update_lock) stage.ignore_outs() diff --git a/dvc/scm/base.py b/dvc/scm/base.py index 6a7950aa2..96d46dee5 100644 --- a/dvc/scm/base.py +++ b/dvc/scm/base.py @@ -1,9 +1,6 @@ """Manages source control systems (e.g. Git) in DVC.""" import os -from contextlib import contextmanager - -from funcy import get_in from dvc.exceptions import DvcException @@ -149,58 +146,6 @@ class Base: """Returns a list of commits in the repo.""" return [] - def cleanup_ignores(self): - """ - This method should clean up ignores (eg. entries in .gitignore), - use, when method editing ignores (eg. add, run, import) fails to - perform its task. - """ - - def reset_ignores(self): - """ - Method to reset in-memory ignore storing mechanism. - """ - - def reset_tracked_files(self): - """ - Method to reset in-memory tracked files storing mechanism. - """ - - def remind_to_track(self): - """ - Method to remind user to track newly created files handled by scm - """ - - def track_changed_files(self): - """ - Method to stage files that have changed - """ - - def track_file(self, path): - """ - Method to add file to mechanism that will remind user - to track new files - """ - - @contextmanager - def track_file_changes(self, config=None, autostage=False): - autostage = get_in( - config or {}, ["core", "autostage"], default=autostage - ) - - try: - yield - except Exception: - self.cleanup_ignores() - raise - - self.reset_ignores() - if autostage: - self.track_changed_files() - else: - self.remind_to_track() - self.reset_tracked_files() - def belongs_to_scm(self, path): """Return boolean whether file belongs to scm""" diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 1c77ad8a5..9e7ea9fe1 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -3,11 +3,10 @@ import logging import os import re -import shlex from collections.abc import Mapping from contextlib import contextmanager from functools import partialmethod -from typing import Dict, Iterable, List, Optional, Set, Tuple, Type +from typing import Dict, Iterable, Optional, Tuple, Type from funcy import cached_property, first from pathspec.patterns import GitWildMatchPattern @@ -82,10 +81,6 @@ class Git(Base): def __init__( self, *args, backends: Optional[Iterable[str]] = None, **kwargs ): - self.ignored_paths: List[str] = [] - self.files_to_track: Set[str] = set() - self.quiet: bool = False - self.backends = GitBackends(backends, *args, **kwargs) first_ = first(self.backends.values()) super().__init__(first_.root_dir) @@ -163,20 +158,13 @@ class Git(Base): return entry, gitignore - def ignore(self, path): + def ignore(self, path: str) -> Optional[str]: entry, gitignore = self._get_gitignore(path) - if self.is_ignored(path): - return - - msg = f"Adding '{relpath(path)}' to '{relpath(gitignore)}'." - logger.debug(msg) + return None self._add_entry_to_gitignore(entry, gitignore) - - self.track_file(relpath(gitignore)) - - self.ignored_paths.append(path) + return relpath(gitignore) def _add_entry_to_gitignore(self, entry, gitignore): entry = GitWildMatchPattern.escape(entry) @@ -195,11 +183,11 @@ class Git(Base): if new_entry not in unique_lines: fobj.write(new_entry) - def ignore_remove(self, path): + def ignore_remove(self, path: str) -> Optional[str]: entry, gitignore = self._get_gitignore(path) if not os.path.exists(gitignore): - return + return None with open(gitignore, encoding="utf-8") as fobj: lines = fobj.readlines() @@ -208,12 +196,11 @@ class Git(Base): if not filtered: os.unlink(gitignore) - return + return None with open(gitignore, "w", encoding="utf-8") as fobj: fobj.writelines(filtered) - - self.track_file(relpath(gitignore)) + return relpath(gitignore) def verify_hook(self, name): if (self.hooks_dir / name).exists(): @@ -237,43 +224,6 @@ class Git(Base): self.gitpython.repo.git.config(f"merge.{name}.name", description) self.gitpython.repo.git.config(f"merge.{name}.driver", driver) - def cleanup_ignores(self): - for path in self.ignored_paths: - self.ignore_remove(path) - self.reset_ignores() - - def reset_ignores(self): - self.ignored_paths = [] - - def reset_tracked_files(self): - self.files_to_track = set() - - def remind_to_track(self): - if self.quiet or not self.files_to_track: - return - - files = " ".join(shlex.quote(path) for path in self.files_to_track) - - logger.info( - "\n" - "To track the changes with git, run:\n" - "\n" - "\tgit add {files}\n" - "\n" - "To enable auto staging, run:\n" - "\n" - "\tdvc config core.autostage true".format(files=files) - ) - - def track_changed_files(self): - if not self.files_to_track: - return - - self.add(self.files_to_track) - - def track_file(self, path: str): - self.files_to_track.add(path) - def belongs_to_scm(self, path): basename = os.path.basename(path) path_parts = os.path.normpath(path).split(os.path.sep) diff --git a/tests/func/test_scm_context.py b/tests/func/test_scm_context.py new file mode 100644 index 000000000..b51021aba --- /dev/null +++ b/tests/func/test_scm_context.py @@ -0,0 +1,62 @@ +def test_scm_context_autostage(tmp_dir, scm, dvc): + tmp_dir.gen("foo", "foo") + with dvc.scm_context(autostage=True) as context: + context.track_file("foo") + + scm._reset() + assert scm.is_tracked("foo") + + +def test_scm_context_ignore(tmp_dir, scm, dvc): + with dvc.scm_context as context: + context.ignore(tmp_dir / "foo") + assert context.files_to_track == {scm.GITIGNORE} + + scm._reset() + assert scm.is_ignored("foo") + + +def test_scm_context_when_already_ignored(tmp_dir, scm, dvc): + scm.ignore(tmp_dir / "foo") + scm._reset() + + with dvc.scm_context() as context: + context.ignore(tmp_dir / "foo") + # If files are already ignored, dvc should not try to track a new + # .gitignore file as it's a no-op. + assert not context.files_to_track + + scm._reset() + assert scm.is_ignored("foo") + + +def test_scm_context_ignore_remove(tmp_dir, scm, dvc): + scm.ignore(tmp_dir / "foo") + scm.ignore(tmp_dir / "bar") + + with dvc.scm_context: + dvc.scm_context.ignore_remove(tmp_dir / "foo") + assert dvc.scm_context.files_to_track == {scm.GITIGNORE} + + scm._reset() + assert not scm.is_ignored("foo") + + +def test_scm_context_try_ignore_remove_non_existing_entry(tmp_dir, dvc, scm): + with dvc.scm_context as context: + context.ignore_remove(tmp_dir / "foo") + assert not context.files_to_track + scm._reset() + assert not scm.is_ignored("foo") + + +def test_scm_context_no_track_on_ignore_remove(tmp_dir, dvc, scm): + # DVC should not keep track of file when nothing actually changed + # i.e. here ignore was reverted back. + scm.ignore(tmp_dir / "foo") + with dvc.scm_context: + dvc.scm_context.ignore_remove(tmp_dir / "foo") + assert not dvc.scm_context.files_to_track + + scm._reset() + assert not scm.is_ignored("foo") diff --git a/tests/unit/repo/test_scm_context.py b/tests/unit/repo/test_scm_context.py new file mode 100644 index 000000000..d11846663 --- /dev/null +++ b/tests/unit/repo/test_scm_context.py @@ -0,0 +1,144 @@ +import re + +import pytest + +from dvc.repo.scm_context import SCMContext +from dvc.scm import Git, NoSCM + + +def pytest_generate_tests(metafunc): + if "scm_context" in metafunc.fixturenames: + metafunc.parametrize("scm_context", ["scm", "no_scm"], indirect=True) + + +@pytest.fixture +def scm_context(request, mocker): + spec = Git if getattr(request, "param", "scm") == "scm" else NoSCM + # we'll test `ignore` and `ignore_remove` in a functional test. + return SCMContext( + scm=mocker.MagicMock( + spec=spec, + **{ + "ignore_remove.return_value": ".gitignore", + "ignore.return_value": ".gitignore", + } + ) + ) + + +def test_scm_track_file(scm_context): + scm_context.track_file("foo") + assert scm_context.files_to_track == {"foo"} + scm_context.track_file("bar") + assert scm_context.files_to_track == {"foo", "bar"} + + +def test_scm_track_changed_files(scm_context): + scm_context.track_changed_files() + scm_context.scm.add.assert_not_called() + + scm_context.track_file("foo") + scm_context.track_changed_files() + scm_context.scm.add.assert_called_once_with({"foo"}) + + +def test_ignore(scm_context): + scm_context.ignore("foo") + + scm_context.scm.ignore.assert_called_once_with("foo") + assert scm_context.files_to_track == {".gitignore"} + assert scm_context.ignored_paths == ["foo"] + + +def test_ignore_remove(scm_context): + scm_context.ignore_remove("foo") + scm_context.scm.ignore_remove.assert_called_once_with("foo") + assert scm_context.files_to_track == {".gitignore"} + + +def test_scm_context_reset_on_exit(scm_context): + with scm_context: + scm_context.ignore("foo") + scm_context.track_file("bar") + assert not scm_context.files_to_track + assert not scm_context.ignored_paths + + +def test_scm_context_autostage_changed_files(scm_context): + scm_context.autostage = True + + with scm_context: + scm_context.track_file("foo") + assert scm_context.files_to_track == {"foo"} + + assert not scm_context.files_to_track + assert not scm_context.ignored_paths + scm_context.scm.add.assert_called_once_with({"foo"}) + + +def test_scm_context_clears_ignores_on_error(scm_context): + class CustomException(Exception): + pass + + with pytest.raises(CustomException), scm_context(): + scm_context.ignore("foo") + assert scm_context.ignored_paths == ["foo"] + raise CustomException + + scm_context.scm.ignore_remove.assert_called_once_with("foo") + assert scm_context.files_to_track == {".gitignore"} + assert not scm_context.ignored_paths + + +@pytest.mark.parametrize("autostage", [True, False]) +@pytest.mark.parametrize("quiet", [True, False]) +def test_scm_context_on_no_files_to_track( + caplog, scm_context, autostage, quiet +): + with scm_context(autostage=autostage, quiet=quiet): + pass + + scm_context.scm.assert_not_called() + assert not caplog.text + + +def test_scm_context_remind_to_track(caplog, scm_context): + with scm_context() as context: + context.track_file("foo") + context.track_file("lorem ipsum") + assert context.files_to_track == {"foo", "lorem ipsum"} + + if isinstance(scm_context.scm, NoSCM): + assert not caplog.text + else: + assert "To track the changes with git, run:" in caplog.text + match = re.search(r"git add(?: (('.*?')|(\S+)))*", caplog.text) + assert match and set(match.groups()) == {"'lorem ipsum'", "foo"} + + +def test_scm_context_remind_disable(caplog, scm_context): + with scm_context(quiet=True) as context: + context.track_file("foo") + assert context.files_to_track == {"foo"} + assert not caplog.text + + assert scm_context.quiet is False + scm_context.quiet = True + with scm_context() as context: + context.track_file("foo") + assert context.files_to_track == {"foo"} + assert not caplog.text + + +def test_scm_context_decorator(scm_context, mocker): + from dvc.repo.scm_context import scm_context as decorator + + repo = mocker.MagicMock(scm_context=scm_context) + + def test_method(repo, *args, **kwargs): # pylint: disable=unused-argument + scm_context.track_file("foo") + + method = mocker.MagicMock(wraps=test_method) + decorator(method, autostage=True)(repo, "arg", kw=1) + method.assert_called_once_with(repo, "arg", kw=1) + scm_context.scm.add.assert_called_once_with({"foo"}) diff --git a/tests/unit/scm/test_git.py b/tests/unit/scm/test_git.py index 085bc8572..e578cb283 100644 --- a/tests/unit/scm/test_git.py +++ b/tests/unit/scm/test_git.py @@ -522,12 +522,6 @@ def test_reset(tmp_dir, scm, git): assert len(unstaged) == 2 -def test_remind_to_track(scm, caplog): - scm.files_to_track = ["fname with spaces.txt", "тест", "foo"] - scm.remind_to_track() - assert "git add 'fname with spaces.txt' 'тест' foo" in caplog.text - - def test_add(tmp_dir, scm, git): if git.test_backend == "pygit2": pytest.skip() diff --git a/tests/unit/scm/test_scm.py b/tests/unit/scm/test_scm.py deleted file mode 100644 index 793bf8a1a..000000000 --- a/tests/unit/scm/test_scm.py +++ /dev/null @@ -1,61 +0,0 @@ -import pytest - -from dvc.repo.scm_context import scm_context -from dvc.scm import NoSCM - - -def test_scm_context(dvc, mocker): - scm = dvc.scm - m = mocker.patch.object( - scm, "track_file_changes", wraps=scm.track_file_changes - ) - - ret_val = 0 - wrapped = scm_context(mocker.Mock(return_value=ret_val)) - - assert wrapped(dvc) == ret_val - m.assert_called_once_with(config=dvc.config) - - -def test_track_file_changes(mocker): - scm = mocker.Mock(NoSCM) - - with NoSCM.track_file_changes(scm): - pass - - assert scm.reset_ignores.call_count == 1 - assert scm.remind_to_track.call_count == 1 - assert scm.track_changed_files.call_count == 0 - assert scm.cleanup_ignores.call_count == 0 - assert scm.reset_tracked_files.call_count == 1 - - -def test_track_file_changes_autostage(mocker): - scm = mocker.Mock(NoSCM) - - config = {"core": {"autostage": True}} - with NoSCM.track_file_changes(scm, config=config): - pass - - assert scm.track_changed_files.call_count == 1 - assert scm.reset_ignores.call_count == 1 - assert scm.remind_to_track.call_count == 0 - assert scm.cleanup_ignores.call_count == 0 - assert scm.reset_tracked_files.call_count == 1 - - -def test_track_file_changes_throw_and_cleanup(mocker): - scm = mocker.Mock(NoSCM) - - class CustomException(Exception): - pass - - with pytest.raises(CustomException, match="oops"): - with NoSCM.track_file_changes(scm): - raise CustomException("oops") - - assert scm.cleanup_ignores.call_count == 1 - assert scm.reset_ignores.call_count == 0 - assert scm.track_changed_files.call_count == 0 - assert scm.remind_to_track.call_count == 0 - assert scm.reset_tracked_files.call_count == 0