scm: split tracking of to-be git-ignored/git-staged into SCMContext

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():

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

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:
 

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:

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

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

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

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()
 

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"""
 

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)

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")

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"})

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()

tests/unit/scm/test_scm.py

@@ -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