StashSaveOptions::pathspec(): return a Result, handle invalid values#1295
StashSaveOptions::pathspec(): return a Result, handle invalid values#1295DanielEScherzer wants to merge 2 commits into
StashSaveOptions::pathspec(): return a Result, handle invalid values#1295Conversation
… specs The test is currently marked as `#[should_panic]` because the implementation panics when the path given is invalid, a follow-up commit will switch to returning a `Result`.
When the provided path spec cannot be converted to a `CString`, return an `Err` variant rather than panicking.
|
@rustbot label +semver-breaking-change |
rustbot
added
the
semver-breaking-change
| /// Add to the array of paths patterns to build the stash. | ||
| pub fn pathspec<T: IntoCString>(&mut self, pathspec: T) -> &mut Self { | ||
| let s = util::cstring_to_repo_path(pathspec).unwrap(); | ||
| pub fn pathspec<T: IntoCString>(&mut self, pathspec: T) -> Result<&mut Self, Error> { |
There was a problem hiding this comment.
I know we kinda work towards this though it is still a bit annoying to users. Most of the people won't hit this issue and they need to take care of Result. I am a bit unsure.
Should we just make it an expect and document it would panic?
There was a problem hiding this comment.
We could, but I think it would be nicer to return a result - if the library is able to avoid a panic without being left in a problematic state, why not do so?
There was a problem hiding this comment.
Yes, and panic should be use mostly as a program invariant. This is kinda like a bad input. However, we can also see this as an invariant of these functions.
I am not sure though. I think technically it is more correct, though sacrifice ergonomic. And it may also be confusing as API caller now don't know how they should handle the error or not unless looking at the function body and realize it only checks bad cstring.
There was a problem hiding this comment.
We could also switch it to requiring that the provided value be a CString rather than just something that can be converted to one?
There was a problem hiding this comment.
That also loses ergonomic in another way 😞.
There was a problem hiding this comment.
i figured a result was more ergonomic than requiring a CString, because I think panicking is worse than either
There was a problem hiding this comment.
Can you point to a realistic workflow where an invalid pathspec reaches this API at runtime and the application needs to recover from it? Like in particular, is this intended for arbitrary user-provided data, or is it primarily addressing the fact that CString conversion is technically fallible?
Note that most OS and filesystem disallowed \0 in the file system
There was a problem hiding this comment.
So I just started working on writing an extension for PHP in Rust that integrates with the git2 rust library for the logic - and that is what I was thinking of when I wanted to reduce the use of panics
Basically, returning a result (from Rust) means that we can throw an exception (in PHP) without needing to do the validation manually
There was a problem hiding this comment.
Basically, returning a result (from Rust) means that we can throw an exception (in PHP) without needing to do the validation manually
I knew the mechanism. Ideally we want to recover from more errors, but in reality what case on application side would hit this? If PHP is going to throw an exception what the difference between that and Rust panic?
BTW, PHP seems to have https://github.com/libgit2/php-git.
There was a problem hiding this comment.
PHP exceptions can be caught by the userland code
https://github.com/libgit2/php-git is very much outdated (and I'm doing this to be able to present a workshop on writing extensions in Rust, not just for fun)
3 participants
When the provided path spec cannot be converted to a
CString, return anErrvariant rather than panicking.See also