From 43d65f9303a5216e786023673e52356bb989702d Mon Sep 17 00:00:00 2001 From: oll-bot Date: Thu, 23 Jul 2026 17:19:41 -0400 Subject: [PATCH 1/5] fix(git): give private/inaccessible clones an accurate, actionable error Cloning a private repo you can't authenticate to produced the useless message "Cannot None from any of the following URLs: [...]" with zero guidance. Two independent defects combined to produce it. 1. The literal "None". `CloneRepoException` defaults operation="clone", but `raise_git_access_error` passed operation=None *explicitly*, clobbering that default before it reached the f-string "Cannot {operation} ...". fetch/pull dodged this only because they pass operation= explicitly; clone alone relied on the subclass default. Fixed by passing operation="clone" at the call site, and changing the parameter default from None to "access" so the trap can't silently recur if a future caller forgets. 2. No guidance for the case that actually fires. All the "check your access / SSH key / it may be private" help is gated on `repository_exists()` returning True, but that probe is an *unauthenticated* HTTP HEAD. A private repo returns 404 to it, so repo_exists is False, every helpful branch is skipped, and we fall through to a bare raise. This is also why switching HTTPS->SSH changed nothing: the SSH URL is rewritten to the same https HEAD, which 404s identically. The unauthenticated probe simply cannot tell "missing" from "private and I'm not logged in". The final fallback now attaches a message that names this ambiguity explicitly and lists the access/credential checks to run, so the most common real cause (auth to a private repo) is surfaced instead of hidden. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G --- taf/git.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/taf/git.py b/taf/git.py index f75047dd..1db3d6ff 100644 --- a/taf/git.py +++ b/taf/git.py @@ -854,7 +854,7 @@ def clone( break if not cloned: - self.raise_git_access_error(CloneRepoException) + self.raise_git_access_error(CloneRepoException, operation="clone") # the path is now a repository; drop any cached negative result from # before the clone @@ -1802,7 +1802,7 @@ 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="" ): hosts = { h for h in (extract_hostname(url) for url in self.urls) if h is not None @@ -1832,7 +1832,11 @@ def raise_git_access_error( else error_msg ), ) - raise error_cls(self, operation=operation) + raise error_cls( + self, + operation=operation, + message=(_repo_not_found_error_message if error_msg == "" else error_msg), + ) def remove_remote(self, remote_name: str) -> None: try: @@ -2226,6 +2230,19 @@ def _find_url(path, url): ) +_repo_not_found_error_message = ( + "The remote repository could not be found. Note that a private repository is " + "indistinguishable from a missing one when you are not authenticated, so this is " + "most likely an access/authentication problem rather than a wrong URL. Please:\n\n" + "1. Double-check the repository URL for typos.\n" + "2. If the repository is private, verify that your account has been granted access.\n" + "3. Verify your credentials are configured 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" From ddc8bf67decd1908a3bca6d885c8164a2c3a6bc7 Mon Sep 17 00:00:00 2001 From: oll-bot Date: Thu, 23 Jul 2026 17:29:37 -0400 Subject: [PATCH 2/5] test(git): lock the clone-error message fix and capture two open defects The prior commit fixed the "Cannot None " access error, but nothing guarded it against regressing, and the investigation surfaced two deeper defects that still need fixing. Encode all three as tests so intent is executable rather than living only in a commit message. Passing tests pin the shipped behaviour: the access error names the git operation ("Cannot clone", never "Cannot None"), the default operation is never None, and the "repository could not be found" fallback carries actionable guidance instead of being a bare error. Two xfail(strict=True) regression tests document the not-yet-fixed defects so they stay red until addressed and turn into a hard failure (prompting removal of the marker) the moment a fix makes them pass: - run() does command[0].split(), so `git -C ` is shattered at the space. A repo path under a home dir with a space in it breaks every clone - the field report that started this. Proven here both end-to-end (clone into a spaced path) and at the _git boundary. - clone() catches the real per-URL git error and logs it only at debug, then raises a generic CloneRepoException that omits it - so the actionable "fatal: cannot change to ..." never reaches the user. This is the recurring "taf swallows the real error" problem. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G --- taf/tests/test_git/test_clone_errors.py | 150 ++++++++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 taf/tests/test_git/test_clone_errors.py 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..938818ef --- /dev/null +++ b/taf/tests/test_git/test_clone_errors.py @@ -0,0 +1,150 @@ +"""Tests around the clone/access error path in ``taf.git``. + +Two things are covered here: + +* the committed fix that stopped the access error from rendering as + ``"Cannot None ..."`` and gave the "repository could not be found" + fallback an actionable message (these pass); and +* the two defects the clone investigation surfaced but did not yet fix - a + command line containing a path with spaces is mangled by ``run()``'s + whitespace split, and the real underlying git error is swallowed (logged at + debug) instead of being surfaced on the raised exception. Those are encoded + as ``xfail(strict=True)`` regression tests so they stay red until fixed and + flip to a hard failure (prompting removal of the marker) once they are. +""" + +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 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 + + +# --------------------------------------------------------------------------- # +# Open defect 1: `run()` splits the command on whitespace, so a repository path +# containing a space (e.g. a Windows home dir "C:\\Users\\Given Surname\\...") +# breaks the `git -C ` argument and every clone attempt fails. +# --------------------------------------------------------------------------- # + + +@pytest.mark.xfail( + strict=True, + reason=( + "run() does command[0].split(), shattering `git -C `; " + "clone into a spaced path fails. Fix by passing argv as a list / quoting." + ), +) +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 + + +@pytest.mark.xfail( + strict=True, + reason="run() splits `git -C ...` on whitespace", +) +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" + + +# --------------------------------------------------------------------------- # +# Open defect 2: the real per-URL git error (which explains *why* the clone +# failed) is only logged at debug and dropped; the surfaced CloneRepoException +# replaces it with a generic guess. +# --------------------------------------------------------------------------- # + + +@pytest.mark.xfail( + strict=True, + reason=( + "clone() catches the per-URL GitError and logs it at debug only; the " + "underlying git message never reaches the raised CloneRepoException." + ), +) +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() + + assert sentinel in str(exc_info.value) From e40fa9ac42d75da05e48037b82b3c14f5690ba71 Mon Sep 17 00:00:00 2001 From: oll-bot Date: Thu, 23 Jul 2026 17:40:54 -0400 Subject: [PATCH 3/5] fix(git): survive spaced repo paths and surface the real clone error A private-repo clone was failing on Windows with a useless message, and the investigation found the failure had nothing to do with access. Three linked changes, each turning one of the just-committed xfail regression tests green. 1. Spaced paths (the actual field bug). _git built the whole invocation as one string and run() whitespace-splits it, so `git -C ` was shattered the moment the repository path contained a space - which it does under a home dir like the reporter's. git then got a truncated -C directory and every clone attempt failed. _git now builds an argv list and passes the path as its own token; cmd.split() keeps the command's own tokenization identical, so no other git invocation changes (the full test_git suite is unchanged). 2. Swallowed errors. clone() caught each per-URL GitError and logged it only at debug, then raised a generic CloneRepoException that omitted it - so the real "fatal: cannot change to ..." never reached the user. The per-URL failures are now collected and surfaced on the exception, alongside (not instead of) the guidance. This is the recurring "taf hides the real error". 3. Misleading guidance. The not-found fallback asserted the cause was "most likely access/authentication" - exactly wrong for a local command- construction bug. It now defers to the concrete git error shown above it and presents the access/URL/credential possibilities without over-claiming. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G --- taf/git.py | 97 +++++++++++++++---------- taf/tests/test_git/test_clone_errors.py | 60 ++++++--------- 2 files changed, 81 insertions(+), 76 deletions(-) diff --git a/taf/git.py b/taf/git.py index 1db3d6ff..d0ff99fa 100644 --- a/taf/git.py +++ b/taf/git.py @@ -292,14 +292,21 @@ def _git(self, cmd, *args, **kwargs): if len(args): cmd = cmd.format(*args) + # Build an argv list rather than one string: run() whitespace-splits a + # single-string command, which shatters `git -C ` when the + # repository path contains a space (e.g. a Windows home dir with a space + # in the user name). Passing the path as its own token keeps it intact, + # while cmd.split() preserves the previous tokenization of the command + # itself, so no other git invocation changes. + 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}"] + command += cmd.split() + 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 +321,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 +330,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,6 +841,7 @@ 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: @@ -848,13 +856,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, operation="clone") + 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 @@ -1802,40 +1813,47 @@ def push( raise PushFailedError(self, message=f"Push operation failed: {e}") def raise_git_access_error( - self, error_cls=GitAccessDeniedException, operation="access", error_msg="" + self, + error_cls=GitAccessDeniedException, + operation="access", + error_msg="", + underlying_errors=None, ): + # 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 - ), - ) + 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=(_repo_not_found_error_message if error_msg == "" else error_msg), + message="\n\n".join(parts) if parts else None, ) def remove_remote(self, remote_name: str) -> None: @@ -2231,12 +2249,13 @@ def _find_url(path, url): _repo_not_found_error_message = ( - "The remote repository could not be found. Note that a private repository is " - "indistinguishable from a missing one when you are not authenticated, so this is " - "most likely an access/authentication problem rather than a wrong URL. Please:\n\n" - "1. Double-check the repository URL for typos.\n" - "2. If the repository is private, verify that your account has been granted access.\n" - "3. Verify your credentials are configured for this host:\n" + "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" diff --git a/taf/tests/test_git/test_clone_errors.py b/taf/tests/test_git/test_clone_errors.py index 938818ef..1dbfef98 100644 --- a/taf/tests/test_git/test_clone_errors.py +++ b/taf/tests/test_git/test_clone_errors.py @@ -1,16 +1,15 @@ """Tests around the clone/access error path in ``taf.git``. -Two things are covered here: - -* the committed fix that stopped the access error from rendering as - ``"Cannot None ..."`` and gave the "repository could not be found" - fallback an actionable message (these pass); and -* the two defects the clone investigation surfaced but did not yet fix - a - command line containing a path with spaces is mangled by ``run()``'s - whitespace split, and the real underlying git error is swallowed (logged at - debug) instead of being surfaced on the raised exception. Those are encoded - as ``xfail(strict=True)`` regression tests so they stay red until fixed and - flip to a hard failure (prompting removal of the marker) once they are. +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 @@ -58,7 +57,7 @@ def test_raise_git_access_error_for_clone_names_clone_not_none(tmp_path, monkeyp 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 found" in message + assert "could not be reached or found" in message def test_raise_git_access_error_default_operation_is_not_none(tmp_path, monkeypatch): @@ -75,19 +74,12 @@ def test_raise_git_access_error_default_operation_is_not_none(tmp_path, monkeypa # --------------------------------------------------------------------------- # -# Open defect 1: `run()` splits the command on whitespace, so a repository path +# 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\\...") -# breaks the `git -C ` argument and every clone attempt fails. +# no longer shatters the `git -C ` argument. # --------------------------------------------------------------------------- # -@pytest.mark.xfail( - strict=True, - reason=( - "run() does command[0].split(), shattering `git -C `; " - "clone into a spaced path fails. Fix by passing argv as a list / quoting." - ), -) def test_clone_into_path_with_spaces(origin_repo: GitRepository, tmp_path): dest = Path(tmp_path) / "Given Surname" / "clone" dest.mkdir(parents=True) @@ -99,10 +91,6 @@ def test_clone_into_path_with_spaces(origin_repo: GitRepository, tmp_path): assert repo.is_git_repository -@pytest.mark.xfail( - strict=True, - reason="run() splits `git -C ...` on whitespace", -) 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 @@ -119,20 +107,14 @@ def test_git_subprocess_handles_spaced_repo_path(tmp_path): # --------------------------------------------------------------------------- # -# Open defect 2: the real per-URL git error (which explains *why* the clone -# failed) is only logged at debug and dropped; the surfaced CloneRepoException -# replaces it with a generic guess. +# The real per-URL git error (which explains *why* the clone failed) is +# surfaced on the raised CloneRepoException, not swallowed at debug level. # --------------------------------------------------------------------------- # -@pytest.mark.xfail( - strict=True, - reason=( - "clone() catches the per-URL GitError and logs it at debug only; the " - "underlying git message never reaches the raised CloneRepoException." - ), -) -def test_clone_surfaces_underlying_git_error(clone_repository: GitRepository, monkeypatch): +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" @@ -147,4 +129,8 @@ def fake_git(*args, **kwargs): with pytest.raises(CloneRepoException) as exc_info: clone_repository.clone() - assert sentinel in str(exc_info.value) + 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 From 6ec836d6d19b52405df9a1da4231f9bd4733b194 Mon Sep 17 00:00:00 2001 From: oll-bot Date: Thu, 23 Jul 2026 18:07:39 -0400 Subject: [PATCH 4/5] docs(git): changelog entry and docstring for the clone-error fix Document the clone-failure fixes (spaced paths, surfaced git error, corrected message) in the changelog, and give raise_git_access_error a docstring now that it takes an underlying_errors argument. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G --- CHANGELOG.md | 4 ++++ taf/git.py | 8 ++++++++ 2 files changed, 12 insertions(+) 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 d0ff99fa..d9db5c73 100644 --- a/taf/git.py +++ b/taf/git.py @@ -1819,6 +1819,14 @@ def raise_git_access_error( 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 From c7f8948d23880cf6bfd860cc206e39724cfe2164 Mon Sep 17 00:00:00 2001 From: oll-bot Date: Thu, 23 Jul 2026 19:18:20 -0400 Subject: [PATCH 5/5] fix(git): keep spaced paths intact in {} args, not just the repo path The first pass only protected `git -C `. On Windows the clone then got one step further and failed at the users-auth `git clone --local .` with "fatal: Too many arguments", because the source was a temp dir under a home directory with a space and `_git` still whitespace-split it into two tokens. Any argument with a space had the same problem, not just the -C path. _git now tokenizes the command template and fills each `{}` placeholder in place, so a substituted value stays a single argv token regardless of spaces. Empty tokens are dropped to match the old split (optional flag args like push's force/no-verify are passed as ""). Callers that had interpolated a path or URL straight into the command string now pass it as a `{}` arg instead: clone_from_disk's local source, set_remote_url, and ls-remote. clone() keeps its flag bundle in the template (controlled flags, meant to expand to multiple tokens) and passes only the URL as an argument. The full test_git suite result is unchanged (identical failure set), and a new regression test clones from a spaced source path. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G --- taf/git.py | 45 ++++++++++++++++++------- taf/tests/test_git/test_clone_errors.py | 18 ++++++++++ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/taf/git.py b/taf/git.py index d9db5c73..bb421124 100644 --- a/taf/git.py +++ b/taf/git.py @@ -290,18 +290,28 @@ 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 `git -C ` when the - # repository path contains a space (e.g. a Windows home dir with a space - # in the user name). Passing the path as its own token keeps it intact, - # while cmd.split() preserves the previous tokenization of the command - # itself, so no other git invocation changes. + # 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 += ["-c", f"safe.directory={self.path}"] - command += cmd.split() + 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: @@ -845,10 +855,12 @@ def clone( 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, @@ -900,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, ) @@ -1513,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) @@ -1961,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.""" diff --git a/taf/tests/test_git/test_clone_errors.py b/taf/tests/test_git/test_clone_errors.py index 1dbfef98..c6eb4fe3 100644 --- a/taf/tests/test_git/test_clone_errors.py +++ b/taf/tests/test_git/test_clone_errors.py @@ -91,6 +91,24 @@ def test_clone_into_path_with_spaces(origin_repo: GitRepository, tmp_path): 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