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. * Update use references All tests still pass. Issue: #1039
jj-vcs · Nov 4, 2024 · 1591633 · 1591633
1 parent 4a4ac40
commit 1591633
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 61 deletions.
diff --git a/cli/src/commands/git/fetch.rs b/cli/src/commands/git/fetch.rs
@@ -14,13 +14,11 @@
 
 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::ui::Ui;
@@ -54,59 +52,13 @@ 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 settings = command.settings();
+    let remotes = get_fetch_remotes(ui, settings, &git_repo, &args.remotes, args.all_remotes)?;
     let mut tx = workspace_command.start_transaction();
-    git_fetch(
-        ui,
-        &mut tx,
-        command.settings(),
-        &git_repo,
-        &remotes,
-        &args.branch,
-    )?;
+    git_fetch(ui, &mut tx, settings, &git_repo, &remotes, &args.branch)?;
     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())
-}
diff --git a/cli/src/commands/git/mod.rs b/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,
-    })
-}
diff --git a/cli/src/commands/git/push.rs b/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;

diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs
@@ -35,6 +35,7 @@ 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;
@@ -67,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")),
@@ -535,3 +544,40 @@ fn warn_if_branches_not_found(
 
     Ok(())
 }
+
+const DEFAULT_REMOTE: &str = "origin";
+pub fn get_fetch_remotes(
+    ui: &Ui,
+    settings: &UserSettings,
+    git_repo: &git2::Repository,
+    remotes: &[String],
+    use_all_remotes: bool,
+) -> Result<Vec<String>, CommandError> {
+    if use_all_remotes {
+        Ok(git_repo
+            .remotes()?
+            .iter()
+            .filter_map(|x| x.map(ToOwned::to_owned))
+            .collect())
+    } else if !remotes.is_empty() {
+        Ok(remotes.iter().map(|s| s.clone()).collect_vec())
+    } 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(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()])
+        }
+    }
+}