Skip to content

Commit 458c876

Browse files
committed
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.
1 parent f60737a commit 458c876

2 files changed

Lines changed: 19 additions & 146 deletions

File tree

cpp-linter/src/git.rs

Lines changed: 4 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -1,114 +1,8 @@
1-
//! This module is primarily used to parse diff blobs.
1+
//! This module was primarily used to parse diff blobs.
22
//!
3-
//! It can also be used (locally) to get a list of files changes from either the last
4-
//! commit or the next commit's staging area.
5-
//!
6-
//! This also includes a private module that is used as a fallback (brute force)
7-
//! mechanism when parsing diffs fail using libgit2. NOTE: parsing a diff from a buffer
8-
//! (str or bytes) only happens in CI or when libgit2 cannot be used to initialize a
9-
//! repository.
10-
11-
use std::{ops::RangeInclusive, path::PathBuf};
12-
13-
// non-std crates
14-
use anyhow::Result;
15-
use git2::{Diff, Patch};
16-
17-
// project specific modules/crates
18-
use crate::{cli::LinesChangedOnly, common_fs::FileObj};
19-
use git_bot_feedback::{FileFilter, error::DiffError};
20-
21-
/// Parses a patch for a single file in a diff.
22-
///
23-
/// Returns the list of line numbers that have additions and the ranges spanning each
24-
/// chunk present in the `patch`.
25-
fn parse_patch(patch: &Patch) -> (Vec<u32>, Vec<RangeInclusive<u32>>) {
26-
let mut additions = Vec::new();
27-
let mut diff_hunks = Vec::new();
28-
for hunk_idx in 0..patch.num_hunks() {
29-
let (hunk, line_count) = patch.hunk(hunk_idx).unwrap();
30-
diff_hunks.push(RangeInclusive::new(
31-
hunk.new_start(),
32-
hunk.new_start() + hunk.new_lines(),
33-
));
34-
for line in 0..line_count {
35-
let diff_line = patch.line_in_hunk(hunk_idx, line).unwrap();
36-
if diff_line.origin_value() == git2::DiffLineType::Addition {
37-
additions.push(diff_line.new_lineno().unwrap());
38-
}
39-
}
40-
}
41-
(additions, diff_hunks)
42-
}
43-
44-
/// Parses a given [`git2::Diff`] and returns a list of [`FileObj`]s.
45-
///
46-
/// The `lines_changed_only` parameter is used to expedite the process and only
47-
/// focus on files that have relevant changes. The `file_filter` parameter applies
48-
/// a filter to only include source files (or ignored files) based on the
49-
/// extensions and ignore patterns specified.
50-
pub fn parse_diff(
51-
diff: &git2::Diff,
52-
file_filter: &FileFilter,
53-
lines_changed_only: &LinesChangedOnly,
54-
) -> Vec<FileObj> {
55-
let mut files: Vec<FileObj> = Vec::new();
56-
for file_idx in 0..diff.deltas().count() {
57-
let diff_delta = diff.get_delta(file_idx).unwrap();
58-
let file_path = diff_delta.new_file().path().unwrap().to_path_buf();
59-
if matches!(
60-
diff_delta.status(),
61-
git2::Delta::Added | git2::Delta::Modified | git2::Delta::Renamed,
62-
) && file_filter.is_qualified(&file_path)
63-
{
64-
let (added_lines, diff_chunks) =
65-
parse_patch(&Patch::from_diff(diff, file_idx).unwrap().unwrap());
66-
if lines_changed_only.is_change_valid(!added_lines.is_empty(), !diff_chunks.is_empty())
67-
{
68-
files.push(FileObj::from(file_path, added_lines, diff_chunks));
69-
}
70-
}
71-
}
72-
files
73-
}
74-
75-
/// Same as [`parse_diff`] but takes a buffer of bytes instead of a [`git2::Diff`].
76-
///
77-
/// In the case that libgit2 fails to parse the buffer of bytes, a private algorithm is
78-
/// used. In such a case, brute force parsing the diff as a string can be costly. So, a
79-
/// log warning and error are output when this occurs. Please report this instance for
80-
/// troubleshooting/diagnosis as this likely means the diff is malformed or there is a
81-
/// bug in libgit2 source.
82-
pub fn parse_diff_from_buf(
83-
buff: &[u8],
84-
file_filter: &FileFilter,
85-
lines_changed_only: &LinesChangedOnly,
86-
) -> Result<Vec<FileObj>, DiffError> {
87-
if let Ok(diff_obj) = &Diff::from_buffer(buff) {
88-
Ok(parse_diff(diff_obj, file_filter, lines_changed_only))
89-
} else {
90-
log::warn!("libgit2 failed to parse the diff");
91-
Ok(git_bot_feedback::parse_diff(
92-
&String::from_utf8_lossy(buff),
93-
file_filter,
94-
&lines_changed_only.clone().into(),
95-
)?
96-
.iter()
97-
.map(|(name, diff_lines)| {
98-
let diff_chunks = diff_lines
99-
.diff_hunks
100-
.iter()
101-
.map(|hunk| hunk.start..=hunk.end)
102-
.collect();
103-
FileObj::from(
104-
PathBuf::from(&name),
105-
diff_lines.added_lines.clone(),
106-
diff_chunks,
107-
)
108-
})
109-
.collect())
110-
}
111-
}
3+
//! Since migrating to git-bot-feedback crate, this module is now purely for regression testing.
4+
//! Any logic that parses diffs from 2 text blobs has been moved to git-bot-feedback.
5+
//! Some diff creation/parsing logic remains in clang_tools/mod.rs module using libgit2 API instead.
1126
1137
#[cfg(test)]
1148
mod test {

cpp-linter/src/rest_client/mod.rs

Lines changed: 15 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,11 @@ impl RestClient {
142142
));
143143
}
144144
let options = ThreadCommentOptions {
145-
policy: match feedback_inputs.thread_comments {
146-
ThreadComments::Update => CommentPolicy::Update,
147-
ThreadComments::On => CommentPolicy::Anew,
148-
ThreadComments::Off => unreachable!(),
145+
policy: if feedback_inputs.thread_comments == ThreadComments::Update {
146+
CommentPolicy::Update
147+
} else {
148+
// feedback_inputs.thread_comments is not Off and not Update, so it must be just On.
149+
CommentPolicy::Anew
149150
},
150151
comment: comment.unwrap_or_default(),
151152
kind: if format_checks_failed == 0 && tidy_checks_failed == 0 {
@@ -258,10 +259,9 @@ impl RestClient {
258259
let title = format!("{}:{}:{}", note.filename, note.line, note.cols);
259260
let annotation = FileAnnotation {
260261
severity: match note.severity.as_str() {
261-
"note" => AnnotationLevel::Notice,
262262
"warning" => AnnotationLevel::Warning,
263263
"error" => AnnotationLevel::Error,
264-
_ => AnnotationLevel::Notice, // default to notice if severity is unrecognized
264+
_ => AnnotationLevel::Notice, // default to notice for all else
265265
},
266266
path,
267267
start_line: None,
@@ -417,7 +417,7 @@ mod test {
417417
default::Default,
418418
env,
419419
io::Read,
420-
path::{Path, PathBuf},
420+
path::PathBuf,
421421
sync::{Arc, Mutex},
422422
};
423423

@@ -439,11 +439,7 @@ mod test {
439439

440440
// ************************* tests for step-summary and output variables
441441

442-
async fn create_comment(
443-
is_lgtm: bool,
444-
fail_gh_out: bool,
445-
fail_summary: bool,
446-
) -> (String, String) {
442+
async fn create_comment(is_lgtm: bool) -> (String, String) {
447443
let tmp_dir = tempdir().unwrap();
448444
unsafe {
449445
// ensure we are mimicking a CI platform
@@ -490,27 +486,14 @@ mod test {
490486
String::from("file")
491487
},
492488
step_summary: true,
489+
file_annotations: false,
493490
..Default::default()
494491
};
495492
let mut step_summary_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
496493
let mut gh_out_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
497494
unsafe {
498-
env::set_var(
499-
"GITHUB_STEP_SUMMARY",
500-
if fail_summary {
501-
Path::new("not-a-file.txt")
502-
} else {
503-
step_summary_path.path()
504-
},
505-
);
506-
env::set_var(
507-
"GITHUB_OUTPUT",
508-
if fail_gh_out {
509-
Path::new("not-a-file.txt")
510-
} else {
511-
gh_out_path.path()
512-
},
513-
);
495+
env::set_var("GITHUB_STEP_SUMMARY", step_summary_path.path());
496+
env::set_var("GITHUB_OUTPUT", gh_out_path.path());
514497
}
515498
let clang_versions = ClangVersions {
516499
format_version: Some(Version::new(1, 2, 3)),
@@ -524,20 +507,16 @@ mod test {
524507
step_summary_path
525508
.read_to_string(&mut step_summary_content)
526509
.unwrap();
527-
if !fail_summary {
528-
assert!(&step_summary_content.contains(USER_OUTREACH));
529-
}
510+
assert!(&step_summary_content.contains(USER_OUTREACH));
530511
let mut gh_out_content = String::new();
531512
gh_out_path.read_to_string(&mut gh_out_content).unwrap();
532-
if !fail_gh_out {
533-
assert!(gh_out_content.starts_with("checks-failed="));
534-
}
513+
assert!(gh_out_content.starts_with("checks-failed="));
535514
(step_summary_content, gh_out_content)
536515
}
537516

538517
#[tokio::test]
539518
async fn check_comment_concerns() {
540-
let (comment, gh_out) = create_comment(false, false, false).await;
519+
let (comment, gh_out) = create_comment(false).await;
541520
assert!(&comment.contains(":warning:\nSome files did not pass the configured checks!\n"));
542521
let fmt_pattern = Regex::new(r"format-checks-failed=(\d+)\n").unwrap();
543522
let tidy_pattern = Regex::new(r"tidy-checks-failed=(\d+)\n").unwrap();
@@ -559,7 +538,7 @@ mod test {
559538
unsafe {
560539
env::set_var("ACTIONS_STEP_DEBUG", "true");
561540
}
562-
let (comment, gh_out) = create_comment(true, false, false).await;
541+
let (comment, gh_out) = create_comment(true).await;
563542
assert!(comment.contains(":heavy_check_mark:\nNo problems need attention."));
564543
assert_eq!(
565544
gh_out,

0 commit comments

Comments
 (0)