mirror of
https://github.com/martinvonz/jj.git
synced 2024-12-25 05:29:39 +00:00
git: update jj git clone|fetch
to use new GitFetch api directly.
Some checks failed
build / build (, macos-13) (push) Has been cancelled
build / build (, macos-14) (push) Has been cancelled
build / build (, ubuntu-latest) (push) Has been cancelled
build / build (, windows-latest) (push) Has been cancelled
build / build (--all-features, ubuntu-latest) (push) Has been cancelled
build / Build jj-lib without Git support (push) Has been cancelled
build / Check protos (push) Has been cancelled
build / Check formatting (push) Has been cancelled
build / Check that MkDocs can build the docs (push) Has been cancelled
build / Check that MkDocs can build the docs with latest Python and uv (push) Has been cancelled
build / cargo-deny (advisories) (push) Has been cancelled
build / cargo-deny (bans licenses sources) (push) Has been cancelled
build / Clippy check (push) Has been cancelled
Some checks failed
build / build (, macos-13) (push) Has been cancelled
build / build (, macos-14) (push) Has been cancelled
build / build (, ubuntu-latest) (push) Has been cancelled
build / build (, windows-latest) (push) Has been cancelled
build / build (--all-features, ubuntu-latest) (push) Has been cancelled
build / Build jj-lib without Git support (push) Has been cancelled
build / Check protos (push) Has been cancelled
build / Check formatting (push) Has been cancelled
build / Check that MkDocs can build the docs (push) Has been cancelled
build / Check that MkDocs can build the docs with latest Python and uv (push) Has been cancelled
build / cargo-deny (advisories) (push) Has been cancelled
build / cargo-deny (bans licenses sources) (push) Has been cancelled
build / Clippy check (push) Has been cancelled
* Make the new `GitFetch` api public. * Move `git::fetch` to `lib/tests/test_git.rs` as `git_fetch`, to minimize churn in the tests. Update test call sites to use `git_fetch` * Delete the `git::fetch` from `lib/src/git.rs`. * Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use the new api directly. Removing one redundant layer of indirection. * This fixes #4920 as it first fetches from all remotes before `import_refs()` is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command. * Add test for the race condition. Fixes: #4920
This commit is contained in:
parent
dc16830151
commit
6757530d82
6 changed files with 1833 additions and 1807 deletions
3408
CHANGELOG.md
3408
CHANGELOG.md
File diff suppressed because it is too large
Load diff
|
@ -19,7 +19,7 @@ use std::num::NonZeroU32;
|
|||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use jj_lib::git;
|
||||
use jj_lib::git::GitFetch;
|
||||
use jj_lib::git::GitFetchError;
|
||||
use jj_lib::git::GitFetchStats;
|
||||
use jj_lib::repo::Repo;
|
||||
|
@ -214,29 +214,32 @@ fn do_git_clone(
|
|||
maybe_add_gitignore(&workspace_command)?;
|
||||
git_repo.remote(remote_name, source).unwrap();
|
||||
let mut fetch_tx = workspace_command.start_transaction();
|
||||
let git_settings = command.settings().git_settings();
|
||||
let mut git_fetch = GitFetch::new(fetch_tx.repo_mut(), &git_repo, &git_settings);
|
||||
|
||||
let stats = with_remote_git_callbacks(ui, None, |cb| {
|
||||
git::fetch(
|
||||
fetch_tx.repo_mut(),
|
||||
&git_repo,
|
||||
remote_name,
|
||||
&[StringPattern::everything()],
|
||||
cb,
|
||||
&command.settings().git_settings(),
|
||||
depth,
|
||||
)
|
||||
})
|
||||
.map_err(|err| match err {
|
||||
GitFetchError::NoSuchRemote(_) => {
|
||||
panic!("shouldn't happen as we just created the git remote")
|
||||
}
|
||||
GitFetchError::GitImportError(err) => CommandError::from(err),
|
||||
GitFetchError::InternalGitError(err) => map_git_error(err),
|
||||
GitFetchError::InvalidBranchPattern => {
|
||||
unreachable!("we didn't provide any globs")
|
||||
}
|
||||
})?;
|
||||
print_git_import_stats(ui, fetch_tx.repo(), &stats.import_stats, true)?;
|
||||
let default_branch =
|
||||
with_remote_git_callbacks(ui, None, |cb| -> Result<Option<String>, CommandError> {
|
||||
git_fetch
|
||||
.fetch(remote_name, &[StringPattern::everything()], cb, depth)
|
||||
.map_err(|err| match err {
|
||||
GitFetchError::NoSuchRemote(_) => {
|
||||
panic!("shouldn't happen as we just created the git remote")
|
||||
}
|
||||
GitFetchError::GitImportError(err) => CommandError::from(err),
|
||||
GitFetchError::InternalGitError(err) => map_git_error(err),
|
||||
GitFetchError::InvalidBranchPattern => {
|
||||
unreachable!("we didn't provide any globs")
|
||||
}
|
||||
})
|
||||
})?;
|
||||
let import_stats = git_fetch.import_refs()?;
|
||||
print_git_import_stats(ui, fetch_tx.repo(), &import_stats, true)?;
|
||||
fetch_tx.finish(ui, "fetch from git remote into empty repo")?;
|
||||
Ok((workspace_command, stats))
|
||||
Ok((
|
||||
workspace_command,
|
||||
GitFetchStats {
|
||||
default_branch,
|
||||
import_stats,
|
||||
},
|
||||
))
|
||||
}
|
||||
|
|
|
@ -27,6 +27,7 @@ use itertools::Itertools;
|
|||
use jj_lib::git;
|
||||
use jj_lib::git::FailedRefExport;
|
||||
use jj_lib::git::FailedRefExportReason;
|
||||
use jj_lib::git::GitFetch;
|
||||
use jj_lib::git::GitFetchError;
|
||||
use jj_lib::git::GitImportStats;
|
||||
use jj_lib::git::RefName;
|
||||
|
@ -456,6 +457,9 @@ export or their "parent" bookmarks."#,
|
|||
Ok(())
|
||||
}
|
||||
|
||||
// TODO: Move this back to cli/src/commands/git/fetch.rs
|
||||
// With the new `GitFetch` api, this function is too specialized
|
||||
// to the `jj git fetch` command and should not be reused.
|
||||
pub fn git_fetch(
|
||||
ui: &mut Ui,
|
||||
tx: &mut WorkspaceCommandTransaction,
|
||||
|
@ -464,39 +468,35 @@ pub fn git_fetch(
|
|||
branch: &[StringPattern],
|
||||
) -> Result<(), CommandError> {
|
||||
let git_settings = tx.settings().git_settings();
|
||||
let mut git_fetch = GitFetch::new(tx.repo_mut(), git_repo, &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(
|
||||
"Branch names may not include `*`.",
|
||||
"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),
|
||||
with_remote_git_callbacks(ui, None, |cb| -> Result<(), CommandError> {
|
||||
git_fetch
|
||||
.fetch(remote, branch, cb, 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(
|
||||
"Branch names may not include `*`.",
|
||||
"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),
|
||||
})?;
|
||||
Ok(())
|
||||
})?;
|
||||
print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
|
||||
}
|
||||
let import_stats = git_fetch.import_refs()?;
|
||||
print_git_import_stats(ui, tx.repo(), &import_stats, true)?;
|
||||
warn_if_branches_not_found(
|
||||
ui,
|
||||
tx,
|
||||
|
|
|
@ -226,6 +226,29 @@ fn test_git_fetch_multiple_remotes_from_config() {
|
|||
"###);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_git_fetch_multiple_remotes_with_same_commit_no_race_condition() {
|
||||
let test_env = TestEnvironment::default();
|
||||
test_env.add_config("git.auto-local-bookmark = true");
|
||||
|
||||
// Create first remote.
|
||||
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
|
||||
let repo_path = test_env.env_root().join("repo");
|
||||
add_git_remote(&test_env, &repo_path, "rem1");
|
||||
add_git_remote(&test_env, &repo_path, "rem2");
|
||||
|
||||
test_env.jj_cmd_ok(
|
||||
&repo_path,
|
||||
&["git", "fetch", "--remote", "rem1", "--remote", "rem2"],
|
||||
);
|
||||
insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @r###"
|
||||
rem1: qxosxrvv 6a211027 message
|
||||
@rem1: qxosxrvv 6a211027 message
|
||||
rem2: yszkquru 2497a8a0 message
|
||||
@rem2: yszkquru 2497a8a0 message
|
||||
"###);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_git_fetch_nonexistent_remote() {
|
||||
let test_env = TestEnvironment::default();
|
||||
|
@ -237,10 +260,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), @"");
|
||||
}
|
||||
|
@ -254,10 +274,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), @"");
|
||||
}
|
||||
|
|
|
@ -1247,30 +1247,27 @@ fn fetch_options(
|
|||
}
|
||||
|
||||
struct FetchedBranches {
|
||||
branches: Vec<StringPattern>,
|
||||
remote: String,
|
||||
branches: Vec<StringPattern>,
|
||||
}
|
||||
|
||||
struct GitFetch<'a> {
|
||||
pub struct GitFetch<'a> {
|
||||
mut_repo: &'a mut MutableRepo,
|
||||
git_repo: &'a git2::Repository,
|
||||
git_settings: &'a GitSettings,
|
||||
fetch_options: git2::FetchOptions<'a>,
|
||||
fetched: Vec<FetchedBranches>,
|
||||
}
|
||||
|
||||
impl<'a> GitFetch<'a> {
|
||||
fn new(
|
||||
pub fn new(
|
||||
mut_repo: &'a mut MutableRepo,
|
||||
git_repo: &'a git2::Repository,
|
||||
git_settings: &'a GitSettings,
|
||||
fetch_options: git2::FetchOptions<'a>,
|
||||
) -> Self {
|
||||
GitFetch {
|
||||
mut_repo,
|
||||
git_repo,
|
||||
git_settings,
|
||||
fetch_options,
|
||||
fetched: vec![],
|
||||
}
|
||||
}
|
||||
|
@ -1280,10 +1277,12 @@ impl<'a> GitFetch<'a> {
|
|||
///
|
||||
/// Keeps track of the {branch_names, remote_name} pair the refs can be
|
||||
/// subsequently imported into the `jj` repo by calling `import_refs()`.
|
||||
fn fetch(
|
||||
pub fn fetch(
|
||||
&mut self,
|
||||
branch_names: &[StringPattern],
|
||||
remote_name: &str,
|
||||
branch_names: &[StringPattern],
|
||||
callbacks: RemoteCallbacks<'_>,
|
||||
depth: Option<NonZeroU32>,
|
||||
) -> Result<Option<String>, GitFetchError> {
|
||||
let mut remote = self.git_repo.find_remote(remote_name).map_err(|err| {
|
||||
if is_remote_not_found_err(&err) {
|
||||
|
@ -1314,7 +1313,7 @@ impl<'a> GitFetch<'a> {
|
|||
}
|
||||
|
||||
tracing::debug!("remote.download");
|
||||
remote.download(&refspecs, Some(&mut self.fetch_options))?;
|
||||
remote.download(&refspecs, Some(&mut fetch_options(callbacks, depth)))?;
|
||||
tracing::debug!("remote.prune");
|
||||
remote.prune(None)?;
|
||||
tracing::debug!("remote.update_tips");
|
||||
|
@ -1326,8 +1325,8 @@ impl<'a> GitFetch<'a> {
|
|||
)?;
|
||||
|
||||
self.fetched.push(FetchedBranches {
|
||||
branches: branch_names.to_vec(),
|
||||
remote: remote_name.to_string(),
|
||||
branches: branch_names.to_vec(),
|
||||
});
|
||||
|
||||
// TODO: We could make it optional to get the default branch since we only care
|
||||
|
@ -1394,31 +1393,6 @@ pub struct GitFetchStats {
|
|||
pub import_stats: GitImportStats,
|
||||
}
|
||||
|
||||
#[tracing::instrument(skip(mut_repo, git_repo, callbacks))]
|
||||
pub fn fetch(
|
||||
mut_repo: &mut MutableRepo,
|
||||
git_repo: &git2::Repository,
|
||||
remote_name: &str,
|
||||
branch_names: &[StringPattern],
|
||||
callbacks: RemoteCallbacks<'_>,
|
||||
git_settings: &GitSettings,
|
||||
depth: Option<NonZeroU32>,
|
||||
) -> Result<GitFetchStats, GitFetchError> {
|
||||
let mut git_fetch = GitFetch::new(
|
||||
mut_repo,
|
||||
git_repo,
|
||||
git_settings,
|
||||
fetch_options(callbacks, depth),
|
||||
);
|
||||
let default_branch = git_fetch.fetch(branch_names, remote_name)?;
|
||||
let import_stats = git_fetch.import_refs()?;
|
||||
let stats = GitFetchStats {
|
||||
default_branch,
|
||||
import_stats,
|
||||
};
|
||||
Ok(stats)
|
||||
}
|
||||
|
||||
#[derive(Error, Debug, PartialEq)]
|
||||
pub enum GitPushError {
|
||||
#[error("No git remote named '{0}'")]
|
||||
|
|
|
@ -17,6 +17,7 @@ use std::collections::HashSet;
|
|||
use std::fs;
|
||||
use std::io::Write;
|
||||
use std::iter;
|
||||
use std::num::NonZeroU32;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::mpsc;
|
||||
|
@ -38,7 +39,9 @@ use jj_lib::commit_builder::CommitBuilder;
|
|||
use jj_lib::git;
|
||||
use jj_lib::git::FailedRefExportReason;
|
||||
use jj_lib::git::GitBranchPushTargets;
|
||||
use jj_lib::git::GitFetch;
|
||||
use jj_lib::git::GitFetchError;
|
||||
use jj_lib::git::GitFetchStats;
|
||||
use jj_lib::git::GitImportError;
|
||||
use jj_lib::git::GitPushError;
|
||||
use jj_lib::git::GitRefUpdate;
|
||||
|
@ -110,6 +113,25 @@ fn get_git_repo(repo: &Arc<ReadonlyRepo>) -> git2::Repository {
|
|||
get_git_backend(repo).open_git_repo().unwrap()
|
||||
}
|
||||
|
||||
fn git_fetch(
|
||||
mut_repo: &mut MutableRepo,
|
||||
git_repo: &git2::Repository,
|
||||
remote_name: &str,
|
||||
branch_names: &[StringPattern],
|
||||
callbacks: git::RemoteCallbacks<'_>,
|
||||
git_settings: &GitSettings,
|
||||
depth: Option<NonZeroU32>,
|
||||
) -> Result<GitFetchStats, GitFetchError> {
|
||||
let mut git_fetch = GitFetch::new(mut_repo, git_repo, git_settings);
|
||||
let default_branch = git_fetch.fetch(remote_name, branch_names, callbacks, depth)?;
|
||||
let import_stats = git_fetch.import_refs()?;
|
||||
let stats = GitFetchStats {
|
||||
default_branch,
|
||||
import_stats,
|
||||
};
|
||||
Ok(stats)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_import_refs() {
|
||||
let settings = testutils::user_settings();
|
||||
|
@ -2262,7 +2284,7 @@ fn test_fetch_empty_repo() {
|
|||
let git_settings = GitSettings::default();
|
||||
|
||||
let mut tx = test_data.repo.start_transaction(&test_data.settings);
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2289,7 +2311,7 @@ fn test_fetch_initial_commit_head_is_not_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);
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2303,7 +2325,7 @@ fn test_fetch_initial_commit_head_is_not_set() {
|
|||
assert_eq!(stats.default_branch, None);
|
||||
assert!(stats.import_stats.abandoned_commits.is_empty());
|
||||
let repo = tx.commit("test").unwrap();
|
||||
// The initial commit is visible after git::fetch().
|
||||
// The initial commit is visible after git_fetch().
|
||||
let view = repo.view();
|
||||
assert!(view.heads().contains(&jj_id(&initial_git_commit)));
|
||||
let initial_commit_target = RefTarget::normal(jj_id(&initial_git_commit));
|
||||
|
@ -2350,7 +2372,7 @@ fn test_fetch_initial_commit_head_is_set() {
|
|||
.unwrap();
|
||||
|
||||
let mut tx = test_data.repo.start_transaction(&test_data.settings);
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2375,7 +2397,7 @@ fn test_fetch_success() {
|
|||
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_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2399,7 +2421,7 @@ fn test_fetch_success() {
|
|||
.unwrap();
|
||||
|
||||
let mut tx = test_data.repo.start_transaction(&test_data.settings);
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2457,7 +2479,7 @@ fn test_fetch_prune_deleted_ref() {
|
|||
let 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_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2481,7 +2503,7 @@ fn test_fetch_prune_deleted_ref() {
|
|||
.delete()
|
||||
.unwrap();
|
||||
// After re-fetching, the bookmark should be deleted
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2509,7 +2531,7 @@ fn test_fetch_no_default_branch() {
|
|||
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_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2533,7 +2555,7 @@ fn test_fetch_no_default_branch() {
|
|||
.set_head_detached(initial_git_commit.id())
|
||||
.unwrap();
|
||||
|
||||
let stats = git::fetch(
|
||||
let stats = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2555,7 +2577,7 @@ fn test_fetch_empty_refspecs() {
|
|||
|
||||
// Base refspecs shouldn't be respected
|
||||
let mut tx = test_data.repo.start_transaction(&test_data.settings);
|
||||
git::fetch(
|
||||
git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"origin",
|
||||
|
@ -2582,7 +2604,7 @@ fn test_fetch_no_such_remote() {
|
|||
let test_data = GitRepoData::create();
|
||||
let git_settings = GitSettings::default();
|
||||
let mut tx = test_data.repo.start_transaction(&test_data.settings);
|
||||
let result = git::fetch(
|
||||
let result = git_fetch(
|
||||
tx.repo_mut(),
|
||||
&test_data.git_repo,
|
||||
"invalid-remote",
|
||||
|
|
Loading…
Reference in a new issue