git: allow users to fall back to system git

This change introduces the config option `experimental.system-git-client` defaulting to `false`. When set to `true`, the subprocess git client implementation is used when cloning a remote repository. This option will be removed in a future release.
python-poetry · abn · May 2, 2022 · Apr 7, 2022 · Apr 8, 2022 · Apr 18, 2022
commit 74ad52b136aba48d6d78bd072687b4b4d734cb90
diff --git a/docs/dependency-specification.md b/docs/dependency-specification.md
@@ -116,6 +116,24 @@ To use an SSH connection, for example in the case of private repositories, use t
 requests = { git = "git@github.com:requests/requests.git" }
 ```
 
+{{% note %}}
+With Poetry 1.2 releases, the default git client used is [Dulwich](https://www.dulwich.io/). We
+fall back to legacy system git client implementation in cases where [gitcredentials](https://git-scm.com/docs/gitcredentials)
+are used. This fallback will be removed in a future release where username/password authentication
+can be better supported natively.
+
+In cases where you encounter issues with the default implementation that used to work prior to
+Poetry 1.2, you may wish to explicitly configure the use of the system git client via a shell
+subprocess call.
+
+```bash
+poetry config experimental.system-git-client true
+```
+
+Keep in mind however, that doing so will surface bugs that existed in versions prior to 1.2 which
+were caused due to the use of the system git client.
+{{% /note %}}
+
 ## `path` dependencies
 
 To depend on a library located in a local directory or file,

diff --git a/src/poetry/config/config.py b/src/poetry/config/config.py
@@ -40,7 +40,7 @@ class Config:
             "options": {"always-copy": False, "system-site-packages": False},
             "prefer-active-python": False,
         },
-        "experimental": {"new-installer": True},
+        "experimental": {"new-installer": True, "system-git-client": False},
         "installer": {"parallel": True, "max-workers": None},
     }
 
@@ -141,6 +141,7 @@ def _get_normalizer(name: str) -> Callable:
             "virtualenvs.options.system-site-packages",
             "virtualenvs.options.prefer-active-python",
             "experimental.new-installer",
+            "experimental.system-git-client",
             "installer.parallel",
         }:
             return boolean_normalizer

diff --git a/src/poetry/vcs/git/backend.py b/src/poetry/vcs/git/backend.py
@@ -18,7 +18,7 @@
 from dulwich.repo import Repo
 
 from poetry.console.exceptions import PoetrySimpleConsoleException
-from poetry.utils.helpers import safe_rmtree
+from poetry.utils.helpers import remove_directory
 
 
 if TYPE_CHECKING:
@@ -196,8 +196,10 @@ def _clone_legacy(url: str, refspec: GitRefSpec, target: Path) -> Repo:
         """
         from poetry.vcs.git.system import SystemGit
 
+        logger.debug("Cloning '%s' using system git client", url)
+
         if target.exists():
-            safe_rmtree(path=target, ignore_errors=True)
+            remove_directory(path=target, force=True)
 
         revision = refspec.tag or refspec.branch or refspec.revision or "HEAD"
 
@@ -270,7 +272,7 @@ def _clone(cls, url: str, refspec: GitRefSpec, target: Path) -> Repo:
             # this implies the ref we need does not exist or is invalid
             if isinstance(e, KeyError):
                 # the local copy is at a bad state, lets remove it
-                safe_rmtree(local.path, ignore_errors=True)
+                remove_directory(local.path, force=True)
 
             if isinstance(e, AssertionError) and "Invalid object name" not in str(e):
                 raise
@@ -313,6 +315,22 @@ def _clone_submodules(cls, repo: Repo) -> None:
                     and not path_absolute.joinpath(".git").is_dir(),
                 )
 
+    @staticmethod
+    def is_using_legacy_client() -> bool:
+        from poetry.factory import Factory
+
+        return (
+            Factory.create_config()
+            .get("experimental", {})
+            .get("system-git-client", False)
+        )
+
+    @staticmethod
+    def get_default_source_root() -> Path:
+        from poetry.factory import Factory
+
+        return Path(Factory.create_config().get("cache-dir")) / "src"
+
     @classmethod
     def clone(
         cls,
@@ -324,11 +342,7 @@ def clone(
         source_root: Path | None = None,
         clean: bool = False,
     ) -> Repo:
-        if not source_root:
-            from poetry.factory import Factory
-
-            source_root = Path(Factory.create_config().get("cache-dir")) / "src"
-
+        source_root = source_root or cls.get_default_source_root()
         source_root.mkdir(parents=True, exist_ok=True)
 
         name = name or cls.get_name_from_source_url(url=url)
@@ -338,7 +352,7 @@ def clone(
         if target.exists():
             if clean:
                 # force clean the local copy if it exists, do not reuse
-                safe_rmtree(target, ignore_errors=True)
+                remove_directory(target, force=True)
             else:
                 # check if the current local copy matches the requested ref spec
                 try:
@@ -348,18 +362,20 @@ def clone(
                         current_sha = current_repo.head().decode("utf-8")
                 except (NotGitRepository, AssertionError, KeyError):
                     # something is wrong with the current checkout, clean it
-                    safe_rmtree(target, ignore_errors=True)
+                    remove_directory(target, force=True)
                 else:
                     if not is_revision_sha(revision=current_sha):
                         # head is not a sha, this will cause issues later, lets reset
-                        safe_rmtree(target, ignore_errors=True)
+                        remove_directory(target, force=True)
                     elif refspec.is_sha and current_sha.startswith(refspec.revision):
                         # if revision is used short-circuit remote fetch head matches
                         return current_repo
 
         try:
-            local = cls._clone(url=url, refspec=refspec, target=target)
-            cls._clone_submodules(repo=local)
+            if not cls.is_using_legacy_client():
+                local = cls._clone(url=url, refspec=refspec, target=target)
+                cls._clone_submodules(repo=local)
+                return local
         except HTTPUnauthorized:
             # we do this here to handle http authenticated repositories as dulwich
             # does not currently support using credentials from git-credential helpers.
@@ -369,10 +385,10 @@ def clone(
             # without additional configuration or changes for existing projects that
             # use http basic auth credentials.
             logger.debug(
-                "Unable to fetch from private repository '{%s}', falling back to"
+                "Unable to fetch from private repository '%s', falling back to"
                 " system git",
                 url,
             )
-            local = cls._clone_legacy(url=url, refspec=refspec, target=target)
 
-        return local
+        # fallback to legacy git client
+        return cls._clone_legacy(url=url, refspec=refspec, target=target)
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -30,7 +30,7 @@
 from poetry.utils.env import EnvManager
 from poetry.utils.env import SystemEnv
 from poetry.utils.env import VirtualEnv
-from poetry.utils.helpers import safe_rmtree
+from poetry.utils.helpers import remove_directory
 from tests.helpers import TestLocker
 from tests.helpers import TestRepository
 from tests.helpers import get_package
@@ -307,7 +307,7 @@ def tmp_dir() -> Iterator[str]:
 
     yield dir_
 
-    safe_rmtree(dir_)
+    remove_directory(dir_, force=True)
 
 
 @pytest.fixture

diff --git a/tests/console/commands/test_config.py b/tests/console/commands/test_config.py
@@ -51,6 +51,7 @@ def test_list_displays_default_value_if_not_set(
     venv_path = json.dumps(os.path.join("{cache-dir}", "virtualenvs"))
     expected = f"""cache-dir = {cache_dir}
 experimental.new-installer = true
+experimental.system-git-client = false
 installer.max-workers = null
 installer.parallel = true
 virtualenvs.create = true
@@ -75,6 +76,7 @@ def test_list_displays_set_get_setting(
     venv_path = json.dumps(os.path.join("{cache-dir}", "virtualenvs"))
     expected = f"""cache-dir = {cache_dir}
 experimental.new-installer = true
+experimental.system-git-client = false
 installer.max-workers = null
 installer.parallel = true
 virtualenvs.create = false
@@ -123,6 +125,7 @@ def test_list_displays_set_get_local_setting(
     venv_path = json.dumps(os.path.join("{cache-dir}", "virtualenvs"))
     expected = f"""cache-dir = {cache_dir}
 experimental.new-installer = true
+experimental.system-git-client = false
 installer.max-workers = null
 installer.parallel = true
 virtualenvs.create = false

diff --git a/tests/integration/test_utils_vcs_git.py b/tests/integration/test_utils_vcs_git.py
@@ -60,6 +60,11 @@ def setup(config: Config) -> None:
 }
 
 
+@pytest.fixture
+def use_system_git_client(config: Config) -> None:
+    config.merge({"experimental": {"system-git-client": True}})
+
+
 @pytest.fixture(scope="module")
 def source_url() -> str:
     return "https://github.com/python-poetry/test-fixture-vcs-repository.git"
@@ -104,11 +109,20 @@ def remote_default_branch(remote_default_ref: bytes) -> str:
 
 
 def test_git_clone_default_branch_head(
-    source_url: str, remote_refs: FetchPackResult, remote_default_ref: bytes
+    source_url: str,
+    remote_refs: FetchPackResult,
+    remote_default_ref: bytes,
+    mocker: MockerFixture,
 ):
+    spy = mocker.spy(Git, "_clone")
+    spy_legacy = mocker.spy(Git, "_clone_legacy")
+
     with Git.clone(url=source_url) as repo:
         assert remote_refs.refs[remote_default_ref] == repo.head()
 
+    spy_legacy.assert_not_called()
+    spy.assert_called()
+
 
 def test_git_clone_fails_for_non_existent_branch(source_url: str):
     branch = uuid.uuid4().hex
@@ -208,7 +222,8 @@ def test_git_clone_clones_submodules(source_url: str) -> None:
 
 
 def test_system_git_fallback_on_http_401(
-    mocker: MockerFixture, source_url: str
+    mocker: MockerFixture,
+    source_url: str,
 ) -> None:
     spy = mocker.spy(Git, "_clone_legacy")
     mocker.patch.object(Git, "_clone", side_effect=HTTPUnauthorized(None, None))
@@ -223,3 +238,23 @@ def test_system_git_fallback_on_http_401(
         refspec=GitRefSpec(branch="0.1", revision=None, tag=None, ref=b"HEAD"),
     )
     spy.assert_called_once()
+
+
+def test_system_git_called_when_configured(
+    mocker: MockerFixture, source_url: str, use_system_git_client: None
+) -> None:
+    spy_legacy = mocker.spy(Git, "_clone_legacy")
+    spy = mocker.spy(Git, "_clone")
+
+    with Git.clone(url=source_url, branch="0.1") as repo:
+        path = Path(repo.path)
+        assert_version(repo, BRANCH_TO_REVISION_MAP["0.1"])
+
+    spy.assert_not_called()
+
+    spy_legacy.assert_called_once()
+    spy_legacy.assert_called_with(
+        url=source_url,
+        target=path,
+        refspec=GitRefSpec(branch="0.1", revision=None, tag=None, ref=b"HEAD"),
+    )