coverage review

Looks like the all production code in the git module is no longer actually used, not in tests nor production.
So what remains is just the regressions tests.

refactored the new rest_client logic a bit, mainly just removing lines that are never triggered by tests.

Author: 2bndy5

cpp-linter/src/git.rs

@@ -1,114 +1,8 @@
-//! This module is primarily used to parse diff blobs.
+//! This module was primarily used to parse diff blobs.
 //!
-//! It can also be used (locally) to get a list of files changes from either the last
-//! commit or the next commit's staging area.
-//!
-//! This also includes a private module that is used as a fallback (brute force)
-//! mechanism when parsing diffs fail using libgit2. NOTE: parsing a diff from a buffer
-//! (str or bytes) only happens in CI or when libgit2 cannot be used to initialize a
-//! repository.
-
-use std::{ops::RangeInclusive, path::PathBuf};
-
-// non-std crates
-use anyhow::Result;
-use git2::{Diff, Patch};
-
-// project specific modules/crates
-use crate::{cli::LinesChangedOnly, common_fs::FileObj};
-use git_bot_feedback::{FileFilter, error::DiffError};
-
-/// Parses a patch for a single file in a diff.
-///
-/// Returns the list of line numbers that have additions and the ranges spanning each
-/// chunk present in the `patch`.
-fn parse_patch(patch: &Patch) -> (Vec<u32>, Vec<RangeInclusive<u32>>) {
-    let mut additions = Vec::new();
-    let mut diff_hunks = Vec::new();
-    for hunk_idx in 0..patch.num_hunks() {
-        let (hunk, line_count) = patch.hunk(hunk_idx).unwrap();
-        diff_hunks.push(RangeInclusive::new(
-            hunk.new_start(),
-            hunk.new_start() + hunk.new_lines(),
-        ));
-        for line in 0..line_count {
-            let diff_line = patch.line_in_hunk(hunk_idx, line).unwrap();
-            if diff_line.origin_value() == git2::DiffLineType::Addition {
-                additions.push(diff_line.new_lineno().unwrap());
-            }
-        }
-    }
-    (additions, diff_hunks)
-}
-
-/// Parses a given [`git2::Diff`] and returns a list of [`FileObj`]s.
-///
-/// The `lines_changed_only` parameter is used to expedite the process and only
-/// focus on files that have relevant changes. The `file_filter` parameter applies
-/// a filter to only include source files (or ignored files) based on the
-/// extensions and ignore patterns specified.
-pub fn parse_diff(
-    diff: &git2::Diff,
-    file_filter: &FileFilter,
-    lines_changed_only: &LinesChangedOnly,
-) -> Vec<FileObj> {
-    let mut files: Vec<FileObj> = Vec::new();
-    for file_idx in 0..diff.deltas().count() {
-        let diff_delta = diff.get_delta(file_idx).unwrap();
-        let file_path = diff_delta.new_file().path().unwrap().to_path_buf();
-        if matches!(
-            diff_delta.status(),
-            git2::Delta::Added | git2::Delta::Modified | git2::Delta::Renamed,
-        ) && file_filter.is_qualified(&file_path)
-        {
-            let (added_lines, diff_chunks) =
-                parse_patch(&Patch::from_diff(diff, file_idx).unwrap().unwrap());
-            if lines_changed_only.is_change_valid(!added_lines.is_empty(), !diff_chunks.is_empty())
-            {
-                files.push(FileObj::from(file_path, added_lines, diff_chunks));
-            }
-        }
-    }
-    files
-}
-
-/// Same as [`parse_diff`] but takes a buffer of bytes instead of a [`git2::Diff`].
-///
-/// In the case that libgit2 fails to parse the buffer of bytes, a private algorithm is
-/// used. In such a case, brute force parsing the diff as a string can be costly. So, a
-/// log warning and error are output when this occurs. Please report this instance for
-/// troubleshooting/diagnosis as this likely means the diff is malformed or there is a
-/// bug in libgit2 source.
-pub fn parse_diff_from_buf(
-    buff: &[u8],
-    file_filter: &FileFilter,
-    lines_changed_only: &LinesChangedOnly,
-) -> Result<Vec<FileObj>, DiffError> {
-    if let Ok(diff_obj) = &Diff::from_buffer(buff) {
-        Ok(parse_diff(diff_obj, file_filter, lines_changed_only))
-    } else {
-        log::warn!("libgit2 failed to parse the diff");
-        Ok(git_bot_feedback::parse_diff(
-            &String::from_utf8_lossy(buff),
-            file_filter,
-            &lines_changed_only.clone().into(),
-        )?
-        .iter()
-        .map(|(name, diff_lines)| {
-            let diff_chunks = diff_lines
-                .diff_hunks
-                .iter()
-                .map(|hunk| hunk.start..=hunk.end)
-                .collect();
-            FileObj::from(
-                PathBuf::from(&name),
-                diff_lines.added_lines.clone(),
-                diff_chunks,
-            )
-        })
-        .collect())
-    }
-}
+//! Since migrating to git-bot-feedback crate, this module is now purely for regression testing.
+//! Any logic that parses diffs from 2 text blobs has been moved to git-bot-feedback.
+//! Some diff creation/parsing logic remains in clang_tools/mod.rs module using libgit2 API instead.
 
 #[cfg(test)]
 mod test {
@@ -127,7 +21,6 @@ mod test {
 
     // used to setup a testing stage
     fn clone_repo(sha: Option<&str>, path: &str, patch_path: Option<&str>) {
-        // let repo = Repository::clone(TEST_REPO_URL, path).unwrap();
         let ok = Command::new("git")
             .args(["clone", TEST_REPO_URL, path])
             .status()
@@ -303,15 +196,6 @@ mod test {
             true,
         )
         .await;
-        Command::new("git")
-            .args([
-                "--no-pager",
-                "show",
-                "a2875ac00e6f1dc3eb4ac19712c7a241b5a76e83",
-                "--format=%b",
-            ])
-            .status()
-            .unwrap();
         eprintln!("files: {files:?}");
         assert!(files.is_empty());
         set_current_dir(cur_dir).unwrap(); // prep to delete temp_folder

cpp-linter/src/rest_client/mod.rs

@@ -142,10 +142,11 @@ impl RestClient {
                 ));
             }
             let options = ThreadCommentOptions {
-                policy: match feedback_inputs.thread_comments {
-                    ThreadComments::Update => CommentPolicy::Update,
-                    ThreadComments::On => CommentPolicy::Anew,
-                    ThreadComments::Off => unreachable!(),
+                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 {
@@ -258,10 +259,9 @@ impl RestClient {
                         let title = format!("{}:{}:{}", note.filename, note.line, note.cols);
                         let annotation = FileAnnotation {
                             severity: match note.severity.as_str() {
-                                "note" => AnnotationLevel::Notice,
                                 "warning" => AnnotationLevel::Warning,
                                 "error" => AnnotationLevel::Error,
-                                _ => AnnotationLevel::Notice, // default to notice if severity is unrecognized
+                                _ => AnnotationLevel::Notice, // default to notice for all else
                             },
                             path,
                             start_line: None,
@@ -417,7 +417,7 @@ mod test {
         default::Default,
         env,
         io::Read,
-        path::{Path, PathBuf},
+        path::PathBuf,
         sync::{Arc, Mutex},
     };
 
@@ -439,11 +439,7 @@ mod test {
 
     // ************************* tests for step-summary and output variables
 
-    async fn create_comment(
-        is_lgtm: bool,
-        fail_gh_out: bool,
-        fail_summary: bool,
-    ) -> (String, String) {
+    async fn create_comment(is_lgtm: bool) -> (String, String) {
         let tmp_dir = tempdir().unwrap();
         unsafe {
             // ensure we are mimicking a CI platform
@@ -490,27 +486,14 @@ mod test {
                 String::from("file")
             },
             step_summary: true,
+            file_annotations: false,
             ..Default::default()
         };
         let mut step_summary_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
         let mut gh_out_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
         unsafe {
-            env::set_var(
-                "GITHUB_STEP_SUMMARY",
-                if fail_summary {
-                    Path::new("not-a-file.txt")
-                } else {
-                    step_summary_path.path()
-                },
-            );
-            env::set_var(
-                "GITHUB_OUTPUT",
-                if fail_gh_out {
-                    Path::new("not-a-file.txt")
-                } else {
-                    gh_out_path.path()
-                },
-            );
+            env::set_var("GITHUB_STEP_SUMMARY", step_summary_path.path());
+            env::set_var("GITHUB_OUTPUT", gh_out_path.path());
         }
         let clang_versions = ClangVersions {
             format_version: Some(Version::new(1, 2, 3)),
@@ -524,20 +507,16 @@ mod test {
         step_summary_path
             .read_to_string(&mut step_summary_content)
             .unwrap();
-        if !fail_summary {
-            assert!(&step_summary_content.contains(USER_OUTREACH));
-        }
+        assert!(&step_summary_content.contains(USER_OUTREACH));
         let mut gh_out_content = String::new();
         gh_out_path.read_to_string(&mut gh_out_content).unwrap();
-        if !fail_gh_out {
-            assert!(gh_out_content.starts_with("checks-failed="));
-        }
+        assert!(gh_out_content.starts_with("checks-failed="));
         (step_summary_content, gh_out_content)
     }
 
     #[tokio::test]
     async fn check_comment_concerns() {
-        let (comment, gh_out) = create_comment(false, false, false).await;
+        let (comment, gh_out) = create_comment(false).await;
         assert!(&comment.contains(":warning:\nSome files did not pass the configured checks!\n"));
         let fmt_pattern = Regex::new(r"format-checks-failed=(\d+)\n").unwrap();
         let tidy_pattern = Regex::new(r"tidy-checks-failed=(\d+)\n").unwrap();
@@ -559,7 +538,7 @@ mod test {
         unsafe {
             env::set_var("ACTIONS_STEP_DEBUG", "true");
         }
-        let (comment, gh_out) = create_comment(true, false, false).await;
+        let (comment, gh_out) = create_comment(true).await;
         assert!(comment.contains(":heavy_check_mark:\nNo problems need attention."));
         assert_eq!(
             gh_out,