git: update jj git clone|fetch to use new GitFetch api directly.

* Make the new `GitFetch` api public.
* Move `git::fetch` to `lib/tests/test_git.rs` as `git_fetch`, to minimize
  churn in the tests. Update test call sites to use `git_fetch`
* Delete the `git::fetch` from `lib/src/git.rs`.
* Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use
  the new api directly. Removing one redundant layer of indirection.
* This fixes #4920 as it first fetches from all remotes before `import_refs()`
  is called, so there is no race condition if the same commit is treated
  differently in different remotes specified in the same command.

Original commit by @essiene

CHANGELOG.md

@@ -132,6 +132,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed bugs
 
+* `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 were specified.
+
 * Fixed diff selection by external tools with `jj split`/`commit -i FILESETS`.
   [#5252](https://github.com/jj-vcs/jj/issues/5252)
 

cli/src/commands/git/clone.rs

@@ -19,8 +19,8 @@ use std::num::NonZeroU32;
 use std::path::Path;
 
 use jj_lib::git;
+use jj_lib::git::GitFetch;
 use jj_lib::git::GitFetchError;
-use jj_lib::git::GitFetchStats;
 use jj_lib::repo::Repo;
 use jj_lib::str_util::StringPattern;
 use jj_lib::workspace::Workspace;
@@ -118,8 +118,9 @@ pub fn cmd_git_clone(
 
     let clone_result = (|| -> Result<_, CommandError> {
         let mut workspace_command = init_workspace(ui, command, &canonical_wc_path, args.colocate)?;
-        let stats = fetch_new_remote(ui, &mut workspace_command, remote_name, &source, args.depth)?;
-        Ok((workspace_command, stats))
+        let default_branch =
+            fetch_new_remote(ui, &mut workspace_command, remote_name, &source, args.depth)?;
+        Ok((workspace_command, default_branch))
     })();
     if clone_result.is_err() {
         let clean_up_dirs = || -> io::Result<()> {
@@ -143,8 +144,8 @@ pub fn cmd_git_clone(
         }
     }
 
-    let (mut workspace_command, stats) = clone_result?;
-    if let Some(default_branch) = &stats.default_branch {
+    let (mut workspace_command, default_branch) = clone_result?;
+    if let Some(default_branch) = &default_branch {
         write_repository_level_trunk_alias(
             ui,
             workspace_command.repo_path(),
@@ -194,7 +195,7 @@ fn fetch_new_remote(
     remote_name: &str,
     source: &str,
     depth: Option<NonZeroU32>,
-) -> Result<GitFetchStats, CommandError> {
+) -> Result<Option<String>, CommandError> {
     let git_repo = get_git_repo(workspace_command.repo().store())?;
     git::add_remote(&git_repo, remote_name, source)?;
     writeln!(
@@ -204,29 +205,23 @@ fn fetch_new_remote(
     )?;
     let git_settings = workspace_command.settings().git_settings()?;
     let mut fetch_tx = workspace_command.start_transaction();
-    let stats = with_remote_git_callbacks(ui, None, &git_settings, |cb| {
-        git::fetch(
-            fetch_tx.repo_mut(),
-            &git_repo,
-            remote_name,
-            &[StringPattern::everything()],
-            cb,
-            &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::Subprocess(err) => user_error(err),
-        GitFetchError::InvalidBranchPattern => {
-            unreachable!("we didn't provide any globs")
-        }
+    let mut git_fetch = GitFetch::new(fetch_tx.repo_mut(), &git_repo, &git_settings);
+    let default_branch = with_remote_git_callbacks(ui, None, &git_settings, |cb| {
+        git_fetch
+            .fetch(remote_name, &[StringPattern::everything()], cb, depth)
+            .map_err(|err| match err {
+                GitFetchError::NoSuchRemote(_) => {
+                    panic!("shouldn't happen as we just created the git remote")
+                }
+                GitFetchError::InternalGitError(err) => map_git_error(err),
+                GitFetchError::Subprocess(err) => user_error(err),
+                GitFetchError::InvalidBranchPattern => {
+                    unreachable!("we didn't provide any globs")
+                }
+            })
     })?;
-    print_git_import_stats(ui, fetch_tx.repo(), &stats.import_stats, true)?;
+    let import_stats = git_fetch.import_refs()?;
+    print_git_import_stats(ui, fetch_tx.repo(), &import_stats, true)?;
     fetch_tx.finish(ui, "fetch from git remote into empty repo")?;
-    Ok(stats)
+    Ok(default_branch)
 }

cli/src/git_util.rs

@@ -33,6 +33,7 @@ use jj_lib::fmt_util::binary_prefix;
 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;
@@ -641,51 +642,47 @@ export or their "parent" bookmarks."#,
     Ok(())
 }
 
+// TODO: move to cli/src/commands/git/fetch
+// No other aprt of the code is using this
 pub fn git_fetch(
     ui: &mut Ui,
     tx: &mut WorkspaceCommandTransaction,
     git_repo: &git2::Repository,
     remotes: &[String],
-    branch: &[StringPattern],
+    branch_names: &[StringPattern],
 ) -> Result<(), CommandError> {
     let git_settings = tx.settings().git_settings()?;
-
-    for remote in remotes {
-        let stats = with_remote_git_callbacks(ui, None, &git_settings, |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(
-                        "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),
+    let mut git_fetch = GitFetch::new(tx.repo_mut(), git_repo, &git_settings);
+
+    for remote_name in remotes {
+        with_remote_git_callbacks(ui, None, &git_settings, |cb| {
+            git_fetch
+                .fetch(remote_name, branch_names, cb, None)
+                .map_err(|err| match err {
+                    GitFetchError::InvalidBranchPattern => {
+                        if branch_names
+                            .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::InternalGitError(err) => map_git_error(err),
+                    _ => user_error(err),
+                })
         })?;
-        print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
     }
+    let import_stats = git_fetch.import_refs()?;
+    print_git_import_stats(ui, tx.repo(), &import_stats, true)?;
     warn_if_branches_not_found(
         ui,
         tx,
-        branch,
+        branch_names,
         &remotes.iter().map(StringPattern::exact).collect_vec(),
     )
 }

cli/tests/test_git_fetch.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 use std::path::Path;
+use std::path::PathBuf;
 
 use test_case::test_case;
 
@@ -51,6 +52,33 @@ fn add_git_remote(test_env: &TestEnvironment, repo_path: &Path, remote: &str) {
     );
 }
 
+/// Clone a git repo from a source, add a commit on top
+fn clone_git_add_commit(test_env: &TestEnvironment, src_remote: &str, remote: &str) {
+    let src_repo_path = test_env.env_root().join(src_remote);
+    let git_repo_path = test_env.env_root().join(remote);
+    let git_repo = git2::Repository::clone(src_repo_path.to_str().unwrap(), git_repo_path).unwrap();
+    let signature =
+        git2::Signature::new("Some One", "[email protected]", &git2::Time::new(0, 0)).unwrap();
+    let mut tree_builder = git_repo.treebuilder(None).unwrap();
+    let file_oid = git_repo.blob(remote.as_bytes()).unwrap();
+    tree_builder
+        .insert("file", file_oid, git2::FileMode::Blob.into())
+        .unwrap();
+    let tree_oid = tree_builder.write().unwrap();
+    let tree = git_repo.find_tree(tree_oid).unwrap();
+    // our branch name is the same as the source branch name
+    git_repo
+        .commit(
+            Some(&format!("refs/heads/{src_remote}")),
+            &signature,
+            &signature,
+            "message",
+            &tree,
+            &[],
+        )
+        .unwrap();
+}
+
 fn get_bookmark_output(test_env: &TestEnvironment, repo_path: &Path) -> String {
     // --quiet to suppress deleted bookmarks hint
     test_env.jj_cmd_success(repo_path, &["bookmark", "list", "--all-remotes", "--quiet"])
@@ -300,10 +328,7 @@ fn test_git_fetch_nonexistent_remote(subprocess: bool) {
         &["git", "fetch", "--remote", "rem1", "--remote", "rem2"],
     );
     insta::allow_duplicates! {
-    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'");
     }
     insta::allow_duplicates! {
     // No remote should have been fetched as part of the failing transaction
@@ -325,11 +350,10 @@ fn test_git_fetch_nonexistent_remote_from_config(subprocess: bool) {
 
     let stderr = &test_env.jj_cmd_failure(&repo_path, &["git", "fetch"]);
     insta::allow_duplicates! {
-    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::allow_duplicates! {
     insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @"");
     }
 }
@@ -1755,3 +1779,47 @@ fn test_git_fetch_remote_only_bookmark(subprocess: bool) {
     "###);
     }
 }
+
+#[test_case(false; "use git2 for remote calls")]
+#[test_case(true; "spawn a git subprocess for remote calls")]
+fn test_git_fetch_multiple_remotes_same_commit(subprocess: bool) {
+    fn setup_env(subprocess: bool) -> (TestEnvironment, PathBuf) {
+        let test_env = TestEnvironment::default();
+        if subprocess {
+            test_env.set_up_git_subprocessing();
+        }
+        test_env.add_config("git.auto-local-bookmark = true");
+        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");
+        clone_git_add_commit(&test_env, "rem1", "rem2");
+        test_env.jj_cmd_ok(&repo_path, &["git", "remote", "add", "rem2", "../rem2"]);
+
+        (test_env, repo_path.clone())
+    }
+
+    // fetch with different orderings
+    let (test_env1, repo_path1) = setup_env(subprocess);
+    test_env1.jj_cmd_ok(
+        &repo_path1,
+        &["git", "fetch", "--remote", "rem1", "--remote", "rem2"],
+    );
+    let (test_env2, repo_path2) = setup_env(subprocess);
+    test_env2.jj_cmd_ok(
+        &repo_path2,
+        &["git", "fetch", "--remote", "rem2", "--remote", "rem1"],
+    );
+
+    let bookmark_output1 = get_bookmark_output(&test_env1, &repo_path1);
+    let bookmark_output2 = get_bookmark_output(&test_env2, &repo_path2);
+    assert_eq!(bookmark_output1, bookmark_output2);
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(bookmark_output1, @r"
+    rem1 (conflicted):
+      + qxosxrvv 6a211027 message
+      + yszkquru 2497a8a0 message
+      @rem1 (behind by 1 commits): qxosxrvv 6a211027 message
+      @rem2 (behind by 1 commits): yszkquru 2497a8a0 message
+    ");
+    }
+}