From 82dd580c27bb16390f649b2fc9a5fb050f51c44a Mon Sep 17 00:00:00 2001 From: Kiril Videlov Date: Thu, 4 Jul 2024 16:34:53 +0200 Subject: [PATCH] move the updating of "last fetch timestamp" logic Removing the updating of project from the virtual branch controller - this is the only dependency between the two controllers and likely indicates that the "last fetched at" should live somewhere else. It will serve us well to not mutate the project state from the virtual branches domain --- .../src/virtual_branches/controller.rs | 31 +++------------ .../virtual_branches/fetch_from_remotes.rs | 39 ------------------- .../tests/suite/virtual_branches/init.rs | 2 +- .../tests/suite/virtual_branches/mod.rs | 3 +- crates/gitbutler-tauri/src/main.rs | 1 - .../gitbutler-tauri/src/virtual_branches.rs | 24 ++++++++++-- 6 files changed, 29 insertions(+), 71 deletions(-) delete mode 100644 crates/gitbutler-core/tests/suite/virtual_branches/fetch_from_remotes.rs diff --git a/crates/gitbutler-core/src/virtual_branches/controller.rs b/crates/gitbutler-core/src/virtual_branches/controller.rs index 4d8bf7144..143c3b7b4 100644 --- a/crates/gitbutler-core/src/virtual_branches/controller.rs +++ b/crates/gitbutler-core/src/virtual_branches/controller.rs @@ -1,18 +1,17 @@ use crate::{ git::BranchExt, ops::entry::{OperationKind, SnapshotDetails}, + projects::FetchResult, types::ReferenceName, }; use anyhow::Result; use std::{path::Path, sync::Arc}; -use anyhow::Context; use tokio::{sync::Semaphore, task::JoinHandle}; use super::{ branch::{BranchId, BranchOwnershipClaims}, - target, target_to_base_branch, BaseBranch, NameConflitResolution, RemoteBranchFile, - VirtualBranchesHandle, + target, BaseBranch, NameConflitResolution, RemoteBranchFile, VirtualBranchesHandle, }; use crate::{ git, project_repository, @@ -21,18 +20,16 @@ use crate::{ #[derive(Clone)] pub struct Controller { - projects: projects::Controller, helper: git::credentials::Helper, semaphore: Arc, } impl Controller { - pub fn new(projects: projects::Controller, helper: git::credentials::Helper) -> Self { + pub fn new(helper: git::credentials::Helper) -> Self { Self { semaphore: Arc::new(Semaphore::new(1)), - projects, helper, } } @@ -446,8 +443,8 @@ impl Controller { &self, project: &Project, askpass: Option, - ) -> Result { - let mut project_repository = project_repository::Repository::open(project)?; + ) -> Result { + let project_repository = project_repository::Repository::open(project)?; let remotes = project_repository.remotes()?; let fetch_results: Vec> = remotes @@ -481,23 +478,7 @@ impl Controller { tracing::warn!(?err, "fetch from push-remote failed"); } } - - let updated_project = self - .projects - .update(&projects::UpdateRequest { - id: project.id, - project_data_last_fetched: Some(project_data_last_fetched), - ..Default::default() - }) - .await - .context("failed to update project")?; - - project_repository.set_project(&updated_project); - - let base_branch = target_to_base_branch(&project_repository, &default_target) - .context("failed to convert target to base branch")?; - - Ok(base_branch) + Ok(project_data_last_fetched) } pub async fn move_commit( diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/fetch_from_remotes.rs b/crates/gitbutler-core/tests/suite/virtual_branches/fetch_from_remotes.rs deleted file mode 100644 index f1345855f..000000000 --- a/crates/gitbutler-core/tests/suite/virtual_branches/fetch_from_remotes.rs +++ /dev/null @@ -1,39 +0,0 @@ -use super::*; - -#[tokio::test] -async fn should_update_last_fetched() { - let Test { - project, - projects, - controller, - .. - } = &Test::default(); - - controller - .set_base_branch(project, &"refs/remotes/origin/master".parse().unwrap()) - .await - .unwrap(); - - let before_fetch = controller.get_base_branch_data(project).await.unwrap(); - assert!(before_fetch.last_fetched_ms.is_none()); - - let fetch = controller.fetch_from_remotes(project, None).await.unwrap(); - assert!(fetch.last_fetched_ms.is_some()); - - let project = &projects.get(project.id).unwrap(); - let after_fetch = controller.get_base_branch_data(project).await.unwrap(); - assert!(after_fetch.last_fetched_ms.is_some()); - assert_eq!(fetch.last_fetched_ms, after_fetch.last_fetched_ms); - - let second_fetch = controller.fetch_from_remotes(project, None).await.unwrap(); - assert!(second_fetch.last_fetched_ms.is_some()); - assert_ne!(fetch.last_fetched_ms, second_fetch.last_fetched_ms); - - let project = &projects.get(project.id).unwrap(); - let after_second_fetch = controller.get_base_branch_data(project).await.unwrap(); - assert!(after_second_fetch.last_fetched_ms.is_some()); - assert_eq!( - second_fetch.last_fetched_ms, - after_second_fetch.last_fetched_ms - ); -} diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs index 8c98f790d..6fc9ea156 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs @@ -8,7 +8,7 @@ async fn twice() { let test_project = TestProject::default(); - let controller = Controller::new(projects.clone(), helper); + let controller = Controller::new(helper); { let project = projects diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs index 3da9c00ca..fb67d772b 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs @@ -42,7 +42,7 @@ impl Default for Test { Self { repository: test_project, project_id: project.id, - controller: Controller::new(projects.clone(), helper), + controller: Controller::new(helper), projects, project, data_dir: Some(data_dir), @@ -66,7 +66,6 @@ mod convert_to_real_branch; mod create_commit; mod create_virtual_branch_from_branch; mod delete_virtual_branch; -mod fetch_from_remotes; mod init; mod insert_blank_commit; mod move_commit_file; diff --git a/crates/gitbutler-tauri/src/main.rs b/crates/gitbutler-tauri/src/main.rs index 264a8af52..b5d3ef195 100644 --- a/crates/gitbutler-tauri/src/main.rs +++ b/crates/gitbutler-tauri/src/main.rs @@ -138,7 +138,6 @@ fn main() { app_handle.manage(git_credentials_controller.clone()); app_handle.manage(gitbutler_core::virtual_branches::controller::Controller::new( - projects_controller.clone(), git_credentials_controller.clone(), )); diff --git a/crates/gitbutler-tauri/src/virtual_branches.rs b/crates/gitbutler-tauri/src/virtual_branches.rs index 36d9e4093..be2b53d69 100644 --- a/crates/gitbutler-tauri/src/virtual_branches.rs +++ b/crates/gitbutler-tauri/src/virtual_branches.rs @@ -485,15 +485,33 @@ pub mod commands { project_id: ProjectId, action: Option, ) -> Result { - let project = handle.state::().get(project_id)?; - let base_branch = handle + let projects = handle.state::(); + let project = projects.get(project_id)?; + + let project_data_last_fetched = handle .state::() .fetch_from_remotes( &project, Some(action.unwrap_or_else(|| "unknown".to_string())), ) .await?; - emit_vbranches(&handle, project_id).await; + + // Updates the project controller with the last fetched timestamp + // + // TODO: This cross dependency likely indicates that last_fetched is stored in the wrong place - value is coupled with virtual branches state + projects + .update(&projects::UpdateRequest { + id: project.id, + project_data_last_fetched: Some(project_data_last_fetched), + ..Default::default() + }) + .await + .context("failed to update project with last fetched timestamp")?; + + let base_branch = handle + .state::() + .get_base_branch_data(&project) + .await?; Ok(base_branch) }