diff --git a/link-git/Cargo.toml b/link-git/Cargo.toml index a564ab9..977d172 100644 --- a/link-git/Cargo.toml +++ b/link-git/Cargo.toml @@ -26,7 +26,7 @@ parking_lot = "0.12" pin-project = "1.0.7" regex = "1.5.4" rustc-hash = "1.1.0" -tempfile = "3.3" +tempfile = "3.4" thiserror = "1.0.30" tracing = "0.1" versions = "3.0.2" diff --git a/link-git/t/Cargo.toml b/link-git/t/Cargo.toml index 313f882..aa342d2 100644 --- a/link-git/t/Cargo.toml +++ b/link-git/t/Cargo.toml @@ -19,7 +19,7 @@ anyhow = "1" bstr = "0.2" futures = "0.3" futures_ringbuf = "0.3" -tempfile = "3.3" +tempfile = "3.4" [dev-dependencies.git2] version = "0.16.1" diff --git a/radicle-git-ext/src/commit.rs b/radicle-git-ext/src/commit.rs index 1668153..dbd51c9 100644 --- a/radicle-git-ext/src/commit.rs +++ b/radicle-git-ext/src/commit.rs @@ -40,9 +40,9 @@ pub struct Commit { } impl Commit { - pub fn new( + pub fn new( tree: Oid, - parents: Vec, + parents: P, author: Author, committer: Author, headers: Headers, @@ -50,10 +50,12 @@ impl Commit { trailers: I, ) -> Self where + P: IntoIterator, I: IntoIterator, OwnedTrailer: From, { let trailers = trailers.into_iter().map(OwnedTrailer::from).collect(); + let parents = parents.into_iter().collect(); Self { tree, parents, @@ -75,9 +77,10 @@ impl Commit { /// Write the given [`Commit`] to the `repo`. The resulting `Oid` /// is the identifier for this commit. - pub fn write(&self, repo: &git2::Repository) -> Result { - let odb = repo.odb()?; - odb.write(ObjectType::Commit, self.to_string().as_bytes()) + pub fn write(&self, repo: &git2::Repository) -> Result { + let odb = repo.odb().map_err(error::Write::Odb)?; + self.verify_for_write(&odb)?; + Ok(odb.write(ObjectType::Commit, self.to_string().as_bytes())?) } /// The tree [`Oid`] this commit points to. @@ -132,6 +135,35 @@ impl Commit { pub fn trailers(&self) -> impl Iterator { self.trailers.iter() } + + fn verify_for_write(&self, odb: &git2::Odb) -> Result<(), error::Write> { + for parent in &self.parents { + verify_object(odb, parent, ObjectType::Commit)?; + } + verify_object(odb, &self.tree, ObjectType::Tree)?; + + Ok(()) + } +} + +fn verify_object(odb: &git2::Odb, oid: &Oid, expected: ObjectType) -> Result<(), error::Write> { + use git2::{Error, ErrorClass, ErrorCode}; + + let (_, _, kind) = odb + .reader(*oid) + .map_err(|err| error::Write::OdbRead { oid: *oid, err })?; + if kind != expected { + Err(error::Write::NotCommit { + oid: *oid, + err: Error::new( + ErrorCode::NotFound, + ErrorClass::Object, + format!("Object '{oid}' is not expected object type {expected}"), + ), + }) + } else { + Ok(()) + } } pub mod error { @@ -141,6 +173,26 @@ pub mod error { use crate::author; + #[derive(Debug, Error)] + pub enum Write { + #[error(transparent)] + Git(#[from] git2::Error), + #[error("the parent '{oid}' provided is not a commit object")] + NotCommit { + oid: git2::Oid, + #[source] + err: git2::Error, + }, + #[error("failed to access git odb")] + Odb(#[source] git2::Error), + #[error("failed to read '{oid}' from git odb")] + OdbRead { + oid: git2::Oid, + #[source] + err: git2::Error, + }, + } + #[derive(Debug, Error)] pub enum Read { #[error(transparent)] @@ -269,7 +321,7 @@ impl ToString for Commit { writeln!(buf, "tree {}", self.tree).ok(); - for parent in &self.parents { + for parent in self.parents() { writeln!(buf, "parent {parent}").ok(); } diff --git a/radicle-git-ext/t/src/commit.rs b/radicle-git-ext/t/src/commit.rs index 73de458..f6719ca 100644 --- a/radicle-git-ext/t/src/commit.rs +++ b/radicle-git-ext/t/src/commit.rs @@ -1,6 +1,10 @@ -use std::str::FromStr as _; +use std::{io, str::FromStr as _}; -use radicle_git_ext::commit::Commit; +use radicle_git_ext::{ + author::{self, Author}, + commit::{headers::Headers, trailers::OwnedTrailer, Commit}, +}; +use test_helpers::tempdir::WithTmpDir; const NO_TRAILER: &str = "\ tree 50d6ef440728217febf9e35716d8b0296608d7f8 @@ -148,9 +152,6 @@ fn test_conversion() { assert_eq!(Commit::from_str(UNSIGNED).unwrap().to_string(), UNSIGNED); } -use std::io; -use test_helpers::tempdir::WithTmpDir; - #[test] fn valid_commits() { let radicle_git = format!( @@ -174,3 +175,70 @@ fn valid_commits() { assert!(commit.is_ok(), "Oid: {oid}, Error: {commit:?}") } } + +#[test] +fn write_valid_commit() { + let repo = WithTmpDir::new(|path| { + git2::Repository::init(path).map_err(|err| io::Error::new(io::ErrorKind::Other, err)) + }) + .unwrap(); + let author = Author { + name: "Terry".to_owned(), + email: "terry.pratchett@proton.mail".to_owned(), + time: author::Time::new(0, 0), + }; + let blob = repo.blob(b"The Colour of Magic").unwrap(); + let mut tb = repo.treebuilder(None).unwrap(); + tb.insert("The Colour of Magic", blob, git2::FileMode::Blob.into()) + .unwrap(); + let tree = tb.write().unwrap(); + let commit = repo + .commit( + None, + &git2::Signature::try_from(&author).unwrap(), + &git2::Signature::try_from(&author).unwrap(), + "New beginnings", + &repo.find_tree(tree).unwrap(), + &[], + ) + .unwrap(); + + let headers = Headers::new(); + let message = "Write Discworld".to_owned(); + + let invalid = Commit::new( + tree, + vec![blob], + author.clone(), + author.clone(), + headers.clone(), + message.clone(), + None::, + ) + .write(&repo); + assert!(invalid.is_err()); + + let invalid = Commit::new( + blob, + vec![commit], + author.clone(), + author.clone(), + headers.clone(), + message.clone(), + None::, + ) + .write(&repo); + assert!(invalid.is_err()); + + let valid = Commit::new( + tree, + vec![commit], + author.clone(), + author, + headers, + message, + None::, + ) + .write(&repo); + assert!(valid.is_ok()) +} diff --git a/test/test-helpers/Cargo.toml b/test/test-helpers/Cargo.toml index 9605f7a..27a7e4d 100644 --- a/test/test-helpers/Cargo.toml +++ b/test/test-helpers/Cargo.toml @@ -17,7 +17,7 @@ log = ">= 0.4" pretty_assertions = "1.1" serde = "1" serde_json = "1" -tempfile = "3.3" +tempfile = "3.4" tracing = "0.1" proptest = "1"