Skip to content
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 git_libgit2_opts binding for get/set search path #656

Merged
merged 7 commits into from
Jan 6, 2021

Conversation

weihanglo
Copy link
Member

Prepare these bindings in order to fix rust-lang/cargo#8863.

Due to the same reason mentioned #126 (comment) , the bindings do not split strings to paths and leave the work for callers to call std::env::join_paths themselves.

@weihanglo
Copy link
Member Author

weihanglo commented Jan 1, 2021

Not sure how to handle path on Windows. Should get_search_path returns CString directly? Or just throw an invalid UTF-8 error like util::byte2path?

git2-rs/src/util.rs

Lines 91 to 95 in 6880563

#[cfg(windows)]
pub fn bytes2path(b: &[u8]) -> &Path {
use std::str;
Path::new(str::from_utf8(b).unwrap())
}

@weihanglo
Copy link
Member Author

weihanglo commented Jan 1, 2021

Also, ccb48dd makes it thread-safe to access global state of search path, but have no idea is that necessary.

@weihanglo
Copy link
Member Author

Not sure how to handle path on Windows. Should get_search_path returns CString directly? Or just throw an invalid UTF-8 error like util::byte2path?

e5d8643 fix: get_search_path will returns CString

I think returning a CString is more straight foward since set_search_search also accept an IntoCString type.

@alexcrichton
Copy link
Member

This seems generally fine but the global-state-ness is worrisome with respect to how this is being used in rust-lang/cargo#9035. If this doesn't work for that PR would you still like to land this?

@weihanglo
Copy link
Member Author

weihanglo commented Jan 4, 2021

This seems generally fine but the global-state-ness is worrisome with respect to how this is being used in rust-lang/cargo#9035. If this doesn't work for that PR would you still like to land this?

If i didn't get it wrong, from the view of git2-rs itself, the global state in git2-rs just reflects the global state in libgit2. To make git2-rs as safe as possible (that's one of it's goal 😂), I would choose an approach similar to this one no matter how callers use it.

@alexcrichton
Copy link
Member

Ok, in that case though I think it may be better to flag these functions as unsafe. They're not safe to mix with other configuration functions in multiple threads and this probably has to happen near the front of libgit2's usage rather than later. I think the synchronization can only really be left up to the caller since we don't know about all the calls to libgit2's configuration functions.

@weihanglo
Copy link
Member Author

Got it. I'll remove the synchronization part. Thanks!

@alexcrichton
Copy link
Member

Looks good! As one final thing though, I think the test may wantt to move to an integration test in something like tests/foo.rs? Otherwise I don't think we can guarantee that it's synchronized with all other tests.

@weihanglo
Copy link
Member Author

Thanks for your advice. The test has been moved to a integration test at adc0ee8.

I have a dumb question to task. If I understand it correctly, the reason moving it to a integration test is that if people what to mutate the same global state, it can edit the "integration test" directly without touching "unit test" part, right?

@alexcrichton alexcrichton merged commit 252f41b into rust-lang:master Jan 6, 2021
@alexcrichton
Copy link
Member

👍

@weihanglo weihanglo deleted the feat/search_path branch January 6, 2021 15:25
bors added a commit to rust-lang/cargo that referenced this pull request Jan 22, 2021
Fix: set default git config search path for tests

Fixes #8863 by setting the default config search path.

Just wait rust-lang/git2-rs#656 being merged and update Cargo.toml the new release of git2-rs 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo test suite fails if default branch is not master
2 participants