From 6c60e3fb7a243ad0c4fda585940a353547a8180d Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Fri, 13 Sep 2024 18:25:48 +0100 Subject: [PATCH] refactor: reimplement git fetch using git --- crates/cli/src/repo/branch.rs | 4 +- crates/cli/src/repo/handlers/advance_main.rs | 2 +- crates/cli/src/repo/handlers/advance_next.rs | 4 +- .../cli/src/repo/tests/branch/advance_next.rs | 12 +++--- crates/cli/src/repo/tests/expect.rs | 2 +- .../src/repo/tests/handlers/advance_main.rs | 8 ++-- .../src/repo/tests/handlers/advance_next.rs | 4 +- .../src/repo/tests/handlers/validate_repo.rs | 4 +- crates/core/src/git/fetch.rs | 3 ++ crates/core/src/git/push.rs | 2 +- crates/core/src/git/repository/open/mod.rs | 2 +- crates/core/src/git/repository/open/oreal.rs | 43 +++++++++---------- crates/core/src/git/repository/open/otest.rs | 2 +- .../src/git/repository/open/tests/fetch.rs | 2 +- crates/core/src/git/tests.rs | 2 +- crates/core/src/git/validation/positions.rs | 2 +- crates/core/src/git/validation/tests.rs | 8 ++-- 17 files changed, 53 insertions(+), 53 deletions(-) diff --git a/crates/cli/src/repo/branch.rs b/crates/cli/src/repo/branch.rs index dfe4da4..9963ccd 100644 --- a/crates/cli/src/repo/branch.rs +++ b/crates/cli/src/repo/branch.rs @@ -19,7 +19,7 @@ use tracing::{info, instrument, warn}; pub fn advance_next( commit: Option, force: git_next_core::git::push::Force, - repo_details: RepoDetails, + repo_details: &RepoDetails, repo_config: RepoConfig, open_repository: &dyn OpenRepositoryLike, message_token: MessageToken, @@ -29,7 +29,7 @@ pub fn advance_next( info!("Advancing next to commit '{}'", commit); reset( open_repository, - &repo_details, + repo_details, &repo_config.branches().next(), &commit.into(), &force, diff --git a/crates/cli/src/repo/handlers/advance_main.rs b/crates/cli/src/repo/handlers/advance_main.rs index 1c18485..59c1fbe 100644 --- a/crates/cli/src/repo/handlers/advance_main.rs +++ b/crates/cli/src/repo/handlers/advance_main.rs @@ -41,7 +41,7 @@ impl Handler for RepoActor { } else { self.update_tui(RepoUpdate::MainUpdated); if let Some(open_repository) = &self.open_repository { - match open_repository.fetch() { + match open_repository.fetch(&repo_details) { Ok(()) => self.update_tui_log(git::graph::log(&self.repo_details)), Err(err) => self.alert_tui(format!("fetching: {err}")), } diff --git a/crates/cli/src/repo/handlers/advance_next.rs b/crates/cli/src/repo/handlers/advance_next.rs index e4908e8..392c4b0 100644 --- a/crates/cli/src/repo/handlers/advance_next.rs +++ b/crates/cli/src/repo/handlers/advance_next.rs @@ -44,14 +44,14 @@ impl Handler for RepoActor { match advance_next( commit, force, - repo_details, + &repo_details, repo_config, &**open_repository, self.message_token, ) { Ok(message_token) => { self.update_tui(RepoUpdate::NextUpdated); - match open_repository.fetch() { + match open_repository.fetch(&repo_details) { Ok(()) => self.update_tui_log(git::graph::log(&self.repo_details)), Err(err) => self.alert_tui(format!("fetching: {err}")), } diff --git a/crates/cli/src/repo/tests/branch/advance_next.rs b/crates/cli/src/repo/tests/branch/advance_next.rs index 9425700..5a57cd3 100644 --- a/crates/cli/src/repo/tests/branch/advance_next.rs +++ b/crates/cli/src/repo/tests/branch/advance_next.rs @@ -7,7 +7,7 @@ fn advance_next_sut( next: &Commit, main: &Commit, dev_commit_history: &[Commit], - repo_details: RepoDetails, + repo_details: &RepoDetails, repo_config: RepoConfig, open_repository: &dyn OpenRepositoryLike, message_token: MessageToken, @@ -42,7 +42,7 @@ mod when_at_dev { &next, main, dev_commit_history, - repo_details, + &repo_details, repo_config, &open_repository, message_token, @@ -77,7 +77,7 @@ mod can_advance { &next, main, dev_commit_history, - repo_details, + &repo_details, repo_config, &open_repository, message_token, @@ -108,7 +108,7 @@ mod can_advance { &next, main, dev_commit_history, - repo_details, + &repo_details, repo_config, &open_repository, message_token, @@ -148,7 +148,7 @@ mod can_advance { &next, main, dev_commit_history, - repo_details, + &repo_details, repo_config, &open_repository, message_token, @@ -180,7 +180,7 @@ mod can_advance { &next, main, dev_commit_history, - repo_details, + &repo_details, repo_config, &open_repository, message_token, diff --git a/crates/cli/src/repo/tests/expect.rs b/crates/cli/src/repo/tests/expect.rs index 16fe882..d3cf13c 100644 --- a/crates/cli/src/repo/tests/expect.rs +++ b/crates/cli/src/repo/tests/expect.rs @@ -11,7 +11,7 @@ pub fn fetch(open_repository: &mut MockOpenRepositoryLike, result: Result<(), fe open_repository .expect_fetch() .times(1) - .return_once(|| result); + .return_once(|_| result); } pub fn push_ok(open_repository: &mut MockOpenRepositoryLike) { diff --git a/crates/cli/src/repo/tests/handlers/advance_main.rs b/crates/cli/src/repo/tests/handlers/advance_main.rs index 2f89656..912b135 100644 --- a/crates/cli/src/repo/tests/handlers/advance_main.rs +++ b/crates/cli/src/repo/tests/handlers/advance_main.rs @@ -19,7 +19,7 @@ async fn when_repo_config_should_fetch_then_push_then_revalidate() -> TestResult .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); open_repository .expect_push() .times(1) @@ -29,7 +29,7 @@ async fn when_repo_config_should_fetch_then_push_then_revalidate() -> TestResult .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); //when let (addr, log) = when::start_actor_with_open_repository( @@ -69,7 +69,7 @@ async fn when_app_config_should_fetch_then_push_then_revalidate() -> TestResult .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); open_repository .expect_push() .times(1) @@ -79,7 +79,7 @@ async fn when_app_config_should_fetch_then_push_then_revalidate() -> TestResult .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); //when let (addr, log) = when::start_actor_with_open_repository( diff --git a/crates/cli/src/repo/tests/handlers/advance_next.rs b/crates/cli/src/repo/tests/handlers/advance_next.rs index 6570be8..310326f 100644 --- a/crates/cli/src/repo/tests/handlers/advance_next.rs +++ b/crates/cli/src/repo/tests/handlers/advance_next.rs @@ -22,7 +22,7 @@ async fn should_fetch_then_push_then_revalidate() -> TestResult { .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); open_repository .expect_push() .times(1) @@ -32,7 +32,7 @@ async fn should_fetch_then_push_then_revalidate() -> TestResult { .expect_fetch() .times(1) .in_sequence(&mut seq) - .return_once(|| Ok(())); + .return_once(|_| Ok(())); //when let (addr, log) = when::start_actor_with_open_repository( diff --git a/crates/cli/src/repo/tests/handlers/validate_repo.rs b/crates/cli/src/repo/tests/handlers/validate_repo.rs index 8fce618..dac3757 100644 --- a/crates/cli/src/repo/tests/handlers/validate_repo.rs +++ b/crates/cli/src/repo/tests/handlers/validate_repo.rs @@ -486,7 +486,7 @@ async fn should_send_validate_repo_when_retryable_error() -> TestResult { //given let fs = given::a_filesystem(); let (mut open_repository, repo_details) = given::an_open_repository(&fs); - open_repository.expect_fetch().return_once(|| Ok(())); + open_repository.expect_fetch().return_once(|_| Ok(())); open_repository .expect_commit_log() .return_once(|_, _| Err(git::commit::log::Error::Lock)); @@ -517,7 +517,7 @@ async fn should_send_notify_user_when_non_retryable_error() -> TestResult { let (mut open_repository, repo_details) = given::an_open_repository(&fs); #[allow(clippy::unwrap_used)] let repo_config = repo_details.repo_config.clone().unwrap(); - open_repository.expect_fetch().return_once(|| Ok(())); + open_repository.expect_fetch().return_once(|_| Ok(())); // branches are all unrelated - non-retryable until each branch is updated let branches = repo_config.branches(); diff --git a/crates/core/src/git/fetch.rs b/crates/core/src/git/fetch.rs index ae6780b..a7443e6 100644 --- a/crates/core/src/git/fetch.rs +++ b/crates/core/src/git/fetch.rs @@ -3,6 +3,9 @@ pub type Result = core::result::Result; #[derive(Debug, thiserror::Error)] pub enum Error { + #[error("io")] + Io(#[from] std::io::Error), + #[error("unable to open repo: {0}")] UnableToOpenRepo(String), diff --git a/crates/core/src/git/push.rs b/crates/core/src/git/push.rs index 25be1b4..7bf084e 100644 --- a/crates/core/src/git/push.rs +++ b/crates/core/src/git/push.rs @@ -61,6 +61,6 @@ pub fn reset( to_commit: &git::GitRef, force: &git::push::Force, ) -> Result<()> { - open_repository.fetch()?; + open_repository.fetch(repo_details)?; open_repository.push(repo_details, branch_name, to_commit, force) } diff --git a/crates/core/src/git/repository/open/mod.rs b/crates/core/src/git/repository/open/mod.rs index df06d81..57cf8af 100644 --- a/crates/core/src/git/repository/open/mod.rs +++ b/crates/core/src/git/repository/open/mod.rs @@ -82,7 +82,7 @@ pub trait OpenRepositoryLike: std::fmt::Debug + Sync { /// /// 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>; + fn fetch(&self, repo_details: &git::RepoDetails) -> Result<(), git::fetch::Error>; /// Performs a `git push` /// diff --git a/crates/core/src/git/repository/open/oreal.rs b/crates/core/src/git/repository/open/oreal.rs index 58414bf..c74cb3f 100644 --- a/crates/core/src/git/repository/open/oreal.rs +++ b/crates/core/src/git/repository/open/oreal.rs @@ -66,35 +66,32 @@ 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; + fn fetch(&self, repo_details: &git::RepoDetails) -> Result<(), git::fetch::Error> { + use secrecy::ExposeSecret; - let Ok(repository) = self.inner.read() else { - #[cfg(not(tarpaulin_include))] // don't test mutex lock failure - return Err(git::fetch::Error::Lock); + let origin = repo_details.origin(); + let command: secrecy::Secret = + format!("/usr/bin/git fetch {}", origin.expose_secret()).into(); + let git_dir = self + .inner + .read() + .map_err(|_| git::fetch::Error::Lock) + .map(|r| r.git_dir().to_path_buf())?; + let ctx = gix::diff::command::Context { + git_dir: Some(git_dir), + ..Default::default() }; - let thread_local = repository.to_thread_local(); - let Some(Ok(remote)) = - thread_local.find_default_remote(git::repository::Direction::Fetch.into()) - else { - #[cfg(not(tarpaulin_include))] // test is on local repo - should always have remotes - return Err(git::fetch::Error::NoFetchRemoteFound); - }; - remote - .connect(gix::remote::Direction::Fetch) - .map_err(|gix| git::fetch::Error::Connect(gix.to_string()))? - .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, &AtomicBool::default()) - .map_err(|gix| git::fetch::Error::Receive(gix.to_string()))?; + gix::command::prepare(command.expose_secret()) + .with_context(ctx) + .with_shell_allow_argument_splitting() + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn()? + .wait()?; 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( diff --git a/crates/core/src/git/repository/open/otest.rs b/crates/core/src/git/repository/open/otest.rs index dd4ac19..53b1514 100644 --- a/crates/core/src/git/repository/open/otest.rs +++ b/crates/core/src/git/repository/open/otest.rs @@ -94,7 +94,7 @@ impl git::repository::OpenRepositoryLike for TestOpenRepository { self.real.find_default_remote(direction) } - fn fetch(&self) -> Result<(), git::fetch::Error> { + fn fetch(&self, _repo_details: &git::RepoDetails) -> Result<(), git::fetch::Error> { let i: usize = *self .fetch_counter .read() diff --git a/crates/core/src/git/repository/open/tests/fetch.rs b/crates/core/src/git/repository/open/tests/fetch.rs index 6eeed67..a6f4745 100644 --- a/crates/core/src/git/repository/open/tests/fetch.rs +++ b/crates/core/src/git/repository/open/tests/fetch.rs @@ -9,5 +9,5 @@ fn should_fetch_from_repo() { let gitdir = GitDir::new(cwd.join("../.."), StoragePathType::External); let repo_details = given::repo_details(&given::a_filesystem()).with_gitdir(gitdir); let_assert!(Ok(repo) = crate::git::repository::factory::real().open(&repo_details)); - let_assert!(Ok(()) = repo.fetch()); + let_assert!(Ok(()) = repo.fetch(&repo_details)); } diff --git a/crates/core/src/git/tests.rs b/crates/core/src/git/tests.rs index 08760a2..2a4939f 100644 --- a/crates/core/src/git/tests.rs +++ b/crates/core/src/git/tests.rs @@ -125,7 +125,7 @@ mod push { .expect_fetch() .times(1) .in_sequence(&mut seq) - .returning(|| Ok(())); + .returning(|_| Ok(())); open_repository .expect_push() .times(1) diff --git a/crates/core/src/git/validation/positions.rs b/crates/core/src/git/validation/positions.rs index 3c3411c..bb278b8 100644 --- a/crates/core/src/git/validation/positions.rs +++ b/crates/core/src/git/validation/positions.rs @@ -33,7 +33,7 @@ pub fn validate( let next_branch = repo_config.branches().next(); let dev_branch = repo_config.branches().dev(); // Collect Commit Histories for `main`, `next` and `dev` branches - open_repository.fetch()?; + open_repository.fetch(repo_details)?; let git_log = git::graph::log(repo_details); let commit_histories = get_commit_histories(open_repository, repo_config)?; diff --git a/crates/core/src/git/validation/tests.rs b/crates/core/src/git/validation/tests.rs index 12d5fac..1d97611 100644 --- a/crates/core/src/git/validation/tests.rs +++ b/crates/core/src/git/validation/tests.rs @@ -64,7 +64,7 @@ mod positions { let mut mock_open_repository = git::repository::open::mock(); mock_open_repository .expect_fetch() - .return_once(|| Err(git::fetch::Error::TestFailureExpected)); + .return_once(|_| Err(git::fetch::Error::TestFailureExpected)); let mut repository_factory = git::repository::factory::mock(); repository_factory .expect_open() @@ -93,7 +93,7 @@ mod positions { let repo_config = given::a_repo_config(); let main_branch = repo_config.branches().main(); let mut mock_open_repository = git::repository::open::mock(); - mock_open_repository.expect_fetch().return_once(|| Ok(())); + mock_open_repository.expect_fetch().return_once(|_| Ok(())); mock_open_repository .expect_commit_log() .returning(move |branch_name, _| { @@ -132,7 +132,7 @@ mod positions { let repo_config = given::a_repo_config(); let next_branch = repo_config.branches().next(); let mut mock_open_repository = git::repository::open::mock(); - mock_open_repository.expect_fetch().return_once(|| Ok(())); + mock_open_repository.expect_fetch().return_once(|_| Ok(())); mock_open_repository .expect_commit_log() .returning(move |branch_name, _| { @@ -171,7 +171,7 @@ mod positions { let repo_config = given::a_repo_config(); let dev_branch = repo_config.branches().dev(); let mut mock_open_repository = git::repository::open::mock(); - mock_open_repository.expect_fetch().return_once(|| Ok(())); + mock_open_repository.expect_fetch().return_once(|_| Ok(())); mock_open_repository .expect_commit_log() .returning(move |branch_name, _| {