From 4b265659ab8720c516790c75bfae26f047e944b6 Mon Sep 17 00:00:00 2001 From: Essien Ita Essien <34972+essiene@users.noreply.github.com> Date: Mon, 4 Nov 2024 20:19:26 +0000 Subject: [PATCH] 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 | 64 ++++++++++--------------------- cli/src/commands/git/mod.rs | 8 ---- cli/src/commands/git/push.rs | 2 +- cli/src/git_util.rs | 71 ++++++++++++++++++++++++++++++----- 4 files changed, 82 insertions(+), 63 deletions(-) diff --git a/cli/src/commands/git/fetch.rs b/cli/src/commands/git/fetch.rs index 0be1df9b63..4b4d1fb7cf 100644 --- a/cli/src/commands/git/fetch.rs +++ b/cli/src/commands/git/fetch.rs @@ -15,16 +15,15 @@ use clap_complete::ArgValueCandidates; 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::complete; +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 @@ -70,52 +69,27 @@ 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.remotes, + args.all_remotes, + )?; 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.branch, + remotes: &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, CommandError> { - const KEY: &str = "git.fetch"; - if let Ok(remotes) = settings.get(KEY) { - Ok(remotes) - } else if let Some(remote) = settings.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, 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 index 1c7fee9818..3aa6105ec1 100644 --- 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, 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 index d3608bf86f..ba2899c86e 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -46,10 +46,10 @@ 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::complete; 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 index 1edb7a6216..e15701818b 100644 --- a/cli/src/git_util.rs +++ b/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, 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 { match store.backend_impl().downcast_ref::() { None => Err(user_error("The repo is not backed by a git repo")), @@ -456,22 +466,26 @@ export or their "parent" bookmarks."#, Ok(()) } +pub struct FetchArgs<'a> { + pub branch: &'a [StringPattern], + pub remotes: &'a [String], +} + 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, @@ -479,7 +493,8 @@ pub fn git_fetch( }) .map_err(|err| match err { GitFetchError::InvalidBranchPattern => { - if branch + if args + .branch .iter() .any(|pattern| pattern.as_exact().is_some_and(|s| s.contains('*'))) { @@ -500,8 +515,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 +550,41 @@ fn warn_if_branches_not_found( Ok(()) } + +const DEFAULT_REMOTE: &str = "origin"; + +pub fn get_fetch_remotes( + ui: &Ui, + settings: &UserSettings, + repo: &git2::Repository, + remotes: &[String], + use_all_remotes: bool, +) -> Result, CommandError> { + if use_all_remotes { + Ok(repo + .remotes()? + .iter() + .filter_map(|x| x.map(ToOwned::to_owned)) + .collect()) + } else if !remotes.is_empty() { + Ok(remotes.to_vec()) + } else { + const KEY: &str = "git.fetch"; + if let Ok(remotes) = settings.get(KEY) { + Ok(remotes) + } else if let Some(remote) = settings.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()]) + } + } +}