Repository: Justintime50/github-archive

Issue:
Error Removing Git Repos that Timed Out On Windows
I'm using below command line

   github-archive.exe --https -s [user] -t [key] -l c:\git-archive --clone

What happens for me:

- some very large starred repositories (https://github.com/jgraph/drawio) time out - there is an error during clone
- this leaves `jgraph/drawio/.git` folder, with `.git` being the only item under `drawio`
- I cannot tell if that `.git` is consistent and valid, don't know how to check
- the next time I run `github-archive`, it assumes that this repo is OK

I probably wouldn't want to keep `drawio` locally, because local `.git` folder was more than 700 MB, but it doesn't look quite right.

And can `github-archive` "update" those cloned repositories? Is it possible to have up-to-date copy of those repositories?

Thanks!

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a7f15f8..ebd6070 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## NEXT RELEASE
 
+* Switches from the `threading` package to the `concurrent.futures` package allowing us to return values from each individual thread (in this case, the names of failed git assets)
+    * Fixes a long-standing bug where git repos that failed to clone would remain in the archive on Windows. This meant that recloning was impossible due to the repo existing and pulling was not possible because the repo wasn't yet fully initialized. The tool now has a proper cleanup step after everything has run that is compatible with both Unix and Windows environments (closes #38)
 * Small adjustment to `include` and `exclude` help verbage used to be more clear they are optional filters
 
 ## v4.4.0 (2021-02-18)
diff --git a/github_archive/archive.py b/github_archive/archive.py
index 8af491c..9953e4a 100644
--- a/github_archive/archive.py
+++ b/github_archive/archive.py
@@ -1,9 +1,10 @@
 import os
 import shutil
+import stat
 import subprocess
+from concurrent.futures import ALL_COMPLETED, ThreadPoolExecutor, wait
 from datetime import datetime
-from threading import BoundedSemaphore, Thread
-from typing import List
+from typing import List, Optional, Union
 
 import woodchips
 from github import Gist, Github, Repository
@@ -76,6 +77,7 @@ class GithubArchive:
         logger = woodchips.get(LOGGER_NAME)
         logger.info('# GitHub Archive started...\n')
         start_time = datetime.now()
+        failed_repo_dirs = []
 
         # Personal (includes personal authenticated items)
         if self.token and self.authenticated_user_in_users and self.users:
@@ -87,10 +89,12 @@ class GithubArchive:
                 self.view_repos(personal_repos)
             if self.clone:
                 logger.info('# Cloning missing personal repos...\n')
-                self.iterate_repos_to_archive(personal_repos, CLONE_OPERATION)
+                failed_repos = self.iterate_repos_to_archive(personal_repos, CLONE_OPERATION)
+                if any(failed_repos):
+                    failed_repo_dirs.extend(failed_repos)
             if self.pull:
                 logger.info('# Pulling changes to personal repos...\n')
-                self.iterate_repos_to_archive(personal_repos, PULL_OPERATION)
+                _ = self.iterate_repos_to_archive(personal_repos, PULL_OPERATION)
 
             # We remove the authenticated user from the list so that we don't double pull their
             # repos for the `users` logic.
@@ -106,10 +110,12 @@ class GithubArchive:
                 self.view_repos(user_repos)
             if self.clone:
                 logger.info('# Cloning missing user repos...\n')
-                self.iterate_repos_to_archive(user_repos, CLONE_OPERATION)
+                failed_repos = self.iterate_repos_to_archive(user_repos, CLONE_OPERATION)
+                if any(failed_repos):
+                    failed_repo_dirs.extend(failed_repos)
             if self.pull:
                 logger.info('# Pulling changes to user repos...\n')
-                self.iterate_repos_to_archive(user_repos, PULL_OPERATION)
+                _ = self.iterate_repos_to_archive(user_repos, PULL_OPERATION)
 
         # Orgs
         if self.orgs:
@@ -121,40 +127,55 @@ class GithubArchive:
                 self.view_repos(org_repos)
             if self.clone:
                 logger.info('# Cloning missing org repos...\n')
-                self.iterate_repos_to_archive(org_repos, CLONE_OPERATION)
+                failed_repos = self.iterate_repos_to_archive(org_repos, CLONE_OPERATION)
+                if any(failed_repos):
+                    failed_repo_dirs.extend(failed_repos)
             if self.pull:
                 logger.info('# Pulling changes to org repos...\n')
-                self.iterate_repos_to_archive(org_repos, PULL_OPERATION)
+                _ = self.iterate_repos_to_archive(org_repos, PULL_OPERATION)
+
+        # Stars
+        if self.stars:
+            logger.info('# Making API call to GitHub for starred repos...\n')
+            starred_repos = self.get_all_git_assets(STAR_CONTEXT)
+
+            if self.view:
+                logger.info('# Viewing stars...\n')
+                self.view_repos(starred_repos)
+            if self.clone:
+                logger.info('# Cloning missing starred repos...\n')
+                failed_repos = self.iterate_repos_to_archive(starred_repos, CLONE_OPERATION)
+                if any(failed_repos):
+                    failed_repo_dirs.extend(failed_repos)
+            if self.pull:
+                logger.info('# Pulling changes to starred repos...\n')
+                _ = self.iterate_repos_to_archive(starred_repos, PULL_OPERATION)
+
+        if failed_repo_dirs:
+            logger.info('Cleaning up repos...\n')
+            self.remove_failed_dirs('repos', failed_repo_dirs)
 
         # Gists
         if self.gists:
             logger.info('# Making API call to GitHub for gists...\n')
             gists = self.get_all_git_assets(GIST_CONTEXT)
+            failed_gist_dirs = []
 
             if self.view:
                 logger.info('# Viewing gists...\n')
                 self.view_gists(gists)
             if self.clone:
                 logger.info('# Cloning missing gists...\n')
-                self.iterate_gists_to_archive(gists, CLONE_OPERATION)
+                failed_gists = self.iterate_gists_to_archive(gists, CLONE_OPERATION)
+                if any(failed_gists):
+                    failed_gist_dirs.extend(failed_gists)
             if self.pull:
                 logger.info('# Pulling changes to gists...\n')
-                self.iterate_gists_to_archive(gists, PULL_OPERATION)
+                _ = self.iterate_gists_to_archive(gists, PULL_OPERATION)
 
-        # Stars
-        if self.stars:
-            logger.info('# Making API call to GitHub for starred repos...\n')
-            starred_repos = self.get_all_git_assets(STAR_CONTEXT)
-
-            if self.view:
-                logger.info('# Viewing stars...\n')
-                self.view_repos(starred_repos)
-            if self.clone:
-                logger.info('# Cloning missing starred repos...\n')
-                self.iterate_repos_to_archive(starred_repos, CLONE_OPERATION)
-            if self.pull:
-                logger.info('# Pulling changes to starred repos...\n')
-                self.iterate_repos_to_archive(starred_repos, PULL_OPERATION)
+            if failed_gist_dirs:
+                logger.info('Cleaning up gists...\n')
+                self.remove_failed_dirs('gists', failed_gist_dirs)
 
         execution_time = f'Execution time: {datetime.now() - start_time}.'
         finish_message = f'GitHub Archive complete! {execution_time}\n'
@@ -207,7 +228,7 @@ class GithubArchive:
     def authenticated_user_in_users(self) -> bool:
         return self.authenticated_user.login.lower() in self.users
 
-    def get_all_git_assets(self, context: str) -> List:
+    def get_all_git_assets(self, context: str) -> List[Union[Repository.Repository, Gist.Gist]]:
         """Retrieve a list of lists via API of git assets (repos, gists) of the
         specified owner(s) (users, orgs). Return a sorted, flat, sorted list of git assets.
         """
@@ -250,13 +271,13 @@ class GithubArchive:
 
         return final_sorted_list
 
-    def iterate_repos_to_archive(self, repos: List[Repository.Repository], operation: str):
+    def iterate_repos_to_archive(self, repos: List[Repository.Repository], operation: str) -> List[Optional[str]]:
         """Iterate over each repository and start a thread if it can be archived.
 
         We ignore repos not in the include or in the exclude list if either are present.
         """
         logger = woodchips.get(LOGGER_NAME)
-        thread_limiter = BoundedSemaphore(self.threads)
+        pool = ThreadPoolExecutor(self.threads)
         thread_list = []
 
         for repo in repos:
@@ -267,46 +288,42 @@ class GithubArchive:
             ):
                 repo_owner_username = repo.owner.login.lower()
                 repo_path = os.path.join(self.location, 'repos', repo_owner_username, repo.name)
-                repo_thread = Thread(
-                    target=self.archive_repo,
-                    args=(
-                        thread_limiter,
-                        repo,
-                        repo_path,
-                        operation,
-                    ),
+                thread_list.append(
+                    pool.submit(
+                        self.archive_repo,
+                        repo=repo,
+                        repo_path=repo_path,
+                        operation=operation,
+                    )
                 )
-                thread_list.append(repo_thread)
-                repo_thread.start()
             else:
                 logger.debug(f'{repo.name} skipped due to include/exclude filtering')
 
-        # Wait for the number of threads in thread_limiter to finish before moving on
-        for thread in thread_list:
-            thread.join()
+        wait(thread_list, return_when=ALL_COMPLETED)
+        failed_repos = [repo.result() for repo in thread_list if repo.result()]
+
+        return failed_repos
 
-    def iterate_gists_to_archive(self, gists: List[Gist.Gist], operation: str):
+    def iterate_gists_to_archive(self, gists: List[Gist.Gist], operation: str) -> List[Optional[str]]:
         """Iterate over each gist and start a thread if it can be archived."""
-        thread_limiter = BoundedSemaphore(self.threads)
+        pool = ThreadPoolExecutor(self.threads)
         thread_list = []
 
         for gist in gists:
             gist_path = os.path.join(self.location, 'gists', gist.id)
-            gist_thread = Thread(
-                target=self.archive_gist,
-                args=(
-                    thread_limiter,
-                    gist,
-                    gist_path,
-                    operation,
-                ),
+            thread_list.append(
+                pool.submit(
+                    self.archive_gist,
+                    gist=gist,
+                    gist_path=gist_path,
+                    operation=operation,
+                )
             )
-            thread_list.append(gist_thread)
-            gist_thread.start()
 
-        # Wait for the number of threads in thread_limiter to finish before moving on
-        for thread in thread_list:
-            thread.join()
+        wait(thread_list, return_when=ALL_COMPLETED)
+        failed_gists = [gist.result() for gist in thread_list if gist.result()]
+
+        return failed_gists
 
     def view_repos(self, repos: List[Repository.Repository]):
         """View a list of repos that will be cloned/pulled."""
@@ -324,11 +341,14 @@ class GithubArchive:
             gist_id = f'{gist.owner.login}/{gist.id}'
             logger.info(gist_id)
 
-    def archive_repo(
-        self, thread_limiter: BoundedSemaphore, repo: Repository.Repository, repo_path: str, operation: str
-    ):
-        """Clone and pull repos based on the operation passed."""
+    def archive_repo(self, repo: Repository.Repository, repo_path: str, operation: str) -> Optional[str]:
+        """Clone and pull repos based on the operation passed.
+
+        We return the name of the repo if its git operation fails, otherwise return None.
+        """
         logger = woodchips.get(LOGGER_NAME)
+        failed_repo = None
+        full_repo_name = os.path.join(repo.owner.login, repo.name)  # We use a path here to properly remove failed dirs
 
         if (os.path.exists(repo_path) and operation == CLONE_OPERATION) or (
             not os.path.exists(repo_path) and operation == PULL_OPERATION
@@ -343,10 +363,10 @@ class GithubArchive:
                 commands.update({CLONE_OPERATION: ['git', 'clone', repo.html_url, repo_path]})
             else:
                 commands.update({CLONE_OPERATION: ['git', 'clone', repo.ssh_url, repo_path]})
+
             git_command = commands[operation]
 
             try:
-                thread_limiter.acquire()
                 subprocess.run(
                     git_command,
                     stdout=subprocess.DEVNULL,
@@ -355,19 +375,24 @@ class GithubArchive:
                     check=True,
                     timeout=self.timeout,
                 )
-                logger.info(f'Repo: {repo.owner.login}/{repo.name} {operation} success!')
+                logger.info(f'Repo: {full_repo_name} {operation} success!')
             except subprocess.TimeoutExpired:
                 logger.error(f'Git operation timed out archiving {repo.name}.')
-                self.remove_failed_dir(repo_path)
+                failed_repo = full_repo_name
             except subprocess.CalledProcessError as error:
                 logger.error(f'Failed to {operation} {repo.name}\n{error}')
-                self.remove_failed_dir(repo_path)
-            finally:
-                thread_limiter.release()
+                failed_repo = full_repo_name
+
+        return failed_repo
+
+    def archive_gist(self, gist: Gist.Gist, gist_path: str, operation: str) -> Optional[str]:
+        """Clone and pull gists based on the operation passed.
 
-    def archive_gist(self, thread_limiter: BoundedSemaphore, gist: Gist.Gist, gist_path: str, operation: str):
-        """Clone and pull gists based on the operation passed."""
+        We return the name of the gist if its git operation fails, otherwise return None.
+        """
         logger = woodchips.get(LOGGER_NAME)
+        failed_gist = None
+        full_gist_id = os.path.join(gist.owner.login, gist.id)  # We use a path here to properly remove failed dirs
 
         if (os.path.exists(gist_path) and operation == CLONE_OPERATION) or (
             not os.path.exists(gist_path) and operation == PULL_OPERATION
@@ -381,7 +406,6 @@ class GithubArchive:
             git_command = commands[operation]
 
             try:
-                thread_limiter.acquire()
                 subprocess.run(
                     git_command,
                     stdout=subprocess.DEVNULL,
@@ -390,19 +414,31 @@ class GithubArchive:
                     check=True,
                     timeout=self.timeout,
                 )
-                logger.info(f'Gist: {gist.owner.login}/{gist.id} {operation} success!')
+                logger.info(f'Gist: {full_gist_id} {operation} success!')
             except subprocess.TimeoutExpired:
                 logger.error(f'Git operation timed out archiving {gist.id}.')
-                self.remove_failed_dir(gist_path)
+                failed_gist = full_gist_id
             except subprocess.CalledProcessError as error:
                 logger.error(f'Failed to {operation} {gist.id}\n{error}')
-                self.remove_failed_dir(gist_path)
-            finally:
-                thread_limiter.release()
+                failed_gist = full_gist_id
+
+        return failed_gist
 
-    def remove_failed_dir(self, path: str):
+    def remove_failed_dirs(self, dirs_location: str, failed_dirs: List[str]):
         """Removes a directory if it fails a git operation due to
         timing out or other errors so it can be retried on the next run.
         """
-        if os.path.exists(path):
-            shutil.rmtree(path)
+        logger = woodchips.get(LOGGER_NAME)
+
+        def make_dir_writable(function, path, exception):
+            """The `.git` folder on Windows cannot be gracefully removed due to being read-only,
+            so we make the directory writable on a failure and retry the original function.
+            """
+            os.chmod(path, stat.S_IWRITE)
+            function(path)
+
+        for directory in set(failed_dirs):
+            path = os.path.join(self.location, dirs_location, directory)
+            if os.path.exists(path):
+                logger.debug(f'Removing {directory} due to a failed git operation...')
+                shutil.rmtree(path, onerror=make_dir_writable)
diff --git a/github_archive/constants.py b/github_archive/constants.py
index 8409e38..d923dbd 100644
--- a/github_archive/constants.py
+++ b/github_archive/constants.py
@@ -1,6 +1,6 @@
 import os
 
 DEFAULT_BASE_URL = 'https://api.github.com'
-DEFAULT_LOCATION = os.path.expanduser('~/github-archive')
+DEFAULT_LOCATION = os.path.expanduser(os.path.join('~', 'github-archive'))
 DEFAULT_NUM_THREADS = 10
 DEFAULT_TIMEOUT = 300


Test patch:
diff --git a/test/unit/test_archive.py b/test/unit/test_archive.py
index edd4cea..a9a076d 100644
--- a/test/unit/test_archive.py
+++ b/test/unit/test_archive.py
@@ -1,5 +1,4 @@
 import subprocess
-from threading import BoundedSemaphore
 from unittest.mock import patch
 
 import pytest
@@ -7,7 +6,6 @@ import pytest
 from github_archive import GithubArchive
 from github_archive.archive import (
     CLONE_OPERATION,
-    DEFAULT_NUM_THREADS,
     GIST_CONTEXT,
     ORG_CONTEXT,
     PULL_OPERATION,
@@ -15,12 +13,6 @@ from github_archive.archive import (
 )
 
 
-def mock_thread_limiter():
-    thread_limiter = BoundedSemaphore(DEFAULT_NUM_THREADS)
-
-    return thread_limiter
-
-
 @patch('github_archive.archive.Github.get_user')
 @patch('github_archive.archive.GithubArchive.authenticated_user_in_users', return_value=True)
 @patch('github_archive.archive.GithubArchive.view_repos')
@@ -320,7 +312,7 @@ def test_authenticated_user_in_users(mock_get_user):
 
 
 @patch('github_archive.archive.Github.get_user')
-def test_get_all_git_assets(mock_get_user):
+def test_get_all_git_assets_repos(mock_get_user):
     GithubArchive(
         users='justintime50',
     ).get_all_git_assets(USER_CONTEXT)
@@ -328,6 +320,16 @@ def test_get_all_git_assets(mock_get_user):
     mock_get_user.assert_called_once()
 
 
+@patch('github_archive.archive.Github.get_user')
+def test_get_all_git_assets_gists(mock_get_user):
+    # TODO: This test doesn't yet do anything because we need to populate the `git_assets` list with mock data
+    GithubArchive(
+        gists='justintime50',
+    ).get_all_git_assets(GIST_CONTEXT)
+
+    mock_get_user.assert_called_once()
+
+
 @patch('github_archive.archive.Github')
 @patch('github_archive.archive.Github.get_repos')
 @patch('github_archive.archive.Github.get_user')
@@ -455,7 +457,7 @@ def test_archive_repo_success(mock_logger, mock_subprocess, mock_git_asset):
     operation = CLONE_OPERATION
     message = f'Repo: {mock_git_asset.owner.login}/{mock_git_asset.name} {operation} success!'
     github_archive = GithubArchive()
-    github_archive.archive_repo(mock_thread_limiter(), mock_git_asset, 'mock/path', operation)
+    github_archive.archive_repo(mock_git_asset, 'mock/path', operation)
 
     mock_subprocess.assert_called()
     mock_logger.assert_called_once_with(message)
@@ -470,48 +472,43 @@ def test_archive_repo_use_https_success(mock_logger, mock_subprocess, mock_git_a
     github_archive = GithubArchive(
         use_https=True,
     )
-    github_archive.archive_repo(mock_thread_limiter(), mock_git_asset, 'mock/path', operation)
+    github_archive.archive_repo(mock_git_asset, 'mock/path', operation)
 
     mock_subprocess.assert_called()
     mock_logger.assert_called_once_with(message)
 
 
+@patch('os.path.join', return_value='mock_user/mock_repo')
 @patch('os.path.exists', return_value=True)
 @patch('subprocess.run')
 @patch('logging.Logger.info')
-def test_archive_repo_clone_exists(mock_logger, mock_subprocess, mock_git_asset):
+def test_archive_repo_clone_exists(mock_logger, mock_subprocess, mock_path_exists, mock_path_join, mock_git_asset):
     operation = CLONE_OPERATION
     github_archive = GithubArchive()
-    github_archive.archive_repo(mock_thread_limiter(), mock_git_asset, 'github_archive', operation)
+    github_archive.archive_repo(mock_git_asset, 'github_archive', operation)
 
     mock_subprocess.assert_not_called()
 
 
-@patch('shutil.rmtree')
 @patch('subprocess.run', side_effect=subprocess.TimeoutExpired(cmd='subprocess.run', timeout=0.1))
 @patch('logging.Logger.error')
-def test_archive_repo_timeout_exception(mock_logger, mock_subprocess, mock_remove_dir, mock_git_asset):
+def test_archive_repo_timeout_exception(mock_logger, mock_subprocess, mock_git_asset):
     operation = CLONE_OPERATION
     message = f'Git operation timed out archiving {mock_git_asset.name}.'
     github_archive = GithubArchive()
-    github_archive.archive_repo(mock_thread_limiter(), mock_git_asset, 'mock/path', operation)
+    github_archive.archive_repo(mock_git_asset, 'mock/path', operation)
 
     mock_logger.assert_called_with(message)
-    # TODO: This is difficult to mock because it must not exist and then later exist in the same function
-    # mock_remove_dir.assert_called_once_with('mock/path')
 
 
-@patch('shutil.rmtree')
 @patch('subprocess.run', side_effect=subprocess.CalledProcessError(returncode=1, cmd='subprocess.run'))
 @patch('logging.Logger.error')
-def test_archive_repo_called_process_error(mock_logger, mock_subprocess, mock_remove_dir, mock_git_asset):
+def test_archive_repo_called_process_error(mock_logger, mock_subprocess, mock_git_asset):
     operation = PULL_OPERATION
     github_archive = GithubArchive()
-    github_archive.archive_repo(mock_thread_limiter(), mock_git_asset, 'github_archive', operation)
+    github_archive.archive_repo(mock_git_asset, 'github_archive', operation)
 
     mock_logger.assert_called_once()
-    # TODO: This is difficult to mock because it must not exist and then later exist in the same function
-    # mock_remove_dir.assert_called_once_with('mock/path')
 
 
 @patch('subprocess.run')
@@ -521,7 +518,7 @@ def test_archive_gist_success(mock_logger, mock_subprocess, mock_git_asset):
     operation = CLONE_OPERATION
     message = f'Gist: {mock_git_asset.owner.login}/{mock_git_asset.id} {operation} success!'
     github_archive = GithubArchive()
-    github_archive.archive_gist(mock_thread_limiter(), mock_git_asset, 'mock/path', operation)
+    github_archive.archive_gist(mock_git_asset, 'mock/path', operation)
 
     mock_subprocess.assert_called()
     mock_logger.assert_called_once_with(message)
@@ -533,33 +530,50 @@ def test_archive_gist_success(mock_logger, mock_subprocess, mock_git_asset):
 def test_archive_gist_clone_exists(mock_logger, mock_subprocess, mock_path_exists, mock_git_asset):
     operation = CLONE_OPERATION
     github_archive = GithubArchive()
-    github_archive.archive_gist(mock_thread_limiter(), mock_git_asset, 'github_archive', operation)
+    github_archive.archive_gist(mock_git_asset, 'github_archive', operation)
 
     mock_subprocess.assert_not_called()
 
 
-@patch('shutil.rmtree')
 @patch('subprocess.run', side_effect=subprocess.TimeoutExpired(cmd='subprocess.run', timeout=0.1))
 @patch('logging.Logger.error')
-def test_archive_gist_timeout_exception(mock_logger, mock_subprocess, mock_remove_dir, mock_git_asset):
+def test_archive_gist_timeout_exception(mock_logger, mock_subprocess, mock_git_asset):
     operation = CLONE_OPERATION
     message = f'Git operation timed out archiving {mock_git_asset.id}.'
     github_archive = GithubArchive()
-    github_archive.archive_gist(mock_thread_limiter(), mock_git_asset, 'mock/path', operation)
+    github_archive.archive_gist(mock_git_asset, 'mock/path', operation)
 
     mock_logger.assert_called_with(message)
-    # TODO: This is difficult to mock because it must not exist and then later exist in the same function
-    # mock_remove_dir.assert_called_once_with('mock/path')
 
 
-@patch('shutil.rmtree')
 @patch('subprocess.run', side_effect=subprocess.CalledProcessError(returncode=1, cmd='subprocess.run'))
 @patch('logging.Logger.error')
-def test_archive_gist_called_process_error(mock_logger, mock_subprocess, mock_remove_dir, mock_git_asset):
+def test_archive_gist_called_process_error(mock_logger, mock_subprocess, mock_git_asset):
     operation = PULL_OPERATION
     github_archive = GithubArchive()
-    github_archive.archive_gist(mock_thread_limiter(), mock_git_asset, 'github_archive', operation)
+    github_archive.archive_gist(mock_git_asset, 'github_archive', operation)
 
     mock_logger.assert_called()
-    # TODO: This is difficult to mock because it must not exist and then later exist in the same function
-    # mock_remove_dir.assert_called_once_with('mock/path')
+
+
+@patch('os.path.exists', return_value=True)
+@patch('shutil.rmtree')
+@patch('logging.Logger.debug')
+def test_remove_failed_dirs(mock_logger, mock_remove_dir, mock_path_exists):
+    """Tests that we remove failed dirs correctly."""
+    GithubArchive().remove_failed_dirs('mock/path', ['mock_dir'])
+
+    mock_logger.assert_called_once()
+    mock_remove_dir.assert_called_once()
+
+
+@patch('os.path.exists', return_value=True)
+@patch('os.chmod')
+@patch('logging.Logger.debug')
+def test_remove_failed_dirs_on_error(mock_logger, mock_chmod, mock_path_exists):
+    """Tests that we remove failed dirs correctly when there's an error such as read-only `.git` folders on Windows."""
+    with pytest.raises(OSError):
+        GithubArchive().remove_failed_dirs('mock/path', ['mock_dir'])
+
+    mock_logger.assert_called_once()
+    mock_chmod.assert_called_once()