From b09bd0426ec40d3d3ff133b6b8649fe2fb2f547c Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Fri, 10 May 2019 07:30:36 -0500 Subject: [PATCH] Document dvc.scm.git::Git::_get_diff_trees and shorten commit hashes for `dvc diff` (#1906) * Document dvc.scm.git::Git::_get_diff_trees and shorten commit hashes in dvc.scm.git::Git::get_diff_trees * diff: Changes to dvc.scm.git so `dvc diff` outputs short SHAs - Docstring for Git class constructor - Renames self.git for self.repo throughout module (involves several test changes) - Better doc and logical refactor in Git._get_diff_trees - `dvc diff` now outputs short SHA strings (Git._get_diff_trees) - hard coded to 7 length str (involves some test changes in tests/func/test_diff.py) For #1902 * Undoes "Renames self.git for self.repo throughout module" introduced in 3d2dc83. * diff: `dvc diff` uses code for `git rev_parse --short` instead of hard `[:7]` (for short commit hashes). - Tests updated too. Per https://github.com/iterative/dvc/pull/1906#discussion_r278510340 * tests: Reformatted with `black tests/func/test_diff.py` * tests: Remove uneccessary `str()` casting form `dvc diff` tests (and reformat) --- dvc/scm/git/__init__.py | 43 ++++++++++++++++++++++++++--------------- tests/func/test_diff.py | 38 +++++++++++++++++++++++------------- 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 5d1b51ce5..85b66db81 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -33,6 +33,9 @@ class Git(Base): GIT_DIR = ".git" def __init__(self, root_dir=os.curdir, repo=None): + """Git class constructor. + Requires `Repo` class from `git` module (from gitpython package). + """ super(Git, self).__init__(root_dir, repo=repo) import git @@ -248,22 +251,30 @@ class Git(Base): return GitTree(self.git, 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.git.head.commit try: - if b_ref is not None: - a_commit = self.git.commit(a_ref) - b_commit = self.git.commit(b_ref) - commits.append(str(a_commit)) - commits.append(str(b_commit)) - else: - a_commit = self.git.commit(a_ref) - b_commit = self.git.head.commit - commits.append(str(a_commit)) - commits.append(str(b_commit)) + a_commit = self.git.git.rev_parse(a_ref, short=True) + b_commit = self.git.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 e: @@ -271,8 +282,8 @@ class Git(Base): 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 + """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 @@ -282,10 +293,10 @@ class Git(Base): dict - dictionary with keys: (a_tree, b_tree, a_ref, b_ref, equal) """ diff_dct = {DIFF_EQUAL: False} - trees, commit_refs = self._get_diff_trees(a_ref, b_ref) - diff_dct[DIFF_A_REF] = commit_refs[0] - diff_dct[DIFF_B_REF] = commit_refs[1] - if commit_refs[0] == commit_refs[1]: + 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 --git a/tests/func/test_diff.py b/tests/func/test_diff.py index 8407fc42e..ee2fd5f5e 100644 --- a/tests/func/test_diff.py +++ b/tests/func/test_diff.py @@ -28,13 +28,15 @@ class TestDiff(TestDvc): self.new_file = "new_test_file" self.create(self.new_file, self.new_file) self.dvc.add(self.new_file) - self.a_ref = self.git.head.commit + self.a_ref = self.git.git.rev_parse(self.git.head.commit, short=True) self.new_checksum = _get_checksum(self.dvc, self.new_file) self.git.index.add([self.new_file + ".dvc"]) self.git.index.commit("adds new_file") self.test_dct = { - diff.DIFF_A_REF: str(self.a_ref), - diff.DIFF_B_REF: str(self.git.head.commit), + diff.DIFF_A_REF: self.a_ref, + diff.DIFF_B_REF: self.git.git.rev_parse( + self.git.head.commit, short=True + ), diff.DIFF_LIST: [ { diff.DIFF_TARGET: self.new_file, @@ -48,8 +50,11 @@ class TestDiff(TestDvc): def test(self): out = self.dvc.scm.get_diff_trees(self.a_ref) self.assertFalse(out[diff.DIFF_EQUAL]) - self.assertEqual(str(self.a_ref), out[diff.DIFF_A_REF]) - self.assertEqual(str(self.git.head.commit), out[diff.DIFF_B_REF]) + self.assertEqual(self.a_ref, out[diff.DIFF_A_REF]) + self.assertEqual( + self.git.git.rev_parse(self.git.head.commit, short=True), + out[diff.DIFF_B_REF], + ) class TestDiffRepo(TestDiff): @@ -63,7 +68,7 @@ class TestDiffCmdLine(TestDiff): with patch("dvc.cli.diff.CmdDiff._show", autospec=True): with patch("dvc.repo.Repo", config="testing") as MockRepo: MockRepo.return_value.diff.return_value = "testing" - ret = main(["diff", "-t", self.new_file, str(self.a_ref)]) + ret = main(["diff", "-t", self.new_file, self.a_ref]) self.assertEqual(ret, 0) @@ -97,7 +102,9 @@ class TestDiffDir(TestDvc): self.dvc.add(self.DATA_DIR) self.git.index.add([self.DATA_DIR + ".dvc"]) self.git.index.commit("adds data_dir") - self.a_ref = str(self.dvc.scm.git.head.commit) + self.a_ref = self.git.git.rev_parse( + self.dvc.scm.git.head.commit, short=True + ) self.old_checksum = _get_checksum(self.dvc, self.DATA_DIR) self.new_file = os.path.join(self.DATA_SUB_DIR, diff.DIFF_NEW_FILE) self.create(self.new_file, self.new_file) @@ -109,8 +116,11 @@ class TestDiffDir(TestDvc): def test(self): out = self.dvc.scm.get_diff_trees(self.a_ref) self.assertFalse(out[diff.DIFF_EQUAL]) - self.assertEqual(str(self.a_ref), out[diff.DIFF_A_REF]) - self.assertEqual(str(self.git.head.commit), out[diff.DIFF_B_REF]) + self.assertEqual(self.a_ref, out[diff.DIFF_A_REF]) + self.assertEqual( + self.git.git.rev_parse(self.git.head.commit, short=True), + out[diff.DIFF_B_REF], + ) class TestDiffDirRepo(TestDiffDir): @@ -119,8 +129,10 @@ class TestDiffDirRepo(TestDiffDir): def test(self): result = self.dvc.diff(self.a_ref, target=self.DATA_DIR) test_dct = { - diff.DIFF_A_REF: str(self.a_ref), - diff.DIFF_B_REF: str(self.git.head.commit), + diff.DIFF_A_REF: self.git.git.rev_parse(self.a_ref, short=True), + diff.DIFF_B_REF: self.git.git.rev_parse( + self.git.head.commit, short=True + ), diff.DIFF_LIST: [ { diff.DIFF_CHANGE: 0, @@ -157,8 +169,8 @@ class TestDiffDirRepoDeletedFile(TestDiffDir): self.a_ref, b_ref=self.b_ref, target=self.DATA_DIR ) test_dct = { - diff.DIFF_A_REF: str(self.a_ref), - diff.DIFF_B_REF: str(self.b_ref), + diff.DIFF_A_REF: self.git.git.rev_parse(self.a_ref, short=True), + diff.DIFF_B_REF: self.git.git.rev_parse(self.b_ref, short=True), diff.DIFF_LIST: [ { diff.DIFF_CHANGE: 0,