use lines-changed-only when getting file changes

move helper function to gh-specific API

Author: 2bndy5

cpp-linter/src/cli/structs.rs

@@ -25,6 +25,14 @@ impl LinesChangedOnly {
             _ => LinesChangedOnly::Off,
         }
     }
+
+    pub fn is_change_valid(&self, added_lines: bool, diff_chunks: bool) -> bool {
+        match self {
+            LinesChangedOnly::Off => true,
+            LinesChangedOnly::Diff => diff_chunks,
+            LinesChangedOnly::On => added_lines,
+        }
+    }
 }
 
 impl Display for LinesChangedOnly {

cpp-linter/src/common_fs/file_filter.rs

@@ -129,23 +129,17 @@ impl FileFilter {
     /// - Is `entry` specified in the list of explicitly `not_ignored` paths? (supersedes
     ///   specified `ignored` paths)
     pub fn is_source_or_ignored(&self, entry: &Path) -> bool {
-        let extension = entry.extension();
-        if extension.is_none() {
+        let extension = entry
+            .extension()
+            .unwrap_or_default() // allow for matching files with no extension
+            .to_string_lossy()
+            .to_string();
+        if !self.extensions.contains(&extension) {
             return false;
         }
-        let mut is_ignored = true;
-        for ext in &self.extensions {
-            if ext == &extension.unwrap().to_os_string().into_string().unwrap() {
-                is_ignored = false;
-                break;
-            }
-        }
-        if !is_ignored {
-            let is_in_ignored = self.is_file_in_list(entry, true);
-            let is_in_not_ignored = self.is_file_in_list(entry, false);
-            if is_in_not_ignored || !is_in_ignored {
-                return true;
-            }
+        let is_in_not_ignored = self.is_file_in_list(entry, false);
+        if is_in_not_ignored || !self.is_file_in_list(entry, true) {
+            return true;
         }
         false
     }

cpp-linter/src/git.rs

@@ -15,7 +15,10 @@ use anyhow::{Context, Result};
 use git2::{Diff, Error, Patch, Repository};
 
 // project specific modules/crates
-use crate::common_fs::{FileFilter, FileObj};
+use crate::{
+    cli::LinesChangedOnly,
+    common_fs::{FileFilter, FileObj},
+};
 
 /// This (re-)initializes the repository located in the specified `path`.
 ///
@@ -100,7 +103,11 @@ fn parse_patch(patch: &Patch) -> (Vec<u32>, Vec<RangeInclusive<u32>>) {
 ///
 /// The specified list of `extensions`, `ignored` and `not_ignored` files are used as
 /// filters to expedite the process and only focus on the data cpp_linter can use.
-pub fn parse_diff(diff: &git2::Diff, file_filter: &FileFilter) -> Vec<FileObj> {
+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();
@@ -112,7 +119,10 @@ pub fn parse_diff(diff: &git2::Diff, file_filter: &FileFilter) -> Vec<FileObj> {
         {
             let (added_lines, diff_chunks) =
                 parse_patch(&Patch::from_diff(diff, file_idx).unwrap().unwrap());
-            files.push(FileObj::from(file_path, added_lines, diff_chunks));
+            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
@@ -125,12 +135,20 @@ pub fn parse_diff(diff: &git2::Diff, file_filter: &FileFilter) -> Vec<FileObj> {
 /// 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) -> Vec<FileObj> {
+pub fn parse_diff_from_buf(
+    buff: &[u8],
+    file_filter: &FileFilter,
+    lines_changed_only: &LinesChangedOnly,
+) -> Vec<FileObj> {
     if let Ok(diff_obj) = &Diff::from_buffer(buff) {
-        parse_diff(diff_obj, file_filter)
+        parse_diff(diff_obj, file_filter, lines_changed_only)
     } else {
         log::warn!("libgit2 failed to parse the diff");
-        brute_force_parse_diff::parse_diff(&String::from_utf8_lossy(buff), file_filter)
+        brute_force_parse_diff::parse_diff(
+            &String::from_utf8_lossy(buff),
+            file_filter,
+            lines_changed_only,
+        )
     }
 }
 
@@ -146,7 +164,10 @@ mod brute_force_parse_diff {
     use regex::Regex;
     use std::{ops::RangeInclusive, path::PathBuf};
 
-    use crate::common_fs::{FileFilter, FileObj};
+    use crate::{
+        cli::LinesChangedOnly,
+        common_fs::{FileFilter, FileObj},
+    };
 
     fn get_filename_from_front_matter(front_matter: &str) -> Option<&str> {
         let diff_file_name = Regex::new(r"(?m)^\+\+\+\sb?/(.*)$").unwrap();
@@ -209,7 +230,11 @@ mod brute_force_parse_diff {
         (additions, diff_chunks)
     }
 
-    pub fn parse_diff(diff: &str, file_filter: &FileFilter) -> Vec<FileObj> {
+    pub fn parse_diff(
+        diff: &str,
+        file_filter: &FileFilter,
+        lines_changed_only: &LinesChangedOnly,
+    ) -> Vec<FileObj> {
         log::error!("Using brute force diff parsing!");
         let mut results = Vec::new();
         let diff_file_delimiter = Regex::new(r"(?m)^diff --git a/.*$").unwrap();
@@ -230,7 +255,11 @@ mod brute_force_parse_diff {
                 let file_path = PathBuf::from(file_name);
                 if file_filter.is_source_or_ignored(&file_path) {
                     let (added_lines, diff_chunks) = parse_patch(&file_diff[hunk_start..]);
-                    results.push(FileObj::from(file_path, added_lines, diff_chunks));
+                    if lines_changed_only
+                        .is_change_valid(!added_lines.is_empty(), !diff_chunks.is_empty())
+                    {
+                        results.push(FileObj::from(file_path, added_lines, diff_chunks));
+                    }
                 }
             }
             // } else {
@@ -247,6 +276,7 @@ mod brute_force_parse_diff {
 
         use super::parse_diff;
         use crate::{
+            cli::LinesChangedOnly,
             common_fs::{FileFilter, FileObj},
             git::parse_diff_from_buf,
         };
@@ -274,6 +304,7 @@ rename to /tests/demo/some source.c
             let files = parse_diff_from_buf(
                 diff_buf,
                 &FileFilter::new(&["target".to_string()], vec!["c".to_string()]),
+                &LinesChangedOnly::Off,
             );
             assert!(!files.is_empty());
             assert!(files
@@ -289,6 +320,7 @@ rename to /tests/demo/some source.c
             let files = parse_diff_from_buf(
                 diff_buf,
                 &FileFilter::new(&["target".to_string()], vec!["c".to_string()]),
+                &LinesChangedOnly::Off,
             );
             assert!(!files.is_empty());
         }
@@ -301,8 +333,13 @@ rename to /tests/demo/some source.c
                 parse_diff_from_buf(
                     buf.as_bytes(),
                     &FileFilter::new(&ignore, extensions.to_owned()),
+                    &LinesChangedOnly::Off,
+                ),
+                parse_diff(
+                    buf,
+                    &FileFilter::new(&ignore, extensions.to_owned()),
+                    &LinesChangedOnly::Off,
                 ),
-                parse_diff(buf, &FileFilter::new(&ignore, extensions.to_owned())),
             )
         }
 
@@ -377,6 +414,7 @@ mod test {
     use tempfile::{tempdir, TempDir};
 
     use crate::{
+        cli::LinesChangedOnly,
         common_fs::FileFilter,
         rest_api::{github::GithubApiClient, RestApiClient},
     };
@@ -406,7 +444,7 @@ mod test {
         env::set_var("CI", "false"); // avoid use of REST API when testing in CI
         rest_api_client
             .unwrap()
-            .get_list_of_changed_files(&file_filter)
+            .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off)
             .await
             .unwrap()
     }

cpp-linter/src/rest_api/github/mod.rs

@@ -14,21 +14,19 @@ use reqwest::{
     header::{HeaderMap, HeaderValue, AUTHORIZATION},
     Client, Method, Url,
 };
-use serde_json;
 
 // project specific modules/crates
 use super::{RestApiClient, RestApiRateLimitHeaders};
 use crate::clang_tools::clang_format::tally_format_advice;
 use crate::clang_tools::clang_tidy::tally_tidy_advice;
 use crate::clang_tools::ClangVersions;
-use crate::cli::{FeedbackInput, ThreadComments};
+use crate::cli::{FeedbackInput, LinesChangedOnly, ThreadComments};
 use crate::common_fs::{FileFilter, FileObj};
 use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf};
 
 // private submodules.
 mod serde_structs;
 mod specific_api;
-use serde_structs::{GithubChangedFile, PushEventFiles};
 
 /// A structure to work with Github REST API.
 pub struct GithubApiClient {
@@ -121,7 +119,15 @@ impl RestApiClient for GithubApiClient {
         Ok(headers)
     }
 
-    async fn get_list_of_changed_files(&self, file_filter: &FileFilter) -> Result<Vec<FileObj>> {
+    /// A way to get the list of changed files using REST API calls that employ a paginated response.
+    ///
+    /// This is a helper to [`RestApiClient::get_list_of_changed_files()`] but takes a formulated URL
+    /// endpoint based on the context of the triggering CI event.
+    async fn get_list_of_changed_files(
+        &self,
+        file_filter: &FileFilter,
+        lines_changed_only: &LinesChangedOnly,
+    ) -> Result<Vec<FileObj>> {
         if env::var("CI").is_ok_and(|val| val.as_str() == "true")
             && self.repo.is_some()
             && self.sha.is_some()
@@ -153,9 +159,13 @@ impl RestApiClient for GithubApiClient {
                 0,
             )
             .await
-            .with_context(|| "Failed to get list of changed files from GitHub server.")?;
+            .with_context(|| "Failed to get list of changed files.")?;
             if response.status().is_success() {
-                Ok(parse_diff_from_buf(&response.bytes().await?, file_filter))
+                Ok(parse_diff_from_buf(
+                    &response.bytes().await?,
+                    file_filter,
+                    lines_changed_only,
+                ))
             } else {
                 let endpoint = if is_pr {
                     Url::parse(format!("{}/files", url.as_str()).as_str())?
@@ -164,69 +174,19 @@ impl RestApiClient for GithubApiClient {
                 };
                 Self::log_response(response, "Failed to get full diff for event").await;
                 log::debug!("Trying paginated request to {}", endpoint.as_str());
-                self.get_changed_files_paginated(endpoint, file_filter)
+                self.get_changed_files_paginated(endpoint, file_filter, lines_changed_only)
                     .await
             }
         } else {
             // get diff from libgit2 API
             let repo = open_repo(".").with_context(|| {
                 "Please ensure the repository is checked out before running cpp-linter."
             })?;
-            let list = parse_diff(&get_diff(&repo)?, file_filter);
+            let list = parse_diff(&get_diff(&repo)?, file_filter, lines_changed_only);
             Ok(list)
         }
     }
 
-    async fn get_changed_files_paginated(
-        &self,
-        url: Url,
-        file_filter: &FileFilter,
-    ) -> Result<Vec<FileObj>> {
-        let mut url = Some(Url::parse_with_params(url.as_str(), &[("page", "1")])?);
-        let mut files = vec![];
-        while let Some(ref endpoint) = url {
-            let request =
-                Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?;
-            let response = Self::send_api_request(
-                self.client.clone(),
-                request,
-                self.rate_limit_headers.clone(),
-                0,
-            )
-            .await;
-            if let Ok(response) = response {
-                url = Self::try_next_page(response.headers());
-                let files_list = if self.event_name != "pull_request" {
-                    let json_value: PushEventFiles = serde_json::from_str(&response.text().await?)
-                        .with_context(|| {
-                            "Failed to deserialize list of changed files from json response"
-                        })?;
-                    json_value.files
-                } else {
-                    serde_json::from_str::<Vec<GithubChangedFile>>(&response.text().await?)
-                        .with_context(|| {
-                            "Failed to deserialize list of file changes from Pull Request event."
-                        })?
-                };
-                for file in files_list {
-                    if let Some(patch) = file.patch {
-                        let diff = format!(
-                            "diff --git a/{old} b/{new}\n--- a/{old}\n+++ b/{new}\n{patch}",
-                            old = file.previous_filename.unwrap_or(file.filename.clone()),
-                            new = file.filename,
-                        );
-                        if let Some(file_obj) =
-                            parse_diff_from_buf(diff.as_bytes(), file_filter).first()
-                        {
-                            files.push(file_obj.to_owned());
-                        }
-                    }
-                }
-            }
-        }
-        Ok(files)
-    }
-
     async fn post_feedback(
         &self,
         files: &[Arc<Mutex<FileObj>>],
@@ -319,7 +279,7 @@ mod test {
             clang_tidy::{TidyAdvice, TidyNotification},
             ClangVersions,
         },
-        cli::FeedbackInput,
+        cli::{FeedbackInput, LinesChangedOnly},
         common_fs::{FileFilter, FileObj},
         logger,
         rest_api::{RestApiClient, USER_OUTREACH},
@@ -478,7 +438,7 @@ mod test {
         env::set_current_dir(tmp_dir.path()).unwrap();
         let rest_client = GithubApiClient::new().unwrap();
         let files = rest_client
-            .get_list_of_changed_files(&FileFilter::new(&[], vec![]))
+            .get_list_of_changed_files(&FileFilter::new(&[], vec![]), &LinesChangedOnly::Off)
             .await;
         assert!(files.is_err())
     }