Merge remote-tracking branch 'origin/verify-commit-data'

* origin/verify-commit-data:
  git-ext: check commit's metadata fields
  meta: update to tempfile 3.4

Signed-off-by: Fintan Halpenny <[email protected]>

Author: FintanH

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"

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"

radicle-git-ext/src/commit.rs

@@ -40,20 +40,22 @@ pub struct Commit {
 }
 
 impl Commit {
-    pub fn new<I, T>(
+    pub fn new<P, I, T>(
         tree: Oid,
-        parents: Vec<Oid>,
+        parents: P,
         author: Author,
         committer: Author,
         headers: Headers,
         message: String,
         trailers: I,
     ) -> Self
     where
+        P: IntoIterator<Item = Oid>,
         I: IntoIterator<Item = T>,
         OwnedTrailer: From<T>,
     {
         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<Oid, git2::Error> {
-        let odb = repo.odb()?;
-        odb.write(ObjectType::Commit, self.to_string().as_bytes())
+    pub fn write(&self, repo: &git2::Repository) -> Result<Oid, error::Write> {
+        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<Item = &OwnedTrailer> {
         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();
         }
 

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: "[email protected]".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::<OwnedTrailer>,
+    )
+    .write(&repo);
+    assert!(invalid.is_err());
+
+    let invalid = Commit::new(
+        blob,
+        vec![commit],
+        author.clone(),
+        author.clone(),
+        headers.clone(),
+        message.clone(),
+        None::<OwnedTrailer>,
+    )
+    .write(&repo);
+    assert!(invalid.is_err());
+
+    let valid = Commit::new(
+        tree,
+        vec![commit],
+        author.clone(),
+        author,
+        headers,
+        message,
+        None::<OwnedTrailer>,
+    )
+    .write(&repo);
+    assert!(valid.is_ok())
+}

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"