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.

Sign up for GitHub

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

Jump to bottom

Fix: set default git config search path for tests #9035

Merged
merged 3 commits into from
Jan 22, 2021
Merged

Fix: set default git config search path for tests #9035

merged 3 commits into from
Jan 22, 2021

Conversation

weihanglo
Copy link
Member

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 😄

@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2021
@alexcrichton
Copy link
Member

Thanks for the PR! I don't think this is the correct solution, however, because each thread has its own unique search path, right? The libgit2 search path is a global configuration option so if all threads are always changing it then I don't think that it is guaranteed to be configured correctly for each test?

@alexcrichton alexcrichton mentioned this pull request Jan 4, 2021
Merged
@weihanglo
Copy link
Member Author

Thanks for the PR! I don't think this is the correct solution, however, because each thread has its own unique search path, right? The libgit2 search path is a global configuration option so if all threads are always changing it then I don't think that it is guaranteed to be configured correctly for each test?

My fault. I didn't notice that 😅. The original goal is to prevent global configurations from affecting tests. What if we set the search path to nowhere (maybe a empty directory?) and let git configs keep the defaults?

@alexcrichton
Copy link
Member

That seems reasonable to me, yeah, having a sort of one-time configuration which points it to a blank location.

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2021
alexcrichton
alexcrichton reviewed Jan 20, 2021
View reviewed changes
crates/cargo-test-support/src/git.rs Outdated
fn default_search_path() {
use crate::paths::GLOBAL_ROOT;
use git2::{opts::set_search_path, ConfigLevel};
lazy_static::lazy_static! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW instead of using lazy_static this can use a sync::sync::Once

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

This pull request is almost ready for review. Is there any plan to rollout a new release of git2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure yeah, I just published a new version.

@weihanglo weihanglo force-pushed the fix/git-init-search-path branch 2 times, most recently from 4a47774 to 86be6d4 Compare January 21, 2021 06:36
@weihanglo weihanglo force-pushed the fix/git-init-search-path branch 2 times, most recently from 35e2277 to 332887e Compare January 22, 2021 04:53
@weihanglo weihanglo marked this pull request as ready for review January 22, 2021 06:07
@weihanglo
Copy link
Member Author

Since git2-rs#0.13.16 is released. This pull request is now ready for review.

@alexcrichton
Copy link
Member

Thanks! Could this bump the version requirement on git2 to ensure it pulls in the required version as well? That'll help updates in existing projects like rust-lang/rust which already have a lock file pointing to the older version of git2

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 22, 2021

📌 Commit 06d65a4 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jan 22, 2021
@bors
Copy link
Contributor

bors commented Jan 22, 2021

⌛ Testing commit 06d65a4 with merge 638f33d...

@bors
Copy link
Contributor

bors commented Jan 22, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 638f33d to master...

@bors bors merged commit 638f33d into rust-lang:master Jan 22, 2021
@weihanglo weihanglo deleted the fix/git-init-search-path branch January 22, 2021 16:07
@ehuss ehuss mentioned this pull request Jan 27, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 28, 2021
@JohnTitor
Rollup merge of rust-lang#81445 - ehuss:update-cargo, r=ehuss
f3dfbfc 
Update cargo

7 commits in 783bc43c660bf39c1e562c8c429b32078ad3099b..c3abcfe8a75901c7c701557a728941e8fb19399e
2021-01-20 19:02:26 +0000 to 2021-01-25 16:16:43 +0000
- Minor update to tracking issue template. (rust-lang/cargo#9097)
- Add some extra help to `cargo new` and invalid package names. (rust-lang/cargo#9098)
- Fix compilation with serde 1.0.122 (rust-lang/cargo#9102)
- Add suggestion for bad package id. (rust-lang/cargo#9095)
- Remove Registry::new. (rust-lang/cargo#9093)
- Fix: set default git config search path for tests (rust-lang/cargo#9035)
- Unstable updates (rust-lang/cargo#9092)
@ehuss ehuss added this to the 1.51.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton Awaiting requested review from alexcrichton

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

cargo test suite fails if default branch is not master
5 participants
@weihanglo @rust-highfive @alexcrichton @bors @ehuss