Skip to content

Commit 460d9b4

Browse files
2bndy52bndy5
committed
review git module
remove unused functions transition to git CLI in tests This is a small step toward removing our dependence on libgit2
1 parent 397ca6d commit 460d9b4

3 files changed

Lines changed: 56 additions & 160 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎cpp-linter/Cargo.toml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ clap = { workspace = true, optional = true }
2222
colored = { workspace = true, optional = true }
2323
fast-glob = "1.0.1"
2424
futures = "0.3.32"
25-
git-bot-feedback = { version = "0.5.3", features = ["file-changes"] }
2625
git2 = { version = "0.21.0", features = ["https"] }
2726
log = { workspace = true }
2827
quick-xml = { version = "0.40.1", features = ["serialize"] }
@@ -34,6 +33,13 @@ serde_json = { workspace = true }
3433
thiserror = { workspace = true }
3534
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
3635

36+
[dependencies.git-bot-feedback]
37+
# version = "0.5.4"
38+
# path = "../../git-bot-feedback"
39+
git = "https://github.com/2bndy5/git-bot-feedback"
40+
branch = "git-diff-no-staged"
41+
features = ["file-changes"]
42+
3743
[dev-dependencies]
3844
mockito = { workspace = true }
3945
tempfile = { workspace = true }

‎cpp-linter/src/git.rs‎

Lines changed: 48 additions & 157 deletions
Original file line numberDiff line numberDiff line change
@@ -8,129 +8,16 @@
88
//! (str or bytes) only happens in CI or when libgit2 cannot be used to initialize a
99
//! repository.
1010
11-
use std::{fmt::Display, ops::RangeInclusive, path::PathBuf};
11+
use std::{ops::RangeInclusive, path::PathBuf};
1212

13-
use anyhow::{Context, Result};
1413
// non-std crates
15-
use git2::{Diff, Error, Patch, Repository};
14+
use anyhow::Result;
15+
use git2::{Diff, Patch};
1616

1717
// project specific modules/crates
1818
use crate::{cli::LinesChangedOnly, common_fs::FileObj};
1919
use git_bot_feedback::{FileFilter, error::DiffError};
2020

21-
/// This (re-)initializes the repository located in the specified `path`.
22-
///
23-
/// This is actually not used in CI for file permissions and ownership reasons.
24-
/// Rather this is only (supposed to be) used when executed on a local developer
25-
/// machine.
26-
pub fn open_repo(path: &str) -> Result<Repository, Error> {
27-
Repository::open(PathBuf::from(path).as_path())
28-
}
29-
30-
/// Fetches the SHA1 of the commit for the specified [`git2::Repository`].
31-
///
32-
/// The optionally specified `depth` can be used to traverse the tree a number of times
33-
/// since the current `"HEAD"`.
34-
fn get_sha<'d, T: Display>(
35-
repo: &'d Repository,
36-
depth: &Option<T>,
37-
) -> Result<git2::Object<'d>, Error> {
38-
match depth {
39-
Some(base) => {
40-
let base = base.to_string();
41-
// First treat base as an explicit refs/SHAs. If that fails, then
42-
// fall back to `HEAD~<base>` if `base` is purely numeric.
43-
match repo.revparse_single(base.as_str()) {
44-
Ok(obj) => Ok(obj),
45-
Err(err) => {
46-
if base.chars().all(|c| c.is_ascii_digit()) {
47-
repo.revparse_single(format!("HEAD~{}", base).as_str())
48-
} else {
49-
Err(err)
50-
}
51-
}
52-
}
53-
}
54-
None => repo.revparse_single("HEAD"),
55-
}
56-
}
57-
58-
/// Fetch the [`git2::Diff`] about a given [`git2::Repository`].
59-
///
60-
/// This is actually not used in CI for file permissions and ownership reasons.
61-
/// Rather, this is only (supposed to be) used when executed on a local developer
62-
/// machine.
63-
///
64-
/// ## Using `diff_base` and `ignore_index`
65-
///
66-
/// The `diff_base` is a commit or ref to use as the base of the diff.
67-
/// Use `ignore_index` to exclude any staged changes in the local index.
68-
///
69-
/// | `diff_base` value | Git index state | Scope of diff |
70-
/// |-------------------|-----------------|---------------|
71-
/// | `None` | No staged changes | `HEAD~1..HEAD` |
72-
/// | `None` | Has staged changes | `HEAD..index` |
73-
/// | `Some(2)` or `Some("HEAD~2")` | No staged changes | `HEAD~2..HEAD` |
74-
/// | `Some(2)` or `Some("HEAD~2")` | Has staged changes | `HEAD~2..index` |
75-
pub fn get_diff<'d, T: Display>(
76-
repo: &'d Repository,
77-
diff_base: &Option<T>,
78-
ignore_index: bool,
79-
) -> Result<git2::Diff<'d>> {
80-
let use_staged_files = if ignore_index {
81-
false
82-
} else {
83-
// check if there are staged file changes
84-
repo.statuses(None)
85-
.with_context(|| "Could not get repo statuses")?
86-
.iter()
87-
.any(|entry| {
88-
entry.status().bits()
89-
& (git2::Status::INDEX_NEW.bits()
90-
| git2::Status::INDEX_MODIFIED.bits()
91-
| git2::Status::INDEX_RENAMED.bits())
92-
> 0
93-
})
94-
};
95-
let base = if diff_base.is_some() {
96-
// diff base is specified (regardless of staged changes)
97-
get_sha(repo, diff_base)
98-
} else if !use_staged_files {
99-
// diff base is unspecified, when the repo has
100-
// no staged changes (and they are not ignored),
101-
// then focus on just the last commit
102-
get_sha(repo, &Some(1))
103-
} else {
104-
// diff base is unspecified and there are staged changes, so
105-
// let base be set to HEAD.
106-
get_sha(repo, &None::<u8>)
107-
}?
108-
.peel_to_tree()?;
109-
110-
// RARE BUG when `head` is the first commit in the repo! Affects local-only runs.
111-
// > Error { code: -3, class: 3, message: "parent 0 does not exist" }
112-
if use_staged_files {
113-
// get diff including staged files
114-
repo.diff_tree_to_index(Some(&base), None, None)
115-
.with_context(|| {
116-
format!(
117-
"Could not get diff for {}..index",
118-
&base.id().to_string()[..7]
119-
)
120-
})
121-
} else {
122-
// get diff for range of commits between base..HEAD
123-
let head = get_sha(repo, &None::<u8>)?.peel_to_tree()?;
124-
repo.diff_tree_to_tree(Some(&base), Some(&head), None)
125-
.with_context(|| {
126-
format!(
127-
"Could not get diff for {}..HEAD",
128-
&base.id().to_string()[..7]
129-
)
130-
})
131-
}
132-
}
133-
13421
/// Parses a patch for a single file in a diff.
13522
///
13623
/// Returns the list of line numbers that have additions and the ranges spanning each
@@ -227,40 +114,56 @@ pub fn parse_diff_from_buf(
227114
mod test {
228115
use std::{
229116
env::{self, current_dir, set_current_dir},
230-
fs::read,
117+
fs,
118+
process::Command,
231119
};
232120

233-
use git2::{ApplyLocation, Diff, IndexAddOption, Repository, build::CheckoutBuilder};
234121
use tempfile::{TempDir, tempdir};
235122

236-
use super::get_sha;
237123
use crate::{cli::LinesChangedOnly, rest_client::RestClient};
238124
use git_bot_feedback::FileFilter;
239125

240126
const TEST_REPO_URL: &str = "https://github.com/cpp-linter/cpp-linter";
241127

242128
// used to setup a testing stage
243-
fn clone_repo(sha: Option<&str>, path: &str, patch_path: Option<&str>) -> Repository {
244-
let repo = Repository::clone(TEST_REPO_URL, path).unwrap();
129+
fn clone_repo(sha: Option<&str>, path: &str, patch_path: Option<&str>) {
130+
// let repo = Repository::clone(TEST_REPO_URL, path).unwrap();
131+
let ok = Command::new("git")
132+
.args(["clone", TEST_REPO_URL, path])
133+
.status()
134+
.expect("Failed to clone repo");
135+
if !ok.success() {
136+
panic!("Failed to clone repo");
137+
}
245138
if let Some(sha) = sha {
246-
let commit = repo.revparse_single(sha).unwrap();
247-
repo.checkout_tree(
248-
&commit,
249-
Some(CheckoutBuilder::new().force().recreate_missing(true)),
250-
)
251-
.unwrap();
252-
repo.set_head_detached(commit.id()).unwrap();
139+
let ok = Command::new("git")
140+
.args(["-c", "advice.detachedHead=false", "checkout", sha])
141+
.current_dir(path)
142+
.status()
143+
.expect("Failed to checkout commit");
144+
if !ok.success() {
145+
panic!("Failed to checkout commit");
146+
}
253147
}
254148
if let Some(patch) = patch_path {
255-
let diff = Diff::from_buffer(&read(patch).unwrap()).unwrap();
256-
repo.apply(&diff, ApplyLocation::Both, None).unwrap();
257-
let mut index = repo.index().unwrap();
258-
index
259-
.add_all(["tests/demo/demo.*"], IndexAddOption::DEFAULT, None)
260-
.unwrap();
261-
index.write().unwrap();
149+
let canonical_path_path = fs::canonicalize(patch).unwrap();
150+
let ok = Command::new("git")
151+
.args(["apply", "--index", canonical_path_path.to_str().unwrap()])
152+
.current_dir(path)
153+
.status()
154+
.expect("Failed to apply patch and stage its changes");
155+
if !ok.success() {
156+
panic!("Failed to apply patch and stage its changes");
157+
}
158+
let ok = Command::new("git")
159+
.args(["status", "-s"])
160+
.current_dir(path)
161+
.status()
162+
.expect("Failed to get git status after applying patch");
163+
if !ok.success() {
164+
panic!("Failed to get git status after applying patch");
165+
}
262166
}
263-
repo
264167
}
265168

266169
fn get_temp_dir() -> TempDir {
@@ -290,7 +193,7 @@ mod test {
290193
let file_filter = FileFilter::new(&["target"], extensions, None);
291194
set_current_dir(tmp).unwrap();
292195
let base_diff = if ignore_staged {
293-
Some("0".to_string())
196+
Some("HEAD".to_string())
294197
} else {
295198
None
296199
};
@@ -395,30 +298,18 @@ mod test {
395298
true,
396299
)
397300
.await;
398-
println!("files = {:?}", files);
399-
let git_status = std::process::Command::new("git")
400-
.args(["diff", "HEAD~1..HEAD"])
401-
.output()
402-
.map(|o| String::from_utf8(o.stdout).unwrap())
301+
Command::new("git")
302+
.args([
303+
"--no-pager",
304+
"show",
305+
"a2875ac00e6f1dc3eb4ac19712c7a241b5a76e83",
306+
"--format=%b",
307+
])
308+
.status()
403309
.unwrap();
404-
eprintln!("git status:\n{git_status}");
405310
eprintln!("files: {files:?}");
406311
assert!(files.is_empty());
407312
set_current_dir(cur_dir).unwrap(); // prep to delete temp_folder
408313
drop(tmp); // delete temp_folder
409314
}
410-
411-
#[test]
412-
fn repo_get_sha() {
413-
let tmp_dir = get_temp_dir();
414-
let repo = clone_repo(None, tmp_dir.path().to_str().unwrap(), None);
415-
for (ours, theirs) in [(None::<u8>, "HEAD"), (Some(2), "HEAD~2")] {
416-
let our_obj = get_sha(&repo, &ours).unwrap();
417-
let their_obj = get_sha(&repo, &Some(theirs)).unwrap();
418-
assert_eq!(our_obj.id(), their_obj.id());
419-
}
420-
// test an invalid ref for coverage measurement
421-
assert!(get_sha(&repo, &Some("1.0")).is_err());
422-
drop(tmp_dir); // delete temp_folder
423-
}
424315
}

0 commit comments

Comments
 (0)