From 7ddf571b45f3a03306fc9a92f5e7bff03fd6945d Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 24 Sep 2026 22:45:58 -0700 Subject: [PATCH 1/4] chore: print patch to stdout if it exists closes #444 --- Cargo.lock | 1 + Cargo.toml | 1 + clang-tools-manager/Cargo.toml | 2 +- cpp-linter/Cargo.toml | 1 + cpp-linter/src/rest_client.rs | 20 +++++++++++++++++++- 5 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 17400205..7a561c63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -406,6 +406,7 @@ dependencies = [ "chrono", "clang-tools-manager", "clap", + "colored", "fast-glob", "futures", "git-bot-feedback", diff --git a/Cargo.toml b/Cargo.toml index 21f93187..1bb9f4ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ strip = "symbols" [workspace.dependencies] anyhow = "1.0.102" clap = { version = "4.6.1", features = ["derive"] } +colored = "3.1.1" log = { version = "0.4.31", features = ["std"] } mockito = "1.7.2" pyo3 = {version = "0.29.0", features = ["extension-module"] } diff --git a/clang-tools-manager/Cargo.toml b/clang-tools-manager/Cargo.toml index 1c570066..4de24a46 100644 --- a/clang-tools-manager/Cargo.toml +++ b/clang-tools-manager/Cargo.toml @@ -25,7 +25,7 @@ required-features = ["bin"] anyhow = { workspace = true, optional = true } blake2 = "0.11.0" clap = { workspace = true, optional = true } -colored = { version = "3.1.1", optional = true } +colored = { workspace = true, optional = true } directories = "6.0.0" log = { workspace = true } regex = { workspace = true } diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml index fe0b3dff..9124da90 100644 --- a/cpp-linter/Cargo.toml +++ b/cpp-linter/Cargo.toml @@ -19,6 +19,7 @@ async-trait = "0.1.89" chrono = "0.4.44" clang-tools-manager = { path = "../clang-tools-manager", features = ["bin"], version = "0.3.1" } clap = { workspace = true, optional = true } +colored = { workspace = true } fast-glob = "1.0.1" futures = "0.3.32" gix-imara-diff = { version = "0.2.2", features = ["unified_diff"] } diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 45b363c3..f8ba1148 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -2,10 +2,12 @@ use std::{ env, + io::BufRead, path::PathBuf, sync::{Arc, Mutex}, }; +use colored::Colorize; use git_bot_feedback::{ AnnotationLevel, CommentKind, CommentPolicy, FileAnnotation, FileFilter, LinesChangedOnly, OutputVariable, RestApiClient, ReviewAction, ReviewOptions, ThreadCommentOptions, @@ -270,11 +272,27 @@ impl RestClient { .repo_root .join(ClangParams::CACHE_DIR) .join(ClangParams::AUTO_FIX_PATCH); - if auto_fix_patch_path.exists() { + 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("\\", "/"), }])?; + + log::info!("Generated patch content:"); + let mut reader = std::io::BufReader::new(patch_file); + let mut line_buf = String::new(); + while let Ok(bytes_read) = reader.read_line(&mut line_buf) + && bytes_read > 0 + { + if line_buf.starts_with('+') { + print!("{}", line_buf.green()); // Green for additions + } else if line_buf.starts_with('-') { + print!("{}", line_buf.red()); // Red for deletions + } else { + print!("{line_buf}"); // Default style for context lines + } + line_buf.clear(); + } } Ok(format_checks_failed + tidy_checks_failed) } From dd034a3bddcf0b0cc5e92f32345d7e41583b7199 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 25 Sep 2026 10:47:38 -0700 Subject: [PATCH 2/4] remove duplicate patch printing from test --- cpp-linter/tests/comments.rs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 53705bf8..70439616 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -1,6 +1,5 @@ #![cfg(feature = "bin")] use chrono::Utc; -use clap::builder::styling::{AnsiColor, Color, Style}; use cpp_linter::rest_client::USER_OUTREACH; use cpp_linter::run::run_main; use cpp_linter::{cli::ThreadComments, rest_client::COMMENT_MARKER}; @@ -309,19 +308,6 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { 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:"); - for l in patch_content.lines() { - let style = if l.starts_with('+') { - Style::new().fg_color(Some(Color::Ansi(AnsiColor::Green))) // Green for additions - } else if l.starts_with('-') { - Style::new().fg_color(Some(Color::Ansi(AnsiColor::Red))) // Red for deletions - } else { - Style::new() // Default style for context lines - }; - println!("{style}{l}{style:#}"); - } let apply_patch_status = std::process::Command::new("git") .arg("apply") From fddca9e40b3fa9bdbc64d577dae54615132715cd Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 25 Sep 2026 12:21:53 -0700 Subject: [PATCH 3/4] avoid fast fail in post_feedback() ensures the patch is printed before an error short-circuits the function. --- cpp-linter/src/error.rs | 4 + cpp-linter/src/rest_client.rs | 135 ++++++++++++++++++++-------------- 2 files changed, 82 insertions(+), 57 deletions(-) diff --git a/cpp-linter/src/error.rs b/cpp-linter/src/error.rs index d934ff7b..ecd49778 100644 --- a/cpp-linter/src/error.rs +++ b/cpp-linter/src/error.rs @@ -70,6 +70,10 @@ pub enum ClientError { #[source] source: std::io::Error, }, + + /// Error when failing to write to stdout. + #[error("Failed to write to stdout: {0}")] + StdoutWriteFailed(#[source] std::io::Error), } /// Errors related to invoking clang tools and processing their output. diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 0b56ec1b..9125b5ce 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -2,7 +2,7 @@ use std::{ env, - io::BufRead, + io::{BufRead, Write, stdout}, path::PathBuf, sync::{Arc, Mutex}, }; @@ -147,11 +147,11 @@ impl RestClient { if feedback_inputs.step_summary { self.client.append_step_summary(&summary)?; } - if let Some(summary_output_file) = feedback_inputs.summary_output_file { + if let Some(summary_output_file) = &feedback_inputs.summary_output_file { let output_file = if summary_output_file.is_absolute() { summary_output_file } else { - feedback_inputs.repo_root.join(&summary_output_file) + &feedback_inputs.repo_root.join(summary_output_file) }; if let Some(parent) = output_file.parent() { std::fs::create_dir_all(parent).map_err(|e| ClientError::MkDirFailed { @@ -159,9 +159,9 @@ impl RestClient { source: e, })?; } - std::fs::write(&output_file, &summary).map_err(|e| { + std::fs::write(output_file, &summary).map_err(|e| { ClientError::SummaryOutputFileWriteFailed { - file_path: output_file, + file_path: output_file.to_owned(), source: e, } })?; @@ -184,35 +184,6 @@ impl RestClient { ]; self.client.write_output_variables(&output_vars)?; - if feedback_inputs.thread_comments != ThreadComments::Off { - // post thread comment for PR or push event - if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) { - comment = Some(Self::make_comment( - files, - format_checks_failed, - tidy_checks_failed, - &clang_versions, - Some(u16::MAX as u64), - )?); - } - let options = ThreadCommentOptions { - policy: if feedback_inputs.thread_comments == ThreadComments::Update { - CommentPolicy::Update - } else { - // feedback_inputs.thread_comments is not Off and not Update, so it must be just On. - CommentPolicy::Anew - }, - comment: comment.unwrap_or_default(), - kind: if format_checks_failed == 0 && tidy_checks_failed == 0 { - CommentKind::Lgtm - } else { - CommentKind::Concerns - }, - marker: COMMENT_MARKER.to_string(), - no_lgtm: feedback_inputs.no_lgtm, - }; - self.client.post_thread_comment(options).await?; - } if self.client.is_pr_event() && feedback_inputs.pr_review { let summary_only = ["true", "on", "1"].contains( &env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY") @@ -230,6 +201,7 @@ impl RestClient { &feedback_inputs.repo_root, )?; } + self.show_patch(&feedback_inputs)?; let mut options = ReviewOptions { marker: COMMENT_MARKER.to_string(), @@ -267,32 +239,37 @@ impl RestClient { .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; file.maybe_append_patch(&feedback_inputs.repo_root)?; } + self.show_patch(&feedback_inputs)?; } - 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) { - self.client.write_output_variables(&[OutputVariable { - name: "fix-patch-path".to_string(), - value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), - }])?; - log::info!("Generated patch content:"); - let mut reader = std::io::BufReader::new(patch_file); - let mut line_buf = String::new(); - while let Ok(bytes_read) = reader.read_line(&mut line_buf) - && bytes_read > 0 - { - if line_buf.starts_with('+') { - print!("{}", line_buf.green()); // Green for additions - } else if line_buf.starts_with('-') { - print!("{}", line_buf.red()); // Red for deletions - } else { - print!("{line_buf}"); // Default style for context lines - } - line_buf.clear(); + if feedback_inputs.thread_comments != ThreadComments::Off { + // post thread comment for PR or push event + if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) { + comment = Some(Self::make_comment( + files, + format_checks_failed, + tidy_checks_failed, + &clang_versions, + Some(u16::MAX as u64), + )?); } + let options = ThreadCommentOptions { + policy: if feedback_inputs.thread_comments == ThreadComments::Update { + CommentPolicy::Update + } else { + // feedback_inputs.thread_comments is not Off and not Update, so it must be just On. + CommentPolicy::Anew + }, + comment: comment.unwrap_or_default(), + kind: if format_checks_failed == 0 && tidy_checks_failed == 0 { + CommentKind::Lgtm + } else { + CommentKind::Concerns + }, + marker: COMMENT_MARKER.to_string(), + no_lgtm: feedback_inputs.no_lgtm, + }; + self.client.post_thread_comment(options).await?; } Ok(format_checks_failed + tidy_checks_failed) } @@ -427,6 +404,50 @@ impl RestClient { comment.push_str(USER_OUTREACH); Ok(comment) } + + /// Show the generated patch content in the console output. + /// + /// 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) { + self.client.write_output_variables(&[OutputVariable { + name: "fix-patch-path".to_string(), + value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), + }])?; + + log::info!("Generated patch content:"); + let stdout_handle = stdout(); + let mut reader = std::io::BufReader::new(patch_file); + let mut line_buf = String::new(); + let mut stdout_lock = stdout_handle.lock(); + while let Ok(bytes_read) = reader.read_line(&mut line_buf) + && bytes_read > 0 + { + if line_buf.starts_with('+') { + // Green for additions + stdout_lock + .write_all(line_buf.green().to_string().as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } else if line_buf.starts_with('-') { + // Red for deletions + stdout_lock + .write_all(line_buf.red().to_string().as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } else { + // Default style for context lines + stdout_lock + .write_all(line_buf.as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } + line_buf.clear(); + } + } + Ok(()) + } } /// A closing tag for details blocks in markdown comments. From 09d2ad1f0c10d970819fbf2cb25b9ed1f10689f4 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 25 Sep 2026 12:49:02 -0700 Subject: [PATCH 4/4] propogate read file error when printing patch file --- cpp-linter/src/error.rs | 4 ++++ cpp-linter/src/rest_client.rs | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cpp-linter/src/error.rs b/cpp-linter/src/error.rs index ecd49778..b01343e6 100644 --- a/cpp-linter/src/error.rs +++ b/cpp-linter/src/error.rs @@ -74,6 +74,10 @@ pub enum ClientError { /// Error when failing to write to stdout. #[error("Failed to write to stdout: {0}")] StdoutWriteFailed(#[source] std::io::Error), + + /// Error when failing to read the generated patch file. + #[error("Failed to read the generated patch file: {0}")] + PatchReadFailed(#[source] std::io::Error), } /// Errors related to invoking clang tools and processing their output. diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 9125b5ce..8b457687 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -424,7 +424,9 @@ impl RestClient { let mut reader = std::io::BufReader::new(patch_file); let mut line_buf = String::new(); let mut stdout_lock = stdout_handle.lock(); - while let Ok(bytes_read) = reader.read_line(&mut line_buf) + while let bytes_read = reader + .read_line(&mut line_buf) + .map_err(ClientError::PatchReadFailed)? && bytes_read > 0 { if line_buf.starts_with('+') {