Skip to content

Commit

Permalink
cli: Refactor out get_fetch_remotes into git_util.rs
Browse files Browse the repository at this point in the history
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
  • Loading branch information
essiene committed Nov 23, 2024
1 parent d409a31 commit 60c695f
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 29 deletions.
34 changes: 23 additions & 11 deletions cli/src/commands/git/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -69,15 +68,28 @@ 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,
&FetchArgs {
branch: &args.branch,
remotes: &args.remotes,
all_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,
all_remotes: args.all_remotes,
},
)?;
tx.finish(
ui,
format!("fetch from git remote(s) {}", remotes.iter().join(",")),
Expand Down
8 changes: 0 additions & 8 deletions cli/src/commands/git/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}
2 changes: 1 addition & 1 deletion cli/src/commands/git/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
70 changes: 61 additions & 9 deletions cli/src/git_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")),
Expand Down Expand Up @@ -456,30 +466,36 @@ export or their "parent" bookmarks."#,
Ok(())
}

pub struct FetchArgs<'a> {
pub branch: &'a [StringPattern],
pub remotes: &'a [String],
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().is_some_and(|s| s.contains('*')))
{
Expand All @@ -500,8 +516,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(),
)
}

Expand Down Expand Up @@ -535,3 +551,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.to_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(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()])
}
}
}

0 comments on commit 60c695f

Please sign in to comment.