Fix Coverity NullReferenceException Issues w/ Remotes #1083
Conversation
|
|
||
| if (url != null) | ||
| { | ||
| remoteHandle = Proxy.git_remote_create_anonymous(repoHandle, url); | ||
| } | ||
| else | ||
| else if (remote != null) |
There was a problem hiding this comment.
We're actually asserting to protect against this. And the call to Debug.Assert is "seen" by Coverity, however it looks like the tool doesn't properly take it into account in the code flow analysis.
There was a problem hiding this comment.
yeah - the ^ operator is the exclusive-or operator.
There was a problem hiding this comment.
Coverity is correct. We in fact do assert but only in debug mode. We then have code flow for when both are null which should be an impossibility. We even dereference a null value in the malformed code flow that we assert, but do not check, will not happen.
There was a problem hiding this comment.
this is correct. However, this is not a public method so we do control all callers of this method. Either way, I agree that this is a bit fragile. What would you think about throwing an exception in the case where we get unexpected input? Or, performing the refactoring suggested in #1084. As you mentioned, I think everyone agrees on the ultimate structure for this code (two methods).
|
Superceded by #1087 |
The issue here is that the method asserts that both parameters are null or neither are, then proceeds to behave like only one is. This results in a NRE too deep into the stack to easily debug/resolve the issue. Moving the error upstream makes it easier to identify the problem.
Question: should we just throw at the call-site if both arguments (
remoteandurl) arenull?Included some beautification as a separate commit.