Skip to content

Commit

Permalink
jj-lib: git::fetch takes multiple remotes.
Browse files Browse the repository at this point in the history 
* Reimplement `git::fetch` in terms of the new lower-level
  `GitFetch` api, which allows more control over the stages
  of the fetch.
* Make `git::fetch` to accept multiple remotes, fetch from them
  all, before calling `import_refs`.
* Set `default_branch` to None in the `GitFetchStats` return value
  from `git::fetch`. 
* Update call sites to use new `git::clone` and `git::fetch`
* Update tests: One important side-effect to note (as demonstrated
  in the changed cli test) is that when there are multiple remotes,
  all fetches must pass before `import_refs` starts; no branch
  imports if a fetch from any remote fails. This is WAI.

Fixes: #4923
  • Loading branch information
@essiene
essiene committed Nov 24, 2024
1 parent e5cb616 commit 73e741d
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 121 deletions.
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().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(|s| s.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
10 changes: 2 additions & 8 deletions cli/tests/test_git_fetch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,7 @@ fn test_git_fetch_nonexistent_remote() {
&repo_path,
&["git", "fetch", "--remote", "rem1", "--remote", "rem2"],
);
insta::assert_snapshot!(stderr, @r###"
bookmark: rem1@rem1 [new] untracked
Error: No git remote named 'rem2'
"###);
insta::assert_snapshot!(stderr, @"Error: No git remote named 'rem2'");
// No remote should have been fetched as part of the failing transaction
insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @"");
}
Expand All @@ -254,10 +251,7 @@ fn test_git_fetch_nonexistent_remote_from_config() {
test_env.add_config(r#"git.fetch = ["rem1", "rem2"]"#);

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'
"###);
insta::assert_snapshot!(stderr, @"Error: No git remote named 'rem2'");
// No remote should have been fetched as part of the failing transaction
insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @"");
}
Expand Down
85 changes: 13 additions & 72 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1383,88 +1383,29 @@ pub fn clone(
Ok(stats)
}

#[tracing::instrument(skip(mut_repo, git_repo, callbacks))]
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 mut fetch_options = git2::FetchOptions::new();
let mut proxy_options = git2::ProxyOptions::new();
proxy_options.auto();
fetch_options.proxy_options(proxy_options);
let callbacks = callbacks.into_git();
fetch_options.remote_callbacks(callbacks);
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);
}
}
let mut git_fetch = GitFetch::new(
mut_repo,
git_repo,
git_settings,
GitFetch::fetch_options(callbacks, depth),
);

for remote_name in remote_names {
git_fetch.fetch(branch_names, remote_name)?;
}
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(_)))
})?;
let import_stats = git_fetch.import_refs(branch_names, remote_names)?;
let stats = GitFetchStats {
default_branch,
default_branch: None,
import_stats,
};
Ok(stats)
Expand Down
21 changes: 11 additions & 10 deletions lib/tests/test_git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2392,6 +2392,8 @@ fn test_clone_initial_commit_head_is_set() {
);
}

#[test]
fn test_clone_fetch_success() {
let mut test_data = GitRepoData::create();
let git_settings = GitSettings {
auto_local_bookmark: true,
Expand All @@ -2400,7 +2402,6 @@ fn test_clone_initial_commit_head_is_set() {
let initial_git_commit = empty_git_commit(&test_data.origin_repo, "refs/heads/main", &[]);

let mut tx = test_data.repo.start_transaction(&test_data.settings);
git::fetch(
git::clone(
tx.repo_mut(),
&test_data.git_repo,
Expand Down Expand Up @@ -2428,15 +2429,15 @@ fn test_clone_initial_commit_head_is_set() {
let stats = git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[StringPattern::everything()],
git::RemoteCallbacks::default(),
&git_settings,
None,
)
.unwrap();
// The default bookmark is "main"
assert_eq!(stats.default_branch, Some("main".to_string()));
// `git::fetch` returns `None` for default branch.
assert_eq!(stats.default_branch, None);
assert!(stats.import_stats.abandoned_commits.is_empty());
let repo = tx.commit("test").unwrap();
// The new commit is visible after we fetch again
Expand Down Expand Up @@ -2486,7 +2487,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 @@ -2510,7 +2511,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 @@ -2538,7 +2539,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 @@ -2562,7 +2563,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 @@ -2584,7 +2585,7 @@ fn test_fetch_empty_refspecs() {
git::fetch(
tx.repo_mut(),
&test_data.git_repo,
"origin",
&["origin"],
&[],
git::RemoteCallbacks::default(),
&git_settings,
Expand All @@ -2611,7 +2612,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 73e741d

Please sign in to comment.