Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cpp-linter/src/cli/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
57 changes: 55 additions & 2 deletions cpp-linter/src/common_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::{
fmt::Debug,
fs,
io::Write,
num::NonZeroU32,
ops::RangeInclusive,
path::{Path, PathBuf},
Expand All @@ -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,
};

Expand Down Expand Up @@ -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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
.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).
Expand All @@ -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)?;
Expand Down
12 changes: 12 additions & 0 deletions cpp-linter/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
19 changes: 18 additions & 1 deletion cpp-linter/src/rest_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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)
}
Expand Down
15 changes: 14 additions & 1 deletion cpp-linter/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,12 @@ pub async fn run_main(args: Vec<String>) -> 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"),
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
Expand Down
27 changes: 25 additions & 2 deletions cpp-linter/tests/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion cpp-linter/tests/reviews.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading