-
Notifications
You must be signed in to change notification settings - Fork 405
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 binding for git_repository_open_ext #126
Add binding for git_repository_open_ext #126
Conversation
I intentionally didn't include the version bumps for libgit2-sys or git2 in this pull request, because I think it makes sense to wait until libgit2/libgit2#3711 and then the rest of #123 get merged, and then bump both to 0.5.0 given the new APIs. |
/// Find and open a repository, with additional options. | ||
pub fn open_ext<P: AsRef<Path>>(path: P, | ||
flags: RepositoryOpenFlags, | ||
ceiling_dirs: Option<&str>) |
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.
I believe that this is actually a naturally-separated list of directories, so perhaps this could be an iterator of T: AsRef<OsStr>
? I think there's some other APIs for consuming an iterator like that elsewhere as well.
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.
There's iter2cstrs
, but that just constructs a git_strarray
. git_repository_open_ext
takes a single string of paths separated by GIT_PATH_LIST_SEPARATOR
.
Also, if the caller already has this parameter in the form libgit2 expects, it seems unfortunate to have to split it just to have git2-rs re-join it.
I think it makes more sense for the caller to call std::env::join_paths
themselves, rather than having open_ext
do it for them; that way, if you already have a pre-joined path in the form you'd get it from $GIT_CEILING_DIRECTORIES
, you can just pass that.
That all sounds good to me, thanks @joshtriplett! |
I expanded the documentation for |
/// list separator) that the search through parent directories will stop | ||
/// before entering. Use the functions in std::env to construct or | ||
/// manipulate such a path list. | ||
pub fn open_ext<P: AsRef<Path>>(path: P, |
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.
I actually tend to use the bound IntoCString
in this repo nad that should help elide the .as_ref()
below as well
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.
For the path? Sure, I can do that. I'd modeled this after other functions in git::Repository
, which also took AsRef<Path>
, but IntoCString
seems easy enough to switch to.
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.
Oh weird, I should probably change those to IntoCString
at some point. In that case feel free to leave 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.
OK, will do.
OK, the version I just pushed uses an I didn't manage to get an I also find it unfortunate that, in the common case of not passing |
Oh yeah Gonna go ahead and merge for now, thanks @joshtriplett! |
oops |
This addresses part of #123.