Dvc: diff: support targets

[DavidGOrtega]

dvc diff -t models HEAD^ HEAD

ERROR: unrecognized arguments: -t HEAD

Please provide information about your setup
Ubuntu 18.04
Dvc 0.84.0

[dmpetrov]

It is a good feature idea but probably not a critical one.

@DavidGOrtega I'd appreciate it if you provide more context to understand how critical the issue is?

[DavidGOrtega]

@dmpetrov I tried it according to the docs and I found that issue. For ci is not an issue aside of having more features.

[jorgeorpinel]

This feature (--target option) was removed in #3229. Do we remember why? 🙂 Cc @efiop

[dmpetrov]

@jorgeorpinel as far as I remember it was removed for two reasons: it was not a priority and it seems fine not to have it because git diff does not support any targets.

Is there a good use case when we need targets (except backward compatibility)? It is important to understand the priority.

[jorgeorpinel]

Ohhh I see. I think this issue is mainly about a discrepancy between dvc diff and its cmd ref in https://dvc.org/doc/command-reference/diff so we can probably transfer it to iterative/dvc.org ?

In fact I thought it was addressed in iterative/dvc.org/pull/953 but I see not. Lmk if you guys agree and I'll transfer it so it's addressed ASAP.

UPDATE: Should we close this issue? The docs are up to date now, actually.

[shcheklein]

@dmpetrov

because git diff does not support any targets

Looks like it does.

git diff HEAD^ -- dvc/api.py

returns:

diff --git a/dvc/api.py b/dvc/api.py
index def9e8cf..d1312baa 100644
--- a/dvc/api.py
+++ b/dvc/api.py
@@ -18,10 +18,10 @@ class UrlNotDvcRepoError(DvcException):


 def get_url(path, repo=None, rev=None, remote=None):
-    """
-    Returns the full URL to the data artifact specified by its `path` in a
-    `repo`.
-    NOTE: There is no guarantee that the file actually exists in that location.
+    """Returns URL to the storage location of a data artifact tracked
+    by DVC, specified by its path in a repo.
+
+    NOTE: There's no guarantee that the file actually exists in that location.

https://git-scm.com/docs/git-diff

[shcheklein]

@jorgeorpinel

In fact I thought it was addressed in iterative/dvc.org/pull/953 but I see not. Lmk if you guys agree and I'll transfer it so it's addressed ASAP.

not sure I understand this, could you elaborate please?

[jorgeorpinel]

Yeah, git diff supports targets.

@shcheklein I was confused. The --target options and -t examples were indeed removed in that PR! I guess David opened this issue before that PR was merged.

So should we close this issue? The docs are up to date now.

[shcheklein]

Let's keep it. This is a pretty relevant feature. Especially considering that there was some confusion around git diff.

[jorgeorpinel]

OK

Looks like (dvc diff) does (support targets).
git diff HEAD^ -- dvc/api.py

You don't even need --, so if we're re-implementing this, maybe just make it an implicit (and optional) [targets [targets]] cmd param like in fetch/pull/push.

[sandeepmistry]

I've started a draft pull request on this if anyone would like to give early feedback the changes: https://github.com/iterative/dvc/pull/4938