From 2af4a66fb85625d8452c0848997db1636dac220d 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] jj-lib: `git::fetch` takes multiple remotes. * Reimplement `git::fetch` in terms of the new lower-level `GitFetch` api, which allows more control over the stages of the fetch. * Make `git::fetch` to accept multiple remotes, fetch from them all, before calling `import_refs`. * Set `default_branch` to None in the `GitFetchStats` return value from `git::fetch`. * Update call sites to use new `git::clone` and `git::fetch` * Update tests: One important side-effect to note (as demonstrated in the changed cli test) is that when there are multiple remotes, all fetches must pass before `import_refs` starts; no branch imports if a fetch from any remote fails. This is WAI. Fixes: #4923 --- cli/src/git_util.rs | 60 +++++++++++++------------- cli/tests/test_git_fetch.rs | 10 +---- lib/src/git.rs | 85 ++++++------------------------------- lib/tests/test_git.rs | 20 ++++----- 4 files changed, 54 insertions(+), 121 deletions(-) diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs index 74ebe2d2fa7..d95caa49977 100644 --- a/cli/src/git_util.rs +++ b/cli/src/git_util.rs @@ -465,38 +465,36 @@ pub fn git_fetch( ) -> Result<(), CommandError> { let git_settings = tx.settings().git_settings(); - for remote in remotes { - let stats = with_remote_git_callbacks(ui, None, |cb| { - git::fetch( - tx.repo_mut(), - git_repo, - remote, - branch, - cb, - &git_settings, - None, - ) - }) - .map_err(|err| match err { - GitFetchError::InvalidBranchPattern => { - if branch - .iter() - .any(|pattern| pattern.as_exact().is_some_and(|s| s.contains('*'))) - { - user_error_with_hint( - err, - "Prefix the pattern with `glob:` to expand `*` as a glob", - ) - } else { - user_error(err) - } + let stats = with_remote_git_callbacks(ui, None, |cb| { + git::fetch( + tx.repo_mut(), + git_repo, + &remotes.iter().map(|s| s.as_str()).collect::>(), + branch, + cb, + &git_settings, + None, + ) + }) + .map_err(|err| match err { + GitFetchError::InvalidBranchPattern => { + if branch + .iter() + .any(|pattern| pattern.as_exact().is_some_and(|s| s.contains('*'))) + { + user_error_with_hint( + err, + "Prefix the pattern with `glob:` to expand `*` as a glob", + ) + } else { + user_error(err) } - GitFetchError::GitImportError(err) => err.into(), - GitFetchError::InternalGitError(err) => map_git_error(err), - _ => user_error(err), - })?; - print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?; - } + } + GitFetchError::GitImportError(err) => err.into(), + GitFetchError::InternalGitError(err) => map_git_error(err), + _ => user_error(err), + })?; + print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?; warn_if_branches_not_found( ui, tx, diff --git a/cli/tests/test_git_fetch.rs b/cli/tests/test_git_fetch.rs index e6c2f96accf..01643867938 100644 --- a/cli/tests/test_git_fetch.rs +++ b/cli/tests/test_git_fetch.rs @@ -237,10 +237,7 @@ fn test_git_fetch_nonexistent_remote() { &repo_path, &["git", "fetch", "--remote", "rem1", "--remote", "rem2"], ); - insta::assert_snapshot!(stderr, @r###" - bookmark: rem1@rem1 [new] untracked - Error: No git remote named 'rem2' - "###); + insta::assert_snapshot!(stderr, @"Error: No git remote named 'rem2'"); // No remote should have been fetched as part of the failing transaction insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @""); } @@ -254,10 +251,7 @@ fn test_git_fetch_nonexistent_remote_from_config() { test_env.add_config(r#"git.fetch = ["rem1", "rem2"]"#); let stderr = &test_env.jj_cmd_failure(&repo_path, &["git", "fetch"]); - insta::assert_snapshot!(stderr, @r###" - bookmark: rem1@rem1 [new] untracked - Error: No git remote named 'rem2' - "###); + insta::assert_snapshot!(stderr, @"Error: No git remote named 'rem2'"); // No remote should have been fetched as part of the failing transaction insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @""); } diff --git a/lib/src/git.rs b/lib/src/git.rs index cd5da0f652d..7ed85669dfc 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1383,88 +1383,29 @@ pub fn clone( Ok(stats) } +#[tracing::instrument(skip(mut_repo, git_repo, callbacks))] pub fn fetch( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, - remote_name: &str, + remote_names: &[&str], branch_names: &[StringPattern], callbacks: RemoteCallbacks<'_>, git_settings: &GitSettings, depth: Option, ) -> Result { - // Perform a `git fetch` on the local git repo, updating the remote-tracking - // branches in the git repo. - let mut remote = git_repo.find_remote(remote_name).map_err(|err| { - if is_remote_not_found_err(&err) { - GitFetchError::NoSuchRemote(remote_name.to_string()) - } else { - GitFetchError::InternalGitError(err) - } - })?; - let mut fetch_options = git2::FetchOptions::new(); - let mut proxy_options = git2::ProxyOptions::new(); - proxy_options.auto(); - fetch_options.proxy_options(proxy_options); - let callbacks = callbacks.into_git(); - fetch_options.remote_callbacks(callbacks); - if let Some(depth) = depth { - fetch_options.depth(depth.get().try_into().unwrap_or(i32::MAX)); - } - // At this point, we are only updating Git's remote tracking branches, not the - // local branches. - let refspecs: Vec<_> = branch_names - .iter() - .map(|pattern| { - pattern - .to_glob() - .filter(|glob| !glob.contains(INVALID_REFSPEC_CHARS)) - .map(|glob| format!("+refs/heads/{glob}:refs/remotes/{remote_name}/{glob}")) - }) - .collect::>() - .ok_or(GitFetchError::InvalidBranchPattern)?; - if refspecs.is_empty() { - // Don't fall back to the base refspecs. - let stats = GitFetchStats::default(); - return Ok(stats); - } - tracing::debug!("remote.download"); - remote.download(&refspecs, Some(&mut fetch_options))?; - tracing::debug!("remote.prune"); - remote.prune(None)?; - tracing::debug!("remote.update_tips"); - remote.update_tips( - None, - git2::RemoteUpdateFlags::empty(), - git2::AutotagOption::Unspecified, - None, - )?; - // TODO: We could make it optional to get the default branch since we only care - // about it on clone. - let mut default_branch = None; - if let Ok(default_ref_buf) = remote.default_branch() { - if let Some(default_ref) = default_ref_buf.as_str() { - // LocalBranch here is the local branch on the remote, so it's really the remote - // branch - if let Some(RefName::LocalBranch(branch_name)) = parse_git_ref(default_ref) { - tracing::debug!(default_branch = branch_name); - default_branch = Some(branch_name); - } - } + let mut git_fetch = GitFetch::new( + mut_repo, + git_repo, + git_settings, + GitFetch::fetch_options(callbacks, depth), + ); + + for remote_name in remote_names { + git_fetch.fetch(branch_names, remote_name)?; } - tracing::debug!("remote.disconnect"); - remote.disconnect()?; - - // Import the remote-tracking branches into the jj repo and update jj's - // local branches. We also import local tags since remote tags should have - // been merged by Git. - tracing::debug!("import_refs"); - let import_stats = import_some_refs(mut_repo, git_settings, |ref_name| { - to_remote_branch(ref_name, remote_name) - .map(|branch| branch_names.iter().any(|pattern| pattern.matches(branch))) - .unwrap_or_else(|| matches!(ref_name, RefName::Tag(_))) - })?; + let import_stats = git_fetch.import_refs(branch_names, remote_names)?; let stats = GitFetchStats { - default_branch, + default_branch: None, import_stats, }; Ok(stats) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index cf88728277b..6805f42979f 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2393,7 +2393,7 @@ fn test_clone_initial_commit_head_is_set() { } #[test] -fn test_fetch_success() { +fn test_clone_fetch_success() { let mut test_data = GitRepoData::create(); let git_settings = GitSettings { auto_local_bookmark: true, @@ -2429,15 +2429,15 @@ fn test_fetch_success() { let stats = git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings, None, ) .unwrap(); - // The default bookmark is "main" - assert_eq!(stats.default_branch, Some("main".to_string())); + // `git::fetch` returns `None` for default branch. + assert_eq!(stats.default_branch, None); assert!(stats.import_stats.abandoned_commits.is_empty()); let repo = tx.commit("test").unwrap(); // The new commit is visible after we fetch again @@ -2487,7 +2487,7 @@ fn test_fetch_prune_deleted_ref() { git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings, @@ -2511,7 +2511,7 @@ fn test_fetch_prune_deleted_ref() { let stats = git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings, @@ -2539,7 +2539,7 @@ fn test_fetch_no_default_branch() { git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings, @@ -2563,7 +2563,7 @@ fn test_fetch_no_default_branch() { let stats = git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings, @@ -2585,7 +2585,7 @@ fn test_fetch_empty_refspecs() { git::fetch( tx.repo_mut(), &test_data.git_repo, - "origin", + &["origin"], &[], git::RemoteCallbacks::default(), &git_settings, @@ -2612,7 +2612,7 @@ fn test_fetch_no_such_remote() { let result = git::fetch( tx.repo_mut(), &test_data.git_repo, - "invalid-remote", + &["invalid-remote"], &[StringPattern::everything()], git::RemoteCallbacks::default(), &git_settings,