diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e734bc5..4fdbad9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,12 @@ and this project adheres to [Semantic Versioning][semver]. ### Fixed - Detect signing scheme from key material instead of assuming RSA ([757]) +- Clone no longer fails when the repository path contains a space (e.g. a Windows home directory with a space in the user name) ([762]) +- Surface the underlying git error when a clone fails, instead of hiding it behind a generic access message ([762]) +- Correct the clone access error that rendered as "Cannot None ..." and stop misattributing a local failure to an access/authentication problem ([762]) +[762]: https://github.com/openlawlibrary/taf/pull/762 [759]: https://github.com/openlawlibrary/taf/pull/759 [757]: https://github.com/openlawlibrary/taf/pull/757 diff --git a/taf/git.py b/taf/git.py index f75047dd..bb421124 100644 --- a/taf/git.py +++ b/taf/git.py @@ -290,16 +290,33 @@ def _git(self, cmd, *args, **kwargs): log_success_msg = kwargs.pop("log_success_msg", "") error_if_not_exists = kwargs.pop("error_if_not_exists", True) - if len(args): - cmd = cmd.format(*args) + # Build an argv list rather than one string: run() whitespace-splits a + # single-string command, which shatters any argument containing a space + # (e.g. a Windows path under a home dir with a space in the user name) - + # both `git -C ` and any `{}` argument such as a local clone + # source. The template is tokenized first and each `{}` placeholder is + # filled positionally in place, so a substituted value stays a single + # argv token no matter what it contains, while the command's own + # tokenization is unchanged. Callers must therefore pass paths/URLs as + # `{}` arguments, not interpolate them into the command string. + command = ["git", "-C", str(self.path)] if self.allow_unsafe: - command = f"git -C {self.path} -c safe.directory={self.path} {cmd}" - else: - command = f"git -C {self.path} {cmd}" + command += ["-c", f"safe.directory={self.path}"] + args_iter = iter(args) + for token in cmd.split(): + placeholders = token.count("{}") + if placeholders: + token = token.format(*[next(args_iter) for _ in range(placeholders)]) + # an empty token means an optional flag arg was "" (e.g. push's + # force/no-verify flags); drop it, matching the old .split() which + # collapsed empties, so it does not reach git as an empty argument + if token != "": + command.append(token) + command_str = " ".join(command) result = None if log_error or log_error_msg: try: - result = run(command, **kwargs) + result = run(*command, **kwargs) if log_success_msg: self._log_debug(log_success_msg) except subprocess.CalledProcessError as e: @@ -314,7 +331,7 @@ def _git(self, cmd, *args, **kwargs): error = GitError(self, message=log_error_msg, error=e) self._log_error(error.message) else: - error = GitError(self, command=command, error=e) + error = GitError(self, command=command_str, error=e) # not every git error indicates a problem # if it does, we expect that either custom error message will be provided # or that the error will be reraised @@ -323,9 +340,9 @@ def _git(self, cmd, *args, **kwargs): raise error else: try: - result = run(command, **kwargs) + result = run(*command, **kwargs) except subprocess.CalledProcessError as e: - raise GitError(self, command=command, error=e) + raise GitError(self, command=command_str, error=e) if log_success_msg: self._log_debug(log_success_msg) return result @@ -834,13 +851,16 @@ def clone( joined_params = " ".join(params) cloned = False + clone_errors = [] for url in self.urls: self._log_info(f"trying to clone from {url}") try: + # url is a `{}` arg (kept as a single token); joined_params holds + # controlled flags meant to expand into multiple tokens, so it + # stays in the template string self._git( - "clone {} . {}", + f"clone {{}} . {joined_params}", url, - joined_params, log_success_msg=f"successfully cloned from {url}", reraise_error=True, timeout=60, @@ -848,13 +868,16 @@ def clone( ) except GitError as e: self._log_debug(f"could not clone from {url} due to {e}") + clone_errors.append(f"{url}: {e}") else: self._log_info(f"successfully cloned from {url}") cloned = True break if not cloned: - self.raise_git_access_error(CloneRepoException) + self.raise_git_access_error( + CloneRepoException, operation="clone", underlying_errors=clone_errors + ) # the path is now a repository; drop any cached negative result from # before the clone @@ -889,8 +912,11 @@ def clone_from_disk( # file-by-file object copy pygit2 performs. git silently falls back to # copying when hardlinks are not possible, so this is never slower. bare_flag = "--bare " if is_bare else "" + # pass the source path as a `{}` arg (the `{{}}` survives the f-string) + # so a path containing a space is not split into separate tokens self._git( - f"clone --local {bare_flag}{local_path} .", + f"clone --local {bare_flag}{{}} .", + str(local_path), error_if_not_exists=False, reraise_error=True, ) @@ -1502,7 +1528,9 @@ def get_last_remote_commit( raise FetchException( "Could not fetch the last remote commit. URL not found" ) - last_commit = self._git(f"--no-pager ls-remote {url} {branch}", log_error=True) + last_commit = self._git( + "--no-pager ls-remote {} {}", url, branch, log_error=True + ) if last_commit: last_commit = last_commit.split("\t", 1)[0] # in some cases (e.g. upstream is defined the result might contain a warning line) @@ -1802,37 +1830,56 @@ def push( raise PushFailedError(self, message=f"Push operation failed: {e}") def raise_git_access_error( - self, error_cls=GitAccessDeniedException, operation=None, error_msg="" + self, + error_cls=GitAccessDeniedException, + operation="access", + error_msg="", + underlying_errors=None, ): + """Raise ``error_cls`` explaining why a git network operation failed. + + ``operation`` names the attempted action (e.g. ``"clone"``) in the + message. ``error_msg``, when given, replaces the auto-selected guidance. + ``underlying_errors`` is the list of concrete per-attempt git failures + (typically one per URL); they are surfaced on the exception so the real + cause is never hidden behind the guidance. + """ + # Pick the guidance that best fits what we can observe about the remote, + # then surface the concrete git failure(s) alongside it. The guidance is + # a best guess (an unauthenticated probe cannot tell "private" from + # "missing"); the underlying errors are the ground truth and must not be + # swallowed - historically only the guidance was shown, hiding the real + # cause (e.g. a command-construction failure, not an access problem). hosts = { h for h in (extract_hostname(url) for url in self.urls) if h is not None } unknown_hosts = [host for host in hosts if not is_host_known(host)] - if len(unknown_hosts): - message = _no_hosts_error_format.format(hostname=",".join(unknown_hosts)) - raise error_cls(self, operation=operation, message=message) - repo_exists = any(repository_exists(url) for url in self.urls) - if repo_exists: + if error_msg: + guidance = error_msg + elif len(unknown_hosts): + guidance = _no_hosts_error_format.format(hostname=",".join(unknown_hosts)) + elif any(repository_exists(url) for url in self.urls): uses_ssh = any(url.startswith("git@") for url in self.urls) - if uses_ssh: - raise error_cls( - self, - operation=operation, - message=( - _clone_or_pull_error_message if error_msg == "" else error_msg - ), - ) - else: - raise error_cls( - self, - operation=operation, - message=( - _clone_or_pull_error_message_no_ssh - if error_msg == "" - else error_msg - ), - ) - raise error_cls(self, operation=operation) + guidance = ( + _clone_or_pull_error_message + if uses_ssh + else _clone_or_pull_error_message_no_ssh + ) + else: + guidance = _repo_not_found_error_message + + parts = [] + if underlying_errors: + parts.append( + "The git command failed with:\n" + "\n".join(underlying_errors) + ) + if guidance: + parts.append(guidance) + raise error_cls( + self, + operation=operation, + message="\n\n".join(parts) if parts else None, + ) def remove_remote(self, remote_name: str) -> None: try: @@ -1931,7 +1978,9 @@ def safely_get_json(self, commit: Commitish, path: str) -> Optional[Dict]: return None def set_remote_url(self, new_url: str, remote: Optional[str] = "origin") -> None: - self._git(f"remote set-url {remote} {new_url}") + # new_url may be a local filesystem path containing spaces - pass it as + # a `{}` arg so it is not split into separate tokens + self._git("remote set-url {} {}", remote, new_url) def set_head_to_branch(self, branch_name: str) -> None: """Point HEAD at the given local branch without checking it out.""" @@ -2226,6 +2275,20 @@ def _find_url(path, url): ) +_repo_not_found_error_message = ( + "The remote repository could not be reached or found. If a git error is shown " + "above, start there - it is the actual failure. Otherwise, note that an " + "unauthenticated check cannot tell a private repository apart from a missing " + "one, so common causes are:\n\n" + "1. A wrong or mistyped repository URL.\n" + "2. A private repository your account has not been granted access to.\n" + "3. Missing or misconfigured credentials for this host:\n" + " - For SSH URLs, ensure your SSH key is added to your Git hosting account and " + "loaded (e.g. `ssh -T git@github.com`).\n" + " - For HTTPS URLs, ensure a valid token/credential helper is configured.\n" +) + + _no_hosts_error_format = ( "Error: The host(s) '{hostname}' not recognized." "To add a host to your known_hosts file, manually connect via SSH using the command:\n" diff --git a/taf/tests/test_git/test_clone_errors.py b/taf/tests/test_git/test_clone_errors.py new file mode 100644 index 00000000..c6eb4fe3 --- /dev/null +++ b/taf/tests/test_git/test_clone_errors.py @@ -0,0 +1,154 @@ +"""Tests around the clone/access error path in ``taf.git``. + +Regression coverage for the clone-failure investigation: + +* the access error names the git operation ("Cannot clone", never + "Cannot None") and its fallback carries actionable guidance; +* a repository path containing a space (e.g. a Windows home dir + "C:\\Users\\Given Surname\\...") no longer breaks the ``git -C `` + argument - ``_git`` passes argv as a list instead of a whitespace-split + string; and +* the real underlying git error is surfaced on the raised + ``CloneRepoException`` instead of being swallowed at debug level. +""" + +from pathlib import Path + +import pytest + +import taf.git as git_module +from taf.exceptions import CloneRepoException, GitAccessDeniedException, GitError +from taf.git import GitRepository + + +def _repo(tmp_path, urls): + """A GitRepository instance that only needs .name/.urls/.log_prefix for the + error-formatting code under test (no on-disk repo required).""" + return GitRepository(path=Path(tmp_path) / "law-private", urls=urls) + + +# --------------------------------------------------------------------------- # +# Committed fix: the access error names the operation and carries guidance. +# --------------------------------------------------------------------------- # + + +def test_clone_repo_exception_defaults_to_clone_operation(tmp_path): + # Defence in depth: even constructed directly, CloneRepoException must name + # "clone" rather than rendering the operation as None. + exc = CloneRepoException(_repo(tmp_path, ["https://example.com/x"])) + message = str(exc) + assert "Cannot clone" in message + assert "Cannot None" not in message + + +def test_raise_git_access_error_for_clone_names_clone_not_none(tmp_path, monkeypatch): + # This is the exact regression from the field report: a private repo that an + # unauthenticated probe cannot see falls through to the final branch. Before + # the fix that branch raised with operation=None -> "Cannot None ...". + monkeypatch.setattr(git_module, "is_host_known", lambda host: True) + monkeypatch.setattr(git_module, "repository_exists", lambda url: False) + + repo = _repo(tmp_path, ["https://github.com/openlawlibrary/law-private"]) + with pytest.raises(CloneRepoException) as exc_info: + repo.raise_git_access_error(CloneRepoException, operation="clone") + + message = str(exc_info.value) + first_line = message.splitlines()[0] + assert "Cannot clone" in first_line + assert "None" not in first_line + # the fallback is no longer a bare error - it carries actionable guidance + assert "could not be reached or found" in message + + +def test_raise_git_access_error_default_operation_is_not_none(tmp_path, monkeypatch): + # A caller that forgets to pass an operation must never produce "Cannot None". + monkeypatch.setattr(git_module, "is_host_known", lambda host: True) + monkeypatch.setattr(git_module, "repository_exists", lambda url: False) + + repo = _repo(tmp_path, ["https://github.com/openlawlibrary/law-private"]) + with pytest.raises(GitAccessDeniedException) as exc_info: + repo.raise_git_access_error() + + first_line = str(exc_info.value).splitlines()[0] + assert "None" not in first_line + + +# --------------------------------------------------------------------------- # +# Spaced repository paths: `_git` passes argv as a list, so a repository path +# containing a space (e.g. a Windows home dir "C:\\Users\\Given Surname\\...") +# no longer shatters the `git -C ` argument. +# --------------------------------------------------------------------------- # + + +def test_clone_into_path_with_spaces(origin_repo: GitRepository, tmp_path): + dest = Path(tmp_path) / "Given Surname" / "clone" + dest.mkdir(parents=True) + repo = GitRepository(path=dest) + # assign after construction to bypass URL validation - the local origin + # path is a valid clone source but not a URL (mirrors the other clone tests) + repo.urls = [str(origin_repo.path)] + repo.clone() + assert repo.is_git_repository + + +def test_clone_from_disk_with_spaced_source_path(repository: GitRepository, tmp_path): + # Regression: the users-auth clone step does `git clone --local .` + # where is a temp dir that, under a spaced Windows home, contains a + # space. clone_from_disk now passes the source as a `{}` arg so it stays one + # token; previously it was split and git reported "fatal: Too many arguments". + spaced_source = Path(tmp_path) / "Given Surname" / "source" + spaced_source.mkdir(parents=True) + # populate the spaced source (spaced *destination* path -> exercises `git -C`) + GitRepository(path=spaced_source).clone_from_disk(repository.path) + assert GitRepository(path=spaced_source).is_git_repository + + # clone *from* the spaced source (spaced value -> exercises the `{}` arg) + dest = Path(tmp_path) / "dest" + repo = GitRepository(path=dest) + repo.clone_from_disk(spaced_source) + assert repo.is_git_repository + + +def test_git_subprocess_handles_spaced_repo_path(tmp_path): + # A local git operation that actually shells out (via _git -> run) must + # tolerate a repository path with spaces. The repo is initialised through + # run()'s list form (which is not split), so only the _git string command + # under test is exercised against the spaced path. + from taf.utils import run + + dest = Path(tmp_path) / "Given Surname" / "repo" + dest.mkdir(parents=True) + run("git", "-C", str(dest), "init") + repo = GitRepository(path=dest) + out = repo._git("rev-parse --is-inside-work-tree", reraise_error=True) + assert out.strip() == "true" + + +# --------------------------------------------------------------------------- # +# The real per-URL git error (which explains *why* the clone failed) is +# surfaced on the raised CloneRepoException, not swallowed at debug level. +# --------------------------------------------------------------------------- # + + +def test_clone_surfaces_underlying_git_error( + clone_repository: GitRepository, monkeypatch +): + clone_repository.urls = ["https://example.com/x.git"] + sentinel = r"fatal: cannot change to 'C:\Users\Given': No such file or directory" + + def fake_git(*args, **kwargs): + raise GitError(clone_repository, message=sentinel) + + monkeypatch.setattr(clone_repository, "_git", fake_git) + # keep raise_git_access_error off the network and on the final fallback + monkeypatch.setattr(git_module, "is_host_known", lambda host: True) + monkeypatch.setattr(git_module, "repository_exists", lambda url: False) + + with pytest.raises(CloneRepoException) as exc_info: + clone_repository.clone() + + message = str(exc_info.value) + # the concrete git failure is surfaced ... + assert sentinel in message + # ... alongside (not instead of) the guidance + assert "could not be reached or found" in message