Skip to content

Commit

Permalink
Auto merge of #14730 - weihanglo:env, r=ehuss
Browse files Browse the repository at this point in the history
refactor(env): remove unnecessary clones

### What does this PR try to resolve?

This was found during the review of #14701.

Not every environment variable access requires an owned type.
To be precise, most of our usages don't.

### How should we test and review this PR?

Refactor only. Should not have any change in behavior.

One drawback is that the fn signatures deviate from `std::env::var{_os}`.
  • Loading branch information
bors committed Oct 25, 2024
2 parents 28274d0 + ce95009 commit e75214e
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 24 deletions.
23 changes: 9 additions & 14 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,31 +837,26 @@ impl LocalFingerprint {
};
for (key, previous) in info.env.iter() {
let current = if key == CARGO_ENV {
Some(
cargo_exe
.to_str()
.ok_or_else(|| {
format_err!(
"cargo exe path {} must be valid UTF-8",
cargo_exe.display()
)
})?
.to_string(),
)
Some(cargo_exe.to_str().ok_or_else(|| {
format_err!(
"cargo exe path {} must be valid UTF-8",
cargo_exe.display()
)
})?)
} else {
if let Some(value) = gctx.env_config()?.get(key) {
value.to_str().and_then(|s| Some(s.to_string()))
value.to_str()
} else {
gctx.get_env(key).ok()
}
};
if current == *previous {
if current == previous.as_deref() {
continue;
}
return Ok(Some(StaleItem::ChangedEnv {
var: key.clone(),
previous: previous.clone(),
current,
current: current.map(Into::into),
}));
}
if *checksum {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/git/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ where
debug_assert!(res.is_err());
let mut attempts = vec![String::from("git")];
if let Ok(s) = gctx.get_env("USER").or_else(|_| gctx.get_env("USERNAME")) {
attempts.push(s);
attempts.push(s.to_string());
}
if let Some(ref s) = cred_helper.username {
attempts.push(s.clone());
Expand Down
15 changes: 8 additions & 7 deletions src/cargo/util/context/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ fn make_case_insensitive_and_normalized_env(
.filter_map(|k| k.to_str())
.map(|k| (k.to_uppercase(), k.to_owned()))
.collect();

let normalized_env = env
.iter()
// Only keep entries where both the key and value are valid UTF-8,
Expand Down Expand Up @@ -112,12 +113,12 @@ impl Env {
///
/// This can be used similarly to [`std::env::var_os`].
/// On Windows, we check for case mismatch since environment keys are case-insensitive.
pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<&OsStr> {
match self.env.get(key.as_ref()) {
Some(s) => Some(s.clone()),
Some(s) => Some(s),
None => {
if cfg!(windows) {
self.get_env_case_insensitive(key).cloned()
self.get_env_case_insensitive(key)
} else {
None
}
Expand All @@ -129,14 +130,14 @@ impl Env {
///
/// This can be used similarly to `std::env::var`.
/// On Windows, we check for case mismatch since environment keys are case-insensitive.
pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<&str> {
let key = key.as_ref();
let s = self
.get_env_os(key)
.ok_or_else(|| anyhow!("{key:?} could not be found in the environment snapshot"))?;

match s.to_str() {
Some(s) => Ok(s.to_owned()),
Some(s) => Ok(s),
None => bail!("environment variable value is not valid unicode: {s:?}"),
}
}
Expand All @@ -146,10 +147,10 @@ impl Env {
/// This is relevant on Windows, where environment variables are case-insensitive.
/// Note that this only works on keys that are valid UTF-8 and it uses Unicode uppercase,
/// which may differ from the OS's notion of uppercase.
fn get_env_case_insensitive(&self, key: impl AsRef<OsStr>) -> Option<&OsString> {
fn get_env_case_insensitive(&self, key: impl AsRef<OsStr>) -> Option<&OsStr> {
let upper_case_key = key.as_ref().to_str()?.to_uppercase();
let env_key: &OsStr = self.case_insensitive_env.get(&upper_case_key)?.as_ref();
self.env.get(env_key)
self.env.get(env_key).map(|v| v.as_ref())
}

/// Get the value of environment variable `key` as a `&str`.
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -815,15 +815,15 @@ impl GlobalContext {
/// [`GlobalContext`].
///
/// This can be used similarly to [`std::env::var`].
pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<&str> {
self.env.get_env(key)
}

/// Get the value of environment variable `key` through the snapshot in
/// [`GlobalContext`].
///
/// This can be used similarly to [`std::env::var_os`].
pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<&OsStr> {
self.env.get_env_os(key)
}

Expand Down

0 comments on commit e75214e

Please sign in to comment.