Switch from pull to fetch and reset

AddonCatalogCacheCreator.py

@@ -481,9 +481,8 @@ def create_local_copy_of_single_addon_with_zip(
 
     def clone_or_update(self, name: str, url: str, branch: str) -> None:
         """If a directory called "name" exists, and it contains a subdirectory called .git,
-        then 'git fetch' is called; otherwise we use 'git clone' to make a bare, shallow
-        copy of the repo (in the normal case where minimal is True), or a normal clone,
-        if minimal is set to False."""
+        then the local copy is fetched and hard reset onto the requested ref; otherwise we use
+        'git clone' to make a shallow copy of the repo."""
 
         if not os.path.exists(os.path.join(os.getcwd(), name, ".git")):
             print(f"Cloning {url} to {name}", flush=True)
@@ -512,24 +511,7 @@ def clone_or_update(self, name: str, url: str, branch: str) -> None:
             old_dir = os.getcwd()
             os.chdir(os.path.join(old_dir, name))
             try:
-                # Determine if we are dealing with a tag, branch, or hash
-                git_ref_type = CacheWriter.determine_git_ref_type(name, url, branch)
-                command = ["git", "fetch"]
-                completed_process = subprocess.run(command)
-                if completed_process.returncode != 0:
-                    os.chdir(old_dir)
-                    raise RuntimeError(f"git fetch failed for {name}")
-                command = ["git", "checkout", branch, "--quiet"]
-                completed_process = subprocess.run(command)
-                if completed_process.returncode != 0:
-                    os.chdir(old_dir)
-                    raise RuntimeError(f"git checkout failed for {name} branch {branch}")
-                if git_ref_type == GitRefType.BRANCH:
-                    command = ["git", "merge", "--quiet"]
-                    completed_process = subprocess.run(command)
-                    if completed_process.returncode != 0:
-                        os.chdir(old_dir)
-                        raise RuntimeError(f"git merge failed for {name} branch {branch}")
+                CacheWriter.fetch_and_reset(name, url, branch)
             except RuntimeError as e:
                 # In the event of basically ANY error, delete the original and re-clone.
                 print(e)
@@ -578,21 +560,29 @@ def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> No
             cwd = os.getcwd()
             os.chdir(os.path.join(cwd, name))
             try:
-                subprocess.run(["git", "pull", "--depth=1"], check=True)
+                subprocess.run(
+                    ["git", "fetch", "--force", "--depth=1", "origin", branch],
+                    check=True,
+                    timeout=CLONE_TIMEOUT,
+                )
+                subprocess.run(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], check=True)
+                subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"], check=True)
             except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
                 self.clone_errors[name] = str(e)
                 print(f"ERROR: {e}")
             os.chdir(cwd)
 
     def add_to_sparse_clone(self, name: str, files: List[str]) -> None:
-        """Clones additional files to an existing sparse clone."""
+        """Checks out additional files in an existing sparse clone. The files are extracted from the
+        commit that is already checked out, so no network access is required."""
         cwd = os.getcwd()
         clone_path = os.path.join(cwd, name)
         os.chdir(clone_path)
         with open(".git/info/sparse-checkout", "a") as f:
             f.write("\n".join(files))
+            f.write("\n")  # So we are safe appending later
         try:
-            subprocess.run(["git", "pull", "--depth=1"], check=True)
+            subprocess.run(["git", "read-tree", "-m", "-u", "HEAD"], check=True)
         except subprocess.CalledProcessError as e:
             self.clone_errors[name] = str(e)
             print(f"ERROR: {e}")
@@ -620,6 +610,30 @@ def get_icon_from_metadata(metadata: addonmanager_metadata.Metadata) -> Optional
         the cache writer)."""
         return addonmanager_metadata.get_icon_from_metadata(metadata)
 
+    @staticmethod
+    def fetch_and_reset(name: str, url: str, branch: str) -> None:
+        """Update the git clone in the current working directory by fetching from its remote and
+        hard resetting onto the requested ref, discarding any local state. A RuntimeError is raised
+        if any of the git calls fails."""
+
+        try:
+            completed_process = subprocess.run(["git", "fetch", "--force"], timeout=CLONE_TIMEOUT)
+        except subprocess.TimeoutExpired:
+            raise RuntimeError(f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds")
+        if completed_process.returncode != 0:
+            raise RuntimeError(f"git fetch failed for {name}")
+
+        git_ref_type = CacheWriter.determine_git_ref_type(name, url, branch)
+        reset_target = f"origin/{branch}" if git_ref_type == GitRefType.BRANCH else branch
+
+        completed_process = subprocess.run(["git", "reset", "--hard", reset_target, "--quiet"])
+        if completed_process.returncode != 0:
+            raise RuntimeError(f"git reset failed for {name} ref {reset_target}")
+
+        completed_process = subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"])
+        if completed_process.returncode != 0:
+            raise RuntimeError(f"git clean failed for {name}")
+
     @staticmethod
     def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType:
         """Determine if the given branch, tag, or hash is a tag, branch, or hash. Returns the type

AddonManagerTest/app/test_addon_catalog_cache_creator.py

@@ -325,3 +325,127 @@ def get_catalog(self):
         mock_create_single_addon.assert_any_call("TestMod1", mock.ANY)
         mock_create_single_addon.assert_any_call("TestMod2", mock.ANY)
         self.assertEqual(3, mock_create_single_addon.call_count)
+
+
+class TestCacheWriterGitUpdate(TestCase):
+    """Tests of the git commands used to bring an existing local clone up to date."""
+
+    def setUp(self):
+        self.setUpPyfakefs()
+
+    @staticmethod
+    def issued_commands(mock_run):
+        """Return the list of command argument lists passed to the mocked subprocess.run."""
+        return [call.args[0] for call in mock_run.call_args_list]
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_with_branch(self, mock_ref_type, mock_run):
+        """A branch is reset onto the remote tracking branch, not merged."""
+        mock_ref_type.return_value = accc.GitRefType.BRANCH
+        mock_run.return_value.returncode = 0
+        accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+        commands = self.issued_commands(mock_run)
+        self.assertEqual(["git", "fetch", "--force"], commands[0])
+        self.assertIn(["git", "reset", "--hard", "origin/main", "--quiet"], commands)
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_run):
+        """A tag is reset onto the tag itself, which has no remote tracking equivalent."""
+        mock_ref_type.return_value = accc.GitRefType.TAG
+        mock_run.return_value.returncode = 0
+        accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "v1.0")
+        self.assertIn(["git", "reset", "--hard", "v1.0", "--quiet"], self.issued_commands(mock_run))
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_run):
+        """A hash is reset onto the hash itself."""
+        mock_ref_type.return_value = accc.GitRefType.HASH
+        mock_run.return_value.returncode = 0
+        accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "abc123")
+        self.assertIn(
+            ["git", "reset", "--hard", "abc123", "--quiet"], self.issued_commands(mock_run)
+        )
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_does_not_merge_or_pull(self, mock_ref_type, mock_run):
+        """Neither pull nor merge is used, so a force push on the remote cannot fail the update."""
+        mock_ref_type.return_value = accc.GitRefType.BRANCH
+        mock_run.return_value.returncode = 0
+        accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+        for command in self.issued_commands(mock_run):
+            self.assertNotIn("pull", command)
+            self.assertNotIn("merge", command)
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_removes_untracked_files(self, mock_ref_type, mock_run):
+        """Files left over from a previous run are removed."""
+        mock_ref_type.return_value = accc.GitRefType.BRANCH
+        mock_run.return_value.returncode = 0
+        accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+        self.assertIn(["git", "clean", "-x", "-f", "-d", "--quiet"], self.issued_commands(mock_run))
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    def test_fetch_and_reset_raises_when_fetch_fails(self, mock_run):
+        """A failed fetch is reported as a RuntimeError so that the caller can re-clone."""
+        mock_run.return_value.returncode = 1
+        with self.assertRaises(RuntimeError):
+            accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    def test_fetch_and_reset_raises_when_fetch_times_out(self, mock_run):
+        """A timed-out fetch is reported as a RuntimeError so that the caller can re-clone."""
+        mock_run.side_effect = accc.subprocess.TimeoutExpired("git fetch", 1)
+        with self.assertRaises(RuntimeError):
+            accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
+    def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run):
+        """A failed reset is reported as a RuntimeError so that the caller can re-clone."""
+        mock_ref_type.return_value = accc.GitRefType.BRANCH
+        mock_run.side_effect = [MagicMock(returncode=0), MagicMock(returncode=1)]
+        with self.assertRaises(RuntimeError):
+            accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset")
+    def test_clone_or_update_reclones_when_update_fails(self, mock_update, mock_run):
+        """If the update fails, the local copy is deleted and cloned again."""
+        mock_update.side_effect = RuntimeError("Update failed")
+        mock_run.return_value.returncode = 0
+        clone_path = os.path.join(os.getcwd(), "TestMod")
+        self.fake_fs().create_dir(os.path.join(clone_path, ".git"))
+        writer = accc.CacheWriter()
+        writer.clone_or_update("TestMod", "https://some.url", "main")
+        self.assertFalse(os.path.exists(clone_path))
+        self.assertIn("clone", self.issued_commands(mock_run)[0])
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run):
+        """An existing sparse clone is updated by fetching and resetting, not by pulling."""
+        mock_run.return_value.returncode = 0
+        self.fake_fs().create_dir(os.path.join(os.getcwd(), "TestMod", ".git"))
+        writer = accc.CacheWriter()
+        writer.sparse_clone("TestMod", "https://some.url", "main", ["package.xml"])
+        commands = self.issued_commands(mock_run)
+        self.assertEqual(["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0])
+        self.assertIn(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], commands)
+        self.assertEqual({}, writer.clone_errors)
+
+    @patch("AddonCatalogCacheCreator.subprocess.run")
+    def test_add_to_sparse_clone_checks_out_without_network_access(self, mock_run):
+        """New sparse checkout entries are taken from the commit that is already local."""
+        mock_run.return_value.returncode = 0
+        sparse_file = os.path.join(os.getcwd(), "TestMod", ".git", "info", "sparse-checkout")
+        self.fake_fs().create_file(sparse_file, contents="package.xml\n")
+        writer = accc.CacheWriter()
+        writer.add_to_sparse_clone("TestMod", ["icon.svg"])
+        commands = self.issued_commands(mock_run)
+        self.assertEqual([["git", "read-tree", "-m", "-u", "HEAD"]], commands)
+        with open(sparse_file, encoding="utf-8") as f:
+            self.assertEqual("package.xml\nicon.svg\n", f.read())