Skip to content

Commit

Permalink
Make fetching multiple repos behave more sanely (fixes jj-vcs#4920)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mateon1
mateon1 committed Nov 19, 2024
1 parent eb91547 commit 6038351
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 85 deletions.
2 changes: 1 addition & 1 deletion cli/src/commands/git/clone.rs
View file
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
View file
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().map_or(false, |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().map_or(false, |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
93 changes: 50 additions & 43 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1242,21 +1242,21 @@ 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| {
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 +1266,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
View file
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

0 comments on commit 6038351

Please sign in to comment.