Skip to content

Document giterr_last() use only after error. #4772#4773

Merged
pks-t merged 4 commits into
libgit2:masterfrom
RandomSort:giterr
Aug 30, 2018
Merged

Document giterr_last() use only after error. #4772#4773
pks-t merged 4 commits into
libgit2:masterfrom
RandomSort:giterr

Conversation

@RandomSort

Copy link
Copy Markdown
Contributor

Hi, thank you for your awesome work.
This is a trivial documentation addition.
I'll be happy to rework it if it does not fit your wishes.

@ethomson

Copy link
Copy Markdown
Member

I'd like to tweak the wording here just a touch:

Do not use this method unless you received an error, it will lead to undefined behaviour.

"Undefined behavior" is, I think, a bit strong and suggests to the reader that it's C-like UB where you might crash or demons might fly out of your nose. Also, I'd say "function" or "API" instead of "method" because we're kicking it old-school C. Perhaps something more like:

Do not use this function unless the prior call to a libgit2 API returned an error, as it can give misleading results. libgit2's error strings are not cleared aggressively, and this function may return an error string that reflects a prior error, possibly even reflecting internal state.

We should also update the API documentation as well, it indicates that NULL is always returned when there was no recent error.

@RandomSort

Copy link
Copy Markdown
Contributor Author

That is a lot better. I will push update.

@RandomSort

Copy link
Copy Markdown
Contributor Author

I have added API documentation, and reworded as requested. Feel free to prioritize other stuff over this. It is minor change from an outsider :)

@RandomSort

Copy link
Copy Markdown
Contributor Author

It looks like the VSTS build is a false negative if I understand the output right.

@pks-t pks-t left a comment

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.

Two more proposed changes, but otherwise this looks good to me. Thanks for working on it!

I have added API documentation, and reworded as requested. Feel free to prioritize other stuff over this. It is minor change from an outsider :)

We've all started as outsiders doing minor changes at some point in time, so we try to keep feedback loops for new contributors as short as possible to hopefully incentivise them and become insiders :P That being said, it doesn't always work out -- time is limited, unfortunately.

Comment thread docs/error-handling.md Outdated
Otherwise this should return a `git_error` object indicating the class
of error and the error message that was generated by the library.
Do not use this function unless the prior call to a libgit2 API
returned an error, as it can give misleading results.

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.

"as it can otherwise give misleading results." Just to make sure that it may only be misleading in this exact case ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Word added.

Comment thread include/git2/errors.h Outdated
*
* The default behaviour of this function is to return NULL if no previous error has occurred.
* However, libgit2's error strings are not cleared aggressively, so a prior
* (unrelated) error may be returned. This can avoided by only calling

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.

"This can be avoided". Missing "be"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is embarrassing, fix pushed :)

@pks-t pks-t merged commit db0c664 into libgit2:master Aug 30, 2018
@pks-t

pks-t commented Aug 30, 2018

Copy link
Copy Markdown
Member

Thanks, @RandomSort!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants