Fix scan-remote-repo --branch logic (#250)

* Fix scan-remote-repo --branch logic A fresh-cloned repository does not have branch information for remote branches. We must follow the information in `remotes.origin.refs` instead of `branches` (which will have only the main/master branch). We add a new `is_remote` flag so the common code knows which strategy to employ. A side-effect appears to be that because HEAD appears in the reference list, we'll scan the default branch twice, but I can live with this. * Changelog update * Remove test duplicated in error Co-authored-by: Scott Bailey <[email protected]>
godaddy · Nov 2, 2021 · 9f13229 · 9f13229
1 parent 0622f4f
commit 9f13229
Show file tree

Hide file tree

Showing 6 changed files with 141 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,14 @@
+vx.y.z - TBD
+------------
+
+Bug fixes:
+
+* [#247](https://github.com/godaddy/tartufo/issues/247) -- the `--branch` option
+  for `scan-remote-repo` has not worked since v2.0.2. Versions v2.2.0 through
+  v2.7.0 failed silently (not scanning the branch, and returning no error).
+  Versions v2.8.0 and later claimed the branch did not exist, even if it did.
+  This option now works correctly.
+
 v2.9.0 - 19 October 2021
 ------------------------
 

diff --git a/tartufo/commands/scan_local_repo.py b/tartufo/commands/scan_local_repo.py
@@ -52,6 +52,7 @@ def main(
         branch=branch,
         fetch=fetch,
         include_submodules=include_submodules,
+        is_remote=False,
     )
     scanner = None
     try:

diff --git a/tartufo/commands/scan_remote_repo.py b/tartufo/commands/scan_remote_repo.py
@@ -59,6 +59,7 @@ def main(
         branch=branch,
         fetch=False,
         include_submodules=include_submodules,
+        is_remote=True,
     )
     repo_path: Optional[Path] = None
     if work_dir:

diff --git a/tartufo/scanner.py b/tartufo/scanner.py
@@ -632,24 +632,47 @@ def chunks(self) -> Generator[types.Chunk, None, None]:
         already_searched: Set[bytes] = set()
 
         try:
-            if self.git_options.branch:
-                # Single branch only
-                if self.git_options.fetch:
-                    self._repo.remotes.origin.fetch(self.git_options.branch)
-                unfiltered_branches = list(self._repo.branches)
-                branches = [
-                    x for x in unfiltered_branches if x.name == self.git_options.branch
-                ]
-
-                if len(branches) == 0:
-                    raise BranchNotFoundException(
-                        f"Branch {self.git_options.branch} was not found."
-                    )
+            if self.git_options.is_remote:
+                # Remote repository - local directory does not have branch info
+                # and we must follow remote references
+                unfiltered_branches = list(self._repo.remotes.origin.refs)
+                if self.git_options.branch:
+                    # Single branch only
+                    branches = [
+                        x
+                        for x in unfiltered_branches
+                        if x.name == f"origin/{self.git_options.branch}"
+                    ]
+
+                    if len(branches) == 0:
+                        raise BranchNotFoundException(
+                            f"Branch {self.git_options.branch} was not found."
+                        )
+                else:
+                    # Everything
+                    branches = unfiltered_branches
             else:
-                # Everything
-                if self.git_options.fetch:
-                    self._repo.remotes.origin.fetch()
-                branches = list(self._repo.branches)
+                # Local repository
+                if self.git_options.branch:
+                    # Single branch only
+                    if self.git_options.fetch:
+                        self._repo.remotes.origin.fetch(self.git_options.branch)
+                    unfiltered_branches = list(self._repo.branches)
+                    branches = [
+                        x
+                        for x in unfiltered_branches
+                        if x.name == self.git_options.branch
+                    ]
+
+                    if len(branches) == 0:
+                        raise BranchNotFoundException(
+                            f"Branch {self.git_options.branch} was not found."
+                        )
+                else:
+                    # Everything
+                    if self.git_options.fetch:
+                        self._repo.remotes.origin.fetch()
+                    branches = list(self._repo.branches)
         except git.GitCommandError as exc:
             raise types.GitRemoteException(exc.stderr.strip()) from exc
 

diff --git a/tartufo/types.py b/tartufo/types.py
@@ -54,12 +54,20 @@ class GlobalOptions:
 
 @dataclass
 class GitOptions:
-    __slots__ = ("since_commit", "max_depth", "branch", "fetch", "include_submodules")
+    __slots__ = (
+        "since_commit",
+        "max_depth",
+        "branch",
+        "fetch",
+        "include_submodules",
+        "is_remote",
+    )
     since_commit: Optional[str]
     max_depth: int
     branch: Optional[str]
     fetch: bool
     include_submodules: bool
+    is_remote: bool
 
 
 class IssueType(enum.Enum):

diff --git a/tests/test_git_repo_scanner.py b/tests/test_git_repo_scanner.py
@@ -148,6 +148,7 @@ class ChunkGeneratorTests(ScannerTestCase):
     def test_single_branch_is_loaded_if_specified(self, mock_repo: mock.MagicMock):
         self.git_options.branch = "foo"
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         mock_fetch = mock.MagicMock()
         mock_fetch.return_value = []
         mock_branch = mock.MagicMock()
@@ -167,6 +168,7 @@ def test_all_branches_are_loaded_if_specified(self, mock_repo: mock.MagicMock):
         mock_fetch.return_value = []
         mock_repo.return_value.remotes.origin.fetch = mock_fetch
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         test_scanner = scanner.GitRepoScanner(
             self.global_options, self.git_options, "."
         )
@@ -193,10 +195,11 @@ def test_explicit_exception_is_raised_if_fetch_fails(
 
     @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
     @mock.patch("git.Repo")
-    def test_all_branches_are_scanned_for_commits(
+    def test_all_local_branches_are_scanned_for_commits(
         self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
     ):
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         mock_repo.return_value.remotes.origin.fetch.return_value = ["foo", "bar"]
         mock_repo.return_value.branches = ["foo", "bar"]
         test_scanner = scanner.GitRepoScanner(
@@ -214,10 +217,36 @@ def test_all_branches_are_scanned_for_commits(
 
     @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
     @mock.patch("git.Repo")
-    def test_scan_all_branches_fetch_false(
+    def test_all_remote_branches_are_scanned_for_commits(
+        self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
+    ):
+        mock_foo = mock.MagicMock()
+        mock_foo.name = "origin/foo"
+        mock_bar = mock.MagicMock()
+        mock_bar.name = "origin/bar"
+
+        self.git_options.is_remote = True
+        mock_repo.return_value.remotes.origin.refs = [mock_foo, mock_bar]
+        test_scanner = scanner.GitRepoScanner(
+            self.global_options, self.git_options, "."
+        )
+        mock_iter_commits.return_value = []
+        for _ in test_scanner.chunks:
+            pass
+        mock_iter_commits.assert_has_calls(
+            (
+                mock.call(mock_repo.return_value, mock_foo),
+                mock.call(mock_repo.return_value, mock_bar),
+            )
+        )
+
+    @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
+    @mock.patch("git.Repo")
+    def test_scan_all_local_branches_fetch_false(
         self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
     ):
         self.git_options.fetch = False
+        self.git_options.is_remote = False
         mock_repo.return_value.remotes.origin.fetch.return_value = ["foo", "bar"]
         mock_repo.return_value.branches = ["foo", "bar"]
         test_scanner = scanner.GitRepoScanner(
@@ -259,10 +288,34 @@ def test_scan_single_branch_fetch_false(
 
     @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
     @mock.patch("git.Repo")
-    def test_scan_single_branch_throws_exception_when_branch_not_found(
+    def test_scan_single_remote_branch(
+        self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
+    ):
+        self.git_options.is_remote = True
+        self.git_options.branch = "bar"
+        mock_foo = mock.MagicMock()
+        mock_foo.name = "origin/foo"
+        mock_bar = mock.MagicMock()
+        mock_bar.name = "origin/bar"
+        mock_repo.return_value.remotes.origin.refs = [mock_foo, mock_bar]
+        test_scanner = scanner.GitRepoScanner(
+            self.global_options, self.git_options, "."
+        )
+        mock_iter_commits.return_value = []
+        for _ in test_scanner.chunks:
+            pass
+        mock_repo.return_value.remotes.branches.assert_not_called()
+        mock_iter_commits.assert_has_calls(
+            (mock.call(mock_repo.return_value, mock_bar),)
+        )
+
+    @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
+    @mock.patch("git.Repo")
+    def test_scan_single_local_branch_throws_exception_when_branch_not_found(
         self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
     ):
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         self.git_options.branch = "not-found"
         self.global_options.entropy = True
         mock_foo = mock.MagicMock()
@@ -278,12 +331,33 @@ def test_scan_single_branch_throws_exception_when_branch_not_found(
         mock_iter_commits.return_value = []
         self.assertRaises(types.BranchNotFoundException, test_scanner.scan)
 
+    @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
+    @mock.patch("git.Repo")
+    def test_scan_single_remote_branch_throws_exception_when_branch_not_found(
+        self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
+    ):
+        self.git_options.is_remote = True
+        self.git_options.branch = "not-found"
+        self.global_options.entropy = True
+        mock_foo = mock.MagicMock()
+        mock_foo.name = "foo"
+        mock_bar = mock.MagicMock()
+        mock_bar.name = "bar"
+        mock_repo.return_value.remotes.origin.refs = [mock_foo, mock_bar]
+
+        test_scanner = scanner.GitRepoScanner(
+            self.global_options, self.git_options, "."
+        )
+        mock_iter_commits.return_value = []
+        self.assertRaises(types.BranchNotFoundException, test_scanner.scan)
+
     @mock.patch("tartufo.scanner.GitRepoScanner._iter_branch_commits")
     @mock.patch("git.Repo")
     def test_scan_single_branch_fetch_true(
         self, mock_repo: mock.MagicMock, mock_iter_commits: mock.MagicMock
     ):
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         self.git_options.branch = "bar"
         mock_foo = mock.MagicMock()
         mock_foo.name = "foo"
@@ -312,6 +386,7 @@ def test_all_commits_are_scanned_for_files(
         mock_iter_commits: mock.MagicMock,
     ):
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         mock_repo.return_value.remotes.origin.fetch.return_value = ["foo"]
         mock_repo.return_value.branches = ["foo"]
         test_scanner = scanner.GitRepoScanner(
@@ -354,6 +429,7 @@ def test_all_files_are_yielded_as_chunks(
         mock_iter_commits: mock.MagicMock,
     ):
         self.git_options.fetch = True
+        self.git_options.is_remote = False
         mock_repo.return_value.remotes.origin.fetch.return_value = ["foo"]
         mock_repo.return_value.branches = ["foo"]
         test_scanner = scanner.GitRepoScanner(