cli: Refactor out get_fetch_remotes into git_util.rs

Similarly to `git_fetch`, this will be used in `jj git sync`, so
moving this out to maintain the same behaviour across `fetch` command and `sync`.

* Refactor out `get_fetch_remotes` to `git_util.rs`.
* inline the different `get_*_remote*` functions into `get_fetch_remotes`;
  The resulting function is still small enough to be easily readable.
* Move `get_single_remote` into `git_util.rs` and update call sites.
* Use common FetchArgs to pass arguments both to `get_fetch_remotes` and `git_fetch`
* Update use references

All tests still pass.

Issue: #1039

cli/src/commands/git/fetch.rs

@@ -14,15 +14,13 @@
 
 use itertools::Itertools;
 use jj_lib::repo::Repo;
-use jj_lib::settings::ConfigResultExt as _;
-use jj_lib::settings::UserSettings;
-use jj_lib::str_util::StringPattern;
 
 use crate::cli_util::CommandHelper;
 use crate::command_error::CommandError;
-use crate::commands::git::get_single_remote;
+use crate::git_util::get_fetch_remotes;
 use crate::git_util::get_git_repo;
 use crate::git_util::git_fetch;
+use crate::git_util::FetchArgs;
 use crate::ui::Ui;
 
 /// Fetch from a Git remote
@@ -31,19 +29,8 @@ use crate::ui::Ui;
 /// commit. This is true in general; it is not specific to this command.
 #[derive(clap::Args, Clone, Debug)]
 pub struct GitFetchArgs {
-    /// Fetch only some of the branches
-    ///
-    /// By default, the specified name matches exactly. Use `glob:` prefix to
-    /// expand `*` as a glob. The other wildcard characters aren't supported.
-    #[arg(long, short, alias="bookmark", default_value = "glob:*", value_parser = StringPattern::parse)]
-    branch: Vec<StringPattern>,
-    /// The remote to fetch from (only named remotes are supported, can be
-    /// repeated)
-    #[arg(long = "remote", value_name = "REMOTE")]
-    remotes: Vec<String>,
-    /// Fetch from all remotes
-    #[arg(long, conflicts_with = "remotes")]
-    all_remotes: bool,
+    #[command(flatten)]
+    fetch: FetchArgs,
 }
 
 #[tracing::instrument(skip(ui, command))]
@@ -54,52 +41,22 @@ pub fn cmd_git_fetch(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let git_repo = get_git_repo(workspace_command.repo().store())?;
-    let remotes = if args.all_remotes {
-        get_all_remotes(&git_repo)?
-    } else if args.remotes.is_empty() {
-        get_default_fetch_remotes(ui, command.settings(), &git_repo)?
-    } else {
-        args.remotes.clone()
-    };
+
+    let remotes = get_fetch_remotes(ui, command.settings(), &git_repo, &args.fetch)?;
     let mut tx = workspace_command.start_transaction();
-    git_fetch(ui, &mut tx, &git_repo, &remotes, &args.branch)?;
+    git_fetch(
+        ui,
+        &mut tx,
+        &git_repo,
+        &FetchArgs {
+            branch: args.fetch.branch.clone(),
+            remotes: remotes.clone(),
+            all_remotes: args.fetch.all_remotes,
+        },
+    )?;
     tx.finish(
         ui,
         format!("fetch from git remote(s) {}", remotes.iter().join(",")),
     )?;
     Ok(())
 }
-
-const DEFAULT_REMOTE: &str = "origin";
-
-fn get_default_fetch_remotes(
-    ui: &Ui,
-    settings: &UserSettings,
-    git_repo: &git2::Repository,
-) -> Result<Vec<String>, CommandError> {
-    const KEY: &str = "git.fetch";
-    if let Ok(remotes) = settings.config().get(KEY) {
-        Ok(remotes)
-    } else if let Some(remote) = settings.config().get_string(KEY).optional()? {
-        Ok(vec![remote])
-    } else if let Some(remote) = get_single_remote(git_repo)? {
-        // if nothing was explicitly configured, try to guess
-        if remote != DEFAULT_REMOTE {
-            writeln!(
-                ui.hint_default(),
-                "Fetching from the only existing remote: {remote}"
-            )?;
-        }
-        Ok(vec![remote])
-    } else {
-        Ok(vec![DEFAULT_REMOTE.to_owned()])
-    }
-}
-
-fn get_all_remotes(git_repo: &git2::Repository) -> Result<Vec<String>, CommandError> {
-    let git_remotes = git_repo.remotes()?;
-    Ok(git_remotes
-        .iter()
-        .filter_map(|x| x.map(ToOwned::to_owned))
-        .collect())
-}

cli/src/commands/git/mod.rs

@@ -94,11 +94,3 @@ pub fn maybe_add_gitignore(workspace_command: &WorkspaceCommandHelper) -> Result
         Ok(())
     }
 }
-
-fn get_single_remote(git_repo: &git2::Repository) -> Result<Option<String>, CommandError> {
-    let git_remotes = git_repo.remotes()?;
-    Ok(match git_remotes.len() {
-        1 => git_remotes.get(0).map(ToOwned::to_owned),
-        _ => None,
-    })
-}

cli/src/commands/git/push.rs

@@ -45,9 +45,9 @@ use crate::cli_util::WorkspaceCommandTransaction;
 use crate::command_error::user_error;
 use crate::command_error::user_error_with_hint;
 use crate::command_error::CommandError;
-use crate::commands::git::get_single_remote;
 use crate::formatter::Formatter;
 use crate::git_util::get_git_repo;
+use crate::git_util::get_single_remote;
 use crate::git_util::map_git_error;
 use crate::git_util::with_remote_git_callbacks;
 use crate::git_util::GitSidebandProgressMessageWriter;

cli/src/git_util.rs

@@ -35,6 +35,8 @@ use jj_lib::op_store::RefTarget;
 use jj_lib::op_store::RemoteRef;
 use jj_lib::repo::ReadonlyRepo;
 use jj_lib::repo::Repo;
+use jj_lib::settings::ConfigResultExt as _;
+use jj_lib::settings::UserSettings;
 use jj_lib::store::Store;
 use jj_lib::str_util::StringPattern;
 use jj_lib::workspace::Workspace;
@@ -66,6 +68,14 @@ pub fn map_git_error(err: git2::Error) -> CommandError {
     }
 }
 
+pub fn get_single_remote(git_repo: &git2::Repository) -> Result<Option<String>, CommandError> {
+    let git_remotes = git_repo.remotes()?;
+    Ok(match git_remotes.len() {
+        1 => git_remotes.get(0).map(ToOwned::to_owned),
+        _ => None,
+    })
+}
+
 pub fn get_git_repo(store: &Store) -> Result<git2::Repository, CommandError> {
     match store.backend_impl().downcast_ref::<GitBackend>() {
         None => Err(user_error("The repo is not backed by a git repo")),
@@ -456,30 +466,47 @@ export or their "parent" bookmarks."#,
     Ok(())
 }
 
+#[derive(clap::Args, Clone, Debug)]
+pub struct FetchArgs {
+    /// Fetch only some of the branches
+    ///
+    /// By default, the specified name matches exactly. Use `glob:` prefix to
+    /// expand `*` as a glob. The other wildcard characters aren't supported.
+    #[arg(long, short, alias="bookmark", default_value = "glob:*", value_parser = StringPattern::parse)]
+    pub branch: Vec<StringPattern>,
+    /// The remote to fetch from (only named remotes are supported, can be
+    /// repeated)
+    #[arg(long = "remote", value_name = "REMOTE")]
+    pub remotes: Vec<String>,
+    /// Fetch from all remotes
+    #[arg(long, conflicts_with = "remotes")]
+    pub all_remotes: bool,
+}
+
 pub fn git_fetch(
     ui: &mut Ui,
     tx: &mut WorkspaceCommandTransaction,
-    git_repo: &git2::Repository,
-    remotes: &[String],
-    branch: &[StringPattern],
+    repo: &git2::Repository,
+    args: &FetchArgs,
 ) -> Result<(), CommandError> {
     let git_settings = tx.settings().git_settings();
 
-    for remote in remotes {
+    for remote in &args.remotes {
         let stats = with_remote_git_callbacks(ui, None, |cb| {
             git::fetch(
                 tx.repo_mut(),
-                git_repo,
+                repo,
                 remote,
-                branch,
+                &args.branch,
                 cb,
                 &git_settings,
                 None,
             )
         })
         .map_err(|err| match err {
             GitFetchError::InvalidBranchPattern => {
-                if branch
+                if args
+                    .branch
                     .iter()
                     .any(|pattern| pattern.as_exact().map_or(false, |s| s.contains('*')))
                 {
@@ -500,8 +527,8 @@ pub fn git_fetch(
     warn_if_branches_not_found(
         ui,
         tx,
-        branch,
-        &remotes.iter().map(StringPattern::exact).collect_vec(),
+        &args.branch,
+        &args.remotes.iter().map(StringPattern::exact).collect_vec(),
     )
 }
 
@@ -535,3 +562,39 @@ fn warn_if_branches_not_found(
 
     Ok(())
 }
+
+const DEFAULT_REMOTE: &str = "origin";
+pub fn get_fetch_remotes(
+    ui: &Ui,
+    settings: &UserSettings,
+    repo: &git2::Repository,
+    args: &FetchArgs,
+) -> Result<Vec<String>, CommandError> {
+    if args.all_remotes {
+        Ok(repo
+            .remotes()?
+            .iter()
+            .filter_map(|x| x.map(ToOwned::to_owned))
+            .collect())
+    } else if !&args.remotes.is_empty() {
+        Ok(args.remotes.clone())
+    } else {
+        const KEY: &str = "git.fetch";
+        if let Ok(remotes) = settings.config().get(KEY) {
+            Ok(remotes)
+        } else if let Some(remote) = settings.config().get_string(KEY).optional()? {
+            Ok(vec![remote])
+        } else if let Some(remote) = get_single_remote(repo)? {
+            // if nothing was explicitly configured, try to guess
+            if remote != DEFAULT_REMOTE {
+                writeln!(
+                    ui.hint_default(),
+                    "Fetching from the only existing remote: {remote}"
+                )?;
+            }
+            Ok(vec![remote])
+        } else {
+            Ok(vec![DEFAULT_REMOTE.to_owned()])
+        }
+    }
+}