From 68e4f1d98b5eab2d3029b1019795060c89c0875a Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Wed, 22 Jul 2026 18:06:15 -0500 Subject: [PATCH] Switch from pull to fetch and reset --- AddonCatalogCacheCreator.py | 62 +++++---- .../app/test_addon_catalog_cache_creator.py | 124 ++++++++++++++++++ 2 files changed, 162 insertions(+), 24 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index a0b968bf..a5b17fcc 100644 --- a/AddonCatalogCacheCreator.py +++ b/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 diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index b318acb7..addb867e 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/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())