fix(git): make clone failures diagnosable (spaced paths, swallowed errors, wrong message) - #762
Open
dgreisen-llm wants to merge 5 commits into
Open
fix(git): make clone failures diagnosable (spaced paths, swallowed errors, wrong message)#762dgreisen-llm wants to merge 5 commits into
dgreisen-llm wants to merge 5 commits into
Conversation
Cloning a private repo you can't authenticate to produced the useless
message "Cannot None <repo> 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G
The prior commit fixed the "Cannot None <repo>" 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 <path with spaces>` 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G
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 <path>` 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G
|
|
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G
The first pass only protected `git -C <path>`. On Windows the clone then got
one step further and failed at the users-auth `git clone --local <source> .`
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N1ffXgu41hkJFqWmq6T79G
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #761.
taf repo cloneof a private repo was failing withCannot None <repo> from any of the following URLs: [...]and no useful guidance. The investigation showed the failure had nothing to do with access — it was three linked defects in the clone/access error path:_gitbuilt the git invocation as one string andrun()whitespace-splits it, sogit -C <path>was shattered whenever the repository path contained a space — e.g. a Windows home directory with a space in the user name, the reporter's case._gitnow builds an argv list with the path as its own token; the command's own tokenization is unchanged, so no other git call is affected (the fulltest_gitsuite has an identical result with and without the change).clone()logged each per-URL git failure at debug and dropped it, then raised a genericCloneRepoException. The concretefatal: cannot change to ...is now collected and surfaced on the exception, alongside (not instead of) the guidance.raise_git_access_errorrenderedoperation=Noneas "Cannot None", and its fallback asserted the cause was "most likely access/authentication" — exactly wrong for a local command-construction failure. The operation is now named, and the fallback defers to the concrete git error shown above it.How to test
pytest taf/tests/test_git/test_clone_errors.py— 6 tests..../Given Surname/...directory and assert the clone succeeds; the swallowing test asserts the real git error appears on the raisedCloneRepoException.Code review checklist (for code reviewer to complete)