Handle exceptions and null returns from CredentialsProvider. #1239
Conversation
|
👍 yes this is a good change. Thanks @chescock . |
| try | ||
| { | ||
| var cred = CredentialsProvider(url, username, types); | ||
| if (cred == null) |
There was a problem hiding this comment.
Now that we get rid of a potential NullReferenceException, what would be the behavior from the user perspective of trying to fetch a (private) repository without passing a CredentialsProvider?
There was a problem hiding this comment.
A LibGit2SharpException exception will be thrown.
When using WinHTTP, passing null for CredentialsProvider currently throws an exception with the message "Request failed with status code: 401". With this change and libgit2/libgit2#3528 together, passing a CredentialsProvider that returns a null Credentials will have the same message. With just this change, it will instead have the message "No error message has been provided by the native library". I think the other transports will have the message "authentication required but no callback set" in all of those cases.
There was a problem hiding this comment.
Thanks for your answer. Considering the less than helpful message (from a user perspective), I'd rather wait for the libgit2 PR to be merged before merging this in.
/cc @carlosmn
There was a problem hiding this comment.
@nulltoken I think the libgit2 changes were pulled in with #1264, so this will now give friendly messages in all cases.
|
@carlosmn with @nulltoken on sabbatical, any chance you can take a look at this? I'm happy with it. |
|
Merged manually into master. |
Add exception handling and null checking around the call to the
CredentialsProvidercallback andGitCredentialHandlermethod. Without these, exceptions will cross the boundary into C code, which I believe is bad.I got here because I want to supply a
CredentialsProviderthat only applies to certain hosts, and this provides a way to return an error code to libgit2 when I have no credentials to provide.Pull request #1130 would add a null check here, but not other exception handling. It would translate null to
GitErrorCode.Error, while I'm proposing to translate it toGitErrorCode.PassThroughso that a null credentials object behaves like a null credentials provider.