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.

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

Improve fetch from multiple remotes #4923

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion cli/src/commands/git/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ fn do_git_clone(
git::fetch(
fetch_tx.repo_mut(),
&git_repo,
remote_name,
&[remote_name],
&[StringPattern::everything()],
cb,
&command.settings().git_settings(),
Expand Down
60 changes: 29 additions & 31 deletions cli/src/git_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,38 +465,36 @@ pub fn git_fetch(
) -> Result<(), CommandError> {
let git_settings = tx.settings().git_settings();

for remote in remotes {
let stats = with_remote_git_callbacks(ui, None, |cb| {
git::fetch(
tx.repo_mut(),
git_repo,
remote,
branch,
cb,
&git_settings,
None,
)
})
.map_err(|err| match err {
GitFetchError::InvalidBranchPattern => {
if branch
.iter()
.any(|pattern| pattern.as_exact().is_some_and(|s| s.contains('*')))
{
user_error_with_hint(
err,
"Prefix the pattern with `glob:` to expand `*` as a glob",
)
} else {
user_error(err)
}
let stats = with_remote_git_callbacks(ui, None, |cb| {
git::fetch(
tx.repo_mut(),
git_repo,
&remotes.iter().map(String::as_str).collect::<Vec<_>>(),
branch,
cb,
&git_settings,
None,
)
})
.map_err(|err| match err {
GitFetchError::InvalidBranchPattern => {
if branch
.iter()
.any(|pattern| pattern.as_exact().is_some_and(|s| s.contains('*')))
{
user_error_with_hint(
err,
"Prefix the pattern with `glob:` to expand `*` as a glob",
)
} else {
user_error(err)
}
GitFetchError::GitImportError(err) => err.into(),
GitFetchError::InternalGitError(err) => map_git_error(err),
_ => user_error(err),
})?;
print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
}
}
GitFetchError::GitImportError(err) => err.into(),
GitFetchError::InternalGitError(err) => map_git_error(err),
_ => user_error(err),
})?;
print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
warn_if_branches_not_found(
ui,
tx,
Expand Down
2 changes: 0 additions & 2 deletions cli/tests/test_git_fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,6 @@ fn test_git_fetch_nonexistent_remote() {
&["git", "fetch", "--remote", "rem1", "--remote", "rem2"],
);
insta::assert_snapshot!(stderr, @r###"
bookmark: rem1@rem1 [new] untracked
Error: No git remote named 'rem2'
"###);
// No remote should have been fetched as part of the failing transaction
Expand All @@ -255,7 +254,6 @@ fn test_git_fetch_nonexistent_remote_from_config() {

let stderr = &test_env.jj_cmd_failure(&repo_path, &["git", "fetch"]);
insta::assert_snapshot!(stderr, @r###"
bookmark: rem1@rem1 [new] untracked
Error: No git remote named 'rem2'
"###);
// No remote should have been fetched as part of the failing transaction
Expand Down
108 changes: 60 additions & 48 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1242,21 +1242,26 @@ pub struct GitFetchStats {
pub fn fetch(
mut_repo: &mut MutableRepo,
git_repo: &git2::Repository,
remote_name: &str,
remote_names: &[&str],
branch_names: &[StringPattern],
callbacks: RemoteCallbacks<'_>,
git_settings: &GitSettings,
depth: Option<NonZeroU32>,
) -> Result<GitFetchStats, GitFetchError> {
// Perform a `git fetch` on the local git repo, updating the remote-tracking
// branches in the git repo.
let mut remote = git_repo.find_remote(remote_name).map_err(|err| {
if is_remote_not_found_err(&err) {
GitFetchError::NoSuchRemote(remote_name.to_string())
} else {
GitFetchError::InternalGitError(err)
}
})?;
let remotes = remote_names
.iter()
.map(|remote_name| {
git_repo.find_remote(remote_name).map_err(|err| {
if is_remote_not_found_err(&err) {
GitFetchError::NoSuchRemote(remote_name.to_string())
} else {
GitFetchError::InternalGitError(err)
}
})
})
.collect::<Result<Vec<_>, _>>()?;
let mut fetch_options = git2::FetchOptions::new();
let mut proxy_options = git2::ProxyOptions::new();
proxy_options.auto();
Expand All @@ -1266,58 +1271,65 @@ pub fn fetch(
if let Some(depth) = depth {
fetch_options.depth(depth.get().try_into().unwrap_or(i32::MAX));
}
// At this point, we are only updating Git's remote tracking branches, not the
// local branches.
let refspecs: Vec<_> = branch_names
.iter()
.map(|pattern| {
pattern
.to_glob()
.filter(|glob| !glob.contains(INVALID_REFSPEC_CHARS))
.map(|glob| format!("+refs/heads/{glob}:refs/remotes/{remote_name}/{glob}"))
})
.collect::<Option<_>>()
.ok_or(GitFetchError::InvalidBranchPattern)?;
if refspecs.is_empty() {
// Don't fall back to the base refspecs.
let stats = GitFetchStats::default();
return Ok(stats);
}
tracing::debug!("remote.download");
remote.download(&refspecs, Some(&mut fetch_options))?;
tracing::debug!("remote.prune");
remote.prune(None)?;
tracing::debug!("remote.update_tips");
remote.update_tips(
None,
git2::RemoteUpdateFlags::empty(),
git2::AutotagOption::Unspecified,
None,
)?;
// TODO: We could make it optional to get the default branch since we only care
// about it on clone.
let mut default_branch = None;
if let Ok(default_ref_buf) = remote.default_branch() {
if let Some(default_ref) = default_ref_buf.as_str() {
// LocalBranch here is the local branch on the remote, so it's really the remote
// branch
if let Some(RefName::LocalBranch(branch_name)) = parse_git_ref(default_ref) {
tracing::debug!(default_branch = branch_name);
default_branch = Some(branch_name);
// At this point, we are only updating Git's remote tracking branches, not the
// local branches.
for (mut remote, remote_name) in remotes.into_iter().zip(remote_names.iter()) {
let refspecs: Vec<_> = branch_names
.iter()
.map(|pattern| {
pattern
.to_glob()
.filter(|glob| !glob.contains(INVALID_REFSPEC_CHARS))
.map(|glob| format!("+refs/heads/{glob}:refs/remotes/{remote_name}/{glob}"))
})
.collect::<Option<_>>()
.ok_or(GitFetchError::InvalidBranchPattern)?;
if refspecs.is_empty() {
// Don't fall back to the base refspecs.
let stats = GitFetchStats::default();
return Ok(stats);
}
tracing::debug!("remote.download");
remote.download(&refspecs, Some(&mut fetch_options))?;
tracing::debug!("remote.prune");
remote.prune(None)?;
tracing::debug!("remote.update_tips");
remote.update_tips(
None,
git2::RemoteUpdateFlags::empty(),
git2::AutotagOption::Unspecified,
None,
)?;
// TODO: Which default branch should we choose when fetching many remotes?
if default_branch.is_none() {
if let Ok(default_ref_buf) = remote.default_branch() {
if let Some(default_ref) = default_ref_buf.as_str() {
// LocalBranch here is the local branch on the remote, so it's really the remote
// branch
if let Some(RefName::LocalBranch(branch_name)) = parse_git_ref(default_ref) {
tracing::debug!(default_branch = branch_name);
default_branch = Some(branch_name);
}
}
}
}
tracing::debug!("remote.disconnect");
remote.disconnect()?;
}
tracing::debug!("remote.disconnect");
remote.disconnect()?;

// Import the remote-tracking branches into the jj repo and update jj's
// local branches. We also import local tags since remote tags should have
// been merged by Git.
tracing::debug!("import_refs");
let import_stats = import_some_refs(mut_repo, git_settings, |ref_name| {
to_remote_branch(ref_name, remote_name)
.map(|branch| branch_names.iter().any(|pattern| pattern.matches(branch)))
.unwrap_or_else(|| matches!(ref_name, RefName::Tag(_)))
remote_names.iter().any(|remote_name| {
to_remote_branch(ref_name, remote_name)
.map(|branch| branch_names.iter().any(|pattern| pattern.matches(branch)))
.unwrap_or_else(|| matches!(ref_name, RefName::Tag(_)))
})
})?;
let stats = GitFetchStats {
default_branch,
Expand Down
20 changes: 10 additions & 10 deletions lib/tests/test_git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2265,7 +2265,7 @@ fn test_fetch_empty_repo() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2292,7 +2292,7 @@ fn test_fetch_initial_commit() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand Down Expand Up @@ -2343,7 +2343,7 @@ fn test_fetch_success() {
git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2367,7 +2367,7 @@ fn test_fetch_success() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand Down Expand Up @@ -2425,7 +2425,7 @@ fn test_fetch_prune_deleted_ref() {
git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2449,7 +2449,7 @@ fn test_fetch_prune_deleted_ref() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand Down Expand Up @@ -2477,7 +2477,7 @@ fn test_fetch_no_default_branch() {
git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2501,7 +2501,7 @@ fn test_fetch_no_default_branch() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2523,7 +2523,7 @@ fn test_fetch_empty_refspecs() {
git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2550,7 +2550,7 @@ fn test_fetch_no_such_remote() {
let result = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"invalid-remote",
&["invalid-remote"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
Expand Down