Repository: iterative/dvc

Issue:
diff: compare dirty vs committed workspace by default
By default `dvc diff` compares workspace to HEAD (or to a single commit if only one arg is provided). This is the one of the most important use cases. I would like to see the "uncommitted" (in a DVC-sense - when we update md5) changes to data files and directories:

```
git clone https://github.com/iterative/example-get-started
cd example-get-started
dvc pull
rm -f data/data.xml
touch data/features/foo
dvc diff
```

The last command produces an empty result. Not very informative to compare my "current" experiment with the baseline.

UPDATE:

Another interesting point that `dvc status` does exactly this - it analyzes actual changes.


Discussion:

Patch:
diff --git a/dvc/config.py b/dvc/config.py
index 50cd8feb3..6e1ccd7f5 100644
--- a/dvc/config.py
+++ b/dvc/config.py
@@ -179,6 +179,7 @@ SCHEMA = {
                     "gdrive_service_account_email": str,
                     "gdrive_service_account_user_email": str,
                     "gdrive_service_account_p12_file_path": str,
+                    Optional("gdrive_trash_only", default=False): Bool,
                     **REMOTE_COMMON,
                 },
                 "http": {**HTTP_COMMON, **REMOTE_COMMON},
diff --git a/dvc/path_info.py b/dvc/path_info.py
index 613a76c01..4f757b00b 100644
--- a/dvc/path_info.py
+++ b/dvc/path_info.py
@@ -115,7 +115,7 @@ class URLInfo(_BasePath):
         assert not p.query and not p.params and not p.fragment
         assert p.password is None
 
-        self.fill_parts(p.scheme, p.hostname, p.username, p.port, p.path)
+        self._fill_parts(p.scheme, p.hostname, p.username, p.port, p.path)
 
     @classmethod
     def from_parts(
@@ -127,10 +127,10 @@ class URLInfo(_BasePath):
             return cls("{}://{}{}".format(scheme, netloc, path))
 
         obj = cls.__new__(cls)
-        obj.fill_parts(scheme, host, user, port, path)
+        obj._fill_parts(scheme, host, user, port, path)
         return obj
 
-    def fill_parts(self, scheme, host, user, port, path):
+    def _fill_parts(self, scheme, host, user, port, path):
         assert scheme != "remote"
         assert isinstance(path, (str, bytes, _URLPathInfo))
 
@@ -244,6 +244,8 @@ class CloudURLInfo(URLInfo):
 
 
 class HTTPURLInfo(URLInfo):
+    __hash__ = URLInfo.__hash__
+
     def __init__(self, url):
         p = urlparse(url)
         stripped = p._replace(params=None, query=None, fragment=None)
@@ -280,7 +282,7 @@ class HTTPURLInfo(URLInfo):
             )
 
         obj = cls.__new__(cls)
-        obj.fill_parts(scheme, host, user, port, path)
+        obj._fill_parts(scheme, host, user, port, path)
         obj.params = params
         obj.query = query
         obj.fragment = fragment
@@ -314,6 +316,3 @@ class HTTPURLInfo(URLInfo):
             and self._path == other._path
             and self._extra_parts == other._extra_parts
         )
-
-    def __hash__(self):
-        return hash(self.parts)
diff --git a/dvc/remote/gdrive.py b/dvc/remote/gdrive.py
index 484bd1a82..a695dd09b 100644
--- a/dvc/remote/gdrive.py
+++ b/dvc/remote/gdrive.py
@@ -39,12 +39,14 @@ class GDriveMissedCredentialKeyError(DvcException):
 
 
 def gdrive_retry(func):
-    retry_re = re.compile(r"HttpError (403|500|502|503|504)")
-
     def should_retry(exc):
         from pydrive2.files import ApiRequestError
 
-        return isinstance(exc, ApiRequestError) and retry_re.search(str(exc))
+        if not isinstance(exc, ApiRequestError):
+            return False
+
+        retry_codes = [403, 500, 502, 503, 504]
+        return exc.error.get("code", 0) in retry_codes
 
     # 15 tries, start at 0.5s, multiply by golden ratio, cap at 20s
     return retry(
@@ -54,6 +56,19 @@ def gdrive_retry(func):
     )(func)
 
 
+def _location(exc):
+    from pydrive2.files import ApiRequestError
+
+    assert isinstance(exc, ApiRequestError)
+
+    # https://cloud.google.com/storage/docs/json_api/v1/status-codes#errorformat
+    return (
+        exc.error["errors"][0].get("location", "")
+        if exc.error.get("errors", [])
+        else ""
+    )
+
+
 class GDriveURLInfo(CloudURLInfo):
     def __init__(self, url):
         super().__init__(url)
@@ -95,6 +110,7 @@ class RemoteGDrive(RemoteBASE):
             )
 
         self._bucket = self.path_info.bucket
+        self._trash_only = config.get("gdrive_trash_only")
         self._use_service_account = config.get("gdrive_use_service_account")
         self._service_account_email = config.get(
             "gdrive_service_account_email"
@@ -119,8 +135,6 @@ class RemoteGDrive(RemoteBASE):
             )
         )
 
-        self._list_params = None
-
     def _validate_config(self):
         # Validate Service Account configuration
         if self._use_service_account and (
@@ -326,10 +340,35 @@ class RemoteGDrive(RemoteBASE):
 
     @gdrive_retry
     def _delete_remote_file(self, remote_id):
+        from pydrive2.files import ApiRequestError
+
         param = {"id": remote_id}
         # it does not create a file on the remote
         item = self.drive.CreateFile(param)
-        item.Delete()
+
+        try:
+            item.Trash() if self._trash_only else item.Delete()
+        except ApiRequestError as exc:
+            http_error_code = exc.error.get("code", 0)
+            if (
+                http_error_code == 403
+                and self.list_params["corpora"] == "drive"
+                and _location(exc) == "file.permissions"
+            ):
+                raise DvcException(
+                    "Insufficient permissions to {}. You should have {} "
+                    "access level for the used shared drive. More details "
+                    "at {}.".format(
+                        "move the file into Trash"
+                        if self._trash_only
+                        else "permanently delete the file",
+                        "Manager or Content Manager"
+                        if self._trash_only
+                        else "Manager",
+                        "https://support.google.com/a/answer/7337554",
+                    )
+                ) from exc
+            raise
 
     @gdrive_retry
     def _get_remote_item(self, name, parents_ids):
diff --git a/dvc/remote/local.py b/dvc/remote/local.py
index 25d409f67..7a93434f3 100644
--- a/dvc/remote/local.py
+++ b/dvc/remote/local.py
@@ -448,13 +448,17 @@ class RemoteLOCAL(RemoteBASE):
         try:
             os.chmod(path, mode)
         except OSError as exc:
-            # In share cache scenario, we might not own the cache file, so we
+            # There is nothing we need to do in case of a read-only file system
+            if exc.errno == errno.EROFS:
+                return
+
+            # In shared cache scenario, we might not own the cache file, so we
             # need to check if cache file is already protected.
             if exc.errno not in [errno.EPERM, errno.EACCES]:
                 raise
 
-            actual = os.stat(path).st_mode
-            if actual & mode != mode:
+            actual = stat.S_IMODE(os.stat(path).st_mode)
+            if actual != mode:
                 raise
 
     def _get_unpacked_dir_path_info(self, checksum):
diff --git a/dvc/repo/diff.py b/dvc/repo/diff.py
index 9c54e7bc1..d18389e05 100644
--- a/dvc/repo/diff.py
+++ b/dvc/repo/diff.py
@@ -3,6 +3,7 @@ import os
 from dvc.exceptions import DvcException
 from dvc.repo import locked
 from dvc.scm.git import Git
+from dvc.scm.tree import is_working_tree
 
 
 @locked
@@ -36,10 +37,23 @@ def diff(self, a_rev="HEAD", b_rev=None):
                 else os.path.join(str(output), "")
             )
 
+        on_working_tree = is_working_tree(self.tree)
+
+        def _to_checksum(output):
+            if on_working_tree:
+                return self.cache.local.get_checksum(output.path_info)
+            return output.checksum
+
+        def _exists(output):
+            if on_working_tree:
+                return output.exists
+            return True
+
         return {
-            _to_path(output): output.checksum
+            _to_path(output): _to_checksum(output)
             for stage in self.stages
             for output in stage.outs
+            if _exists(output)
         }
 
     working_tree = self.tree
diff --git a/dvc/repo/metrics/diff.py b/dvc/repo/metrics/diff.py
index 1ed20e387..ef25d09d8 100644
--- a/dvc/repo/metrics/diff.py
+++ b/dvc/repo/metrics/diff.py
@@ -68,7 +68,11 @@ def _diff(old_raw, new_raw):
     if isinstance(new, dict) or isinstance(old, dict):
         return _diff_dicts(old, new)
 
-    return {"": _diff_vals(old, new)}
+    val_diff = _diff_vals(old, new)
+    if val_diff:
+        return {"": val_diff}
+
+    return {}
 
 
 def _get_metrics(repo, *args, rev=None, **kwargs):
diff --git a/dvc/version.py b/dvc/version.py
index ce5f6b8e6..9ffc08ac6 100644
--- a/dvc/version.py
+++ b/dvc/version.py
@@ -7,7 +7,7 @@ import os
 import subprocess
 
 
-_BASE_VERSION = "0.91.0"
+_BASE_VERSION = "0.91.1"
 
 
 def _generate_version(base_version):
diff --git a/setup.py b/setup.py
index d9a29bbc3..573024882 100644
--- a/setup.py
+++ b/setup.py
@@ -88,7 +88,7 @@ install_requires = [
 # Extra dependencies for remote integrations
 
 gs = ["google-cloud-storage==1.19.0"]
-gdrive = ["pydrive2>=1.4.6"]
+gdrive = ["pydrive2>=1.4.8"]
 s3 = ["boto3>=1.9.201"]
 azure = ["azure-storage-blob==2.1.0"]
 oss = ["oss2==2.6.1"]


Test patch:
diff --git a/tests/func/test_diff.py b/tests/func/test_diff.py
index bb474cd24..918f5ba40 100644
--- a/tests/func/test_diff.py
+++ b/tests/func/test_diff.py
@@ -159,21 +159,51 @@ def test_directories(tmp_dir, scm, dvc):
 
 
 def test_diff_no_cache(tmp_dir, scm, dvc):
-    (stage,) = tmp_dir.dvc_gen(
-        {"dir": {"file": "file content"}}, commit="first"
-    )
+    tmp_dir.dvc_gen({"dir": {"file": "file content"}}, commit="first")
     scm.tag("v1")
+
     tmp_dir.dvc_gen(
         {"dir": {"file": "modified file content"}}, commit="second"
     )
+    scm.tag("v2")
 
-    remove(first(stage.outs).cache_path)
-    remove("dir")
+    remove(dvc.cache.local.cache_dir)
 
     # invalidate_dir_info to force cache loading
     dvc.cache.local._dir_info = {}
 
-    diff = dvc.diff("v1")
+    diff = dvc.diff("v1", "v2")
     assert diff["added"] == []
     assert diff["deleted"] == []
     assert first(diff["modified"])["path"] == os.path.join("dir", "")
+
+
+def test_diff_dirty(tmp_dir, scm, dvc):
+    tmp_dir.dvc_gen(
+        {"file": "file_content", "dir": {"dir_file1": "dir file content"}},
+        commit="initial",
+    )
+
+    (tmp_dir / "file").unlink()
+    tmp_dir.gen({"dir": {"dir_file2": "dir file 2 content"}})
+    tmp_dir.dvc_gen("new_file", "new_file_content")
+
+    result = dvc.diff()
+
+    assert result == {
+        "added": [
+            {"hash": "86d049de17c76ac44cdcac146042ec9b", "path": "new_file"}
+        ],
+        "deleted": [
+            {"hash": "7f0b6bb0b7e951b7fd2b2a4a326297e1", "path": "file"}
+        ],
+        "modified": [
+            {
+                "hash": {
+                    "new": "38175ad60f0e58ac94e0e2b7688afd81.dir",
+                    "old": "92daf39af116ca2fb245acaeb2ae65f7.dir",
+                },
+                "path": os.path.join("dir", ""),
+            }
+        ],
+    }
diff --git a/tests/func/test_metrics.py b/tests/func/test_metrics.py
index 8339b45ba..fab19c86d 100644
--- a/tests/func/test_metrics.py
+++ b/tests/func/test_metrics.py
@@ -895,6 +895,20 @@ def test_metrics_diff_raw(tmp_dir, scm, dvc):
     }
 
 
+def test_metrics_diff_raw_unchanged(tmp_dir, scm, dvc):
+    def _gen(val):
+        tmp_dir.gen({"metrics": val})
+        dvc.run(cmd="", metrics=["metrics"])
+        dvc.scm.add(["metrics.dvc"])
+        dvc.scm.commit(str(val))
+
+    _gen("raw 1")
+    _gen("raw 2")
+    _gen("raw 1")
+
+    assert dvc.metrics.diff(a_rev="HEAD~2") == {}
+
+
 @pytest.mark.parametrize("xpath", [True, False])
 def test_metrics_diff_json(tmp_dir, scm, dvc, xpath):
     def _gen(val):
@@ -919,6 +933,25 @@ def test_metrics_diff_json(tmp_dir, scm, dvc, xpath):
     assert expected == dvc.metrics.diff(a_rev="HEAD~2")
 
 
+@pytest.mark.parametrize("xpath", [True, False])
+def test_metrics_diff_json_unchanged(tmp_dir, scm, dvc, xpath):
+    def _gen(val):
+        metrics = {"a": {"b": {"c": val, "d": 1, "e": str(val)}}}
+        tmp_dir.gen({"m.json": json.dumps(metrics)})
+        dvc.run(cmd="", metrics=["m.json"])
+        dvc.metrics.modify("m.json", typ="json")
+        if xpath:
+            dvc.metrics.modify("m.json", xpath="a.b.c")
+        dvc.scm.add(["m.json.dvc"])
+        dvc.scm.commit(str(val))
+
+    _gen(1)
+    _gen(2)
+    _gen(1)
+
+    assert dvc.metrics.diff(a_rev="HEAD~2") == {}
+
+
 def test_metrics_diff_broken_json(tmp_dir, scm, dvc):
     metrics = {"a": {"b": {"c": 1, "d": 1, "e": "3"}}}
     tmp_dir.gen({"m.json": json.dumps(metrics)})
diff --git a/tests/unit/remote/test_local.py b/tests/unit/remote/test_local.py
index d18e3b4fb..04e216bde 100644
--- a/tests/unit/remote/test_local.py
+++ b/tests/unit/remote/test_local.py
@@ -1,4 +1,5 @@
 import os
+import errno
 
 import pytest
 
@@ -58,3 +59,30 @@ def test_is_protected(tmp_dir, link_name):
         assert remote.is_protected(foo)
     else:
         assert not remote.is_protected(foo)
+
+
+@pytest.mark.parametrize("err", [errno.EPERM, errno.EACCES])
+def test_protect_ignore_errors(tmp_dir, mocker, err):
+    tmp_dir.gen("foo", "foo")
+    foo = PathInfo("foo")
+    remote = RemoteLOCAL(None, {})
+
+    remote.protect(foo)
+
+    mock_chmod = mocker.patch(
+        "os.chmod", side_effect=OSError(err, "something")
+    )
+    remote.protect(foo)
+    assert mock_chmod.called
+
+
+def test_protect_ignore_erofs(tmp_dir, mocker):
+    tmp_dir.gen("foo", "foo")
+    foo = PathInfo("foo")
+    remote = RemoteLOCAL(None, {})
+
+    mock_chmod = mocker.patch(
+        "os.chmod", side_effect=OSError(errno.EROFS, "read-only fs")
+    )
+    remote.protect(foo)
+    assert mock_chmod.called