diff --git a/Cargo.toml b/Cargo.toml index 28d7374f..0315fa0a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ strip = "symbols" [workspace.dependencies] anyhow = "1.0.104" -clap = { version = "4.6.7", features = ["derive"] } +clap = { version = "4.6.7", features = ["derive", "env"] } colored = "3.1.1" log = { version = "0.4.34", features = ["std"] } mockito = "1.7.2" diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index ef7e52e6..f904de91 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -8,9 +8,7 @@ use std::str::FromStr; #[cfg(feature = "bin")] use clang_tools_manager::{RequestedVersion, logger::CLI_HELP_STYLE}; #[cfg(feature = "bin")] -use clap::{ - ArgAction, Args, Parser, Subcommand, ValueEnum, builder::FalseyValueParser, value_parser, -}; +use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum, builder::FalseyValueParser}; mod structs; pub use structs::{ClangParams, FeedbackInput, LinesChangedOnly, ThreadComments}; @@ -410,13 +408,15 @@ pub struct TidyOptions { /// attempted through all parent paths of the first input file. See [LLVM docs about /// setup tooling](https://clang.llvm.org/docs/HowToSetupToolingForLLVM.html) /// for an example of setting up Clang Tooling on a source tree. - #[cfg_attr(feature = "bin", arg( - short = 'p', - long, - value_name = "PATH", - value_parser = value_parser!(PathBuf), - help_heading = "Clang-tidy options", - ))] + #[cfg_attr( + feature = "bin", + arg( + short = 'p', + long, + value_name = "PATH", + help_heading = "Clang-tidy options", + ) + )] pub database: Option, /// A string of extra arguments passed to clang-tidy for use as compiler arguments. @@ -502,13 +502,15 @@ pub struct FeedbackOptions { /// /// If the given path is relative, then it shall be relative to the given /// [`--repo-root`](#-r-repo-root) path. - #[cfg_attr(feature = "bin", arg( - short = 'o', - long, - value_name = "PATH", - value_parser = value_parser!(PathBuf), - help_heading = "Feedback options", - ))] + #[cfg_attr( + feature = "bin", + arg( + short = 'o', + long, + value_name = "PATH", + help_heading = "Feedback options", + ) + )] pub summary_output_file: Option, /// Provide this option with a path to which clang-tidy diagnostics are @@ -528,12 +530,10 @@ pub struct FeedbackOptions { /// > In GitHub Actions, the SARIF file can be uploaded to /// > [GitHub code scanning](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/uploading-a-sarif-file-to-github) /// > with the `github/codeql-action/upload-sarif` action. - #[cfg_attr(feature = "bin", arg( - long, - value_name = "PATH", - value_parser = value_parser!(PathBuf), - help_heading = "Feedback options", - ))] + #[cfg_attr( + feature = "bin", + arg(long, value_name = "PATH", help_heading = "Feedback options",) + )] pub sarif_file: Option, /// Set this option to false to disable the use of @@ -593,6 +593,33 @@ pub struct FeedbackOptions { help_heading = "Feedback options", ))] pub gen_patch: bool, + + /// Write the patch file of applied clang fixes to this path. + /// + /// Setting this implies [`--gen-patch`](#-f-gen-patch). + /// A relative path is relative to the current working directory. + /// If not set, the patch is written to `.cpp-linter-cache/auto-fix.patch` + /// in the [`--repo-root`](#-r-repo-root) folder. + /// + /// This can also be set with the `GEN_PATCH_PATH` environment variable, + /// so the same variable can be used to apply the patch afterward: + /// + /// ```shell + /// export GEN_PATCH_PATH=fixes.patch + /// cpp-linter + /// git apply "$GEN_PATCH_PATH" + /// ``` + #[cfg_attr( + feature = "bin", + arg( + long, + value_name = "PATH", + env = "GEN_PATCH_PATH", + help_heading = "Feedback options", + verbatim_doc_comment, + ) + )] + pub gen_patch_path: Option, } /// Parses a single value of the `--extensions` option. diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 9c2a91fb..e0f201dc 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -1,4 +1,7 @@ -use std::{fmt::Display, path::PathBuf}; +use std::{ + fmt::Display, + path::{Path, PathBuf}, +}; #[cfg(feature = "bin")] use clap::{ValueEnum, builder::PossibleValue}; @@ -237,6 +240,11 @@ impl ClangParams { /// The file name for aggregating auto-fixes into a unified patch. pub(crate) const AUTO_FIX_PATCH: &str = "auto-fix.patch"; + /// The default path of the generated patch file, in the cache directory of `repo_root`. + pub(crate) fn default_patch_path(repo_root: &Path) -> PathBuf { + repo_root.join(Self::CACHE_DIR).join(Self::AUTO_FIX_PATCH) + } + pub(crate) fn get_cache_path(&self) -> PathBuf { self.repo_root.join(Self::CACHE_DIR).join("patched") } @@ -289,7 +297,9 @@ impl From<&Cli> for ClangParams { tidy_filter, format_filter, repo_root, - gen_patch: args.feedback_options.pr_review || args.feedback_options.gen_patch, + gen_patch: args.feedback_options.pr_review + || args.feedback_options.gen_patch + || args.feedback_options.gen_patch_path.is_some(), } } } @@ -333,6 +343,9 @@ pub struct FeedbackInput { /// /// Only used when `pr_review` is false. pub gen_patch: bool, + + /// The path of the generated patch file. + pub patch_path: PathBuf, } #[cfg(feature = "bin")] @@ -350,7 +363,12 @@ impl From<&Cli> for FeedbackInput { repo_root: args.source_options.repo_root.clone(), summary_output_file: args.feedback_options.summary_output_file.clone(), sarif_file: args.feedback_options.sarif_file.clone(), - gen_patch: args.feedback_options.gen_patch, + gen_patch: args.feedback_options.gen_patch + || args.feedback_options.gen_patch_path.is_some(), + patch_path: match &args.feedback_options.gen_patch_path { + Some(path) => std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()), + None => ClangParams::default_patch_path(&args.source_options.repo_root), + }, } } } @@ -370,6 +388,7 @@ impl Default for FeedbackInput { summary_output_file: None, sarif_file: None, gen_patch: false, + patch_path: ClangParams::default_patch_path(Path::new(".")), } } } @@ -380,7 +399,9 @@ mod test { use clap::{Parser, ValueEnum}; - use super::{ClangParams, Cli, LinesChangedOnly, ThreadComments}; + use std::path::PathBuf; + + use super::{ClangParams, Cli, FeedbackInput, LinesChangedOnly, ThreadComments}; #[test] fn parse_positional() { @@ -443,6 +464,45 @@ mod test { } } + #[test] + fn default_patch_path() { + let cli = Cli::parse_from(["cpp-linter", "--repo-root", "sub"]); + let feedback_input = FeedbackInput::from(&cli); + assert!(!feedback_input.gen_patch); + assert_eq!( + feedback_input.patch_path, + PathBuf::from("sub/.cpp-linter-cache/auto-fix.patch") + ); + } + + #[test] + fn gen_patch_path_implies_gen_patch() { + let cli = Cli::parse_from(["cpp-linter", "--gen-patch-path", "fixes.patch"]); + let feedback_input = FeedbackInput::from(&cli); + assert!(feedback_input.gen_patch); + // a relative path is relative to the working directory + assert_eq!( + feedback_input.patch_path, + std::env::current_dir().unwrap().join("fixes.patch") + ); + assert!(ClangParams::from(&cli).gen_patch); + } + + #[test] + fn gen_patch_path_from_env() { + unsafe { + std::env::set_var("GEN_PATCH_PATH", "from-env.patch"); + } + let cli = Cli::parse_from(["cpp-linter"]); + unsafe { + std::env::remove_var("GEN_PATCH_PATH"); + } + assert_eq!( + cli.feedback_options.gen_patch_path, + Some(PathBuf::from("from-env.patch")) + ); + } + #[test] fn absolute_db_path() { let tmp_dir = tempfile::tempdir().unwrap(); diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index 43e1c9ec..2fcab9c8 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -17,7 +17,7 @@ use crate::{ clang_tools::{ ReviewComments, Suggestion, clang_format::FormatAdvice, clang_tidy::TidyAdvice, make_patch, }, - cli::{ClangParams, LinesChangedOnly}, + cli::LinesChangedOnly, error::FileObjError, }; @@ -169,7 +169,11 @@ impl FileObj { /// a PR review is not being posted. Both function have to create a patch by /// reading the original file and patched file (in cache), but /// [`FileObj::make_suggestions_from_patch()`] does more with the diff than this function. - pub fn maybe_append_patch(&self, repo_root: &Path) -> Result<(), FileObjError> { + pub fn maybe_append_patch( + &self, + repo_root: &Path, + patch_path: &Path, + ) -> Result<(), FileObjError> { let patched = match &self.patched_path { Some(patched_path) if patched_path.exists() => { fs::read_to_string(patched_path).map_err(FileObjError::ReadFile)? @@ -180,29 +184,28 @@ impl FileObj { fs::read_to_string(repo_root.join(&self.name)).map_err(FileObjError::ReadFile)?; let (diff, input) = make_patch(patched.as_str(), &original_content); let file_name = self.name.to_string_lossy().replace("\\", "/"); - Self::append_patch(&file_name, &input, &diff, repo_root)?; + Self::append_patch(&file_name, &input, &diff, patch_path)?; Ok(()) } - /// write fixes to a unified patch file in the cache directory. + /// write fixes to the unified patch file at `patch_path`. fn append_patch( file_name: &str, input: &InternedInput<&str>, diff: &Diff, - repo_root: &Path, + patch_path: &Path, ) -> Result<(), FileObjError> { let printer = BasicLineDiffPrinter(&input.interner); let diff_config = UnifiedDiffConfig::default(); let unified_diff = diff.unified_diff(&printer, diff_config, input).to_string(); if !unified_diff.is_empty() { - let patch_path_parent = repo_root.join(ClangParams::CACHE_DIR); - fs::create_dir_all(&patch_path_parent).map_err(FileObjError::MkDirFailed)?; - let patch_file_path = patch_path_parent.join(ClangParams::AUTO_FIX_PATCH); + let patch_path_parent = patch_path.parent().unwrap_or(Path::new("")); + fs::create_dir_all(patch_path_parent).map_err(FileObjError::MkDirFailed)?; let mut patch_file = fs::OpenOptions::new() .append(true) .create(true) .truncate(false) - .open(&patch_file_path) + .open(patch_path) .map_err(FileObjError::OpenPatchFileFailed)?; patch_file .write_all( @@ -224,6 +227,7 @@ impl FileObj { review_comments: &mut ReviewComments, summary_only: bool, repo_root: &Path, + patch_path: &Path, ) -> Result<(), FileObjError> { let patched = match &self.patched_path { Some(patched_path) if patched_path.exists() => { @@ -235,7 +239,7 @@ impl FileObj { fs::read_to_string(repo_root.join(&self.name)).map_err(FileObjError::ReadFile)?; let (diff, input) = make_patch(patched.as_str(), &original_content); let file_name = self.name.to_string_lossy().replace("\\", "/"); - Self::append_patch(&file_name, &input, &diff, repo_root)?; + Self::append_patch(&file_name, &input, &diff, patch_path)?; self.get_suggestions(review_comments, &diff, &input, summary_only) .map_err(FileObjError::DisplayStringFailed)?; @@ -425,7 +429,10 @@ mod test { use tempfile::{NamedTempFile, TempDir}; use super::FileObj; - use crate::{clang_tools::ReviewComments, cli::LinesChangedOnly}; + use crate::{ + clang_tools::ReviewComments, + cli::{ClangParams, LinesChangedOnly}, + }; // *********************** tests for FileObj::get_ranges() @@ -508,7 +515,12 @@ mod test { let mut review_comments = ReviewComments::default(); file_obj - .make_suggestions_from_patch(&mut review_comments, false, repo_root.path()) + .make_suggestions_from_patch( + &mut review_comments, + false, + repo_root.path(), + &ClangParams::default_patch_path(repo_root.path()), + ) .unwrap(); assert_eq!(review_comments.comments.len(), 1); @@ -568,7 +580,12 @@ mod test { let mut review_comments = ReviewComments::default(); file_obj - .make_suggestions_from_patch(&mut review_comments, false, repo_root.path()) + .make_suggestions_from_patch( + &mut review_comments, + false, + repo_root.path(), + &ClangParams::default_patch_path(repo_root.path()), + ) .unwrap(); assert_eq!(review_comments.comments.len(), 2); // 1 fix and 2 unfixed diagnostics, all within the diff (in 2 comments) @@ -599,7 +616,12 @@ mod test { // Processing the same file again merges duplicate suggestions file_obj - .make_suggestions_from_patch(&mut review_comments, false, repo_root.path()) + .make_suggestions_from_patch( + &mut review_comments, + false, + repo_root.path(), + &ClangParams::default_patch_path(repo_root.path()), + ) .unwrap(); assert_eq!(review_comments.comments.len(), 2); assert_eq!( @@ -613,7 +635,12 @@ mod test { // Only counts the unfixed diagnostics in summary-only mode let mut summary_comments = ReviewComments::default(); file_obj - .make_suggestions_from_patch(&mut summary_comments, true, repo_root.path()) + .make_suggestions_from_patch( + &mut summary_comments, + true, + repo_root.path(), + &ClangParams::default_patch_path(repo_root.path()), + ) .unwrap(); assert!(summary_comments.comments.is_empty()); assert_eq!(summary_comments.tool_total, 3); diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 337bf978..be0c35aa 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -20,7 +20,7 @@ use crate::{ clang_format::{summarize_style, tally_format_advice}, clang_tidy::tally_tidy_advice, }, - cli::{ClangParams, FeedbackInput, ThreadComments}, + cli::{FeedbackInput, ThreadComments}, common_fs::FileObj, error::ClientError, sarif::SarifLog, @@ -273,6 +273,7 @@ impl RestClient { &mut review_comments, summary_only, &feedback_inputs.repo_root, + &feedback_inputs.patch_path, )?; } self.show_patch(&feedback_inputs)?; @@ -323,7 +324,7 @@ impl RestClient { let file = file .lock() .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; - file.maybe_append_patch(&feedback_inputs.repo_root)?; + file.maybe_append_patch(&feedback_inputs.repo_root, &feedback_inputs.patch_path)?; } self.show_patch(&feedback_inputs)?; } @@ -502,11 +503,8 @@ impl RestClient { /// /// And also write the patch path to the output variable `fix-patch-path` for CI platforms. fn show_patch(&self, feedback_inputs: &FeedbackInput) -> Result<(), ClientError> { - let auto_fix_patch_path = feedback_inputs - .repo_root - .join(ClangParams::CACHE_DIR) - .join(ClangParams::AUTO_FIX_PATCH); - if let Ok(patch_file) = std::fs::File::open(&auto_fix_patch_path) { + let auto_fix_patch_path = &feedback_inputs.patch_path; + if let Ok(patch_file) = std::fs::File::open(auto_fix_patch_path) { self.client.write_output_variables(&[OutputVariable { name: "fix-patch-path".to_string(), value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 6f971372..572df290 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -170,10 +170,10 @@ pub async fn run_main(args: Vec) -> Result<()> { let cache_dir = clang_params.repo_root.join(ClangParams::CACHE_DIR); std::fs::create_dir_all(&cache_dir) .with_context(|| "Failed to create a local cache directory.")?; + let user_inputs = FeedbackInput::from(&cli); // delete old patch file - let patch_file = cache_dir.join(ClangParams::AUTO_FIX_PATCH); - if patch_file.exists() { - std::fs::remove_file(&patch_file) + if user_inputs.patch_path.exists() { + std::fs::remove_file(&user_inputs.patch_path) .with_context(|| "Failed to remove old patch file from previous runs.")?; } // add gitignore file in project cache dir @@ -182,7 +182,6 @@ pub async fn run_main(args: Vec) -> Result<()> { "# Automatically created by cpp-linter\n*\n", ) .with_context(|| "Failed to write .cpp-linter-cache/.gitignore file")?; - let user_inputs = FeedbackInput::from(&cli); let clang_versions = capture_clang_tools_output( &arc_files, &cli.general_options.version, @@ -244,6 +243,34 @@ pub(crate) mod test { drop(tmp_gh_out); } + // The patch is written to `--gen-patch-path`, and a stale patch there is replaced. + #[tokio::test] + async fn custom_patch_path() { + let tmp_gh_out = setup_tmp_gh_out_path(); + let tmp_workspace = setup_tmp_workspace(); + let patch_path = tmp_workspace.path().join("out").join("fixes.patch"); + std::fs::create_dir_all(patch_path.parent().unwrap()).unwrap(); + std::fs::write(&patch_path, "stale").unwrap(); + run_main(vec![ + "cpp-linter".to_string(), + "-l".to_string(), + "false".to_string(), + "--tidy-checks=-*".to_string(), + "--gen-patch-path".to_string(), + patch_path.to_str().unwrap().to_string(), + "--repo-root".to_string(), + tmp_workspace.path().to_str().unwrap().to_string(), + "demo/demo.cpp".to_string(), + ]) + .await + .unwrap(); + let patch = std::fs::read_to_string(&patch_path).unwrap(); + assert!(patch.starts_with("--- a/demo/demo.cpp\n"), "{patch}"); + let default_patch = ClangParams::default_patch_path(tmp_workspace.path()); + assert!(!default_patch.exists()); + drop(tmp_gh_out); + } + #[tokio::test] async fn pr_review_outside_ci() { let tmp_gh_out = setup_tmp_gh_out_path();