From 8e9d8145cd73dda1aec47e2c9e19e7b56add537c Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Wed, 29 Jan 2020 17:40:59 -0600 Subject: [PATCH] diff: remove deadcode, replace diffable with dicts --- dvc/command/diff.py | 11 +++--- dvc/repo/brancher.py | 2 -- dvc/repo/diff.py | 64 +++++++++++++++------------------ dvc/scm/git/__init__.py | 62 +------------------------------- tests/func/test_diff.py | 36 +++++++++---------- tests/unit/command/test_diff.py | 16 ++++----- 6 files changed, 59 insertions(+), 132 deletions(-) diff --git a/dvc/command/diff.py b/dvc/command/diff.py index 42113f900..3c6b2f447 100644 --- a/dvc/command/diff.py +++ b/dvc/command/diff.py @@ -15,7 +15,7 @@ class CmdDiff(CmdBase): @staticmethod def _format(diff): """ - Given a diff structure, generate a string of filenames separated + Given a diff structure, generate a string of paths separated by new lines and grouped together by their state. A group's header is colored and its entries are sorted to enhance @@ -57,17 +57,16 @@ class CmdDiff(CmdBase): header=state.capitalize(), nc=colorama.Fore.RESET, entries="\n".join( - "{space}{checksum}{separator}{filename}".format( + "{space}{checksum}{separator}{path}".format( space=" ", checksum=_digest(entry.get("checksum")), separator=" " if entry.get("checksum") else "", - filename=entry["filename"], + path=entry["path"], ) - for entry in entries + for entry in diff[state] ), ) - for state, entries in diff.items() - if entries + for state in ["added", "deleted", "modified"] ) def run(self): diff --git a/dvc/repo/brancher.py b/dvc/repo/brancher.py index 9d1090190..955b13513 100644 --- a/dvc/repo/brancher.py +++ b/dvc/repo/brancher.py @@ -41,8 +41,6 @@ def brancher( # noqa: E302 if all_tags: revs.extend(scm.list_tags()) - revs = filter(None, revs) - try: if revs: for sha, names in group_by(scm.resolve_rev, revs).items(): diff --git a/dvc/repo/diff.py b/dvc/repo/diff.py index e6735250e..b8970c7b3 100644 --- a/dvc/repo/diff.py +++ b/dvc/repo/diff.py @@ -1,4 +1,3 @@ -import collections import os from dvc.exceptions import DvcException @@ -18,16 +17,15 @@ def diff(self, a_ref="HEAD", b_ref=None): if type(self.scm) is not Git: raise DvcException("only supported for Git repositories") - - def _checksums_by_filenames(): + def _paths_checksums(): """ - A dictionary of checksums addressed by filenames collected from + A dictionary of checksums addressed by relpaths collected from the current tree outputs. Unpack directories to include their entries To help distinguish between a directory and a file output, - the former one will come with a trailing slash in the filename: + the former one will come with a trailing slash in the path: directory: "data/" file: "data" @@ -43,42 +41,36 @@ def diff(self, a_ref="HEAD", b_ref=None): result.update({os.path.join(str(output), ""): output.checksum}) for entry in output.dir_cache: - filename = str(output.path_info / entry["relpath"]) - result.update({filename: entry["md5"]}) + path = str(output.path_info / entry["relpath"]) + result.update({path: entry["md5"]}) return result + working_tree = self.tree + a_tree = self.scm.get_tree(a_ref) + b_tree = self.scm.get_tree(b_ref) if b_ref else working_tree - def _compare_trees(a_ref, b_ref): - working_tree = self.tree + try: + self.tree = a_tree + old = _paths_checksums() - a_tree = self.scm.get_tree(self.scm.resolve_rev(a_ref)) - b_tree = self.scm.get_tree(self.scm.resolve_rev(b_ref)) if b_tree else working_tree + self.tree = b_tree + new = _paths_checksums() + finally: + self.tree = working_tree - breakpoint() + # Compare paths between the old and new tree. + # set() efficiently converts dict keys to a set + added = sorted(set(new) - set(old)) + deleted = sorted(set(old) - set(new)) + modified = sorted(set(old) & set(new)) - old = outs[a_ref] - new = outs[b_ref or "working tree"] - - added = new - old - deleted = old - new - delta = old ^ new - - result = { - "added": [entry._asdict() for entry in sorted(added)], - "deleted": [entry._asdict() for entry in sorted(deleted)], - "modified": [], + return { + "added": [{"path": path, "checksum": new[path]} for path in added], + "deleted": [{"path": path, "checksum": old[path]} for path in deleted], + "modified": [ + {"path": path, "checksum": {"old": old[path], "new": new[path]}} + for path in modified + if old[path] != new[path] + ], } - - for _old, _new in zip(sorted(old - delta), sorted(new - delta)): - if _old.checksum == _new.checksum: - continue - - result["modified"].append( - { - "filename": _new.filename, - "checksum": {"old": _old.checksum, "new": _new.checksum}, - } - ) - - return result diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 9ae9e1514..47f737975 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -22,13 +22,6 @@ from dvc.utils.fs import path_isin logger = logging.getLogger(__name__) -DIFF_A_TREE = "a_tree" -DIFF_B_TREE = "b_tree" -DIFF_A_REF = "a_ref" -DIFF_B_REF = "b_ref" -DIFF_EQUAL = "equal" - - class Git(Base): """Class for managing Git.""" @@ -308,60 +301,7 @@ class Git(Base): return basename == self.ignore_file or Git.GIT_DIR in path_parts def get_tree(self, rev): - return GitTree(self.repo, rev) - - def _get_diff_trees(self, a_ref, b_ref): - """Private method for getting the trees and commit hashes of 2 git - references. Requires `gitdb` module (from gitpython package). - - Args: - a_ref (str): git reference - b_ref (str): second git reference. If None, uses HEAD - - Returns: - tuple: tuple with elements: (trees, commits) - """ - from gitdb.exc import BadObject, BadName - - trees = {DIFF_A_TREE: None, DIFF_B_TREE: None} - commits = [] - if b_ref is None: - b_ref = self.repo.head.commit - try: - a_commit = self.repo.git.rev_parse(a_ref, short=True) - b_commit = self.repo.git.rev_parse(b_ref, short=True) - # See https://gitpython.readthedocs.io - # /en/2.1.11/reference.html#git.objects.base.Object.__str__ - commits.append(a_commit) - commits.append(b_commit) - trees[DIFF_A_TREE] = self.get_tree(commits[0]) - trees[DIFF_B_TREE] = self.get_tree(commits[1]) - except (BadName, BadObject) as exc: - raise SCMError("git problem") from exc - return trees, commits - - def get_diff_trees(self, a_ref, b_ref=None): - """Method for getting two repo trees between two git tag commits. - Returns the dvc hash names of changed file/directory - - Args: - a_ref (str): git reference - b_ref (str): optional second git reference, default None - - Returns: - dict: dictionary with keys: {a_ref, b_ref, equal} - or {a_ref, b_ref, a_tree, b_tree} - """ - diff_dct = {DIFF_EQUAL: False} - trees, commits = self._get_diff_trees(a_ref, b_ref) - diff_dct[DIFF_A_REF] = commits[0] - diff_dct[DIFF_B_REF] = commits[1] - if commits[0] == commits[1]: - diff_dct[DIFF_EQUAL] = True - return diff_dct - diff_dct[DIFF_A_TREE] = trees[DIFF_A_TREE] - diff_dct[DIFF_B_TREE] = trees[DIFF_B_TREE] - return diff_dct + return GitTree(self.repo, self.resolve_rev(rev)) def get_rev(self): return self.repo.git.rev_parse("HEAD") diff --git a/tests/func/test_diff.py b/tests/func/test_diff.py index 446f7f246..7af8f3afc 100644 --- a/tests/func/test_diff.py +++ b/tests/func/test_diff.py @@ -22,7 +22,7 @@ def test_added(tmp_dir, scm, dvc): tmp_dir.dvc_gen("file", "text") assert dvc.diff() == { - "added": [{"filename": "file", "checksum": digest("text")}], + "added": [{"path": "file", "checksum": digest("text")}], "deleted": [], "modified": [], } @@ -41,14 +41,14 @@ def test_no_cache_entry(tmp_dir, scm, dvc): assert dvc.diff() == { "added": [ - {"filename": os.path.join("dir", ""), "checksum": dir_checksum}, - {"filename": os.path.join("dir", "1"), "checksum": digest("1")}, - {"filename": os.path.join("dir", "2"), "checksum": digest("2")}, + {"path": os.path.join("dir", ""), "checksum": dir_checksum}, + {"path": os.path.join("dir", "1"), "checksum": digest("1")}, + {"path": os.path.join("dir", "2"), "checksum": digest("2")}, ], "deleted": [], "modified": [ { - "filename": "file", + "path": "file", "checksum": {"old": digest("first"), "new": digest("second")}, } ], @@ -61,7 +61,7 @@ def test_deleted(tmp_dir, scm, dvc): assert dvc.diff() == { "added": [], - "deleted": [{"filename": "file", "checksum": digest("text")}], + "deleted": [{"path": "file", "checksum": digest("text")}], "modified": [], } @@ -75,7 +75,7 @@ def test_modified(tmp_dir, scm, dvc): "deleted": [], "modified": [ { - "filename": "file", + "path": "file", "checksum": {"old": digest("first"), "new": digest("second")}, } ], @@ -95,7 +95,7 @@ def test_refs(tmp_dir, scm, dvc): "added": [], "deleted": [], "modified": [ - {"filename": "file", "checksum": {"old": HEAD_1, "new": HEAD}} + {"path": "file", "checksum": {"old": HEAD_1, "new": HEAD}} ], } @@ -103,7 +103,7 @@ def test_refs(tmp_dir, scm, dvc): "added": [], "deleted": [], "modified": [ - {"filename": "file", "checksum": {"old": HEAD_2, "new": HEAD_1}} + {"path": "file", "checksum": {"old": HEAD_2, "new": HEAD_1}} ], } @@ -127,31 +127,29 @@ def test_directories(tmp_dir, scm, dvc): assert dvc.diff(":/init", ":/directory") == { "added": [ { - "filename": os.path.join("dir", ""), + "path": os.path.join("dir", ""), "checksum": "5fb6b29836c388e093ca0715c872fe2a.dir", }, - {"filename": os.path.join("dir", "1"), "checksum": digest("1")}, - {"filename": os.path.join("dir", "2"), "checksum": digest("2")}, + {"path": os.path.join("dir", "1"), "checksum": digest("1")}, + {"path": os.path.join("dir", "2"), "checksum": digest("2")}, ], "deleted": [], "modified": [], } assert dvc.diff(":/directory", ":/modify") == { - "added": [ - {"filename": os.path.join("dir", "3"), "checksum": digest("3")} - ], + "added": [{"path": os.path.join("dir", "3"), "checksum": digest("3")}], "deleted": [], "modified": [ { - "filename": os.path.join("dir", ""), + "path": os.path.join("dir", ""), "checksum": { "old": "5fb6b29836c388e093ca0715c872fe2a.dir", "new": "9b5faf37366b3370fd98e3e60ca439c1.dir", }, }, { - "filename": os.path.join("dir", "2"), + "path": os.path.join("dir", "2"), "checksum": {"old": digest("2"), "new": digest("two")}, }, ], @@ -160,11 +158,11 @@ def test_directories(tmp_dir, scm, dvc): assert dvc.diff(":/modify", ":/delete") == { "added": [], "deleted": [ - {"filename": os.path.join("dir", "2"), "checksum": digest("two")} + {"path": os.path.join("dir", "2"), "checksum": digest("two")} ], "modified": [ { - "filename": os.path.join("dir", ""), + "path": os.path.join("dir", ""), "checksum": { "old": "9b5faf37366b3370fd98e3e60ca439c1.dir", "new": "83ae82fb367ac9926455870773ff09e6.dir", diff --git a/tests/unit/command/test_diff.py b/tests/unit/command/test_diff.py index fb29e7e76..1f6b450d8 100644 --- a/tests/unit/command/test_diff.py +++ b/tests/unit/command/test_diff.py @@ -5,7 +5,7 @@ def test_default(mocker, caplog): args = parse_args(["diff"]) cmd = args.func(args) diff = { - "added": [{"filename": "file", "checksum": "00000000"}], + "added": [{"path": "file", "checksum": "00000000"}], "deleted": [], "modified": [], } @@ -21,12 +21,12 @@ def test_checksums(mocker, caplog): diff = { "added": [], "deleted": [ - {"filename": "bar", "checksum": "00000000"}, - {"filename": "foo", "checksum": "11111111"}, + {"path": "bar", "checksum": "00000000"}, + {"path": "foo", "checksum": "11111111"}, ], "modified": [ { - "filename": "file", + "path": "file", "checksum": {"old": "AAAAAAAA", "new": "BBBBBBBB"}, } ], @@ -47,7 +47,7 @@ def test_json(mocker, caplog): args = parse_args(["diff", "--show-json"]) cmd = args.func(args) diff = { - "added": [{"filename": "file", "checksum": "00000000"}], + "added": [{"path": "file", "checksum": "00000000"}], "deleted": [], "modified": [], } @@ -55,7 +55,7 @@ def test_json(mocker, caplog): assert 0 == cmd.run() assert ( - '{"added": [{"filename": "file"}], "deleted": [], "modified": []}' + '{"added": [{"path": "file"}], "deleted": [], "modified": []}' in caplog.text ) @@ -64,7 +64,7 @@ def test_json_checksums(mocker, caplog): args = parse_args(["diff", "--show-json", "--checksums"]) cmd = args.func(args) diff = { - "added": [{"filename": "file", "checksum": "00000000"}], + "added": [{"path": "file", "checksum": "00000000"}], "deleted": [], "modified": [], } @@ -72,6 +72,6 @@ def test_json_checksums(mocker, caplog): assert 0 == cmd.run() assert ( - '{"added": [{"filename": "file", "checksum": "00000000"}], ' + '{"added": [{"path": "file", "checksum": "00000000"}], ' '"deleted": [], "modified": []}' in caplog.text )