-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bindings for git_index_find_prefix #903
Conversation
src/index.rs
Outdated
if result == 0 { | ||
Ok(Some(at_pos)) | ||
} else { | ||
Ok(None) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't ever seem to return an error. Should this perhaps be:
if result == 0 { | |
Ok(Some(at_pos)) | |
} else { | |
Ok(None) | |
} | |
if result == 0 { | |
Ok(Some(at_pos)) | |
} else if result == raw::GIT_ENOTFOUND { | |
Ok(None) | |
} else { | |
Err(Error::last_error(result).unwrap()) | |
} |
I understand git_index_find_prefix
currently never returns any other error, but I'm not very comfortable assuming that will be the case forever.
Also, I'm a little uncomfortable doing the ENOTFOUND check. I think it makes for a more natural Rust interface, but I don't see any other methods in the library doing that (they all just return NotFound). What were your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I imagined that returning None in situations when the prefix hasn't been found would be more rusty, but after reviewing some other similar functions, it seems like NotFound error is never handled this way, and as there's no point in going against the design of the rest of the library I just decided to leave it inside the Error, as you suggested.
I hope it's okay now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Plus a test that checks
find_prefix
andget_path