Add bindings for git_reference_name_is_valid, git_remote_name_is_vali… #882
Conversation
sanjayts
force-pushed
the
git2rs_714
branch
from
e86688f
to
4581880
Compare
|
lgtm |
|
@extrawurst Great, can this please be merged? |
src/reference.rs
Outdated
| pub fn name_is_valid(refname: &str) -> Result<bool, Error> { | ||
| crate::init(); | ||
| let refname = CString::new(refname)?; | ||
| let mut valid: libc::c_int = 0; | ||
| unsafe { | ||
| try_call!(raw::git_reference_name_is_valid( | ||
| &mut valid, | ||
| refname.as_ptr() | ||
| )); | ||
| } | ||
| Ok(valid == 1) |
There was a problem hiding this comment.
Since this is identical to git_reference_is_valid_name, instead of adding a new method, can you just update the is_valid_name to call the new git_reference_name_is_valid function?
Otherwise, it seems quite confusing to have two nearly identically named functions which do the exact same thing.
There was a problem hiding this comment.
@ehuss You bring up a good point, though one concern I have here is deciding between:
- Silently gobbling up error and panicking as done in the existing implementing by calling unwrap (
let refname = CString::new(refname).unwrap();) - Updating the existing contract (and make a breaking change) by returning a Result as opposed to a scalar bool
- Deprecating existing
name_is_validand go with the PR changes
WDYT?
There was a problem hiding this comment.
Hm. I believe the only way git_reference_name_is_valid can return an error is on OOM, right? I would be inclined to just panic if git_reference_name_is_valid fails. I probably wouldn't bother with changing the existing behavior with CString::new, but if I were starting from scratch, I would also change that to return false since internal nuls aren't really valid.
I think perhaps in a future semver breaking release, the method could be changed to return a Result, but I probably wouldn't bother.
There was a problem hiding this comment.
@ehuss I've made the changes -- I've also added comments to the lib.rs file to ensure there is no confusion around why we don't use a given binding.
There was a problem hiding this comment.
I'm a bit confused by the current PR since it seems to still have two methods doing the same thing. Would it be possible to keep just is_valid_name with its signature and panic if git_reference_name_is_valid returns an error? I'm not sure the error return is particularly useful here, and I'd like to avoid complicating things.
src/remote.rs
Outdated
| @@ -95,6 +95,20 @@ impl<'repo> Remote<'repo> { | |||
| unsafe { raw::git_remote_is_valid_name(remote_name.as_ptr()) == 1 } | |||
| } | |||
|
|
|||
| /// Ensure the remote name is well-formed. | |||
| pub fn name_is_valid(remote_name: &str) -> Result<bool, Error> { | |||
There was a problem hiding this comment.
Same comment about reusing the existing method to call the new API.
sanjayts
force-pushed
the
git2rs_714
branch
3 times, most recently
from
5487a54
to
e9bc0f1
Compare
|
Just wanted to check in to see if this can be merged or should I close this PR? |
libgit2-sys/lib.rs
Outdated
| @@ -2246,7 +2246,14 @@ extern "C" { | |||
| remote: *const git_remote, | |||
| ) -> c_int; | |||
| pub fn git_remote_get_refspec(remote: *const git_remote, n: size_t) -> *const git_refspec; | |||
|
|
|||
| // This function is no longer used by kept here for the sake of completeness and to ensure | |||
There was a problem hiding this comment.
Not sure about the wording here?
| // This function is no longer used by kept here for the sake of completeness and to ensure | |
| // This function is no longer used but is kept here for the sake of completeness and to ensure |
Also, I'm not really sure this comment is necessary. There are quite a few other functions that I believe are no longer used. We don't do any sort of auditing of what is missing or not.
There was a problem hiding this comment.
@ehuss I have removed the comments and actioned the changes requested below. Please take a look.
I still think there is scope for improvement of the API by propagating errors but I'm assuming that can only be done with a major release. LMK if you want me to create a ticket for this?
src/reference.rs
Outdated
| pub fn name_is_valid(refname: &str) -> Result<bool, Error> { | ||
| crate::init(); | ||
| let refname = CString::new(refname)?; | ||
| let mut valid: libc::c_int = 0; | ||
| unsafe { | ||
| try_call!(raw::git_reference_name_is_valid( | ||
| &mut valid, | ||
| refname.as_ptr() | ||
| )); | ||
| } | ||
| Ok(valid == 1) |
There was a problem hiding this comment.
I'm a bit confused by the current PR since it seems to still have two methods doing the same thing. Would it be possible to keep just is_valid_name with its signature and panic if git_reference_name_is_valid returns an error? I'm not sure the error return is particularly useful here, and I'd like to avoid complicating things.
…d & git_tag_name_is_valid
This fixes #714