Skip to content

Commit aa1c380

Browse files
authored
feat: add fix-patch-path output variable (#378)
resolves #376 This is the result of - #358 - #354 - #347 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. Remember, an unset output variable is treated as an empty string (a false value) in `steps.linter.outputs.fix-patch-path` context. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Auto-fix patches are aggregated into a single unified patch file in the repository cache directory. * CI/CD now receives an optional `fix-patch-path` output (with normalized forward slashes) pointing to the generated patch when present. * Cached patch aggregation is recorded consistently when producing suggestions. * **Bug Fixes** * Clear any previously generated unified patch before starting a new run to prevent stale outputs. * **Tests** * Updated test inputs to ignore `build/**` and added validation for the generated patch file contents. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent fc75245 commit aa1c380

7 files changed

Lines changed: 129 additions & 7 deletions

File tree

cpp-linter/src/cli/structs.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,10 @@ pub struct ClangParams {
227227
impl ClangParams {
228228
/// The directory name to use for caching clang-tidy and clang-format results.
229229
pub(crate) const CACHE_DIR: &str = ".cpp-linter-cache";
230+
231+
/// The file name for aggregating auto-fixes into a unified patch.
232+
pub(crate) const AUTO_FIX_PATCH: &str = "auto-fix.patch";
233+
230234
pub(crate) fn get_cache_path(&self) -> PathBuf {
231235
self.repo_root.join(Self::CACHE_DIR).join("patched")
232236
}

cpp-linter/src/common_fs.rs

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
use std::{
44
fmt::Debug,
55
fs,
6+
io::Write,
67
num::NonZeroU32,
78
ops::RangeInclusive,
89
path::{Path, PathBuf},
@@ -16,7 +17,7 @@ use crate::{
1617
clang_tools::{
1718
ReviewComments, Suggestion, clang_format::FormatAdvice, clang_tidy::TidyAdvice, make_patch,
1819
},
19-
cli::LinesChangedOnly,
20+
cli::{ClangParams, LinesChangedOnly},
2021
error::FileObjError,
2122
};
2223

@@ -162,6 +163,57 @@ impl FileObj {
162163
false
163164
}
164165

166+
/// If the file has a cached fixes, then append them to a unified patched file.
167+
///
168+
/// This is the alternative to [`FileObj::make_suggestions_from_patch()`] when
169+
/// a PR review is not being posted. Both function have to create a patch by
170+
/// reading the original file and patched file (in cache), but
171+
/// [`FileObj::make_suggestions_from_patch()`] does more with the diff than this function.
172+
pub fn maybe_append_patch(&self, repo_root: &Path) -> Result<(), FileObjError> {
173+
let patched = match &self.patched_path {
174+
Some(patched_path) if patched_path.exists() => {
175+
fs::read_to_string(patched_path).map_err(FileObjError::ReadFile)?
176+
}
177+
_ => return Ok(()),
178+
};
179+
let original_content =
180+
fs::read_to_string(repo_root.join(&self.name)).map_err(FileObjError::ReadFile)?;
181+
let (diff, input) = make_patch(patched.as_str(), &original_content);
182+
let file_name = self.name.to_string_lossy().replace("\\", "/");
183+
Self::append_patch(&file_name, &input, &diff, repo_root)?;
184+
Ok(())
185+
}
186+
187+
/// write fixes to a unified patch file in the cache directory.
188+
fn append_patch(
189+
file_name: &str,
190+
input: &InternedInput<&str>,
191+
diff: &Diff,
192+
repo_root: &Path,
193+
) -> Result<(), FileObjError> {
194+
let printer = BasicLineDiffPrinter(&input.interner);
195+
let mut diff_config = UnifiedDiffConfig::default();
196+
diff_config.context_len(0);
197+
let unified_diff = diff.unified_diff(&printer, diff_config, input).to_string();
198+
if !unified_diff.is_empty() {
199+
let patch_path_parent = repo_root.join(ClangParams::CACHE_DIR);
200+
fs::create_dir_all(&patch_path_parent).map_err(FileObjError::MkDirFailed)?;
201+
let patch_file_path = patch_path_parent.join(ClangParams::AUTO_FIX_PATCH);
202+
let mut patch_file = fs::OpenOptions::new()
203+
.append(true)
204+
.create(true)
205+
.truncate(false)
206+
.open(&patch_file_path)
207+
.map_err(FileObjError::OpenPatchFileFailed)?;
208+
patch_file
209+
.write_all(
210+
format!("--- a/{file_name}\n+++ b/{file_name}\n{unified_diff}",).as_bytes(),
211+
)
212+
.map_err(FileObjError::WritePatchFailed)?;
213+
}
214+
Ok(())
215+
}
216+
165217
/// Create a list of [`Suggestion`](struct@crate::clang_tools::Suggestion) from a
166218
/// generated diff and store them in the given
167219
/// [`ReviewComments`](struct@crate::clang_tools::ReviewComments).
@@ -183,7 +235,8 @@ impl FileObj {
183235
let original_content =
184236
fs::read_to_string(repo_root.join(&self.name)).map_err(FileObjError::ReadFile)?;
185237
let (diff, input) = make_patch(patched.as_str(), &original_content);
186-
let file_name = self.name.to_str().unwrap_or_default().replace("\\", "/");
238+
let file_name = self.name.to_string_lossy().replace("\\", "/");
239+
Self::append_patch(&file_name, &input, &diff, repo_root)?;
187240

188241
self.get_suggestions(review_comments, &diff, &input, summary_only)
189242
.map_err(FileObjError::DisplayStringFailed)?;

cpp-linter/src/error.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,18 @@ pub enum FileObjError {
1717
/// Error when failing to generate a patch for a file.
1818
#[error("Failed to print a hunk to a string buffer: {0}")]
1919
DisplayStringFailed(#[source] std::fmt::Error),
20+
21+
/// Error when failing to create the cache directory for the patch file.
22+
#[error("Failed to create cache directory for the patch file: {0}")]
23+
MkDirFailed(#[source] std::io::Error),
24+
25+
/// Error when failing to open the patch file for writing.
26+
#[error("Failed to open patch file for writing: {0}")]
27+
OpenPatchFileFailed(#[source] std::io::Error),
28+
29+
/// Error when failing to write to the patch file.
30+
#[error("Failed to write to the patch file: {0}")]
31+
WritePatchFailed(#[source] std::io::Error),
2032
}
2133

2234
/// Errors related to the REST client used for posting feedback and special logging.

cpp-linter/src/rest_client.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use crate::{
1818
clang_format::{summarize_style, tally_format_advice},
1919
clang_tidy::tally_tidy_advice,
2020
},
21-
cli::{FeedbackInput, ThreadComments},
21+
cli::{ClangParams, FeedbackInput, ThreadComments},
2222
common_fs::FileObj,
2323
error::ClientError,
2424
};
@@ -230,6 +230,23 @@ impl RestClient {
230230
summary_only,
231231
);
232232
self.client.post_pr_review(&options).await?;
233+
} else {
234+
for file in files {
235+
let file = file
236+
.lock()
237+
.map_err(|e| ClientError::MutexPoisoned(e.to_string()))?;
238+
file.maybe_append_patch(&feedback_inputs.repo_root)?;
239+
}
240+
}
241+
let auto_fix_patch_path = feedback_inputs
242+
.repo_root
243+
.join(ClangParams::CACHE_DIR)
244+
.join(ClangParams::AUTO_FIX_PATCH);
245+
if auto_fix_patch_path.exists() {
246+
self.client.write_output_variables(&[OutputVariable {
247+
name: "fix-patch-path".to_string(),
248+
value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"),
249+
}])?;
233250
}
234251
Ok(format_checks_failed + tidy_checks_failed)
235252
}

cpp-linter/src/run.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,12 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
157157
let cache_dir = clang_params.repo_root.join(ClangParams::CACHE_DIR);
158158
std::fs::create_dir_all(&cache_dir)
159159
.with_context(|| "Failed to create a local cache directory.")?;
160+
// delete old patch file
161+
let patch_file = cache_dir.join(ClangParams::AUTO_FIX_PATCH);
162+
if patch_file.exists() {
163+
std::fs::remove_file(&patch_file)
164+
.with_context(|| "Failed to remove old patch file from previous runs.")?;
165+
}
160166
// add gitignore file in project cache dir
161167
std::fs::write(
162168
cache_dir.join(".gitignore"),
@@ -193,7 +199,7 @@ pub(crate) mod test {
193199
#![allow(clippy::unwrap_used)]
194200

195201
use super::run_main;
196-
use crate::test_common::setup_tmp_workspace;
202+
use crate::{cli::ClangParams, test_common::setup_tmp_workspace};
197203
use std::env;
198204

199205
/// helper to avoid writing to the same GITHUB_OUTPUT file in parallel-running tests.
@@ -238,6 +244,13 @@ pub(crate) mod test {
238244
async fn force_debug_output() {
239245
let tmp_gh_out = setup_tmp_gh_out_path();
240246
let tmp_workspace = setup_tmp_workspace();
247+
248+
// create a dummy patch file to ensure it is deleted (in code coverage).
249+
let cache_dir = tmp_workspace.path().join(ClangParams::CACHE_DIR);
250+
std::fs::create_dir_all(&cache_dir).unwrap();
251+
let patch_path = cache_dir.join(ClangParams::AUTO_FIX_PATCH);
252+
std::fs::write(&patch_path, "").unwrap();
253+
241254
run_main(vec![
242255
"cpp-linter".to_string(),
243256
"-l".to_string(),

cpp-linter/tests/comments.rs

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use cpp_linter::cli::ThreadComments;
44
use cpp_linter::run::run_main;
55
use git_bot_feedback::LinesChangedOnly;
66
use mockito::Matcher;
7-
use std::{env, fmt::Display, io::Write, path::Path};
7+
use std::{env, fmt::Display, fs, io::Write, path::Path};
88
use tempfile::{NamedTempFile, TempDir};
99

1010
mod common;
@@ -252,7 +252,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
252252
format!("--thread-comments={}", test_params.thread_comments),
253253
format!("--no-lgtm={}", test_params.no_lgtm),
254254
"-p=build".to_string(),
255-
"-i=build".to_string(),
255+
"-i=build/**".to_string(),
256256
format!("--repo-root={}", tmp_dir.path().to_str().unwrap()),
257257
];
258258
if test_params.force_lgtm {
@@ -274,6 +274,29 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
274274
for mock in mocks {
275275
mock.assert();
276276
}
277+
278+
// Read the GITHUB_OUTPUT file to check for the patch path
279+
let output_vars = fs::read_to_string(tmp_out_file.path()).unwrap();
280+
let patch_path_from_output = output_vars
281+
.lines()
282+
.find(|line| line.starts_with("fix-patch-path="))
283+
.map(|line| {
284+
line.trim_start_matches("fix-patch-path=")
285+
.trim()
286+
.to_string()
287+
});
288+
if let Some(patch_path_from_output) = patch_path_from_output {
289+
// verify patch path exists and print its content
290+
println!("Patch path from GITHUB_OUTPUT: {patch_path_from_output}");
291+
let patch_path = Path::new(&patch_path_from_output);
292+
assert!(
293+
patch_path.exists(),
294+
"Patch file does not exist at expected path."
295+
);
296+
let patch_content =
297+
std::fs::read_to_string(patch_path).expect("Failed to read generated patch file.");
298+
println!("Generated patch content:\n{patch_content}");
299+
}
277300
}
278301

279302
async fn test_comment(test_params: &TestParams) {

cpp-linter/tests/reviews.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
226226
format!("--passive-reviews={}", test_params.passive_reviews),
227227
format!("--no-lgtm={}", test_params.no_lgtm),
228228
"-p=build".to_string(),
229-
"-i=build".to_string(),
229+
"-i=build/**".to_string(),
230230
format!("--repo-root={}", tmp_dir.path().to_str().unwrap()),
231231
];
232232
if test_params.force_lgtm {

0 commit comments

Comments
 (0)