diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bd6784..2f0359e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`git2` requirement bumped 0.20.4 → 0.21.0** (cargo-dependencies group). + 0.21 ships two breaking changes that touched this crate: + 1. **`default` features are now empty** (0.20 enabled `ssh` + `https`). + `Cargo.toml` now requests `features = ["https", "ssh"]` explicitly — + the exact set 0.20.4 enabled by default — which also pulls in the new + `cred` feature that gates `Cred::credential_helper`. No change to the + credential or transport behaviour. + 2. **`TreeEntry::name()` and `Commit::message()` now return + `Result<&str, Error>`** (UTF-8 validation) instead of `Option<&str>`. + The three `let Some(name) = entry.name()` walk callbacks in + `streaming/tar.rs` and `git2_ops/submodule.rs` became + `let Ok(name) = …`, preserving the "skip non-UTF-8 names" behaviour; + one `push.rs` test assertion moved from `Some(..)` to `Ok(..)`. + Also replaced the now-deprecated `Oid::zero()` with the `Oid::ZERO_SHA1` + constant in three `submodule.rs` tests. No production behaviour change. + Three new regression tests close the unit-coverage gap on the two + migrated submodule walk callbacks (previously only exercised by the + Python integration tests, which don't run in the PR coverage job): + `find_submodule_entries_detects_gitlink_in_tree` and + `find_submodule_entries_gitlink_missing_from_gitmodules_is_skipped` + build a tree with a real gitlink (mode 160000) entry, and + `write_submodules_to_tar_writes_submodule_files` drives + `write_submodules_to_tar` against a locally-created bare repo via a new + test-only `FetchResult::from_parts_for_test` constructor (no network). - **`download_with_retry` and `download_with_progress` collapsed into a single `download_chunked` helper.** The two had drifted on observability (the progress variant was missing `url` in its `warn!` calls) and the diff --git a/Cargo.toml b/Cargo.toml index a5ed13e..44af6c4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,8 +50,11 @@ glob = "0.3" # Platform-specific directories dirs = "6.0.0" -# git2 library for in-process git operations (Tier 1 credential relay) -git2 = "0.20.4" +# git2 library for in-process git operations (Tier 1 credential relay). +# 0.21 changed `default` features to empty (0.20 enabled "ssh" + "https"), +# so they must now be requested explicitly. "https" + "ssh" transitively +# enable the new "cred" feature that gates `Cred::credential_helper`. +git2 = { version = "0.21.0", features = ["https", "ssh"] } # Compression for tar.gz archives flate2 = "1.0" diff --git a/src/git2_ops/clone.rs b/src/git2_ops/clone.rs index 13688e5..747b540 100644 --- a/src/git2_ops/clone.rs +++ b/src/git2_ops/clone.rs @@ -40,6 +40,28 @@ pub struct FetchResult { _temp_dir: TempDir, } +#[cfg(test)] +impl FetchResult { + /// Construct a `FetchResult` from already-prepared parts. Test-only: lets + /// unit tests in other modules (e.g. `streaming::tar`) build a fetched-repo + /// handle around a locally-created bare repo without a real network fetch. + /// The private `_temp_dir` field keeps the temp directory alive for the + /// lifetime of the returned value, exactly as the production path does. + pub(crate) fn from_parts_for_test( + repo: Repository, + head_commit: Oid, + branch: String, + temp_dir: TempDir, + ) -> Self { + Self { + repo, + head_commit, + branch, + _temp_dir: temp_dir, + } + } +} + /// Options for fetch operations. #[derive(Debug, Clone, Default)] pub struct FetchOptions2 { diff --git a/src/git2_ops/push.rs b/src/git2_ops/push.rs index ec53579..21061ef 100644 --- a/src/git2_ops/push.rs +++ b/src/git2_ops/push.rs @@ -534,7 +534,8 @@ mod tests { .find_reference("refs/heads/main") .expect("refs/heads/main must exist after unbundle"); let commit = reference.peel_to_commit().unwrap(); - assert_eq!(commit.message(), Some("seed commit")); + // git2 0.21: `Commit::message()` returns `Result` (UTF-8 check). + assert_eq!(commit.message(), Ok("seed commit")); } #[test] diff --git a/src/git2_ops/submodule.rs b/src/git2_ops/submodule.rs index 4e39e7c..241b053 100644 --- a/src/git2_ops/submodule.rs +++ b/src/git2_ops/submodule.rs @@ -260,7 +260,9 @@ pub fn find_submodule_entries( tree.walk(TreeWalkMode::PreOrder, |dir, entry| { // Check if this is a submodule entry (mode 160000) if entry.filemode() == SUBMODULE_MODE && entry.kind() == Some(ObjectType::Commit) { - let Some(name) = entry.name() else { + // git2 0.21: `TreeEntry::name()` returns `Result` (UTF-8 check); + // `Err` means a non-UTF-8 name, which we skip as before. + let Ok(name) = entry.name() else { return TreeWalkResult::Ok; }; @@ -903,6 +905,85 @@ mod tests { assert!(entries.is_empty()); } + #[test] + fn find_submodule_entries_detects_gitlink_in_tree() { + // A tree containing a real gitlink (mode 160000) exercises the + // `let Ok(name) = entry.name()` walk-callback branch that the + // no-submodule test above never reaches. + let temp = tempfile::TempDir::new().unwrap(); + let repo = Repository::init_bare(temp.path()).unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + + // A commit to serve as the gitlink target oid (lives in this same repo + // for the test; `find_submodule_entries` only reads the tree entry's + // id/name, never peels the target). + let blob = repo.blob(b"sub\n").unwrap(); + let mut sub_tb = repo.treebuilder(None).unwrap(); + sub_tb.insert("f.txt", blob, 0o100_644).unwrap(); + let sub_tree = repo.find_tree(sub_tb.write().unwrap()).unwrap(); + let gitlink_target = repo + .commit(None, &sig, &sig, "submodule commit", &sub_tree, &[]) + .unwrap(); + + // Parent commit whose tree has a gitlink at the root path "sub". + let mut root_tb = repo.treebuilder(None).unwrap(); + root_tb + .insert("sub", gitlink_target, SUBMODULE_MODE) + .unwrap(); + let root_tree = repo.find_tree(root_tb.write().unwrap()).unwrap(); + let commit_oid = repo + .commit(None, &sig, &sig, "root commit", &root_tree, &[]) + .unwrap(); + + let mut gitmodules = HashMap::new(); + gitmodules.insert( + "sub".to_string(), + SubmoduleInfo { + name: "sub".to_string(), + path: "sub".to_string(), + url: "https://example.com/sub.git".to_string(), + branch: None, + }, + ); + + let entries = find_submodule_entries(&repo, commit_oid, &gitmodules).unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].path, "sub"); + assert_eq!(entries[0].commit, gitlink_target); + assert_eq!(entries[0].url, "https://example.com/sub.git"); + } + + #[test] + fn find_submodule_entries_gitlink_missing_from_gitmodules_is_skipped() { + // Same gitlink tree, but with an empty .gitmodules map: the entry is + // found in the tree (hitting the same walk branch) yet produces no + // `SubmoduleEntry` because there's no URL to fetch from. + let temp = tempfile::TempDir::new().unwrap(); + let repo = Repository::init_bare(temp.path()).unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + + let blob = repo.blob(b"sub\n").unwrap(); + let mut sub_tb = repo.treebuilder(None).unwrap(); + sub_tb.insert("f.txt", blob, 0o100_644).unwrap(); + let sub_tree = repo.find_tree(sub_tb.write().unwrap()).unwrap(); + let gitlink_target = repo + .commit(None, &sig, &sig, "submodule commit", &sub_tree, &[]) + .unwrap(); + + let mut root_tb = repo.treebuilder(None).unwrap(); + root_tb + .insert("sub", gitlink_target, SUBMODULE_MODE) + .unwrap(); + let root_tree = repo.find_tree(root_tb.write().unwrap()).unwrap(); + let commit_oid = repo + .commit(None, &sig, &sig, "root commit", &root_tree, &[]) + .unwrap(); + + let gitmodules = HashMap::new(); + let entries = find_submodule_entries(&repo, commit_oid, &gitmodules).unwrap(); + assert!(entries.is_empty()); + } + #[test] fn find_submodule_entries_invalid_commit_returns_error() { let temp = tempfile::TempDir::new().unwrap(); @@ -917,7 +998,7 @@ mod tests { fn submodule_entry_struct_fields() { let entry = SubmoduleEntry { path: "vendor/x".to_string(), - commit: Oid::zero(), + commit: Oid::ZERO_SHA1, url: "https://example.com/x.git".to_string(), }; assert_eq!(entry.path, "vendor/x"); @@ -951,7 +1032,7 @@ mod tests { fn fetch_submodule_with_invalid_url_fails() { let entry = SubmoduleEntry { path: "vendor/x".to_string(), - commit: Oid::zero(), + commit: Oid::ZERO_SHA1, url: "not-a-valid-url".to_string(), }; let result = fetch_submodule(&entry, None); @@ -962,7 +1043,7 @@ mod tests { fn fetch_submodule_with_file_url_rejected() { let entry = SubmoduleEntry { path: "vendor/x".to_string(), - commit: Oid::zero(), + commit: Oid::ZERO_SHA1, url: "file:///etc/passwd".to_string(), }; assert!(fetch_submodule(&entry, None).is_err()); diff --git a/src/streaming/tar.rs b/src/streaming/tar.rs index 6c0e75a..7707c75 100644 --- a/src/streaming/tar.rs +++ b/src/streaming/tar.rs @@ -342,7 +342,9 @@ pub fn create_tar_from_tree_with_options( // Walk the tree and add each blob to the tar tree.walk(TreeWalkMode::PreOrder, |dir, entry| { - let Some(name) = entry.name() else { + // git2 0.21: `TreeEntry::name()` returns `Result` (UTF-8 check); + // `Err` means a non-UTF-8 name, which we skip as before. + let Ok(name) = entry.name() else { return TreeWalkResult::Skip; }; @@ -623,7 +625,9 @@ fn write_submodules_to_tar( let mut submodule_files = 0usize; let walk_result = submodule_tree.walk(TreeWalkMode::PreOrder, |dir, entry| { - let Some(name) = entry.name() else { + // git2 0.21: `TreeEntry::name()` returns `Result` (UTF-8 check); + // `Err` means a non-UTF-8 name, which we skip as before. + let Ok(name) = entry.name() else { return TreeWalkResult::Skip; }; @@ -754,6 +758,86 @@ mod tests { assert_eq!(encoded, "aGVsbG8gd29ybGQ="); } + #[test] + fn write_submodules_to_tar_writes_submodule_files() { + // Exercises `write_submodules_to_tar` (including its + // `let Ok(name) = entry.name()` walk branch) without a network fetch, + // by building a `FetchedSubmodule` around a locally-created bare repo. + use crate::git2_ops::clone::FetchResult; + use crate::git2_ops::submodule::SubmoduleEntry; + + let temp = tempfile::TempDir::new().unwrap(); + let commit_oid = { + let repo = Repository::init_bare(temp.path()).unwrap(); + let blob = repo.blob(b"submodule file contents\n").unwrap(); + let mut tb = repo.treebuilder(None).unwrap(); + tb.insert("README.md", blob, 0o100_644).unwrap(); + let tree = repo.find_tree(tb.write().unwrap()).unwrap(); + let sig = git2::Signature::now("Test", "test@example.com").unwrap(); + repo.commit(None, &sig, &sig, "submodule commit", &tree, &[]) + .unwrap() + }; + let repo = Repository::open_bare(temp.path()).unwrap(); + + let fetched = FetchedSubmodule { + entry: SubmoduleEntry { + path: "vendor/sub".to_string(), + commit: commit_oid, + url: "https://example.com/sub.git".to_string(), + }, + fetch_result: FetchResult::from_parts_for_test( + repo, + commit_oid, + "main".to_string(), + temp, + ), + children: Vec::new(), + }; + + let mut archive = Vec::new(); + { + let encoder = GzEncoder::new(&mut archive, Compression::fast()); + let mut tar_builder = tar::Builder::new(encoder); + + let mut file_count = 0usize; + let mut uncompressed_size = 0u64; + let mut skipped_by_filter = 0usize; + let mut skipped_binary = 0usize; + let mut skipped_too_large = 0usize; + let mut skipped_path_too_long = 0usize; + let mut submodules_included = 0usize; + let mut submodules_failed = 0usize; + + write_submodules_to_tar( + std::slice::from_ref(&fetched), + "", + None, + false, + None, + &mut tar_builder, + &mut file_count, + &mut uncompressed_size, + &mut skipped_by_filter, + &mut skipped_binary, + &mut skipped_too_large, + &mut skipped_path_too_long, + &mut submodules_included, + &mut submodules_failed, + None, + ); + + tar_builder.finish().unwrap(); + + assert_eq!(file_count, 1, "the submodule's single file should be added"); + assert_eq!(submodules_included, 1); + assert_eq!(submodules_failed, 0); + assert!(uncompressed_size > 0); + } + + // The archive should contain the submodule file under its path prefix. + assert!(!archive.is_empty()); + } + #[test] fn sparse_filter_empty_matches_all() { let filter = SparseFilter::new(&[]);