WIP: refactor: cleanup pedantic clippy in core crate

crates/core/Cargo.toml

@@ -6,6 +6,15 @@ license = { workspace = true }
 repository = { workspace = true }
 description = "core for git-next, the trunk-based development manager"
 
+[lints.clippy]
+nursery = { level = "warn", priority = -1 }
+pedantic = { level = "warn", priority = -1 }
+unwrap_used = "warn"
+expect_used = "warn"
+
+[lints.rust]
+unexpected_cfgs = { level = "warn", check-cfg = ['cfg(tarpaulin_include)'] }
+
 [features]
 default = ["forgejo", "github"]
 forgejo = []
@@ -54,12 +63,3 @@ assert2 = { workspace = true }
 rand = { workspace = true }
 test-log = { workspace = true }
 pretty_assertions = { workspace = true }
-
-[lints.clippy]
-nursery = { level = "warn", priority = -1 }
-# pedantic = "warn"
-unwrap_used = "warn"
-expect_used = "warn"
-
-[lints.rust]
-unexpected_cfgs = { level = "warn", check-cfg = ['cfg(tarpaulin_include)'] }

crates/core/src/config/api_token.rs

@@ -1,6 +1,6 @@
 /// The API Token for the [user]
-/// ForgeJo: https://{hostname}/user/settings/applications
+/// `ForgeJo`: <https://{hostname}/user/settings/applications>
-/// Github:  https://github.com/settings/tokens
+/// `Github`:  <https://github.com/settings/tokens>
 #[derive(Clone, Debug, derive_more::Constructor)]
 pub struct ApiToken(secrecy::Secret<String>);
 /// The API Token is in effect a password, so it must be explicitly exposed to access its value
@@ -11,6 +11,6 @@ impl secrecy::ExposeSecret<String> for ApiToken {
 }
 impl Default for ApiToken {
     fn default() -> Self {
-        Self("".to_string().into())
+        Self(String::new().into())
     }
 }

crates/core/src/config/branch_name.rs

@@ -1,4 +1,6 @@
 use derive_more::derive::Display;
 use serde::Serialize;
 
-crate::newtype!(BranchName: String, Display, Default, Hash, Serialize: "The name of a Git branch");
+use crate::newtype;
+
+newtype!(BranchName: String, Display, Default, Hash, Serialize: "The name of a Git branch");

crates/core/src/git/commit.rs

@@ -22,9 +22,11 @@ pub struct Commit {
     message: Message,
 }
 impl Commit {
+    #[must_use]
     pub const fn sha(&self) -> &Sha {
         &self.sha
     }
+    #[must_use]
     pub const fn message(&self) -> &Message {
         &self.message
     }

crates/core/src/git/generation.rs

@@ -7,6 +7,6 @@ newtype!(Generation: u32, Display, Default, Copy: r#"A counter for the server ge
 This counter is increased by one each time the server restarts itself when the configuration file is updated."#);
 impl Generation {
     pub fn inc(&mut self) {
-        self.0 += 1
+        self.0 += 1;
     }
 }

crates/core/src/git/git_remote.rs

@@ -9,11 +9,13 @@ pub struct GitRemote {
     host: Hostname,
     repo_path: RepoPath,
 }
+
+#[cfg(test)]
 impl GitRemote {
-    pub const fn host(&self) -> &Hostname {
+    pub(crate) const fn host(&self) -> &Hostname {
         &self.host
     }
-    pub const fn repo_path(&self) -> &RepoPath {
+    pub(crate) const fn repo_path(&self) -> &RepoPath {
         &self.repo_path
     }
 }

crates/core/src/git/push.rs

@@ -10,7 +10,7 @@ impl std::fmt::Display for Force {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             Self::No => write!(f, "fast-forward"),
-            Self::From(from) => write!(f, "force-if-from:{}", from),
+            Self::From(from) => write!(f, "force-if-from:{from}"),
         }
     }
 }
@@ -45,6 +45,15 @@ pub enum Error {
     TestResult(#[from] Box<dyn std::error::Error>),
 }
 
+/// Resets the position of a branch in the remote repo
+///
+/// Performs a 'git fetch' first to ensure we have up-to-date branch positions before
+/// performing `git push`.
+///
+/// # Errors
+///
+/// Will return an `Err` if their is no remote fetch defined in .git/config, or
+/// if there are any network connectivity issues with the remote server.
 pub fn reset(
     open_repository: &dyn OpenRepositoryLike,
     repo_details: &git::RepoDetails,

crates/core/src/git/repo_details.rs

@@ -3,7 +3,7 @@ use crate::{
     git::{
         self,
         repository::open::{oreal::RealOpenRepository, OpenRepositoryLike},
-        Generation, GitRemote,
+        Generation,
     },
     pike, BranchName, ForgeAlias, ForgeConfig, ForgeDetails, GitDir, Hostname, RemoteUrl,
     RepoAlias, RepoConfig, RepoPath, ServerRepoConfig, StoragePathType,
@@ -11,7 +11,8 @@ use crate::{
 
 use std::sync::{Arc, RwLock};
 
-use secrecy::Secret;
+use secrecy::{ExposeSecret, Secret};
+use tracing::instrument;
 
 /// The derived information about a repo, used to interact with it
 #[derive(Clone, Debug, derive_more::Display, derive_with::With)]
@@ -26,6 +27,7 @@ pub struct RepoDetails {
     pub gitdir: GitDir,
 }
 impl RepoDetails {
+    #[must_use]
     pub fn new(
         generation: Generation,
         repo_alias: &RepoAlias,
@@ -50,26 +52,23 @@ impl RepoDetails {
             ),
         }
     }
-    pub fn origin(&self) -> secrecy::Secret<String> {
+    pub(crate) fn origin(&self) -> secrecy::Secret<String> {
         let repo_details = self;
         let user = &repo_details.forge.user();
         let hostname = &repo_details.forge.hostname();
         let repo_path = &repo_details.repo_path;
         let expose_secret = repo_details.forge.token();
-        use secrecy::ExposeSecret;
+
         let token = expose_secret.expose_secret();
         let origin = format!("https://{user}:{token}@{hostname}/{repo_path}.git");
         origin.into()
     }
 
-    pub const fn gitdir(&self) -> &GitDir {
+    pub(crate) const fn gitdir(&self) -> &GitDir {
         &self.gitdir
     }
 
-    pub fn git_remote(&self) -> GitRemote {
+    #[must_use]
-        GitRemote::new(self.forge.hostname().clone(), self.repo_path.clone())
-    }
-
     pub fn with_hostname(mut self, hostname: Hostname) -> Self {
         let forge = self.forge;
         self.forge = forge.with_hostname(hostname);
@@ -77,21 +76,17 @@ impl RepoDetails {
     }
 
     // url is a secret as it contains auth token
-    pub fn url(&self) -> Secret<String> {
+    pub(crate) fn url(&self) -> Secret<String> {
         let user = self.forge.user();
-        use secrecy::ExposeSecret;
         let token = self.forge.token().expose_secret();
-        let auth_delim = match token.is_empty() {
+        let auth_delim = if token.is_empty() { "" } else { ":" };
-            true => "",
-            false => ":",
-        };
         let hostname = self.forge.hostname();
         let repo_path = &self.repo_path;
         format!("https://{user}{auth_delim}{token}@{hostname}/{repo_path}.git").into()
     }
 
     #[allow(clippy::result_large_err)]
-    pub fn open(&self) -> Result<impl OpenRepositoryLike, git::validation::remotes::Error> {
+    pub(crate) fn open(&self) -> Result<impl OpenRepositoryLike, git::validation::remotes::Error> {
         let gix_repo = pike! {
             self
             |> Self::gitdir
@@ -107,12 +102,13 @@ impl RepoDetails {
         Ok(repo)
     }
 
+    #[must_use]
     pub fn remote_url(&self) -> Option<RemoteUrl> {
         use secrecy::ExposeSecret;
         RemoteUrl::parse(self.url().expose_secret())
     }
 
-    #[tracing::instrument]
+    #[instrument]
     pub fn assert_remote_url(&self, found: Option<RemoteUrl>) -> git::repository::Result<()> {
         let Some(found) = found else {
             tracing::debug!("No remote url found to assert");
@@ -152,10 +148,10 @@ impl RepoDetails {
         let config_file = fs.file_read_to_string(config_filename)?;
         let mut config_lines = config_file
             .lines()
-            .map(|l| l.to_owned())
+            .map(ToOwned::to_owned)
             .collect::<Vec<_>>();
         tracing::debug!(?config_lines, "original file");
-        let url_line = format!(r#"   url = "{}""#, url);
+        let url_line = format!(r#"   url = "{url}""#);
         if config_lines
             .iter()
             .any(|line| *line == r#"[remote "origin"]"#)
@@ -163,7 +159,7 @@ impl RepoDetails {
             tracing::debug!("has an 'origin' remote");
             config_lines
                 .iter_mut()
-                .filter(|line| line.starts_with(r#"   url = "#))
+                .filter(|line| line.starts_with(r"   url = "))
                 .for_each(|line| line.clone_from(&url_line));
         } else {
             tracing::debug!("has no 'origin' remote");

crates/core/src/git/repository/open/mod.rs

@@ -19,6 +19,7 @@ use std::{
     sync::{Arc, RwLock},
 };
 
+#[allow(clippy::module_name_repetitions)]
 #[derive(Clone, Debug)]
 pub enum OpenRepository {
     /// A real git repository.
@@ -43,7 +44,7 @@ pub fn real(gix_repo: gix::Repository) -> OpenRepository {
 }
 
 #[cfg(not(tarpaulin_include))] // don't test mocks
-pub fn test(
+pub(crate) fn test(
     gitdir: &GitDir,
     fs: kxio::fs::FileSystem,
     on_fetch: Vec<otest::OnFetch>,
@@ -52,12 +53,35 @@ pub fn test(
     OpenRepository::Test(TestOpenRepository::new(gitdir, fs, on_fetch, on_push))
 }
 
+#[allow(clippy::module_name_repetitions)]
 #[mockall::automock]
 pub trait OpenRepositoryLike: std::fmt::Debug + Sync {
+    /// Creates a clone of the `OpenRepositoryLike`.
     fn duplicate(&self) -> Box<dyn OpenRepositoryLike>;
+
+    /// Returns a `Vec` of all the branches in the remote repo.
+    ///
+    /// # Errors
+    ///
+    ///  Will return `Err` if there are any network connectivity issues with
+    ///  the remote server.
     fn remote_branches(&self) -> git::push::Result<Vec<BranchName>>;
     fn find_default_remote(&self, direction: Direction) -> Option<RemoteUrl>;
+
+    /// Performs a `git fetch`
+    ///
+    /// # Errors
+    ///
+    /// Will return an `Err` if their is no remote fetch defined in .git/config, or
+    /// if there are any network connectivity issues with the remote server.
     fn fetch(&self) -> Result<(), git::fetch::Error>;
+
+    /// Performs a `git push`
+    ///
+    /// # Errors
+    ///
+    /// Will return an `Err` if their is no remote push defined in .git/config, or
+    /// if there are any network connectivity issues with the remote server.
     fn push(
         &self,
         repo_details: &git::RepoDetails,
@@ -67,6 +91,10 @@ pub trait OpenRepositoryLike: std::fmt::Debug + Sync {
     ) -> git::push::Result<()>;
 
     /// List of commits in a branch, optionally up-to any specified commit.
+    ///
+    /// # Errors
+    ///
+    /// Will return `Err` if there are any network connectivity issues with the remote server.
     fn commit_log(
         &self,
         branch_name: &BranchName,
@@ -76,10 +104,15 @@ pub trait OpenRepositoryLike: std::fmt::Debug + Sync {
     /// Read the contents of a file as a string.
     ///
     /// Only handles files in the root of the repo.
+    ///
+    /// # Errors
+    ///
+    /// Will return `Err` if the file does not exists on the specified branch.
     fn read_file(&self, branch_name: &BranchName, file_name: &Path) -> git::file::Result<String>;
 }
 
-pub fn mock() -> Box<MockOpenRepositoryLike> {
+#[cfg(test)]
+pub(crate) fn mock() -> Box<MockOpenRepositoryLike> {
     Box::new(MockOpenRepositoryLike::new())
 }
 

crates/core/src/git/repository/open/oreal.rs

@@ -9,7 +9,9 @@ use gix::bstr::BStr;
 use tracing::{info, warn};
 
 use std::{
+    borrow::ToOwned,
     path::Path,
+    result::Result,
     sync::{Arc, RwLock},
 };
 
@@ -24,11 +26,12 @@ impl super::OpenRepositoryLike for RealOpenRepository {
             .and_then(|repo| {
                 Ok(repo.to_thread_local().references()?).and_then(|refs| {
                     Ok(refs.remote_branches().map(|rb| {
-                        rb.filter_map(|rbi| rbi.ok())
+                        rb.filter_map(Result::ok)
                             .map(|r| r.name().to_owned())
                             .map(|n| n.to_string())
                             .filter_map(|p| {
-                                p.strip_prefix("refs/remotes/origin/").map(|v| v.to_owned())
+                                p.strip_prefix("refs/remotes/origin/")
+                                    .map(ToOwned::to_owned)
                             })
                             .filter(|b| b.as_str() != "HEAD")
                             .map(BranchName::new)
@@ -61,6 +64,8 @@ impl super::OpenRepositoryLike for RealOpenRepository {
     #[tracing::instrument(skip_all)]
     #[cfg(not(tarpaulin_include))] // would require writing to external service
     fn fetch(&self) -> Result<(), git::fetch::Error> {
+        use std::sync::atomic::AtomicBool;
+
         let Ok(repository) = self.0.read() else {
             #[cfg(not(tarpaulin_include))] // don't test mutex lock failure
             return Err(git::fetch::Error::Lock);
@@ -75,9 +80,12 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         remote
             .connect(gix::remote::Direction::Fetch)
             .map_err(|gix| git::fetch::Error::Connect(gix.to_string()))?
-            .prepare_fetch(gix::progress::Discard, Default::default())
+            .prepare_fetch(
+                gix::progress::Discard,
+                gix::remote::ref_map::Options::default(),
+            )
             .map_err(|gix| git::fetch::Error::Prepare(gix.to_string()))?
-            .receive(gix::progress::Discard, &Default::default())
+            .receive(gix::progress::Discard, &AtomicBool::default())
             .map_err(|gix| git::fetch::Error::Receive(gix.to_string()))?;
         info!("Fetch okay");
         Ok(())
@@ -93,15 +101,16 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         to_commit: &git::GitRef,
         force: &git::push::Force,
     ) -> Result<(), git::push::Error> {
+        use secrecy::ExposeSecret as _;
+
         let origin = repo_details.origin();
         let force = match force {
-            git::push::Force::No => "".to_string(),
+            git::push::Force::No => String::new(),
             git::push::Force::From(old_ref) => {
                 format!("--force-with-lease={branch_name}:{old_ref}")
             }
         };
         // INFO: never log the command as it contains the API token within the 'origin'
-        use secrecy::ExposeSecret;
         let command: secrecy::Secret<String> = format!(
             "/usr/bin/git push {} {to_commit}:{branch_name} {force}",
             origin.expose_secret()
@@ -131,10 +140,7 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         branch_name: &BranchName,
         find_commits: &[git::Commit],
     ) -> Result<Vec<git::Commit>, git::commit::log::Error> {
-        let limit = match find_commits.is_empty() {
+        let limit = if find_commits.is_empty() { 1 } else { 25 };
-            true => 1,
-            false => 25,
-        };
         self.0
             .read()
             .map_err(|_| git::commit::log::Error::Lock)
@@ -195,8 +201,7 @@ impl super::OpenRepositoryLike for RealOpenRepository {
             .map_err(|_| git::file::Error::Lock)
             .and_then(|repo| {
                 let thread_local = repo.to_thread_local();
-                let fref =
+                let fref = thread_local.find_reference(format!("origin/{branch_name}").as_str())?;
-                    thread_local.find_reference(format!("origin/{}", branch_name).as_str())?;
                 let id = fref.try_id().ok_or(git::file::Error::TryId)?;
                 let oid = id.detach();
                 let obj = thread_local.find_object(oid)?;

crates/core/src/git/repository/open/otest.rs

@@ -23,6 +23,12 @@ pub struct OnFetch {
     action: OnFetchFn,
 }
 impl OnFetch {
+    /// Invokes the action function.
+    ///
+    /// # Errors
+    ///
+    /// Will return any `Err` if there is no fetch remote defined in .git/config
+    /// of if there are any network connectivity issues with the remote server.
     pub fn invoke(&self) -> git::fetch::Result<()> {
         (self.action)(&self.repo_branches, &self.gitdir, &self.fs)
     }
@@ -45,6 +51,12 @@ pub struct OnPush {
     action: OnPushFn,
 }
 impl OnPush {
+    /// Invokes the action function.
+    ///
+    /// # Errors
+    ///
+    /// Will return any `Err` if there is no push remote defined in .git/config
+    /// of if there are any network connectivity issues with the remote server.
     pub fn invoke(
         &self,
         repo_details: &git::RepoDetails,
@@ -93,9 +105,12 @@ impl git::repository::OpenRepositoryLike for TestOpenRepository {
             .write()
             .map_err(|_| git::fetch::Error::Lock)
             .map(|mut c| *c += 1)?;
-        self.on_fetch.get(i).map(|f| f.invoke()).unwrap_or_else(|| {
+        self.on_fetch.get(i).map_or_else(
-            unimplemented!("Unexpected fetch");
+            || {
-        })
+                unimplemented!("Unexpected fetch");
+            },
+            OnFetch::invoke,
+        )
     }
 
     fn push(

crates/core/src/git/tests.rs

@@ -101,7 +101,7 @@ mod push {
 
     #[test]
     fn force_no_should_display() {
-        assert_eq!(git::push::Force::No.to_string(), "fast-forward")
+        assert_eq!(git::push::Force::No.to_string(), "fast-forward");
     }
 
     #[test]
@@ -111,7 +111,7 @@ mod push {
         assert_eq!(
             git::push::Force::From(GitRef::from(commit)).to_string(),
             format!("force-if-from:{sha}")
-        )
+        );
     }
 
     mod reset {
@@ -138,7 +138,7 @@ mod push {
             let commit = given::a_commit();
             let gitref = GitRef::from(commit);
             let_assert!(
-                Ok(_) = git::push::reset(
+                Ok(()) = git::push::reset(
                     &*open_repository,
                     &repo_details,
                     branch_name,
@@ -182,38 +182,6 @@ mod repo_details {
             "https://user:token@host/repo.git"
         );
     }
-    #[test]
-    fn should_return_git_remote() {
-        let rd = RepoDetails::new(
-            Generation::default(),
-            &RepoAlias::new("foo"),
-            &ServerRepoConfig::new(
-                "user/repo".to_string(),
-                "branch".to_string(),
-                None,
-                None,
-                None,
-                None,
-            ),
-            &ForgeAlias::new("default".to_string()),
-            &ForgeConfig::new(
-                ForgeType::MockForge,
-                "host".to_string(),
-                "user".to_string(),
-                "token".to_string(),
-                BTreeMap::new(),
-            ),
-            GitDir::new(PathBuf::default().join("foo"), StoragePathType::Internal),
-        );
-
-        assert_eq!(
-            rd.git_remote(),
-            GitRemote::new(
-                Hostname::new("host".to_string()),
-                RepoPath::new("user/repo".to_string())
-            )
-        );
-    }
 }
 pub mod given {
     use super::*;
@@ -267,7 +235,7 @@ pub mod given {
             format!("hostname-{}", a_name()),
             format!("user-{}", a_name()),
             format!("token-{}", a_name()),
-            Default::default(), // no repos
+            BTreeMap::default(), // no repos
         )
     }
 
@@ -349,7 +317,7 @@ pub mod given {
         // add use are origin url
         let mut config_lines = config_file.lines().collect::<Vec<_>>();
         config_lines.push(r#"[remote "origin"]"#);
-        let url_line = format!(r#"   url = "{}""#, url);
+        let url_line = format!(r#"   url = "{url}""#);
         tracing::info!(?url, %url_line, "writing");
         config_lines.push(&url_line);
         // write config file back out
@@ -436,7 +404,7 @@ pub mod then {
 
     pub fn git_checkout_new_branch(branch_name: &BranchName, gitdir: &GitDir) -> TestResult {
         exec(
-            format!("git checkout -b {}", branch_name),
+            &format!("git checkout -b {branch_name}"),
             std::process::Command::new("/usr/bin/git")
                 .current_dir(gitdir.to_path_buf())
                 .args(["checkout", "-b", branch_name.to_string().as_str()])
@@ -447,7 +415,7 @@ pub mod then {
 
     pub fn git_switch(branch_name: &BranchName, gitdir: &GitDir) -> TestResult {
         exec(
-            format!("git switch {}", branch_name),
+            &format!("git switch {branch_name}"),
             std::process::Command::new("/usr/bin/git")
                 .current_dir(gitdir.to_path_buf())
                 .args(["switch", branch_name.to_string().as_str()])
@@ -455,7 +423,7 @@ pub mod then {
         )
     }
 
-    fn exec(label: String, output: Result<std::process::Output, std::io::Error>) -> TestResult {
+    fn exec(label: &str, output: Result<std::process::Output, std::io::Error>) -> TestResult {
         println!("== {label}");
         match output {
             Ok(output) => {
@@ -479,7 +447,7 @@ pub mod then {
 
     fn git_add_file(gitdir: &GitDir, file: &Path) -> TestResult {
         exec(
-            format!("git add {file:?}"),
+            &format!("git add {file:?}"),
             std::process::Command::new("/usr/bin/git")
                 .current_dir(gitdir.to_path_buf())
                 .args(["add", file.display().to_string().as_str()])
@@ -489,7 +457,7 @@ pub mod then {
 
     fn git_commit(gitdir: &GitDir, file: &Path) -> TestResult {
         exec(
-            format!(r#"git commit -m"Added {file:?}""#),
+            &format!(r#"git commit -m"Added {file:?}""#),
             std::process::Command::new("/usr/bin/git")
                 .current_dir(gitdir.to_path_buf())
                 .args([
@@ -502,7 +470,7 @@ pub mod then {
 
     pub fn git_log_all(gitdir: &GitDir) -> TestResult {
         exec(
-            "git log --all --oneline --decorate --graph".to_string(),
+            "git log --all --oneline --decorate --graph",
             std::process::Command::new("/usr/bin/git")
                 .current_dir(gitdir.to_path_buf())
                 .args(["log", "--all", "--oneline", "--decorate", "--graph"])

crates/core/src/macros/newtype.rs

@@ -38,6 +38,7 @@ macro_rules! newtype {
                 Self(value.into())
             }
             #[allow(clippy::missing_const_for_fn)]
+            #[must_use]
             pub fn unwrap(self) -> $type {
                 self.0
             }