Skip to content

Show human-readable git errors #8152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Dec 7, 2024
Merged

Show human-readable git errors #8152

merged 12 commits into from
Dec 7, 2024

Conversation

yyvch
Copy link
Contributor

@yyvch yyvch commented Dec 3, 2024

Show human-readable git errors

Motivation:

I noticed that when swiftpm cache is empty and there's a network issue, then swift build throws an error which is not helpful, something like this:

error: GitShellError(result: <AsyncProcessResult: exit: terminated(code: 128), output:
 
>)

where it does say "output" but doesn't actually include output or show any details about what dependency caused the error.

Modifications:

  • GitShellError now conforms to CustomStringConvertible protocol, so that when it is thrown then it will print what was the git error
  • GitShelError has now package access to allow catching it explicitly.

Result:

Errors now tell details of the failure:

error: Git command 'git -C /Users/yy/Library/Caches/org.swift.swiftpm/repositories/postgres-kit-e4358808 config --get remote.origin.url' failed: fatal: cannot change to '/Users/yy/Library/Caches/org.swift.swiftpm/repositories/postgres-kit-e4358808': No such file or directory

yyvch added 3 commits December 2, 2024 18:35
### Motivation

Now and then `swift build` throws a GitShellError which doesn't have
details about what was an issue and with which dependency.

### Modification

GitShellError now conforms to CustomStringConvertible protocol which
makes it printable.

### Result

`swift build` now shows the error output from the git command
instead of showing an error code when GitShellError is thrown.
… git repo.

### Motivation

isValidDirectory is called before a repo URL is established, and before a context
to throw a meaningful error is established. The errors that are thrown are not useful
to humans.

### Modification

This change supresses the GitShellError errors and allows to establish a context
for more meaningful GitCloneError errors.

### Result

Before the change:

> error: GitShellError:     fatal: cannot change to '/Users/yy/Library/Caches/org.swift.swiftpm/repositories/postgres-kit-e4358808': No such file or directory

After the change:

> error: Failed to clone repository https://github.com/vapor/postgres-kit.git:
>     Cloning into bare repository '/Users/yy/work/pub/tst/testpackage/.build/repositories/postgres-kit-e4358808'...
>     fatal: unable to access 'https://github.com/vapor/postgres-kit.git/': Could not resolve host: github.com
yyvch added 3 commits December 3, 2024 13:17
Revert "Do not throw an error from isValidDirectory if the directory is not a git repo."

This reverts commit 1a5db35.

`isValidDirectory` is a public API and we should avoid changing behavior, since
external code might depend on raised exception for the flow when the target
directory isn't a git repo or when git itself isn't available.
GitShelError can be thrown in public `isValidDirectory`, make the
error public too, so it can be caught.
Git command might be helpful to reproduce access issues, let's include
it in the description of GitShelError.
The exception has been private prior to 301fa08. There's not
yet usecase where public is necessary, let's keep it package
until we do have a usecase.
@yyvch yyvch requested a review from MaxDesiatov December 4, 2024 19:58
@plemarquand
Copy link
Contributor

@swift-ci test

Copy link
Contributor

@bripeticca bripeticca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just noticed a few typos 😄

yyvch added 2 commits December 4, 2024 16:06
XCTAssertThrowsError and inspecting the error is more accurate assert
than catching the exception ourselves.

Previous version: test passes if the error isn't raised
Current version: test fails if the error isn't raised.
@yyvch yyvch requested review from weissi and bripeticca December 5, 2024 00:57
@plemarquand
Copy link
Contributor

@swift-ci test

@dschaefer2
Copy link
Member

@swift-ci please test

@dschaefer2
Copy link
Member

@swift-ci please test windows

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@yyvch yyvch requested a review from MaxDesiatov December 5, 2024 17:52
@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@xedin xedin merged commit 0d990b2 into swiftlang:main Dec 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug diagnostics source control Changes to SCM-related code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants