From 43363faf7061c9b0a1d976e3ffa930d5a5a4c114 Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Mon, 19 Oct 2026 06:39:21 -0400 Subject: [PATCH] Make use of Clippy repo read-only --- driver/build/extra_symbols.rs | 26 +- internal/src/clippy_utils/mod.rs | 14 +- internal/src/clippy_utils/repository.rs | 19 +- .../src/clippy_utils/revs_no_preinstall.rs | 277 +++++++++++++++--- 4 files changed, 263 insertions(+), 73 deletions(-) diff --git a/driver/build/extra_symbols.rs b/driver/build/extra_symbols.rs index 655ae906d..1837dc5fd 100644 --- a/driver/build/extra_symbols.rs +++ b/driver/build/extra_symbols.rs @@ -1,16 +1,15 @@ -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow}; use dylint_internal::{ - clippy_utils::{Rev, Revs, clippy_repository, parse_as_nightly}, + clippy_utils::{Rev, Revs, clippy_repository, file_at_revision, parse_as_nightly}, env, rustup::parse_toolchain, }; use git2::Oid; use proc_macro2::TokenTree; use std::{ - fs::{OpenOptions, read_to_string, write}, + fs::{OpenOptions, write}, io::Write, - path::PathBuf, - process::Command, + path::{Path, PathBuf}, }; use syn::{Item, ItemMacro, Macro, parse_file}; @@ -87,22 +86,7 @@ fn rev_from_channel(channel: &str) -> Result { fn clippy_utils_sym_rs(rev: Oid) -> Result { let repository = clippy_repository(false)?; - let Some(workdir) = repository.workdir() else { - bail!("Repository has no working directory"); - }; - - let mut command = Command::new("git"); - command - .current_dir(workdir) - .args(["checkout", &rev.to_string()]); - let status = command - .status() - .with_context(|| format!("command failed: {command:?}"))?; - assert!(status.success()); - - let sym_path = workdir.join("clippy_utils/src/sym.rs"); - read_to_string(&sym_path) - .with_context(|| format!("`read_to_string` failed for `{}`", sym_path.display())) + file_at_revision(&repository, rev, Path::new("clippy_utils/src/sym.rs")) } fn quoted_symbols(sym_rs: &str) -> Result> { diff --git a/internal/src/clippy_utils/mod.rs b/internal/src/clippy_utils/mod.rs index 9f7116d1c..7a8116bd5 100644 --- a/internal/src/clippy_utils/mod.rs +++ b/internal/src/clippy_utils/mod.rs @@ -7,7 +7,7 @@ use std::{ use toml_edit::{DocumentMut, Item, Value}; mod repository; -pub use repository::{clippy_repository, parse_as_nightly}; +pub use repository::{clippy_repository, file_at_revision, parse_as_nightly}; mod revs_no_preinstall; pub use revs_no_preinstall::{Rev, Revs}; @@ -28,7 +28,11 @@ pub fn clippy_utils_package_version(path: &Path) -> Result { cargo_toml.to_string_lossy(), ) })?; - let table = toml::from_str::(&contents)?; + package_version_from_contents(&contents) +} + +fn package_version_from_contents(contents: &str) -> Result { + let table = toml::from_str::(contents)?; let version = table .get("package") .and_then(toml::Value::as_table) @@ -76,7 +80,11 @@ pub fn toolchain_channel(path: &Path) -> Result { let Some((_, contents)) = read_rust_toolchain_file(path)? else { bail!("Could not find rust-toolchain file at `{}`", path.display()); }; - let table = toml::from_str::(&contents)?; + toolchain_channel_from_contents(&contents) +} + +fn toolchain_channel_from_contents(contents: &str) -> Result { + let table = toml::from_str::(contents)?; table .get("toolchain") .and_then(toml::Value::as_table) diff --git a/internal/src/clippy_utils/repository.rs b/internal/src/clippy_utils/repository.rs index c51feec84..5a588f9da 100644 --- a/internal/src/clippy_utils/repository.rs +++ b/internal/src/clippy_utils/repository.rs @@ -1,6 +1,9 @@ -use crate::{clone, git2::Repository}; +use crate::{ + clone, + git2::{Oid, Repository}, +}; use anyhow::{Context, Result}; -use std::{cell::RefCell, rc::Rc}; +use std::{cell::RefCell, path::Path, rc::Rc}; use tempfile::{TempDir, tempdir}; const RUST_CLIPPY_URL: &str = "https://github.com/rust-lang/rust-clippy"; @@ -26,6 +29,18 @@ pub fn clippy_repository(quiet: bool) -> Result> { }) } +/// Reads a UTF-8 file from a commit without changing HEAD or the working tree. +pub fn file_at_revision(repository: &Repository, rev: Oid, path: &Path) -> Result { + (|| -> Result { + let commit = repository.find_commit(rev)?; + let tree = commit.tree()?; + let entry = tree.get_path(path)?; + let blob = repository.find_blob(entry.id())?; + Ok(std::str::from_utf8(blob.content())?.to_owned()) + })() + .with_context(|| format!("Could not read `{}` at revision `{rev}`", path.display())) +} + pub fn parse_as_nightly(channel: &str) -> Option<[u32; 3]> { channel.strip_prefix("nightly-").and_then(parse_date) } diff --git a/internal/src/clippy_utils/revs_no_preinstall.rs b/internal/src/clippy_utils/revs_no_preinstall.rs index 4beefe114..70a72162f 100644 --- a/internal/src/clippy_utils/revs_no_preinstall.rs +++ b/internal/src/clippy_utils/revs_no_preinstall.rs @@ -1,8 +1,11 @@ -use super::{clippy_utils_package_version, repository::clippy_repository, toolchain_channel}; -use crate::git2::{Commit, ObjectType, Oid, Repository}; -use anyhow::{Context, Result, anyhow}; +use super::{ + file_at_revision, package_version_from_contents, repository::clippy_repository, + toolchain_channel_from_contents, +}; +use crate::git2::{Commit, Oid, Repository}; +use anyhow::{Context, Result}; use semver::Version; -use std::rc::Rc; +use std::{path::Path, rc::Rc}; #[derive(Debug, Eq, PartialEq)] pub struct Rev { @@ -11,6 +14,29 @@ pub struct Rev { pub oid: Oid, } +impl Rev { + fn from_commit(repository: &Repository, commit: &Commit<'_>) -> Result { + let oid = commit.id(); + let manifest = file_at_revision(repository, oid, Path::new("clippy_utils/Cargo.toml"))?; + let version = package_version_from_contents(&manifest)?; + // Match `read_rust_toolchain_file`'s precedence, including failing rather than + // falling back if a present `rust-toolchain` file is malformed. + let tree = commit.tree()?; + let filename = if tree.get_name("rust-toolchain").is_some() { + "rust-toolchain" + } else { + "rust-toolchain.toml" + }; + let toolchain = file_at_revision(repository, oid, Path::new(filename))?; + let channel = toolchain_channel_from_contents(&toolchain)?; + Ok(Self { + version, + channel, + oid, + }) + } +} + pub struct Revs { repository: Rc, } @@ -44,29 +70,17 @@ impl Revs { #[allow(clippy::iter_not_returning_iterator)] fn iter(&self, filter: Filter) -> Result> { - let path = self + // Start from the branch used by `clippy_repository`, independently of the checkout. + let commit = self .repository - .workdir() - .ok_or_else(|| anyhow!("Could not get working directory"))?; - let object = { - let head = self.repository.head()?; - let oid = head.target().ok_or_else(|| anyhow!("Could not get HEAD"))?; - self.repository.find_object(oid, Some(ObjectType::Commit))? - }; - let commit = object - .as_commit() - .ok_or_else(|| anyhow!("Object is not a commit"))?; - let version = clippy_utils_package_version(path)?; - let channel = toolchain_channel(path)?; - let oid = commit.id(); + .find_reference("refs/heads/master") + .and_then(|reference| reference.peel_to_commit()) + .with_context(|| "Could not resolve Clippy's local `master` branch")?; + let rev = Rev::from_commit(&self.repository, &commit)?; Ok(RevIter { repository: self.repository.clone(), - commit: commit.clone(), - curr_rev: Some(Rev { - version, - channel, - oid, - }), + commit, + curr_rev: Some(rev), filter, }) } @@ -86,10 +100,6 @@ impl Iterator for RevIter<'_> { let curr_rev = if let Some(rev) = self.curr_rev.take() { rev } else { - let path = self - .repository - .workdir() - .ok_or_else(|| anyhow!("Could not get working directory"))?; // smoelius: Note that this is not `git log`'s default behavior. Rather, this // behavior corresponds to: // git log --first-parent @@ -99,24 +109,7 @@ impl Iterator for RevIter<'_> { } else { return Ok(None); }; - self.repository - .checkout_tree(commit.as_object(), None) - .with_context(|| { - format!("`checkout_tree` failed for `{:?}`", commit.as_object()) - })?; - self.repository - .set_head_detached(commit.id()) - .with_context(|| { - format!("`set_head_detached` failed for `{}`", commit.id()) - })?; - let version = clippy_utils_package_version(path)?; - let channel = toolchain_channel(path)?; - let oid = commit.id(); - Rev { - version, - channel, - oid, - } + Rev::from_commit(&self.repository, &commit)? }; #[allow(clippy::collapsible_if)] if let Some(prev_rev) = prev_rev { @@ -139,7 +132,197 @@ impl Iterator for RevIter<'_> { #[cfg(test)] mod tests { use super::*; - use std::{str::FromStr, sync::LazyLock}; + use crate::git2::{IndexAddOption, RepositoryInitOptions, Signature, build::RepoBuilder}; + use std::{ + fs::{create_dir_all, read, read_to_string, remove_file, write}, + str::FromStr, + sync::LazyLock, + }; + use tempfile::{TempDir, tempdir}; + + // Each filesystem mutation is confined to this test's temporary directory. + #[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] + #[test] + fn local_searches_leave_checkout_unchanged() { + let (dir, repository, commits) = local_repository(); + repository.set_head_detached(commits[1]).unwrap(); + let manifest = dir.path().join("clippy_utils/Cargo.toml"); + write(&manifest, "uncommitted, invalid TOML").unwrap(); + let index = read(repository.path().join("index")).unwrap(); + let head = read(repository.path().join("HEAD")).unwrap(); + let revs = Revs { + repository: Rc::new(repository), + }; + + // Independent searches may overlap and may be repeated after an early exit. + let mut versions = revs.version_iter().unwrap(); + let mut channels = revs.channel_iter().unwrap(); + assert_eq!(commits[3], versions.next().unwrap().unwrap().oid); + assert_eq!(commits[3], channels.next().unwrap().unwrap().oid); + assert_eq!(commits[2], channels.next().unwrap().unwrap().oid); + assert_eq!(commits[1], versions.next().unwrap().unwrap().oid); + assert_eq!(commits[1], channels.next().unwrap().unwrap().oid); + assert!(versions.next().is_none()); + assert!(channels.next().is_none()); + assert_eq!( + commits[3], + revs.version_iter().unwrap().next().unwrap().unwrap().oid + ); + // Symbol reads also use the selected commit, independent of the working tree. + assert_eq!( + "symbols at nightly-2025-05-03", + file_at_revision( + &revs.repository, + commits[2], + Path::new("clippy_utils/src/sym.rs") + ) + .unwrap() + ); + assert_eq!(head, read(revs.repository.path().join("HEAD")).unwrap()); + assert_eq!(index, read(revs.repository.path().join("index")).unwrap()); + assert_eq!( + "uncommitted, invalid TOML", + read_to_string(manifest).unwrap() + ); + assert_eq!( + "symbols at nightly-2025-05-04", + read_to_string(dir.path().join("clippy_utils/src/sym.rs")).unwrap() + ); + } + + // Each filesystem mutation is confined to this test's temporary directory. + #[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] + #[test] + fn local_searches_support_bare_repositories() { + let (dir, _, commits) = local_repository(); + let bare_dir = tempdir().unwrap(); + let repository = RepoBuilder::new() + .bare(true) + .clone(dir.path().to_str().unwrap(), bare_dir.path()) + .unwrap(); + assert!(repository.workdir().is_none()); + let revs = Revs { + repository: Rc::new(repository), + }; + let revisions = revs + .channel_iter() + .unwrap() + .collect::>>() + .unwrap(); + assert_eq!( + vec![commits[3], commits[2], commits[1]], + revisions.iter().map(|rev| rev.oid).collect::>() + ); + // The legacy file takes precedence at this historical revision. + assert_eq!("nightly-2025-05-02", revisions[2].channel); + assert_eq!( + "symbols at nightly-2025-05-02", + file_at_revision( + &revs.repository, + commits[1], + Path::new("clippy_utils/src/sym.rs") + ) + .unwrap() + ); + } + + // Each filesystem mutation is confined to this test's temporary directory. + #[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] + #[test] + fn local_search_errors_leave_checkout_unchanged() { + let (dir, repository, _) = local_repository(); + // A malformed legacy file must not fall back to the valid TOML file. + write(dir.path().join("rust-toolchain"), "invalid TOML!").unwrap(); + let oid = commit_files(&repository); + let revs = Revs { + repository: Rc::new(repository), + }; + assert!(revs.version_iter().is_err()); + assert_eq!(Some(oid), revs.repository.head().unwrap().target()); + assert!(file_at_revision(&revs.repository, oid, Path::new("missing")).is_err()); + // Also exercise an error reached after traversal has begun. + remove_file(dir.path().join("rust-toolchain")).unwrap(); + commit_files(&revs.repository); + let mut iter = revs.channel_iter().unwrap(); + assert!(iter.next().unwrap().is_err()); + assert_eq!( + "symbols at nightly-2025-05-04", + read_to_string(dir.path().join("clippy_utils/src/sym.rs")).unwrap() + ); + assert_eq!( + revs.repository + .find_reference("refs/heads/master") + .unwrap() + .target(), + revs.repository.head().unwrap().target() + ); + } + + fn local_repository() -> (TempDir, Repository, Vec) { + let dir = tempdir().unwrap(); + let mut options = RepositoryInitOptions::new(); + options.initial_head("master"); + let repository = Repository::init_opts(dir.path(), &options).unwrap(); + create_dir_all(dir.path().join("clippy_utils/src")).unwrap(); + let mut commits = Vec::new(); + for (day, version) in [(1, "0.1.80"), (2, "0.1.81"), (3, "0.1.81"), (4, "0.1.82")] { + let channel = format!("nightly-2025-05-{day:02}"); + write( + dir.path().join("clippy_utils/Cargo.toml"), + format!("[package]\nversion = \"{version}\"\n"), + ) + .unwrap(); + write( + dir.path().join("rust-toolchain.toml"), + format!("[toolchain]\nchannel = \"{channel}\"\n"), + ) + .unwrap(); + if day == 2 { + write( + dir.path().join("rust-toolchain"), + format!("[toolchain]\nchannel = \"{channel}\"\n"), + ) + .unwrap(); + write( + dir.path().join("rust-toolchain.toml"), + "[toolchain]\nchannel = \"nightly-2099-01-01\"\n", + ) + .unwrap(); + } else if dir.path().join("rust-toolchain").exists() { + remove_file(dir.path().join("rust-toolchain")).unwrap(); + } + write( + dir.path().join("clippy_utils/src/sym.rs"), + format!("symbols at {channel}"), + ) + .unwrap(); + commits.push(commit_files(&repository)); + } + (dir, repository, commits) + } + + fn commit_files(repository: &Repository) -> Oid { + let mut index = repository.index().unwrap(); + index.add_all(["*"], IndexAddOption::DEFAULT, None).unwrap(); + let tree = repository.find_tree(index.write_tree().unwrap()).unwrap(); + index.write().unwrap(); + let parent = repository + .head() + .ok() + .map(|head| head.peel_to_commit().unwrap()); + let parents = parent.iter().collect::>(); + let signature = Signature::now("Test", "test@example.com").unwrap(); + repository + .commit( + Some("HEAD"), + &signature, + &signature, + "test", + &tree, + &parents, + ) + .unwrap() + } static VERSION_EXAMPLES: LazyLock<[Rev; 6]> = LazyLock::new(|| { [