Merge pull request #12 from ekala-project/publish-cleanups

fix: atoms must derive from initialized history

crates/atom/src/publish/error.rs

@@ -1,6 +1,7 @@
 //! # Publishing Errors
 //!
 //! This module contains the error types for errors that might occur during publishing.
+use crate::store::git::Root;
 use gix::object;
 use std::path::PathBuf;
 use thiserror::Error;
@@ -49,6 +50,14 @@ pub enum GitError {
     /// A transparent wrapper for a [`tokio::task::JoinError`]
     #[error(transparent)]
     JoinFailed(#[from] tokio::task::JoinError),
+    /// The reported root & the atom root are inconsistent.
+    #[error("Atom does not derive from the initialized history")]
+    InconsistentRoot {
+        /// The root according to the remote we are publishing to.
+        remote: Root,
+        /// The root of history for the source from which the atom is derived.
+        atom: Root,
+    },
     /// The remote is not initialized as an Ekala store.
     #[error("Remote is not initialized")]
     NotInitialized,
@@ -76,9 +85,20 @@ pub enum GitError {
 }
 
 impl GitError {
+    const INCONSISTENT_ROOT_SUGGESTION: &str =
+        "You may need to reinitalize the remote if the issue persists";
+
     /// Warn the user about specific error conditions encountered during publishing.
     pub fn warn(&self) {
         match self {
+            GitError::InconsistentRoot { remote, atom } => {
+                tracing::warn!(
+                    message = %self,
+                    atom_root = %**atom,
+                    remote_root = %**remote,
+                    suggest = GitError::INCONSISTENT_ROOT_SUGGESTION
+                )
+            }
             GitError::Invalid(e, path) => {
                 tracing::warn!(message = %self, path = %path.display(), message = format!("\n{}", e))
             }

crates/atom/src/publish/git/inner.rs

@@ -82,6 +82,12 @@ impl<'a> GitContext<'a> {
         self.verify_manifest(&entry.object()?, path)
             .and_then(|spec| {
                 let id = AtomId::compute(&self.commit, spec.id.clone())?;
+                if self.root != *id.root() {
+                    return Err(GitError::InconsistentRoot {
+                        remote: self.root,
+                        atom: *id.root(),
+                    });
+                };
                 let entries = match (lock, dir) {
                     (None, None) => smallvec![entry],
                     (None, Some(dir)) => smallvec![entry, dir],

crates/atom/src/publish/git/mod.rs

@@ -45,6 +45,8 @@ pub struct GitContext<'a> {
     commit: Commit<'a>,
     /// Store the given remote name as a &str for convenient use.
     remote_str: &'a str,
+    /// The reported root commit according to the remote.
+    root: Root,
     /// a JoinSet of push tasks to avoid blocking on them.
     push_tasks: RefCell<JoinSet<Result<Vec<u8>, GitError>>>,
     /// Path buf for efficient tree searches
@@ -124,19 +126,31 @@ use super::{Builder, ValidAtoms};
 
 /// The type representing a Git specific Atom publisher.
 pub struct GitPublisher<'a> {
-    source: &'a Repository,
+    repo: &'a Repository,
     remote: &'a str,
     spec: &'a str,
+    root: Root,
 }
 
 impl<'a> GitPublisher<'a> {
     /// Constructs a new [`GitPublisher`].
-    pub fn new(source: &'a Repository, remote: &'a str, spec: &'a str) -> Self {
-        GitPublisher {
-            source,
+    pub fn new(repo: &'a Repository, remote: &'a str, spec: &'a str) -> GitResult<Self> {
+        use crate::store::Init;
+        let root = repo
+            .find_remote(remote)
+            .map_err(Box::new)?
+            .ekala_root()
+            .map_err(|e| {
+                e.warn();
+                GitError::NotInitialized
+            })?;
+
+        Ok(GitPublisher {
+            repo,
             remote,
             spec,
-        }
+            root,
+        })
     }
 }
 
@@ -202,7 +216,7 @@ impl<'a> Builder<'a, Root> for GitPublisher<'a> {
     type Publisher = GitContext<'a>;
 
     fn build(&self) -> Result<(ValidAtoms, Self::Publisher), Self::Error> {
-        let publisher = GitContext::set(self.source, self.remote, self.spec)?;
+        let publisher = GitContext::set(self.repo, self.remote, self.spec, self.root)?;
         let atoms = GitPublisher::validate(&publisher)?;
         Ok((atoms, publisher))
     }
@@ -325,7 +339,12 @@ impl<'a> AtomContext<'a> {
 }
 
 impl<'a> GitContext<'a> {
-    fn set(repo: &'a Repository, remote_str: &'a str, refspec: &str) -> GitResult<Self> {
+    fn set(
+        repo: &'a Repository,
+        remote_str: &'a str,
+        refspec: &str,
+        root: Root,
+    ) -> GitResult<Self> {
         // short-circuit publishing if the passed remote doesn't exist
         let _remote = repo.find_remote(remote_str).map_err(Box::new)?;
         let commit = repo
@@ -339,6 +358,7 @@ impl<'a> GitContext<'a> {
 
         Ok(Self {
             repo,
+            root,
             tree,
             commit,
             remote_str,

crates/atom/src/publish/git/test.rs

@@ -90,7 +90,7 @@ async fn publish_atom() -> Result<(), anyhow::Error> {
     let id = "foo";
     let (file_path, src) = repo.mock(id, "0.1.0", "some atom")?;
 
-    let (paths, publisher) = GitPublisher::new(&repo, "origin", "HEAD").build()?;
+    let (paths, publisher) = GitPublisher::new(&repo, "origin", "HEAD")?.build()?;
 
     let path = paths.get(&Id::try_from(id)?).context("path is messed up")?;
     let result = publisher.publish_atom(path)?;

crates/atom/src/publish/mod.rs

@@ -65,7 +65,7 @@ pub trait Builder<'a, R> {
 trait StateValidator<R> {
     type Error;
     type Publisher: Publish<R>;
-    /// Validate the state of the Atom source before.
+    /// Validate the state of the Atom source.
     ///
     /// This function is called during construction to ensure that we
     /// never allow for an inconsistent state in the final Ekala store.

crates/atom/src/store.rs

@@ -5,15 +5,15 @@ use std::path::{Path, PathBuf};
 use bstr::BStr;
 
 /// A trait representing the methods required to initialize an Ekala store.
-pub trait Init<R> {
+pub trait Init<R, O> {
     /// The error type returned by the methods of this trait.
     type Error;
     /// Sync with the Ekala store, for implementations that require it.
-    fn sync(&self) -> Result<R, Self::Error>;
+    fn sync(&self) -> Result<O, Self::Error>;
     /// Initialize the Ekala store.
     fn ekala_init(&self) -> Result<(), Self::Error>;
-    /// Returns a [`bool`] signifying whether the store has already been initialized.
-    fn is_ekala_store(&self) -> bool;
+    /// Returns the root as reported by the remote store, or an error if it is inconsistent.
+    fn ekala_root(&self) -> Result<R, Self::Error>;
 }
 
 /// A trait containing a path normalization method, to normalize paths in an Ekala store

crates/atom/src/store/git.rs

@@ -31,6 +31,9 @@ pub enum Error {
     /// The repository root calculation failed.
     #[error("Failed to calculate the repositories root commit")]
     RootNotFound,
+    /// The calculated root does not match what was reported by the remote.
+    #[error("The calculated root does not match the reported one")]
+    RootInconsistent,
     /// A transparent wrapper for a [`gix::revision::walk::Error`]
     #[error(transparent)]
     WalkFailure(#[from] gix::revision::walk::Error),
@@ -63,16 +66,25 @@ pub enum Error {
     WriteRef(#[from] Box<gix::reference::edit::Error>),
 }
 
+impl Error {
+    pub(crate) fn warn(self) -> Self {
+        tracing::warn!(message = %self);
+        self
+    }
+}
+
 /// Provide a lazyily instantiated static reference to the git repository.
 static REPO: OnceLock<Option<ThreadSafeRepository>> = OnceLock::new();
 
 use std::borrow::Cow;
 static DEFAULT_REMOTE: OnceLock<Cow<str>> = OnceLock::new();
 
-/// The wrapper type unambiguously identifying the underlying type which will be used to represent
-/// the "root" identifier for an [`crate::AtomId`]. In the case of the git implementation, this is an object
-/// id representing the original commit made in the repositories history.
-#[derive(Clone)]
+/// The wrapper type for the underlying type which will be used to represent
+/// the "root" identifier for an [`crate::AtomId`]. For git, this is a [`gix::ObjectId`]
+/// representing the original commit made in the repositories history.
+///
+/// The wrapper helps disambiguate at the type level between object ids and the root id.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct Root(ObjectId);
 
 /// Return a static reference the the local Git repository.
@@ -225,39 +237,61 @@ impl AsRef<[u8]> for Root {
     }
 }
 
+trait EkalaRemote {
+    type Error;
+    const ANONYMOUS: &str = "<unamed>";
+    fn try_symbol(&self) -> Result<&str, Self::Error>;
+    fn symbol(&self) -> &str {
+        self.try_symbol().unwrap_or(Self::ANONYMOUS)
+    }
+}
+
+impl<'repo> EkalaRemote for gix::Remote<'repo> {
+    type Error = Error;
+    fn try_symbol(&self) -> Result<&str, Self::Error> {
+        use gix::remote::Name;
+        self.name()
+            .and_then(Name::as_symbol)
+            .ok_or(Error::NoRemote(Box::new(
+                gix::remote::find::existing::Error::NotFound {
+                    name: Self::ANONYMOUS.into(),
+                },
+            )))
+    }
+}
+
 const V1_ROOT: &str = "refs/tags/ekala/root/v1";
 
 use super::Init;
-impl<'repo> Init<ObjectId> for gix::Remote<'repo> {
+impl<'repo> Init<Root, ObjectId> for gix::Remote<'repo> {
     type Error = Error;
     /// Determines if this remote is a valid Ekala store by pulling HEAD and the root
-    /// tag, ensuring the latter is actually the root of HEAD.
-    fn is_ekala_store(&self) -> bool {
+    /// tag, ensuring the latter is actually the root of HEAD, returning the root.
+    fn ekala_root(&self) -> Result<Root, Self::Error> {
         use crate::id::CalculateRoot;
 
         let repo = self.repo();
-        self.get_refs(["HEAD", V1_ROOT])
-            .map(|i| {
-                let mut i = i.into_iter();
-                let fst = i
-                    .next()
-                    .and_then(|id| repo.find_commit(id).ok())
+        self.get_refs(["HEAD", V1_ROOT]).map(|i| {
+            let mut i = i.into_iter();
+            let root_for = |i: &mut dyn Iterator<Item = ObjectId>| {
+                i.next()
+                    .ok_or(Error::NoRef(V1_ROOT.to_owned(), self.symbol().to_owned()))
+                    .and_then(|id| Ok(repo.find_commit(id).map_err(Box::new)?))
                     .and_then(|c| {
                         (c.parent_ids().count() != 0)
-                            .then(|| c.calculate_root().ok().map(|r| *r))
-                            .unwrap_or(Some(c.id))
-                    });
-                let snd = i
-                    .next()
-                    .and_then(|id| repo.find_commit(id).ok())
-                    .and_then(|c| {
-                        (c.parent_ids().count() != 0)
-                            .then(|| c.calculate_root().ok().map(|r| *r))
-                            .unwrap_or(Some(c.id))
-                    });
-                fst == snd
-            })
-            .unwrap_or(false)
+                            .then(|| c.calculate_root().map(|r| *r))
+                            .unwrap_or(Ok(c.id))
+                    })
+            };
+
+            let fst = root_for(&mut i)?;
+            let snd = root_for(&mut i)?;
+            if fst == snd {
+                Ok(Root(fst))
+            } else {
+                Err(Error::RootInconsistent)
+            }
+        })?
     }
     /// Sync with the given remote and get the most up to date HEAD according to it.
     fn sync(&self) -> Result<ObjectId, Error> {
@@ -266,16 +300,8 @@ impl<'repo> Init<ObjectId> for gix::Remote<'repo> {
 
     /// Initialize the repository by calculating the root, according to the latest HEAD.
     fn ekala_init(&self) -> Result<(), Error> {
-        use gix::remote::Name;
         // fail early if the remote is not persistented to disk
-        let name = self
-            .name()
-            .and_then(Name::as_symbol)
-            .ok_or(Error::NoRemote(Box::new(
-                gix::remote::find::existing::Error::NotFound {
-                    name: "<unamed>".into(),
-                },
-            )))?;
+        let name = self.try_symbol()?;
 
         let head = self.sync()?;
 
@@ -374,16 +400,12 @@ impl<'repo> super::QueryStore<ObjectId> for gix::Remote<'repo> {
             .filter_map(|r| {
                 let (name, target, peeled) = r.unpack();
                 requested.get(name)?;
-                Some(peeled.or(target).map(ToOwned::to_owned).ok_or_else(|| {
-                    Error::NoRef(
-                        name.to_string(),
-                        remote
-                            .name()
-                            .and_then(|n| n.as_symbol())
-                            .unwrap_or("<unamed>")
-                            .into(),
-                    )
-                }))
+                Some(
+                    peeled
+                        .or(target)
+                        .map(ToOwned::to_owned)
+                        .ok_or_else(|| Error::NoRef(name.to_string(), self.symbol().to_owned())),
+                )
             })
             .collect::<Result<HashSet<_>, _>>()
     }
@@ -394,13 +416,9 @@ impl<'repo> super::QueryStore<ObjectId> for gix::Remote<'repo> {
     {
         let name = target.as_ref().to_string();
         self.get_refs(Some(target)).and_then(|r| {
-            r.into_iter().next().ok_or(Error::NoRef(
-                name,
-                self.name()
-                    .and_then(|n| n.as_symbol())
-                    .unwrap_or("<unamed>")
-                    .into(),
-            ))
+            r.into_iter()
+                .next()
+                .ok_or(Error::NoRef(name, self.symbol().to_owned()))
         })
     }
 }

crates/atom/src/store/git/test.rs

@@ -45,7 +45,7 @@ fn init_repo() -> Result<(), anyhow::Error> {
     let repo = gix::open(dir.as_ref())?;
     let remote = repo.find_remote("origin")?;
     remote.ekala_init()?;
-    assert!(remote.is_ekala_store());
+    assert!(remote.ekala_root().is_ok());
     Ok(())
 }
 
@@ -54,6 +54,6 @@ fn uninitialized_repo() -> Result<(), anyhow::Error> {
     let (dir, _remote) = init_repo_and_remote()?;
     let repo = gix::open(dir.as_ref())?;
     let remote = repo.find_remote("origin")?;
-    assert!(!remote.is_ekala_store());
+    assert!(remote.ekala_root().is_err());
     Ok(())
 }

src/cli/commands/publish/git/mod.rs

@@ -38,7 +38,6 @@ pub(super) async fn run(
     repo: &ThreadSafeRepository,
     args: PublishArgs,
 ) -> GitResult<(Vec<GitResult<GitOutcome>>, Vec<GitError>)> {
-    use atom::store::Init;
     use atom::{
         publish::{git::GitPublisher, Builder, Publish},
         store::NormalizeStorePath,
@@ -48,15 +47,7 @@ pub(super) async fn run(
 
     let GitArgs { remote, spec } = args.store.git;
 
-    if !repo
-        .find_remote(remote.as_str())
-        .map_err(Box::new)?
-        .is_ekala_store()
-    {
-        return Err(GitError::NotInitialized);
-    };
-
-    let (atoms, publisher) = GitPublisher::new(&repo, &remote, &spec).build()?;
+    let (atoms, publisher) = GitPublisher::new(&repo, &remote, &spec)?.build()?;
 
     let mut errors = Vec::with_capacity(args.path.len());
     let results = if args.recursive {