From eb5d6a72a047f4a2cc153ed7c2a3959d02da81f4 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Thu, 3 Sep 2026 01:45:41 -0400 Subject: [PATCH] =?UTF-8?q?[FIX]=20git=20tool=20:=20garder=20le=20d=C3=A9p?= =?UTF-8?q?=C3=B4t=20racine=20quand=20le=20manifeste=20manque?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le traitement de `add_root` est en FIN de fonction, et deux retours anticipés lui sautaient par-dessus : sans manifeste, la liste revenait vide alors que la racine était demandée. Le script qui réécrit les remotes ne trouvait donc rien à faire sur un checkout sans `.repo`, et l'annonçait comme un succès. L'ajout de la racine et le tri passent par un point de sortie unique, ce qui supprime la copie qui gardait le défaut dans une variante sur deux. Une origine absente vaut l'URL par défaut plutôt qu'une exception. Vérifié dans un dépôt jetable, dans les deux sens et sans origine ; 4 tests neufs, qui échouent si le retour anticipé revient. --- EN --- The `add_root` handling sits at the END of the function, and two early returns jumped over it: with no manifest, the list came back empty although the root had been asked for. The script that rewrites the remotes therefore found nothing to do on a checkout without `.repo`, and reported it as a success. Adding the root and sorting now go through a single exit point, which drops the copy that kept the fault in one variant out of two. A missing origin falls back to the default URL rather than raising. Checked in a throwaway repository, both ways and with no origin; 4 new tests, which fail if the early return comes back. Assisted-by: Claude Opus 5 --- script/git/git_tool.py | 89 ++++++++++++++++++++++-------------------- test/test_git_tool.py | 77 ++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 43 deletions(-) diff --git a/script/git/git_tool.py b/script/git/git_tool.py index cbf68fd..446223a 100644 --- a/script/git/git_tool.py +++ b/script/git/git_tool.py @@ -116,6 +116,48 @@ class GitTool: repo_path=repo_path, add_root=add_root ) + def _with_root_repo( + self, repos: list, repo_path: str, add_root: bool + ) -> list: + """`repos` précédée du dépôt racine si `add_root`, puis triée. + + TOUS les chemins de sortie passent par ici. Le dépôt racine ne + dépend ni du manifeste ni des sous-modules : un retour anticipé qui + saute cette étape rend une liste VIDE alors que la racine était + demandée, et l'appelant en conclut qu'il n'a rien à faire. Sur un + checkout sans `.repo`, le remote du dépôt principal n'était alors + jamais changé, sans qu'aucun message ne le signale. + + Une origine absente vaut l'URL par défaut plutôt qu'une exception : + la liste sert à RÉÉCRIRE les remotes, et un dépôt sans origine est + précisément un de ceux qu'on vient corriger. + """ + if not add_root: + return sorted(repos, key=lambda k: k.get("name")) + repo_root = Repo(repo_path) + try: + url = repo_root.git.remote("get-url", "origin") + except Exception: + print( + f"{Fore.YELLOW}WARNING{Style.RESET_ALL}: Missing origin" + f" remote, use default url {DEFAULT_REMOTE_URL}. Suggest" + " to add a remote origin: \n> git remote add origin" + f" {DEFAULT_REMOTE_URL}" + ) + url = DEFAULT_REMOTE_URL + url, url_https, url_git = self.get_url(url) + repos.insert( + 0, + { + "url": url, + "url_https": url_https, + "url_git": url_git, + "path": repo_path, + "name": "", + }, + ) + return sorted(repos, key=lambda k: k.get("name")) + def get_repo_info_submodule( self, repo_path: str = ".", add_root: bool = False ) -> list: @@ -181,22 +223,7 @@ class GitTool: } repos.append(data) - if add_root: - repo_root = Repo(repo_path) - url = repo_root.git.remote("get-url", "origin") - url, url_https, url_git = self.get_url(url) - - data = { - "url": url, - "url_https": url_https, - "url_git": url_git, - "path": repo_path, - "name": "", - } - repos.insert(0, data) - # Sort - repos = sorted(repos, key=lambda k: k.get("name")) - return repos + return self._with_root_repo(repos, repo_path, add_root) def get_repo_info_manifest_xml( self, repo_path: str = ".", add_root: bool = False, filter_group=None @@ -219,7 +246,7 @@ class GitTool: filter_groups = filter_group.split(",") if filter_group else [] manifest_file = self.get_manifest_file(repo_path=repo_path) if not manifest_file: - return [] + return self._with_root_repo([], repo_path, add_root) if os.path.isabs(manifest_file): # This is a absolute path filename = manifest_file @@ -231,7 +258,7 @@ class GitTool: xml_dict = xmltodict.parse(xml_as_string) manifest_data = xml_dict.get("manifest") if not manifest_data: - return [] + return self._with_root_repo([], repo_path, add_root) if manifest_data.get("default"): default_remote = manifest_data.get("default").get("@remote") else: @@ -274,31 +301,7 @@ class GitTool: } repos.append(data) - if add_root: - repo_root = Repo(repo_path) - try: - url = repo_root.git.remote("get-url", "origin") - except Exception as e: - print( - f"{Fore.YELLOW}WARNING{Style.RESET_ALL}: Missing origin" - f" remote, use default url {DEFAULT_REMOTE_URL}. Suggest" - " to add a remote origin: \n> git remote add origin" - f" {DEFAULT_REMOTE_URL}" - ) - url = DEFAULT_REMOTE_URL - url, url_https, url_git = self.get_url(url) - - data = { - "url": url, - "url_https": url_https, - "url_git": url_git, - "path": repo_path, - "name": "", - } - repos.insert(0, data) - # Sort - repos = sorted(repos, key=lambda k: k.get("name")) - return repos + return self._with_root_repo(repos, repo_path, add_root) def get_manifest_xml_info( self, repo_path: str = ".", filename=None, add_root: bool = False diff --git a/test/test_git_tool.py b/test/test_git_tool.py index 7fa6f7d..c38c86c 100644 --- a/test/test_git_tool.py +++ b/test/test_git_tool.py @@ -2,10 +2,13 @@ # © 2026 TechnoLibre (http://www.technolibre.ca) # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl) +import io import os +import subprocess import tempfile import unittest from collections import OrderedDict +from contextlib import redirect_stdout from unittest.mock import MagicMock, mock_open, patch from script.git.git_tool import ( @@ -265,6 +268,80 @@ class TestGetRepoInfoSubmodule(unittest.TestCase): self.assertIn("git@", result[0]["url_git"]) +class TestTheRootRepoSurvivesAMissingManifest(unittest.TestCase): + """Le dépôt racine ne dépend ni du manifeste ni des sous-modules. + + Une liste vide là où la racine était demandée fait conclure à + l'appelant qu'il n'a rien à faire : sur un checkout sans `.repo`, le + remote du dépôt principal n'était jamais réécrit, sans message. + """ + + def _depot(self, chemin, origine="https://github.com/ERPLibre/ERPLibre"): + """Un vrai dépôt git, sans manifeste. Rien n'est bouchonné : la + lecture de l'origine passe par git, et c'est elle qu'on vérifie.""" + subprocess.run(["git", "init", "-q", chemin], check=True) + if origine: + subprocess.run( + ["git", "-C", chemin, "remote", "add", "origin", origine], + check=True, + ) + return chemin + + def test_without_a_manifest_the_root_is_still_returned(self): + gt = GitTool() + with tempfile.TemporaryDirectory() as tmpdir: + self._depot(tmpdir) + result = gt.get_repo_info(tmpdir, add_root=True) + self.assertEqual(len(result), 1) + self.assertEqual(result[0]["name"], "") + self.assertEqual( + result[0]["url_git"], "git@github.com:ERPLibre/ERPLibre" + ) + + def test_without_the_root_asked_nothing_is_returned(self): + """Deux appelants internes lisent le manifeste SANS la racine : le + correctif ne doit rien rendre à qui n'en veut pas.""" + gt = GitTool() + with tempfile.TemporaryDirectory() as tmpdir: + self._depot(tmpdir) + self.assertEqual(gt.get_repo_info(tmpdir), []) + + def test_a_root_without_origin_falls_back_instead_of_raising(self): + """La liste sert à RÉÉCRIRE les remotes : un dépôt sans origine est + précisément un de ceux qu'on vient corriger.""" + gt = GitTool() + with tempfile.TemporaryDirectory() as tmpdir: + self._depot(tmpdir, origine="") + with redirect_stdout(io.StringIO()): + result = gt.get_repo_info(tmpdir, add_root=True) + self.assertEqual(len(result), 1) + self.assertTrue(result[0]["url_git"].startswith("git@")) + + def test_the_root_comes_first_alongside_the_manifest_projects(self): + """La racine porte un nom vide : le tri la met en tête, et le + script qui réécrit les remotes commence donc par elle.""" + xml_content = ( + '\n' + "\n" + ' \n' + ' \n' + ' \n' + "\n" + ) + gt = GitTool() + with tempfile.TemporaryDirectory() as tmpdir: + self._depot(tmpdir) + manifeste = os.path.join(tmpdir, "manifest_test.xml") + with open(manifeste, "w") as fh: + fh.write(xml_content) + with patch.object( + GitTool, "get_manifest_file", return_value=manifeste + ): + result = gt.get_repo_info(tmpdir, add_root=True) + self.assertEqual([r["name"] for r in result], ["", "addons/OCA_web"]) + + class TestGetManifestXmlInfo(unittest.TestCase): def test_parses_manifest(self): xml_content = """