From a846163c71f9a7847fe0861b882469098c827a8f Mon Sep 17 00:00:00 2001 From: Essien Ita Essien <34972+essiene@users.noreply.github.com> Date: Sat, 23 Nov 2024 21:04:47 +0000 Subject: [PATCH] GitFetch API: Refactor `jj git clone` to use new GitFetch api directly. * Make the new `GitFetch` api public. * Rewrite all tests that used `git::fetch*` to use the new API. One interesting artifact of the new API is that it holds onto a reference of the `tx.repo_mut()`, so the GitFetch object needs to fall go of scope completely before we can make any other mutable calls to `tx`. * Delete the `git::fetch_and_get_default_branch` --- cli/src/commands/git/clone.rs | 44 +++--- lib/src/git.rs | 57 +------- lib/tests/test_git.rs | 262 +++++++++++++++++++--------------- 3 files changed, 175 insertions(+), 188 deletions(-) diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 66c2102617..bcac4e49ad 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -20,6 +20,7 @@ use std::path::PathBuf; use jj_lib::config::ConfigNamePathBuf; use jj_lib::git; +use jj_lib::git::GitFetch; use jj_lib::git::GitFetchError; use jj_lib::git::GitFetchStats; use jj_lib::repo::Repo; @@ -220,27 +221,34 @@ fn do_git_clone( git_repo.remote(remote_name, source).unwrap(); let mut fetch_tx = workspace_command.start_transaction(); - let stats = with_remote_git_callbacks(ui, None, |cb| { - git::fetch_and_get_default_branch( + let stats = with_remote_git_callbacks(ui, None, |cb| -> Result { + let git_settings = command.settings().git_settings(); + let mut git_fetch = GitFetch::new( fetch_tx.repo_mut(), &git_repo, - remote_name, - &[StringPattern::everything()], - cb, - &command.settings().git_settings(), - depth, - ) - }) - .map_err(|err| match err { - GitFetchError::NoSuchRemote(_) => { - panic!("shouldn't happen as we just created the git remote") - } - GitFetchError::GitImportError(err) => CommandError::from(err), - GitFetchError::InternalGitError(err) => map_git_error(err), - GitFetchError::InvalidBranchPattern => { - unreachable!("we didn't provide any globs") - } + &git_settings, + git::fetch_options(cb, depth), + ); + + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], remote_name) + .map_err(|err| match err { + GitFetchError::NoSuchRemote(_) => { + panic!("shouldn't happen as we just created the git remote") + } + GitFetchError::GitImportError(err) => CommandError::from(err), + GitFetchError::InternalGitError(err) => map_git_error(err), + GitFetchError::InvalidBranchPattern => { + unreachable!("we didn't provide any globs") + } + })?; + let import_stats = git_fetch.import_refs()?; + Ok(GitFetchStats { + default_branch, + import_stats, + }) })?; + print_git_import_stats(ui, fetch_tx.repo(), &stats.import_stats, true)?; fetch_tx.finish(ui, "fetch from git remote into empty repo")?; Ok((workspace_command, stats)) diff --git a/lib/src/git.rs b/lib/src/git.rs index 6610abd64c..48aa412e30 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1229,7 +1229,7 @@ pub enum GitFetchError { InternalGitError(#[from] git2::Error), } -fn fetch_options( +pub fn fetch_options( callbacks: RemoteCallbacks<'_>, depth: Option, ) -> git2::FetchOptions<'_> { @@ -1251,7 +1251,7 @@ struct FetchedBranches { remote: String, } -struct GitFetch<'a> { +pub struct GitFetch<'a> { mut_repo: &'a mut MutableRepo, git_repo: &'a git2::Repository, git_settings: &'a GitSettings, @@ -1260,7 +1260,7 @@ struct GitFetch<'a> { } impl<'a> GitFetch<'a> { - fn new( + pub fn new( mut_repo: &'a mut MutableRepo, git_repo: &'a git2::Repository, git_settings: &'a GitSettings, @@ -1275,7 +1275,7 @@ impl<'a> GitFetch<'a> { } } - fn fetch( + pub fn fetch( &mut self, branch_names: &[StringPattern], remote_name: &str, @@ -1378,55 +1378,6 @@ pub struct GitFetchStats { pub import_stats: GitImportStats, } -#[tracing::instrument(skip(mut_repo, git_repo, callbacks))] -pub fn fetch_and_get_default_branch( - mut_repo: &mut MutableRepo, - git_repo: &git2::Repository, - remote_name: &str, - branch_names: &[StringPattern], - callbacks: RemoteCallbacks<'_>, - git_settings: &GitSettings, - depth: Option, -) -> Result { - let mut git_fetch = GitFetch::new( - mut_repo, - git_repo, - git_settings, - fetch_options(callbacks, depth), - ); - let default_branch = git_fetch.fetch(branch_names, remote_name)?; - let import_stats = git_fetch.import_refs()?; - let stats = GitFetchStats { - default_branch, - import_stats, - }; - Ok(stats) -} - -pub fn fetch( - mut_repo: &mut MutableRepo, - git_repo: &git2::Repository, - remote_name: &str, - branch_names: &[StringPattern], - callbacks: RemoteCallbacks<'_>, - git_settings: &GitSettings, - depth: Option, -) -> Result { - let mut git_fetch = GitFetch::new( - mut_repo, - git_repo, - git_settings, - fetch_options(callbacks, depth), - ); - git_fetch.fetch(branch_names, remote_name)?; - let import_stats = git_fetch.import_refs()?; - let stats = GitFetchStats { - default_branch: None, - import_stats, - }; - Ok(stats) -} - #[derive(Error, Debug, PartialEq)] pub enum GitPushError { #[error("No git remote named '{0}'")] diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 81b50d37ed..c38bd1a68e 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -38,6 +38,7 @@ use jj_lib::commit_builder::CommitBuilder; use jj_lib::git; use jj_lib::git::FailedRefExportReason; use jj_lib::git::GitBranchPushTargets; +use jj_lib::git::GitFetch; use jj_lib::git::GitFetchError; use jj_lib::git::GitImportError; use jj_lib::git::GitPushError; @@ -2262,19 +2263,24 @@ fn test_clone_empty_repo() { let git_settings = GitSettings::default(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch_and_get_default_branch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); - // No default bookmark and no refs - assert_eq!(stats.default_branch, None); - assert!(stats.import_stats.abandoned_commits.is_empty()); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + let import_stats = git_fetch.import_refs().unwrap(); + + // No default bookmark and no refs + assert_eq!(default_branch, None); + assert!(import_stats.abandoned_commits.is_empty()); + }; + assert_eq!(*tx.repo_mut().view().git_refs(), btreemap! {}); assert_eq!(tx.repo_mut().view().bookmarks().count(), 0); } @@ -2289,21 +2295,26 @@ fn test_clone_initial_commit_head_is_not_set() { let initial_git_commit = empty_git_commit(&test_data.origin_repo, "refs/heads/main", &[]); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch_and_get_default_branch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); - // No default bookmark because the origin repo's HEAD wasn't set - assert_eq!(stats.default_branch, None); - assert!(stats.import_stats.abandoned_commits.is_empty()); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + let import_stats = git_fetch.import_refs().unwrap(); + + // No default bookmark because the origin repo's HEAD wasn't set + assert_eq!(default_branch, None); + assert!(import_stats.abandoned_commits.is_empty()); + } + let repo = tx.commit("test").unwrap(); - // The initial commit is visible after git::fetch(). + // The initial commit is visible after git_fetch.fetch(). let view = repo.view(); assert!(view.heads().contains(&jj_id(&initial_git_commit))); let initial_commit_target = RefTarget::normal(jj_id(&initial_git_commit)); @@ -2350,20 +2361,25 @@ fn test_fetch_initial_commit_head_is_set() { .unwrap(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch_and_get_default_branch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); - assert_eq!(stats.default_branch, Some("main".to_string())); - assert!(stats.import_stats.abandoned_commits.is_empty()); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + let import_stats = git_fetch.import_refs().unwrap(); + + assert_eq!(default_branch, Some("main".to_string())); + assert!(import_stats.abandoned_commits.is_empty()); + } + let repo = tx.commit("test").unwrap(); - // The new commit visible after git::fetch_and_get_default_branch(). + // The new commit visible after git_fetch.fetch(). let view = repo.view(); assert!(view.heads().contains(&jj_id(&new_git_commit))); let commit_target = RefTarget::normal(jj_id(&new_git_commit)); @@ -2402,16 +2418,20 @@ fn test_fetch_success() { let initial_git_commit = empty_git_commit(&test_data.origin_repo, "refs/heads/main", &[]); let mut tx = test_data.repo.start_transaction(&test_data.settings); - git::fetch_and_get_default_branch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + git_fetch.import_refs().unwrap(); + } + test_data.repo = tx.commit("test").unwrap(); test_data.origin_repo.set_head("refs/heads/main").unwrap(); @@ -2426,19 +2446,23 @@ fn test_fetch_success() { .unwrap(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); - // git::fetch doesn't return a default branch - assert_eq!(stats.default_branch, None); - assert!(stats.import_stats.abandoned_commits.is_empty()); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + let import_stats = git_fetch.import_refs().unwrap(); + + assert_eq!(default_branch, Some("main".to_string())); + assert!(import_stats.abandoned_commits.is_empty()); + } + let repo = tx.commit("test").unwrap(); // The new commit is visible after we fetch again let view = repo.view(); @@ -2484,16 +2508,20 @@ fn test_fetch_prune_deleted_ref() { let commit = empty_git_commit(&test_data.origin_repo, "refs/heads/main", &[]); let mut tx = test_data.repo.start_transaction(&test_data.settings); - git::fetch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + git_fetch.import_refs().unwrap(); + } + // Test the setup assert!(tx.repo_mut().get_local_bookmark("main").is_present()); assert!(tx @@ -2508,17 +2536,22 @@ fn test_fetch_prune_deleted_ref() { .delete() .unwrap(); // After re-fetching, the bookmark should be deleted - let stats = git::fetch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); - assert_eq!(stats.import_stats.abandoned_commits, vec![jj_id(&commit)]); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + let import_stats = git_fetch.import_refs().unwrap(); + + assert_eq!(import_stats.abandoned_commits, vec![jj_id(&commit)]); + } + assert!(tx.repo_mut().get_local_bookmark("main").is_absent()); assert!(tx .repo_mut() @@ -2536,16 +2569,17 @@ fn test_fetch_no_default_branch() { let initial_git_commit = empty_git_commit(&test_data.origin_repo, "refs/heads/main", &[]); let mut tx = test_data.repo.start_transaction(&test_data.settings); - git::fetch( + let mut git_fetch = GitFetch::new( tx.repo_mut(), &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), &git_settings, - None, - ) - .unwrap(); + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + git_fetch.import_refs().unwrap(); empty_git_commit( &test_data.origin_repo, @@ -2560,18 +2594,12 @@ fn test_fetch_no_default_branch() { .set_head_detached(initial_git_commit.id()) .unwrap(); - let stats = git::fetch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); + let default_branch = git_fetch + .fetch(&[StringPattern::everything()], "origin") + .unwrap(); + git_fetch.import_refs().unwrap(); // There is no default bookmark - assert_eq!(stats.default_branch, None); + assert_eq!(default_branch, None); } #[test] @@ -2582,16 +2610,18 @@ fn test_fetch_empty_refspecs() { // Base refspecs shouldn't be respected let mut tx = test_data.repo.start_transaction(&test_data.settings); - git::fetch( - tx.repo_mut(), - &test_data.git_repo, - "origin", - &[], - git::RemoteCallbacks::default(), - &git_settings, - None, - ) - .unwrap(); + { + let mut git_fetch = GitFetch::new( + tx.repo_mut(), + &test_data.git_repo, + &git_settings, + git::fetch_options(git::RemoteCallbacks::default(), None), + ); + + git_fetch.fetch(&[], "origin").unwrap(); + git_fetch.import_refs().unwrap(); + } + assert!(tx .repo_mut() .get_remote_bookmark("main", "origin") @@ -2609,15 +2639,13 @@ fn test_fetch_no_such_remote() { let test_data = GitRepoData::create(); let git_settings = GitSettings::default(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let result = git::fetch( + let mut git_fetch = GitFetch::new( tx.repo_mut(), &test_data.git_repo, - "invalid-remote", - &[StringPattern::everything()], - git::RemoteCallbacks::default(), &git_settings, - None, + git::fetch_options(git::RemoteCallbacks::default(), None), ); + let result = git_fetch.fetch(&[StringPattern::everything()], "invalid-remote"); assert!(matches!(result, Err(GitFetchError::NoSuchRemote(_)))); }