lib: refactor git fetch ahead of future updates

cli/src/commands/git/clone.rs

@@ -21,6 +21,7 @@ use std::path::Path;
 use jj_lib::git;
 use jj_lib::git::FetchTagsOverride;
 use jj_lib::git::GitFetch;
+use jj_lib::git::expand_fetch_refspecs;
 use jj_lib::ref_name::RefNameBuf;
 use jj_lib::ref_name::RemoteName;
 use jj_lib::ref_name::RemoteNameBuf;
@@ -268,7 +269,7 @@ fn fetch_new_remote(
     with_remote_git_callbacks(ui, |cb| {
         git_fetch.fetch(
             remote_name,
-            &[StringPattern::everything()],
+            expand_fetch_refspecs(remote_name, vec![StringPattern::everything()])?,
             cb,
             depth,
             match fetch_tags {

cli/src/commands/git/fetch.rs

@@ -19,6 +19,7 @@ use itertools::Itertools as _;
 use jj_lib::config::ConfigGetResultExt as _;
 use jj_lib::git;
 use jj_lib::git::GitFetch;
+use jj_lib::git::expand_fetch_refspecs;
 use jj_lib::ref_name::RemoteName;
 use jj_lib::repo::Repo as _;
 use jj_lib::str_util::StringPattern;
@@ -147,7 +148,7 @@ pub fn cmd_git_fetch(
     };
 
     let mut tx = workspace_command.start_transaction();
-    do_git_fetch(ui, &mut tx, &branches_by_remote)?;
+    do_git_fetch(ui, &mut tx, branches_by_remote)?;
     warn_if_branches_not_found(ui, &tx, &args.branch, &remotes)?;
     tx.finish(
         ui,
@@ -196,7 +197,7 @@ fn parse_remote_pattern(remote: &str) -> Result<StringPattern, CommandError> {
 fn do_git_fetch(
     ui: &mut Ui,
     tx: &mut WorkspaceCommandTransaction,
-    branches_by_remote: &[(&RemoteName, Vec<StringPattern>)],
+    branches_by_remote: Vec<(&RemoteName, Vec<StringPattern>)>,
 ) -> Result<(), CommandError> {
     let git_settings = tx.settings().git_settings()?;
     let mut git_fetch = GitFetch::new(tx.repo_mut(), &git_settings)?;
@@ -207,7 +208,13 @@ fn do_git_fetch(
             continue;
         }
         with_remote_git_callbacks(ui, |callbacks| {
-            git_fetch.fetch(remote, branches, callbacks, None, None)
+            git_fetch.fetch(
+                remote,
+                expand_fetch_refspecs(remote, branches)?,
+                callbacks,
+                None,
+                None,
+            )
         })?;
     }
 

lib/src/git.rs

@@ -2108,11 +2108,18 @@ struct FetchedBranches {
     branches: Vec<StringPattern>,
 }
 
-fn expand_fetch_refspecs(
+/// Represents the refspecs to fetch from a remote
+pub struct ExpandedFetchRefSpecs {
+    expected_branch_names: Vec<StringPattern>,
+    refspecs: Vec<RefSpec>,
+}
+
+/// Expand a list of branch string patterns to refspecs to fetch
+pub fn expand_fetch_refspecs(
     remote: &RemoteName,
-    branch_names: &[StringPattern],
-) -> Result<Vec<RefSpec>, GitFetchError> {
-    branch_names
+    branch_names: Vec<StringPattern>,
+) -> Result<ExpandedFetchRefSpecs, GitFetchError> {
+    let refspecs = branch_names
         .iter()
         .map(|pattern| {
             pattern
@@ -2130,7 +2137,12 @@ fn expand_fetch_refspecs(
                 })
                 .ok_or_else(|| GitFetchError::InvalidBranchPattern(pattern.clone()))
         })
-        .collect()
+        .try_collect()?;
+
+    Ok(ExpandedFetchRefSpecs {
+        expected_branch_names: branch_names,
+        refspecs,
+    })
 }
 
 /// Helper struct to execute multiple `git fetch` operations
@@ -2169,7 +2181,10 @@ impl<'a> GitFetch<'a> {
     pub fn fetch(
         &mut self,
         remote_name: &RemoteName,
-        branch_names: &[StringPattern],
+        ExpandedFetchRefSpecs {
+            expected_branch_names,
+            refspecs: mut remaining_refspecs,
+        }: ExpandedFetchRefSpecs,
         mut callbacks: RemoteCallbacks<'_>,
         depth: Option<NonZeroU32>,
         fetch_tags_override: Option<FetchTagsOverride>,
@@ -2184,9 +2199,7 @@ impl<'a> GitFetch<'a> {
         {
             return Err(GitFetchError::NoSuchRemote(remote_name.to_owned()));
         }
-        // At this point, we are only updating Git's remote tracking branches, not the
-        // local branches.
-        let mut remaining_refspecs: Vec<_> = expand_fetch_refspecs(remote_name, branch_names)?;
+
         if remaining_refspecs.is_empty() {
             // Don't fall back to the base refspecs.
             return Ok(());
@@ -2224,7 +2237,7 @@ impl<'a> GitFetch<'a> {
 
         self.fetched.push(FetchedBranches {
             remote: remote_name.to_owned(),
-            branches: branch_names.to_vec(),
+            branches: expected_branch_names,
         });
         Ok(())
     }

lib/tests/test_git.rs

@@ -48,6 +48,7 @@ use jj_lib::git::GitPushStats;
 use jj_lib::git::GitRefKind;
 use jj_lib::git::GitRefUpdate;
 use jj_lib::git::GitResetHeadError;
+use jj_lib::git::expand_fetch_refspecs;
 use jj_lib::git_backend::GitBackend;
 use jj_lib::hex_util;
 use jj_lib::object_id::ObjectId as _;
@@ -139,14 +140,14 @@ fn get_git_repo(repo: &Arc<ReadonlyRepo>) -> gix::Repository {
 fn git_fetch(
     mut_repo: &mut MutableRepo,
     remote_name: &RemoteName,
-    branch_names: &[StringPattern],
+    branch_names: Vec<StringPattern>,
     git_settings: &GitSettings,
     fetch_tags_override: Option<FetchTagsOverride>,
 ) -> Result<GitFetchStats, GitFetchError> {
     let mut git_fetch = GitFetch::new(mut_repo, git_settings).unwrap();
     git_fetch.fetch(
         remote_name,
-        branch_names,
+        expand_fetch_refspecs(remote_name, branch_names)?,
         git::RemoteCallbacks::default(),
         None,
         fetch_tags_override,
@@ -2831,7 +2832,7 @@ fn test_fetch_empty_repo() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -2856,7 +2857,7 @@ fn test_fetch_initial_commit_head_is_not_set() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -2920,7 +2921,7 @@ fn test_fetch_initial_commit_head_is_set() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -2943,7 +2944,7 @@ fn test_fetch_success() {
     git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -2970,7 +2971,7 @@ fn test_fetch_success() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3026,7 +3027,7 @@ fn test_fetch_prune_deleted_ref() {
     git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3049,7 +3050,7 @@ fn test_fetch_prune_deleted_ref() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3076,7 +3077,7 @@ fn test_fetch_no_default_branch() {
     git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3095,7 +3096,7 @@ fn test_fetch_no_default_branch() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3112,7 +3113,14 @@ fn test_fetch_empty_refspecs() {
 
     // Base refspecs shouldn't be respected
     let mut tx = test_data.repo.start_transaction();
-    git_fetch(tx.repo_mut(), "origin".as_ref(), &[], &git_settings, None).unwrap();
+    git_fetch(
+        tx.repo_mut(),
+        "origin".as_ref(),
+        vec![],
+        &git_settings,
+        None,
+    )
+    .unwrap();
     assert!(
         tx.repo_mut()
             .get_remote_bookmark(remote_symbol("main", "origin"))
@@ -3135,7 +3143,7 @@ fn test_fetch_no_such_remote() {
     let result = git_fetch(
         tx.repo_mut(),
         "invalid-remote".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     );
@@ -3155,7 +3163,7 @@ fn test_fetch_multiple_branches() {
     let fetch_stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[
+        vec![
             StringPattern::Exact("main".to_string()),
             StringPattern::Exact("noexist1".to_string()),
             StringPattern::Exact("noexist2".to_string()),
@@ -3233,7 +3241,7 @@ fn test_fetch_with_fetch_tags_override() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )
@@ -3245,7 +3253,7 @@ fn test_fetch_with_fetch_tags_override() {
     let stats = git_fetch(
         tx.repo_mut(),
         "origin".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         Some(FetchTagsOverride::AllTags),
     )
@@ -3271,7 +3279,7 @@ fn test_fetch_with_fetch_tags_override() {
     let stats = git_fetch(
         tx.repo_mut(),
         "originAllTags".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         Some(FetchTagsOverride::NoTags),
     )
@@ -3283,7 +3291,7 @@ fn test_fetch_with_fetch_tags_override() {
     let stats = git_fetch(
         tx.repo_mut(),
         "originAllTags".as_ref(),
-        &[StringPattern::everything()],
+        vec![StringPattern::everything()],
         &git_settings,
         None,
     )