Skip to content

Fixing Coverity Defects Part Deux#1130

Closed
whoisj wants to merge 5 commits into
libgit2:vNextfrom
whoisj:coverity-fixes
Closed

Fixing Coverity Defects Part Deux#1130
whoisj wants to merge 5 commits into
libgit2:vNextfrom
whoisj:coverity-fixes

Conversation

@whoisj

@whoisj whoisj commented Jul 1, 2015

Copy link
Copy Markdown

Another round of fixes for Coverity discovered defects.

Please closely review the NRE fixes, as there are likely better ways to resolve them that I have no considered.

@nulltoken nulltoken changed the title Fixing Coverity Defects Part Duex Fixing Coverity Defects Part Deux Jul 1, 2015
Comment thread LibGit2Sharp/Patch.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mono-style hits LibGit2Sharp for 42 damage points.

@whoisj

whoisj commented Jul 2, 2015

Copy link
Copy Markdown
Author

@Therzok update made, can you please re-review? 🙏

@Therzok

Therzok commented Jul 2, 2015

Copy link
Copy Markdown
Member

Looks good to me otherwise 👍

@whoisj

whoisj commented Jul 2, 2015

Copy link
Copy Markdown
Author

@Therzok 🙇 thank you sir.

@nulltoken you're up: what's your opinion about return -1 vs throw new InvalidOperationException in this situation?

@Therzok

Therzok commented Jul 8, 2015

Copy link
Copy Markdown
Member

This needs a rebase.

@whoisj

whoisj commented Jul 8, 2015

Copy link
Copy Markdown
Author

This needs a rebase.

Done. Not painful - sorry to disappoint 😜

Comment thread LibGit2Sharp/Patch.cs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure about. This will brutally flow back to libgit2.

Is there even a case when currentChange could actually be null?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Is there even a case when currentChange could actually be null?

@nulltoken Coverity sure seems to think there is. Perhaps in some kind of repo corruption case? Not sure really. Null checking doesn't seem like a bad thing. Very little overhead and it would be correct to route and error back to the base lib if the value were null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants