diff --git a/CHANGELOG.md b/CHANGELOG.md index 36967fd893..0f40f39666 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj config unset ` no longer removes a table (such as `[ui]`.) +* `jj git fetch` with multiple remotes will now fetch from all remotes before + importing refs into the jj repo. This fixes a race condition where the treatment + of a commit that is found in multiple fetch remotes depended on the order the + remotes where specified. + ## [0.23.0] - 2024-11-06 ### Security fixes diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 57f0261089..d7a433e460 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -21,6 +21,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; @@ -221,27 +222,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( + 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(remote_name, &[StringPattern::everything()]) + .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/cli/src/git_util.rs b/cli/src/git_util.rs index 1edb7a6216..69777238d7 100644 --- a/cli/src/git_util.rs +++ b/cli/src/git_util.rs @@ -27,6 +27,7 @@ use itertools::Itertools; use jj_lib::git; use jj_lib::git::FailedRefExport; use jj_lib::git::FailedRefExportReason; +use jj_lib::git::GitFetch; use jj_lib::git::GitFetchError; use jj_lib::git::GitImportStats; use jj_lib::git::RefName; @@ -456,6 +457,9 @@ export or their "parent" bookmarks."#, Ok(()) } +// TODO: Move this back to cli/src/commands/git/fetch.rs +// With the new `GitFetch` api, this function is too specialized +// to the `jj git fetch` command and should not be reused. pub fn git_fetch( ui: &mut Ui, tx: &mut WorkspaceCommandTransaction, @@ -463,40 +467,39 @@ pub fn git_fetch( remotes: &[String], branch: &[StringPattern], ) -> Result<(), CommandError> { - let git_settings = tx.settings().git_settings(); - - for remote in remotes { - let stats = with_remote_git_callbacks(ui, None, |cb| { - git::fetch( + let import_stats = + with_remote_git_callbacks(ui, None, |cb| -> Result { + let git_settings = tx.settings().git_settings(); + let mut git_fetch = GitFetch::new( 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( - "Branch names may not include `*`.", - "Prefix the pattern with `glob:` to expand `*` as a glob", - ) - } else { - user_error(err) - } + git::fetch_options(cb, None), + ); + + for remote in remotes { + git_fetch.fetch(remote, branch).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( + "Branch names may not include `*`.", + "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), + })?; } - GitFetchError::GitImportError(err) => err.into(), - GitFetchError::InternalGitError(err) => map_git_error(err), - _ => user_error(err), + Ok(git_fetch.import_refs()?) })?; - print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?; - } + print_git_import_stats(ui, tx.repo(), &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 232ddae660..48468a8dad 100644 --- a/cli/tests/test_git_fetch.rs +++ b/cli/tests/test_git_fetch.rs @@ -226,6 +226,29 @@ fn test_git_fetch_multiple_remotes_from_config() { "###); } +#[test] +fn test_git_fetch_multiple_remotes_with_same_commit_no_race_condition() { + let test_env = TestEnvironment::default(); + test_env.add_config("git.auto-local-bookmark = true"); + + // Create first remote. + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + add_git_remote(&test_env, &repo_path, "rem1"); + add_git_remote(&test_env, &repo_path, "rem2"); + + test_env.jj_cmd_ok( + &repo_path, + &["git", "fetch", "--remote", "rem1", "--remote", "rem2"], + ); + insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @r###" + rem1: qxosxrvv 6a211027 message + @rem1: qxosxrvv 6a211027 message + rem2: yszkquru 2497a8a0 message + @rem2: yszkquru 2497a8a0 message + "###); +} + #[test] fn test_git_fetch_nonexistent_remote() { let test_env = TestEnvironment::default(); @@ -237,10 +260,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 +274,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 cce00162b6..0685094970 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<'_> { @@ -1247,11 +1247,11 @@ fn fetch_options( } struct FetchedBranches { - branches: Vec, remote: String, + branches: Vec, } -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, @@ -1280,10 +1280,10 @@ impl<'a> GitFetch<'a> { /// /// Keeps track of the {branch_names, remote_name} pair the refs can be /// subsequently imported into the `jj` repo by calling `import_refs()`. - fn fetch( + pub fn fetch( &mut self, - branch_names: &[StringPattern], remote_name: &str, + branch_names: &[StringPattern], ) -> Result, GitFetchError> { let mut remote = self.git_repo.find_remote(remote_name).map_err(|err| { if is_remote_not_found_err(&err) { @@ -1326,8 +1326,8 @@ impl<'a> GitFetch<'a> { )?; self.fetched.push(FetchedBranches { - branches: branch_names.to_vec(), remote: remote_name.to_string(), + branches: branch_names.to_vec(), }); // TODO: We could make it optional to get the default branch since we only care @@ -1394,31 +1394,6 @@ pub struct GitFetchStats { pub import_stats: GitImportStats, } -#[tracing::instrument(skip(mut_repo, git_repo, callbacks))] -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), - ); - 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) -} - #[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 7f773509f3..f6d55b3974 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -17,6 +17,7 @@ use std::collections::HashSet; use std::fs; use std::io::Write; use std::iter; +use std::num::NonZeroU32; use std::path::Path; use std::path::PathBuf; use std::sync::mpsc; @@ -38,7 +39,9 @@ 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::GitFetchStats; use jj_lib::git::GitImportError; use jj_lib::git::GitPushError; use jj_lib::git::GitRefUpdate; @@ -110,6 +113,30 @@ fn get_git_repo(repo: &Arc) -> git2::Repository { get_git_backend(repo).open_git_repo().unwrap() } +fn git_fetch( + mut_repo: &mut MutableRepo, + git_repo: &git2::Repository, + remote_name: &str, + branch_names: &[StringPattern], + callbacks: git::RemoteCallbacks<'_>, + git_settings: &GitSettings, + depth: Option, +) -> Result { + let mut git_fetch = GitFetch::new( + mut_repo, + git_repo, + git_settings, + git::fetch_options(callbacks, depth), + ); + let default_branch = git_fetch.fetch(remote_name, branch_names)?; + let import_stats = git_fetch.import_refs()?; + let stats = GitFetchStats { + default_branch, + import_stats, + }; + Ok(stats) +} + #[test] fn test_import_refs() { let settings = testutils::user_settings(); @@ -2262,7 +2289,7 @@ fn test_fetch_empty_repo() { let git_settings = GitSettings::default(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2289,7 +2316,7 @@ fn test_fetch_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( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2303,7 +2330,7 @@ fn test_fetch_initial_commit_head_is_not_set() { assert_eq!(stats.default_branch, None); assert!(stats.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(). 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,7 +2377,7 @@ fn test_fetch_initial_commit_head_is_set() { .unwrap(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2375,7 +2402,7 @@ 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( + git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2399,7 +2426,7 @@ fn test_fetch_success() { .unwrap(); let mut tx = test_data.repo.start_transaction(&test_data.settings); - let stats = git::fetch( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2457,7 +2484,7 @@ 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( + git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2481,7 +2508,7 @@ fn test_fetch_prune_deleted_ref() { .delete() .unwrap(); // After re-fetching, the bookmark should be deleted - let stats = git::fetch( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2509,7 +2536,7 @@ 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( + git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2533,7 +2560,7 @@ fn test_fetch_no_default_branch() { .set_head_detached(initial_git_commit.id()) .unwrap(); - let stats = git::fetch( + let stats = git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2555,7 +2582,7 @@ fn test_fetch_empty_refspecs() { // Base refspecs shouldn't be respected let mut tx = test_data.repo.start_transaction(&test_data.settings); - git::fetch( + git_fetch( tx.repo_mut(), &test_data.git_repo, "origin", @@ -2582,7 +2609,7 @@ 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 result = git_fetch( tx.repo_mut(), &test_data.git_repo, "invalid-remote",