WIP: add FIXME notes for commit_log

CHANGELOG.md

@@ -2,16 +2,8 @@
 
 All notable changes to this project will be documented in this file.
 
-## [0.6.1] - 2024-05-25
-
-### Bug Fixes
-
-- New commit_log matches original from API request ([3642b2c](https://git.kemitix.net/kemitix/git-next/commit/3642b2cdd11de2bf49c1214c9938a86517d6a7fd))
-
 ## [0.6.0] - 2024-05-25
 
-[8616225](https://git.kemitix.net/kemitix/git-next/commit/8616225a28d94964401c6de6442c7408848f6c1f)...[6cab8bb](https://git.kemitix.net/kemitix/git-next/commit/6cab8bb2baf7ae8300f9496c1e843531839e30e5)
-
 ### Features
 
 - Config file watcher will respond to touch ([ebbb655](https://git.kemitix.net/kemitix/git-next/commit/ebbb655bfca3561059e068606a32dcb17d490f5e))
@@ -19,7 +11,6 @@ All notable changes to this project will be documented in this file.
 ### Miscellaneous Tasks
 
 - Don't directly open coverage report ([c92e41e](https://git.kemitix.net/kemitix/git-next/commit/c92e41ee564f36470511b2d093c1e00ae66078d4))
-- Release 0.6.0 ([6cab8bb](https://git.kemitix.net/kemitix/git-next/commit/6cab8bb2baf7ae8300f9496c1e843531839e30e5))
 
 ### Refactor
 

Cargo.toml

@@ -12,7 +12,7 @@ members = [
 ]
 
 [workspace.package]
-version = "0.6.1"
+version = "0.6.0"
 edition = "2021"
 
 [workspace.lints.clippy]

crates/forge-forgejo/src/branch/validate_positions.rs

@@ -14,7 +14,6 @@ pub async fn validate_positions(
 ) -> git::validation::Result {
     let repo_details = &forge.repo_details;
     // Collect Commit Histories for `main`, `next` and `dev` branches
-    repository.fetch()?;
     let commit_histories =
         get_commit_histories(repository, repo_details, &repo_config, &forge.net).await;
     let commit_histories = match commit_histories {
@@ -24,6 +23,7 @@ pub async fn validate_positions(
             return Err(git::validation::Error::Network(Box::new(err)));
         }
     };
+
     // Validations
     let Some(main) = commit_histories.main.first().cloned() else {
         warn!(
@@ -60,6 +60,7 @@ pub async fn validate_positions(
     let next_is_ancestor_of_dev = commit_histories.dev.iter().any(|dev| dev == &next);
     if !next_is_ancestor_of_dev {
         info!("Next is not an ancestor of dev - resetting next to main");
+
         if let Err(err) = forge.branch_reset(
             repository,
             repo_config.branches().next(),
@@ -76,12 +77,11 @@ pub async fn validate_positions(
             repo_config.branches().next(),
         ));
     }
+
     let next_commits = commit_histories
         .next
         .into_iter()
-        .take(2) // next should never be more than one commit ahead of main, so this should be
-        // either be next and main on two adjacent commits, or next and main on the same commit,
-        // plus the parent of main.
+        .take(2)
         .collect::<Vec<_>>();
     if !next_commits.contains(&main) {
         warn!(
@@ -128,6 +128,7 @@ pub async fn validate_positions(
             repo_config.branches().next(),
         )); // dev is not based on next
     }
+
     let Some(dev) = commit_histories.dev.first().cloned() else {
         warn!(
             "No commits on dev branch '{}'",
@@ -137,6 +138,7 @@ pub async fn validate_positions(
             repo_config.branches().dev(),
         ));
     };
+
     Ok(git::validation::Positions {
         main,
         next,
@@ -168,11 +170,12 @@ async fn get_commit_histories(
         net,
     )
     .await)?;
+    let next_head = next[0].clone();
     let dev = (get_commit_history(
         repository,
         repo_details,
         &repo_config.branches().dev(),
-        &[main_head],
+        &[next_head, main_head],
         net,
     )
     .await)?;
@@ -186,7 +189,6 @@ async fn get_commit_histories(
     Ok(histories)
 }
 
-#[tracing::instrument(skip_all, fields(%branch_name, ?find_commits))]
 async fn get_commit_history(
     repository: &git::repository::OpenRepository,
     repo_details: &git::RepoDetails,
@@ -202,7 +204,9 @@ async fn get_commit_history(
                 info!("new version matches");
                 Ok(updated)
             } else {
-                error!("new version doesn't match original");
+                // FIXME: log original and updated on seperate log lines - can't find where one ends and
+                // the other beings!
+                error!(?updated, ?original, "new version doesn't match original");
                 Ok(original)
             }
         }
@@ -267,13 +271,8 @@ async fn original_get_commit_history(
             || find_commits
                 .iter()
                 .any(|find_commit| commits.iter().any(|commit| commit == find_commit));
-        let at_end = commits.len() < limit; // i.e. less than the number of commits requested
-        for commit in commits {
-            all_commits.push(commit.clone());
-            if find_commits.contains(&commit) {
-                break;
-            }
-        }
+        let at_end = commits.len() < limit;
+        all_commits.extend(commits);
         if found || at_end {
             break;
         }

crates/forge-github/src/lib.rs

@@ -1,68 +1,21 @@
-//
-#![allow(dead_code)]
-
-use derive_more::Constructor;
-use git_next_config as config;
-use git_next_git as git;
-
 use kxio::network::Network;
 
-#[cfg(test)]
-mod tests;
-
-#[derive(Clone, Debug, Constructor)]
-pub struct Github {
+struct Github;
+pub struct GithubEnv {
     net: Network,
 }
+impl GithubEnv {
+    pub(crate) const fn new(net: Network) -> GithubEnv {
+        Self { net }
+    }
+}
 #[async_trait::async_trait]
-impl git_next_git::ForgeLike for Github {
+impl git_next_git::ForgeLike for GithubEnv {
     fn name(&self) -> String {
         "github".to_string()
     }
-    async fn branches_get_all(&self) -> Result<Vec<config::BranchName>, git::branch::Error> {
-        todo!();
-    }
 
-    /// Returns the contents of the file.
-    async fn file_contents_get(
-        &self,
-        _branch_name: &config::BranchName,
-        _file_path: &str,
-    ) -> Result<String, git::file::Error> {
-        todo!();
-    }
-
-    /// Assesses the relative positions of the main, next and dev branch and updates their
-    /// positions as needed.
-    async fn branches_validate_positions(
-        &self,
-        _repository: git::OpenRepository,
-        _repo_config: config::RepoConfig,
-    ) -> git::validation::Result {
-        todo!();
-    }
-
-    /// Moves a branch to a new commit.
-    fn branch_reset(
-        &self,
-        _repository: &git::OpenRepository,
-        _branch_name: config::BranchName,
-        _to_commit: git::GitRef,
-        _force: git::push::Force,
-    ) -> git::push::Result {
-        todo!();
-    }
-
-    /// Checks the results of any (e.g. CI) status checks for the commit.
-    async fn commit_status(&self, _commit: &git::Commit) -> git::commit::Status {
-        todo!();
-    }
-
-    /// Clones a repo to disk.
-    fn repo_clone(
-        &self,
-        _gitdir: config::GitDir,
-    ) -> Result<git::OpenRepository, git::repository::Error> {
-        todo!();
+    async fn branches_get_all(&self) -> Vec<Branch> {
+        todo!()
     }
 }

crates/forge-github/src/tests/mod.rs

@@ -1,8 +0,0 @@
-use git_next_git::ForgeLike as _;
-
-#[test]
-fn test_name() {
-    let net = kxio::network::Network::new_mock();
-    let forge = crate::Github::new(net);
-    assert_eq!(forge.name(), "github");
-}

crates/forge/src/lib.rs

@@ -1,6 +1,5 @@
-//
-use git_next_forge_forgejo as forgejo;
-use git_next_forge_github as github;
+#![allow(dead_code)]
+
 use git_next_git as git;
 use kxio::network::Network;
 
@@ -8,19 +7,17 @@ mod mock_forge;
 
 #[derive(Clone, Debug)]
 pub enum Forge {
-    Mock(mock_forge::MockForge),
-
+    Mock(mock_forge::MockForgeEnv),
+    #[allow(clippy::enum_variant_names)]
     #[cfg(feature = "forgejo")]
     ForgeJo(git_next_forge_forgejo::ForgeJo),
-
     #[cfg(feature = "github")]
-    Github(git_next_forge_github::Github),
+    Github(git_next_forge_github::GithubEnv),
 }
 impl Forge {
     pub const fn new_mock() -> Self {
-        Self::Mock(mock_forge::MockForge::new())
+        Self::Mock(mock_forge::MockForgeEnv::new())
     }
-
     #[cfg(feature = "forgejo")]
     pub const fn new_forgejo(
         repo_details: git::RepoDetails,
@@ -29,10 +26,9 @@ impl Forge {
     ) -> Self {
         Self::ForgeJo(forgejo::ForgeJo::new(repo_details, net, repo))
     }
-
     #[cfg(feature = "github")]
     pub const fn new_github(net: Network) -> Self {
-        Self::Github(github::Github::new(net))
+        Self::Github(github::GithubEnv::new(net))
     }
 }
 impl std::ops::Deref for Forge {
@@ -43,7 +39,7 @@ impl std::ops::Deref for Forge {
             #[cfg(feature = "forgejo")]
             Self::ForgeJo(env) => env,
             #[cfg(feature = "github")]
-            Self::Github(env) => env,
+            Forge::Github(env) => env,
         }
     }
 }

crates/forge/src/mock_forge.rs

@@ -1,16 +1,20 @@
 //
 #![cfg(not(tarpaulin_include))]
 
-use derive_more::Constructor;
 use git::OpenRepository;
 use git_next_config as config;
 use git_next_git as git;
 
-#[derive(Clone, Debug, Constructor)]
-pub struct MockForge;
-
+struct MockForge;
+#[derive(Clone, Debug)]
+pub struct MockForgeEnv;
+impl MockForgeEnv {
+    pub(crate) const fn new() -> Self {
+        Self
+    }
+}
 #[async_trait::async_trait]
-impl git::ForgeLike for MockForge {
+impl git::ForgeLike for MockForgeEnv {
     fn name(&self) -> String {
         "mock".to_string()
     }

crates/forge/src/tests/github.rs

@@ -0,0 +1,8 @@
+use super::*;
+
+#[test]
+fn test_name() {
+    let net = Network::new_mock();
+    let forge = Forge::new_github(net);
+    assert_eq!(forge.name(), "github");
+}

crates/forge/src/tests/mod.rs

@@ -4,6 +4,9 @@ use super::*;
 use git_next_config as config;
 use git_next_git as git;
 
+#[cfg(feature = "github")]
+mod github;
+
 #[test]
 fn test_mock_name() {
     let forge = Forge::new_mock();

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

@@ -22,7 +22,6 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         remote.url(direction.into()).map(Into::into)
     }
 
-    #[tracing::instrument(skip_all)]
     fn fetch(&self) -> Result<(), git::fetch::Error> {
         let Ok(repository) = self.0.lock() else {
             #[cfg(not(tarpaulin_include))] // don't test mutex lock failure
@@ -45,13 +44,12 @@ impl super::OpenRepositoryLike for RealOpenRepository {
             }
             Err(e) => Err(git::fetch::Error::Fetch(e.to_string()))?,
         }
-        info!("Fetch okay");
+
         Ok(())
     }
 
     // TODO: (#72) reimplement using `gix`
     #[cfg(not(tarpaulin_include))] // would require writing to external service
-    #[tracing::instrument(skip_all)]
     fn push(
         &self,
         repo_details: &git::RepoDetails,
@@ -73,11 +71,13 @@ impl super::OpenRepositoryLike for RealOpenRepository {
             origin.expose_secret()
         )
         .into();
+
         let git_dir = self
             .0
             .lock()
             .map_err(|_| git::push::Error::Lock)
             .map(|r| r.git_dir().to_path_buf())?;
+
         let ctx = gix::diff::command::Context {
             git_dir: Some(git_dir.clone()),
             ..Default::default()
@@ -91,7 +91,7 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         {
             Ok(mut child) => match child.wait() {
                 Ok(_) => {
-                    info!("Push okay");
+                    info!("Branch updated");
                     Ok(())
                 }
                 Err(err) => {
@@ -111,15 +111,12 @@ impl super::OpenRepositoryLike for RealOpenRepository {
         branch_name: &config::BranchName,
         find_commits: &[git::Commit],
     ) -> Result<Vec<crate::Commit>, git::commit::log::Error> {
-        let limit = match find_commits.is_empty() {
-            true => 1,
-            false => 50,
-        };
+        // FIXME: only finds 'main', does it need to prefix branch_name with 'origin/'?
         self.0
             .lock()
             .map_err(|_| git::commit::log::Error::Lock)
             .map(|repo| {
-                let branch_name = format!("remotes/origin/{branch_name}");
+                let branch_name = branch_name.to_string();
                 let branch_name = BStr::new(&branch_name);
                 let branch_head = repo
                     .rev_parse_single(branch_name)
@@ -131,7 +128,7 @@ impl super::OpenRepositoryLike for RealOpenRepository {
                     .all()
                     .map_err(|e| e.to_string())?;
                 let mut commits = vec![];
-                for item in walk.take(limit) {
+                for item in walk {
                     let item = item.map_err(|e| e.to_string())?;
                     let commit = item.object().map_err(|e| e.to_string())?;
                     let id = commit.id().to_string();
@@ -140,15 +137,12 @@ impl super::OpenRepositoryLike for RealOpenRepository {
                         git::commit::Sha::new(id),
                         git::commit::Message::new(message),
                     );
-                    info!(?commit, "found");
                     if find_commits.contains(&commit) {
-                        info!("Is in find_commits");
                         commits.push(commit);
                         break;
                     }
                     commits.push(commit);
                 }
-                info!("finished walkfing");
                 Ok(commits)
             })?
     }

crates/git/src/validation.rs

@@ -13,25 +13,13 @@ pub struct Positions {
 #[derive(Debug, derive_more::Display)]
 pub enum Error {
     Network(Box<kxio::network::NetworkError>),
-
-    Fetch(git::fetch::Error),
-
     #[display("Failed to Reset Branch {branch} to {commit}")]
     FailedToResetBranch {
         branch: BranchName,
         commit: git::Commit,
     },
-
     BranchReset(BranchName),
-
     BranchHasNoCommits(BranchName),
-
     DevBranchNotBasedOn(BranchName),
 }
 impl std::error::Error for Error {}
-
-impl From<git::fetch::Error> for Error {
-    fn from(value: git::fetch::Error) -> Self {
-        Self::Fetch(value)
-    }
-}