diff --git a/AddonManagerOptions.py b/AddonManagerOptions.py index a592bfda..feb001d8 100644 --- a/AddonManagerOptions.py +++ b/AddonManagerOptions.py @@ -474,7 +474,8 @@ def _add_custom_repo_clicked(self): """Callback: show the Add custom repo dialog""" dlg = CustomRepositoryDialog() url, branch = dlg.exec() - if url and branch: + if url: + # An empty branch is allowed: it means the repository's default branch self.table_model.appendData(url, branch) def _remove_custom_repo_clicked(self): @@ -494,7 +495,7 @@ def _row_double_clicked(self, item): dlg.dialog.urlLineEdit.setText(self.table_model.data(url_index)) dlg.dialog.branchLineEdit.setText(self.table_model.data(branch_index)) url, branch = dlg.exec() - if url and branch: + if url: self.table_model.setData(url_index, url) self.table_model.setData(branch_index, branch) @@ -515,17 +516,14 @@ def load_model(self): pref_entry: str = self.pref.GetString("CustomRepositories", "") # The entry is saved as a space- and newline-delimited text block: break it into its - # constituent parts + # constituent parts. A line with no branch means the repository's default branch. lines = pref_entry.split("\n") self.model = [] for line in lines: if not line: continue split_data = line.split() - if len(split_data) > 1: - branch = split_data[1] - else: - branch = "master" + branch = split_data[1] if len(split_data) > 1 else "" url = split_data[0] self.model.append([url, branch]) @@ -533,7 +531,8 @@ def save_model(self): """Save the data into a preferences entry""" entry = "" for row in self.model: - entry += f"{row[0]} {row[1]}\n" + url, branch = row[0], row[1] + entry += f"{url} {branch}\n" if branch else f"{url}\n" self.pref.SetString("CustomRepositories", entry) def rowCount(self, parent: QtCore.QModelIndex = QtCore.QModelIndex()) -> int: @@ -623,12 +622,16 @@ def __init__(self): os.path.join(os.path.dirname(__file__), "AddonManagerOptions_AddCustomRepository.ui") ) self.dialog.setObjectName("AddonManager_AddCustomRepositoryDialog") + self.dialog.branchLineEdit.setPlaceholderText( + translate("AddonsInstaller", "Leave blank to use the repository's default branch") + ) def exec(self): - """Run the dialog as a modal, and return either None or a tuple of (url,branch)""" + """Run the dialog as a modal, and return either None or a tuple of (url,branch). The branch + may be empty, meaning the repository's default branch, whatever that turns out to be.""" result = self.dialog.exec() if result == QtWidgets.QDialog.Accepted: - url = self.dialog.urlLineEdit.text() - branch = self.dialog.branchLineEdit.text() + url = self.dialog.urlLineEdit.text().strip() + branch = self.dialog.branchLineEdit.text().strip() return url, branch return None, None diff --git a/AddonManagerTest/app/test_workers_startup.py b/AddonManagerTest/app/test_workers_startup.py index 84d13c92..bb180207 100644 --- a/AddonManagerTest/app/test_workers_startup.py +++ b/AddonManagerTest/app/test_workers_startup.py @@ -28,6 +28,7 @@ import addonmanager_utilities as utils import addonmanager_workers_startup from Addon import Addon +from addonmanager_git import GitFailed from PySideWrapper import QtCore @@ -355,6 +356,142 @@ def test_a_host_that_changed_software_is_identified_again(self, mock_network_man self.assertEqual("My Custom Addon", addon.display_name) +class TestCustomAddonDefaultBranch(unittest.TestCase): + """A custom repository whose branch the user did not name. Its default branch has to be worked + out: guessing "master" has been wrong by default since GitHub renamed it in 2020.""" + + _URL = "https://git.example.com/user/addon" + + def setUp(self): + # Looking for the branch identifies the git host as a side effect, and that is remembered + utils.forget_git_hosts() + self.addCleanup(utils.forget_git_hosts) + + def _worker(self) -> addonmanager_workers_startup.CreateAddonListWorker: + worker = addonmanager_workers_startup.CreateAddonListWorker() + worker.current_thread = MagicMock() + worker.current_thread.isInterruptionRequested.return_value = False + return worker + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.initialize_git") + def test_git_is_asked_for_the_default_branch(self, mock_initialize_git, _): + """Git knows the answer exactly, whatever the branch is called and whatever the host is.""" + mock_initialize_git.return_value.default_branch.return_value = "development" + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("development", branch) + mock_initialize_git.return_value.default_branch.assert_called_once_with(self._URL) + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.NetworkManager.AM_NETWORK_MANAGER") + @patch("addonmanager_workers_startup.initialize_git") + def test_without_git_the_usual_names_are_tried(self, mock_initialize_git, mock_network, _): + """Without git, the repository is asked for a package.xml on each of the usual names.""" + mock_initialize_git.return_value = None + mock_network.blocking_get_with_retries.side_effect = _serve_urls( + {f"{self._URL}/-/raw/main/package.xml": _package_xml("1.0.0")} + ) + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("main", branch) + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.NetworkManager.AM_NETWORK_MANAGER") + @patch("addonmanager_workers_startup.initialize_git") + def test_without_git_master_is_used_if_main_does_not_exist( + self, mock_initialize_git, mock_network, _ + ): + mock_initialize_git.return_value = None + mock_network.blocking_get_with_retries.side_effect = _serve_urls( + {f"{self._URL}/-/raw/master/package.xml": _package_xml("1.0.0")} + ) + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("master", branch) + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.NetworkManager.AM_NETWORK_MANAGER") + @patch("addonmanager_workers_startup.initialize_git") + def test_git_failing_falls_back_to_the_usual_names(self, mock_initialize_git, mock_network, _): + """Git is there but cannot answer, so the usual names are tried instead of giving up.""" + mock_initialize_git.return_value.default_branch.side_effect = GitFailed("no such repo") + mock_network.blocking_get_with_retries.side_effect = _serve_urls( + {f"{self._URL}/-/raw/main/package.xml": _package_xml("1.0.0")} + ) + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("main", branch) + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.NetworkManager.AM_NETWORK_MANAGER") + @patch("addonmanager_workers_startup.initialize_git") + def test_nothing_works_so_the_old_default_is_used(self, mock_initialize_git, mock_network, _): + """When the branch cannot be worked out at all, the addon still gets the branch it has + always been given, and the user is told to name one themselves.""" + mock_initialize_git.return_value = None + mock_network.blocking_get_with_retries.side_effect = _serve_urls({}) + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("master", branch) + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.NetworkManager.AM_NETWORK_MANAGER") + @patch("addonmanager_workers_startup.initialize_git") + def test_a_sign_in_page_is_not_mistaken_for_a_branch( + self, mock_initialize_git, mock_network, _ + ): + """A git host that requires a login answers every request with a 200 and a sign-in page, + which must not be taken as proof that the branch exists.""" + mock_initialize_git.return_value = None + mock_network.blocking_get_with_retries.side_effect = lambda url, *_a, **_k: ( + _make_network_reply(b"Please sign in") + ) + + branch = self._worker()._default_branch_of(self._URL) + + self.assertEqual("master", branch) # The fallback, not "main" + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.CreateAddonListWorker.addon_repo") + @patch("addonmanager_workers_startup.fci.Preferences") + @patch("addonmanager_workers_startup.initialize_git") + def test_a_branch_the_user_named_is_never_second_guessed( + self, mock_initialize_git, mock_preferences, mock_addon_repo_signal, _ + ): + """The user named a branch, so the repository is not asked about it at all.""" + mock_preferences.return_value.get.return_value = f"{self._URL} their-branch" + worker = self._worker() + + with patch.object(worker, "_create_custom_addon") as mock_create: + worker._get_custom_addons() + + mock_initialize_git.assert_not_called() + mock_create.assert_called_once_with("addon", self._URL, "their-branch") + + @patch("addonmanager_workers_startup.fci.Console") + @patch("addonmanager_workers_startup.CreateAddonListWorker.addon_repo") + @patch("addonmanager_workers_startup.fci.Preferences") + @patch("addonmanager_workers_startup.initialize_git") + def test_the_detected_branch_is_the_one_the_addon_is_built_with( + self, mock_initialize_git, mock_preferences, mock_addon_repo_signal, _ + ): + """The branch that was worked out is the one the addon is fetched and installed from.""" + mock_preferences.return_value.get.return_value = self._URL # No branch named + mock_initialize_git.return_value.default_branch.return_value = "development" + worker = self._worker() + + with patch.object(worker, "_create_custom_addon") as mock_create: + worker._get_custom_addons() + + mock_create.assert_called_once_with("addon", self._URL, "development") + + class TestCustomAddons(unittest.TestCase): """Tests for the custom repository handling in CreateAddonListWorker.""" @@ -394,7 +531,8 @@ def _create_addon(self) -> Addon: @patch("addonmanager_workers_startup.fci.Preferences") def test_parse_custom_repositories(self, mock_preferences): - """Each line is parsed into a URL and a branch, with "master" as the default branch.""" + """Each line is parsed into a URL and a branch. A line with no branch yields an empty one: + the repository is asked what its default branch is, rather than being assumed.""" mock_preferences.return_value.get.return_value = "\n".join( [ "https://github.com/myorg/no-branch-given", @@ -409,10 +547,10 @@ def test_parse_custom_repositories(self, mock_preferences): self.assertEqual( [ - ("https://github.com/myorg/no-branch-given", "master"), + ("https://github.com/myorg/no-branch-given", ""), ("https://github.com/myorg/branch-given", "other-branch"), - ("https://github.com/myorg/trailing-slash", "master"), - ("https://github.com/myorg/dot-git", "master"), + ("https://github.com/myorg/trailing-slash", ""), + ("https://github.com/myorg/dot-git", ""), ], result, ) diff --git a/addonmanager_git.py b/addonmanager_git.py index 3d88dd53..715ae5ff 100644 --- a/addonmanager_git.py +++ b/addonmanager_git.py @@ -270,6 +270,22 @@ def repair(self, remote, local_path): os.chdir(original_cwd) shutil.rmtree(backup_path, ignore_errors=True) + def default_branch(self, remote: str) -> str: + """Get the name of the default branch of a remote repository, without cloning it. Works with + any git host, since it asks the repository itself rather than the software hosting it. + Raises GitFailed if the default branch cannot be determined.""" + + response = self._synchronous_call_git(["ls-remote", "--symref", remote, "HEAD"]) + for line in response.split("\n"): + # The line we want looks like: + # ref: refs/heads/main HEAD + if line.startswith("ref:"): + reference = line.split()[1] + prefix = "refs/heads/" + if reference.startswith(prefix): + return reference[len(prefix) :] # A branch name may itself contain a slash + raise GitFailed(f"Could not determine the default branch of {remote}") + def get_remote(self, local_path) -> str: """Get the repository that this local path is set to fetch from""" old_dir = os.getcwd() diff --git a/addonmanager_workers_startup.py b/addonmanager_workers_startup.py index 73a4653d..b945bacc 100644 --- a/addonmanager_workers_startup.py +++ b/addonmanager_workers_startup.py @@ -66,6 +66,11 @@ class CreateAddonListWorker(QtCore.QThread): RETRY_DELAY_MS = 3000 ATTEMPT_TIMEOUT_MS = 30000 + # The names a default branch is given, tried in this order when git is not available to ask the + # repository what its default branch actually is. The last of them is the fallback when nothing + # else works, which is what a custom repository has always used. + DEFAULT_BRANCH_NAMES = ("main", "master") + def __init__(self): QtCore.QThread.__init__(self) self.setObjectName("CreateAddonListWorker") @@ -111,6 +116,8 @@ def _get_custom_addons(self): + "\n" ) continue + if not branch: + branch = self._default_branch_of(url) fci.Console.PrintLog(f"Adding custom location {url} with branch {branch}\n") self.package_names.append(name) self.addon_repo.emit(self._create_custom_addon(name, url, branch)) @@ -118,16 +125,68 @@ def _get_custom_addons(self): @staticmethod def _parse_custom_repositories() -> List[Tuple[str, str]]: """Parse the CustomRepositories preference into a list of (url, branch) pairs. Each line of - the preference is a repository URL, optionally followed by a space and a branch name.""" + the preference is a repository URL, optionally followed by a space and a branch name. The + branch is empty when the user did not name one, and has to be worked out from the + repository itself.""" repositories = [] for line in fci.Preferences().get("CustomRepositories").split("\n"): url, _, branch = line.strip().partition(" ") url = url.rstrip("/").split(".git")[0] if url: - repositories.append((url, branch.strip() if branch.strip() else "master")) + repositories.append((url, branch.strip())) return repositories + def _default_branch_of(self, url: str) -> str: + """Work out which branch to use for a custom repository whose branch the user did not name. + + Asking git is exact and works with any host, because the repository is asked about itself + rather than the software hosting it. Without git, the two names that a default branch almost + always has are tried instead, which does not cover a repository whose default branch is + named anything else.""" + + git_manager = initialize_git() + if git_manager: + try: + branch = git_manager.default_branch(url) + fci.Console.PrintLog(f"The default branch of {url} is '{branch}'\n") + return branch + except GitFailed as e: + fci.Console.PrintLog(f"Could not ask git for the default branch of {url}: {e}\n") + + for candidate in CreateAddonListWorker.DEFAULT_BRANCH_NAMES: + if self._repository_has_branch(url, candidate): + fci.Console.PrintLog(f"Using branch '{candidate}' of {url}\n") + return candidate + + fci.Console.PrintWarning( + translate( + "AddonsInstaller", + "Could not determine the default branch of the custom repository {}, so '{}' is " + "being used. If that is the wrong branch, set the correct one in the Addon Manager " + "preferences.", + ).format(url, CreateAddonListWorker.DEFAULT_BRANCH_NAMES[-1]) + + "\n" + ) + return CreateAddonListWorker.DEFAULT_BRANCH_NAMES[-1] + + def _repository_has_branch(self, url: str, branch: str) -> bool: + """Whether the repository serves an addon's package.xml on the given branch. + + If the software running the git host has not been identified yet, then which URL the file + would live at is not known either, so every layout is asked: the host gets identified in the + process, and the answer is kept for later.""" + + location = SimpleNamespace(url=url, branch=branch) + if utils.git_host_of(location) is not None: + return self._serves_package_xml(utils.construct_git_url(location, "package.xml")) + + host = utils.identify_git_host(location, self._serves_package_xml) + if host is None: + return False + utils.remember_git_host(location, host) + return True + def _create_custom_addon(self, name: str, url: str, branch: str) -> Addon: """Create an Addon for a custom repository by synthesizing the catalog entry that the remote addon catalog would have contained for it, so that a custom addon is constructed by