Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: Refactor out get_fetch_remotes into git_util.rs #4768

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cli: Refactor out get_fetch_remotes into git_util.rs #4768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 19 additions & 43 deletions cli/src/commands/git/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ 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 @@ -70,52 +71,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,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we can't use the original args.fetch? That makes args.fetch useless..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe FetchRemoteArgs can be extracted instead of all-in FetchArgs, but I'm not pretty sure if that makes sense. Sorry for the noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries at all, I may have jumped the gun on the Refactor before finalizing how it will be used. For sync it turns out we need to fetch all branches for all configured remotes[1], so the help text will be different after all, and I don't need to flatten them in. I've reverted that last change, back to just passing FetchArgs{} around with branch, remotes and all_remotes as the fields.

[1] - https://docs.google.com/document/d/1K4j1xQPFxtINhyOyKuCMTZ_iNrw8g-5_giBFMGoLjaI/edit?tab=t.0#bookmark=id.y91cr92o9czf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not add branch to the args struct then. It doesn't make sense to pass branch in to get_fetch_remotes(), and all_remotes to git_fetch().

It might also be simpler to just make both get_all_remotes() and get_default_fetch_remotes() public.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think I finally have something that looks descent:

  • git_fetch takes FetchArgs; which only has fields for branch and remotes. This is useful since FetchArgs disambiguated the fields and makes it easier to know what goes where.
  • get_fetch_remotes doesn't need FetchArgs and since remotes and all_remotes are different types completely, it doesn't look terrible if I just pass them in directly

wdyt?

Copy link
Contributor

@yuja yuja Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay.

FWIW, I wonder if git_fetch() can actually be reused in "git sync". Suppose "git sync" does rebase based on the stats returned by git::fetch(), "git sync" will have to use lower-level API.

Regarding args.all_remotes, copy-pasting if args.all_remotes { ... } block wouldn't be so bad.

)?;
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.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<Vec<String>, CommandError> {
let git_remotes = git_repo.remotes()?;
Ok(git_remotes
.iter()
.filter_map(|x| x.map(ToOwned::to_owned))
.collect())
}
8 changes: 0 additions & 8 deletions cli/src/commands/git/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,6 @@ pub fn maybe_add_gitignore(workspace_command: &WorkspaceCommandHelper) -> Result
}
}

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,
})
}

/// Sets repository level `trunk()` alias to the specified remote branch.
fn write_repository_level_trunk_alias(
ui: &Ui,
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/git/push.rs
View file
Open in desktop
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
71 changes: 62 additions & 9 deletions cli/src/git_util.rs
View file
Open in desktop
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,35 @@ 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,
)
})
.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 +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(),
)
}

Expand Down Expand Up @@ -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,
essiene marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Vec<String>, 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()])
}
}
}
Loading