Repository navigation
fix: handle overrides on paths that later became symlinks #5113
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
base: main
Are you sure you want to change the base?
Changes from all commits
c081dec
2a5d141
6a55c24
5c00d77
8072250
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ use std::{ | |
| env::consts::EXE_SUFFIX, | ||
| fmt, | ||
| io::{self, Write}, | ||
| path::{Path, PathBuf}, | ||
| path::{Path, PathBuf, absolute}, | ||
| process::ExitStatus, | ||
| str::FromStr, | ||
| sync::Arc, | ||
|
|
@@ -1750,7 +1750,12 @@ async fn override_add( | |
| Err(e) => Err(e)?, | ||
| } | ||
|
|
||
| cfg.make_override(path.unwrap_or(&cfg.current_dir), &toolchain)?; | ||
| // Relative to the directory rustup runs in, which `absolute` alone would | ||
| // take from the OS rather than from `cfg`. | ||
| let path = path | ||
| .map(|path| absolute(cfg.current_dir.join(path))) | ||
| .transpose()?; | ||
|
Comment on lines
+1755
to
+1757
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd prefer using |
||
| cfg.make_override(path.as_deref().unwrap_or(&cfg.current_dir), &toolchain)?; | ||
| Ok(ExitCode::SUCCESS) | ||
| } | ||
|
|
||
|
|
@@ -1774,7 +1779,7 @@ fn override_remove( | |
| } | ||
| list | ||
| } else if let Some(path) = path { | ||
| vec![path.to_owned()] | ||
| vec![absolute(cfg.current_dir.join(path))?] | ||
| } else { | ||
| vec![cfg.current_dir.clone()] | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,13 @@ | ||
| use std::{ | ||
| cell::RefCell, | ||
| collections::BTreeMap, | ||
| collections::{BTreeMap, HashMap}, | ||
| fmt, | ||
| path::{Path, PathBuf}, | ||
| str::FromStr, | ||
| }; | ||
|
|
||
| use anyhow::Context; | ||
| use same_file::Handle; | ||
| use serde::{Deserialize, Serialize}; | ||
| use tracing::info; | ||
|
|
||
|
|
@@ -104,31 +105,43 @@ pub struct Settings { | |
| } | ||
|
|
||
| impl Settings { | ||
| fn path_to_key(path: &Path) -> String { | ||
| if path.exists() { | ||
| utils::canonicalize_path(path).display().to_string() | ||
| } else { | ||
| path.display().to_string() | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn remove_override(&mut self, path: &Path) -> bool { | ||
| let key = Self::path_to_key(path); | ||
| self.overrides.remove(&key).is_some() | ||
| let key = path.display().to_string(); | ||
| if self.overrides.remove(&key).is_some() { | ||
| return true; | ||
| } | ||
| // The directory may be stored under another path that leads to it. | ||
| let Ok(dir) = Handle::from_path(path) else { | ||
| return false; | ||
| }; | ||
| let count = self.overrides.len(); | ||
| self.overrides.retain(|stored, _| !leads_to(stored, &dir)); | ||
| self.overrides.len() < count | ||
| } | ||
|
|
||
| pub(crate) fn add_override(&mut self, path: &Path, toolchain: String) { | ||
| let key = Self::path_to_key(path); | ||
| let key = path.display().to_string(); | ||
| // Keep a single override per directory, whichever path it was stored under. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "Keep a single..." is achieved by removing every entry then adding a single entry at the end with the key probably updated to the currently-"canonical" form. Probably you can be more explicit by saying just that in the comments 🙏 |
||
| if let Ok(dir) = Handle::from_path(path) { | ||
| self.overrides.retain(|stored, _| !leads_to(stored, &dir)); | ||
| } | ||
| info!( | ||
| "override toolchain for {} set to {toolchain}", | ||
| path.display(), | ||
| ); | ||
| self.overrides.insert(key, toolchain); | ||
| } | ||
|
|
||
| pub(crate) fn dir_override(&self, dir: &Path) -> Option<String> { | ||
| let key = Self::path_to_key(dir); | ||
| self.overrides.get(&key).cloned() | ||
| /// Indexes the overrides by the directories they refer to, for a series of lookups. | ||
| pub(crate) fn override_lookup(&self) -> OverrideLookup<'_> { | ||
| let mut by_dir = HashMap::new(); | ||
| for (path, toolchain) in &self.overrides { | ||
| // A directory that cannot be opened cannot be the current one either. | ||
| if let Ok(dir) = Handle::from_path(path) { | ||
| by_dir.entry(dir).or_insert(toolchain.as_str()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: A method call is probably better followed with |
||
| } | ||
| } | ||
| OverrideLookup { by_dir } | ||
| } | ||
|
|
||
| pub(crate) fn parse(data: &str) -> anyhow::Result<Self> { | ||
|
|
@@ -140,6 +153,27 @@ impl Settings { | |
| } | ||
| } | ||
|
|
||
| /// Whether `path` leads to the directory behind `dir`. | ||
| fn leads_to(path: &str, dir: &Handle) -> bool { | ||
| Handle::from_path(path).is_ok_and(|other| other == *dir) | ||
| } | ||
|
|
||
| /// Overrides keyed by the identity of their directories rather than by their | ||
| /// paths, so that an override applies however its directory is reached. | ||
| pub(crate) struct OverrideLookup<'a> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I believe you are trying to say "lookup table" or something with the term |
||
| by_dir: HashMap<Handle, &'a str>, | ||
| } | ||
|
|
||
| impl OverrideLookup<'_> { | ||
| pub(crate) fn get(&self, dir: &Path) -> Option<&str> { | ||
| if self.by_dir.is_empty() { | ||
| return None; | ||
| } | ||
| let dir = Handle::from_path(dir).ok()?; | ||
| self.by_dir.get(&dir).copied() | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] | ||
| pub enum MetadataVersion { | ||
| #[serde(rename = "2")] | ||
|
|
||
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.
Nit: I think this should eventually become a use of
Path::absolute()but before that is stabilized, let'suse std::pathand call it likepath::absolute()here just to be clear.