From 5c0b165f6aeeb8e5044fbd4f6da6be24e7e0fb9f Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 20 Jun 2026 03:49:11 -0700 Subject: [PATCH] feat: add `fix-patch-path` output variable resolves #376 If there are any auto-fixes available, then a patch full of them is created and its path is set to a new output `fix-patch-path` output variable. If there were no auto-fixes, then the output variable is not set, which is treated as an empty in `steps.linter.outputs.fix-patch-path` context). --- cpp-linter/src/cli/structs.rs | 4 +++ cpp-linter/src/common_fs.rs | 57 +++++++++++++++++++++++++++++++++-- cpp-linter/src/error.rs | 12 ++++++++ cpp-linter/src/rest_client.rs | 19 +++++++++++- cpp-linter/src/run.rs | 15 ++++++++- cpp-linter/tests/comments.rs | 27 +++++++++++++++-- cpp-linter/tests/reviews.rs | 2 +- 7 files changed, 129 insertions(+), 7 deletions(-) diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 470e713..eb9ed46 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -227,6 +227,10 @@ pub struct ClangParams { impl ClangParams { /// The directory name to use for caching clang-tidy and clang-format results. pub(crate) const CACHE_DIR: &str = ".cpp-linter-cache"; + + /// The file name for aggregating auto-fixes into a unified patch. + pub(crate) const AUTO_FIX_PATCH: &str = "auto-fix.patch"; + pub(crate) fn get_cache_path(&self) -> PathBuf { self.repo_root.join(Self::CACHE_DIR).join("patched") } diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index ac4da15..8e148ba 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -3,6 +3,7 @@ use std::{ fmt::Debug, fs, + io::Write, num::NonZeroU32, ops::RangeInclusive, path::{Path, PathBuf}, @@ -16,7 +17,7 @@ use crate::{ clang_tools::{ ReviewComments, Suggestion, clang_format::FormatAdvice, clang_tidy::TidyAdvice, make_patch, }, - cli::LinesChangedOnly, + cli::{ClangParams, LinesChangedOnly}, error::FileObjError, }; @@ -162,6 +163,57 @@ impl FileObj { false } + /// If the file has a cached fixes, then append them to a unified patched file. + /// + /// This is the alternative to [`FileObj::make_suggestions_from_patch()`] when + /// 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> { + let patched = match &self.patched_path { + Some(patched_path) if patched_path.exists() => { + fs::read_to_string(patched_path).map_err(FileObjError::ReadFile)? + } + _ => return Ok(()), + }; + let original_content = + 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)?; + Ok(()) + } + + /// write fixes to a unified patch file in the cache directory. + fn append_patch( + file_name: &str, + input: &InternedInput<&str>, + diff: &Diff, + repo_root: &Path, + ) -> Result<(), FileObjError> { + let printer = BasicLineDiffPrinter(&input.interner); + let mut diff_config = UnifiedDiffConfig::default(); + diff_config.context_len(0); + 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 mut patch_file = fs::OpenOptions::new() + .append(true) + .create(true) + .truncate(false) + .open(&patch_file_path) + .map_err(FileObjError::OpenPatchFileFailed)?; + patch_file + .write_all( + format!("--- a/{file_name}\n+++ b/{file_name}\n{unified_diff}",).as_bytes(), + ) + .map_err(FileObjError::WritePatchFailed)?; + } + Ok(()) + } + /// Create a list of [`Suggestion`](struct@crate::clang_tools::Suggestion) from a /// generated diff and store them in the given /// [`ReviewComments`](struct@crate::clang_tools::ReviewComments). @@ -183,7 +235,8 @@ impl FileObj { let original_content = 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_str().unwrap_or_default().replace("\\", "/"); + let file_name = self.name.to_string_lossy().replace("\\", "/"); + Self::append_patch(&file_name, &input, &diff, repo_root)?; self.get_suggestions(review_comments, &diff, &input, summary_only) .map_err(FileObjError::DisplayStringFailed)?; diff --git a/cpp-linter/src/error.rs b/cpp-linter/src/error.rs index ade8283..6b1c45e 100644 --- a/cpp-linter/src/error.rs +++ b/cpp-linter/src/error.rs @@ -17,6 +17,18 @@ pub enum FileObjError { /// Error when failing to generate a patch for a file. #[error("Failed to print a hunk to a string buffer: {0}")] DisplayStringFailed(#[source] std::fmt::Error), + + /// Error when failing to create the cache directory for the patch file. + #[error("Failed to create cache directory for the patch file: {0}")] + MkDirFailed(#[source] std::io::Error), + + /// Error when failing to open the patch file for writing. + #[error("Failed to open patch file for writing: {0}")] + OpenPatchFileFailed(#[source] std::io::Error), + + /// Error when failing to write to the patch file. + #[error("Failed to write to the patch file: {0}")] + WritePatchFailed(#[source] std::io::Error), } /// Errors related to the REST client used for posting feedback and special logging. diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 05392a0..487006e 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -18,7 +18,7 @@ use crate::{ clang_format::{summarize_style, tally_format_advice}, clang_tidy::tally_tidy_advice, }, - cli::{FeedbackInput, ThreadComments}, + cli::{ClangParams, FeedbackInput, ThreadComments}, common_fs::FileObj, error::ClientError, }; @@ -230,6 +230,23 @@ impl RestClient { summary_only, ); self.client.post_pr_review(&options).await?; + } else { + for file in files { + let file = file + .lock() + .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; + file.maybe_append_patch(&feedback_inputs.repo_root)?; + } + } + let auto_fix_patch_path = feedback_inputs + .repo_root + .join(ClangParams::CACHE_DIR) + .join(ClangParams::AUTO_FIX_PATCH); + if auto_fix_patch_path.exists() { + self.client.write_output_variables(&[OutputVariable { + name: "fix-patch-path".to_string(), + value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), + }])?; } Ok(format_checks_failed + tidy_checks_failed) } diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 10c624c..ac60573 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -157,6 +157,12 @@ 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.")?; + // delete old patch file + let patch_file = cache_dir.join(ClangParams::AUTO_FIX_PATCH); + if patch_file.exists() { + std::fs::remove_file(&patch_file) + .with_context(|| "Failed to remove old patch file from previous runs.")?; + } // add gitignore file in project cache dir std::fs::write( cache_dir.join(".gitignore"), @@ -193,7 +199,7 @@ pub(crate) mod test { #![allow(clippy::unwrap_used)] use super::run_main; - use crate::test_common::setup_tmp_workspace; + use crate::{cli::ClangParams, test_common::setup_tmp_workspace}; use std::env; /// helper to avoid writing to the same GITHUB_OUTPUT file in parallel-running tests. @@ -238,6 +244,13 @@ pub(crate) mod test { async fn force_debug_output() { let tmp_gh_out = setup_tmp_gh_out_path(); let tmp_workspace = setup_tmp_workspace(); + + // create a dummy patch file to ensure it is deleted (in code coverage). + let cache_dir = tmp_workspace.path().join(ClangParams::CACHE_DIR); + std::fs::create_dir_all(&cache_dir).unwrap(); + let patch_path = cache_dir.join(ClangParams::AUTO_FIX_PATCH); + std::fs::write(&patch_path, "").unwrap(); + run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 4565674..746f284 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -4,7 +4,7 @@ use cpp_linter::cli::ThreadComments; use cpp_linter::run::run_main; use git_bot_feedback::LinesChangedOnly; use mockito::Matcher; -use std::{env, fmt::Display, io::Write, path::Path}; +use std::{env, fmt::Display, fs, io::Write, path::Path}; use tempfile::{NamedTempFile, TempDir}; mod common; @@ -252,7 +252,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { format!("--thread-comments={}", test_params.thread_comments), format!("--no-lgtm={}", test_params.no_lgtm), "-p=build".to_string(), - "-i=build".to_string(), + "-i=build/**".to_string(), format!("--repo-root={}", tmp_dir.path().to_str().unwrap()), ]; if test_params.force_lgtm { @@ -274,6 +274,29 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { for mock in mocks { mock.assert(); } + + // Read the GITHUB_OUTPUT file to check for the patch path + let output_vars = fs::read_to_string(tmp_out_file.path()).unwrap(); + let patch_path_from_output = output_vars + .lines() + .find(|line| line.starts_with("fix-patch-path=")) + .map(|line| { + line.trim_start_matches("fix-patch-path=") + .trim() + .to_string() + }); + if let Some(patch_path_from_output) = patch_path_from_output { + // verify patch path exists and print its content + println!("Patch path from GITHUB_OUTPUT: {patch_path_from_output}"); + let patch_path = Path::new(&patch_path_from_output); + assert!( + patch_path.exists(), + "Patch file does not exist at expected path." + ); + let patch_content = + std::fs::read_to_string(patch_path).expect("Failed to read generated patch file."); + println!("Generated patch content:\n{patch_content}"); + } } async fn test_comment(test_params: &TestParams) { diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index e4df77e..17789f0 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -226,7 +226,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { format!("--passive-reviews={}", test_params.passive_reviews), format!("--no-lgtm={}", test_params.no_lgtm), "-p=build".to_string(), - "-i=build".to_string(), + "-i=build/**".to_string(), format!("--repo-root={}", tmp_dir.path().to_str().unwrap()), ]; if test_params.force_lgtm {