mirrors/jj
1
0
Fork 1
mirror of https://github.com/martinvonz/jj.git synced 2025-01-26 22:10:52 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

git: simplify error handling by passing git repo into git module functions

Browse source
This commit is contained in:
Martin von Zweigbergk 2021-01-10 20:13:52 -08:00
parent b1588afc63
commit 19b542b318
3 changed files with 82 additions and 109 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
lib/src/git.rs
View file

@ -20,16 +20,16 @@ use thiserror::Error;
#[derive(Error, Debug, PartialEq)]
pub enum GitImportError {
#[error("The repo is not backed by a git repo")]
NotAGitRepo,
#[error("Unexpected git error when importing refs: {0}")]
InternalGitError(#[from] git2::Error),
}
// Reflect changes made in the underlying Git repo in the Jujube repo.
pub fn import_refs(tx: &mut Transaction) -> Result<(), GitImportError> {
pub fn import_refs(
tx: &mut Transaction,
git_repo: &git2::Repository,
) -> Result<(), GitImportError> {
let store = tx.store().clone();
let git_repo = store.git_repo().ok_or(GitImportError::NotAGitRepo)?;
let git_refs = git_repo.references()?;
let existing_git_refs: Vec<_> = tx.as_repo().view().git_refs().keys().cloned().collect();
// TODO: Store the id of the previous import and read it back here, so we can
@ -61,8 +61,6 @@ pub fn import_refs(tx: &mut Transaction) -> Result<(), GitImportError> {
#[derive(Error, Debug, PartialEq)]
pub enum GitFetchError {
#[error("The repo is not backed by a git repo")]
NotAGitRepo,
#[error("No git remote named '{0}'")]
NoSuchRemote(String),
// TODO: I'm sure there are other errors possible, such as transport-level errors.
@ -70,8 +68,11 @@ pub enum GitFetchError {
InternalGitError(#[from] git2::Error),
}
pub fn fetch(tx: &mut Transaction, remote_name: &str) -> Result<(), GitFetchError> {
let git_repo = tx.store().git_repo().ok_or(GitFetchError::NotAGitRepo)?;
pub fn fetch(
tx: &mut Transaction,
git_repo: &git2::Repository,
remote_name: &str,
) -> Result<(), GitFetchError> {
let mut remote =
git_repo
.find_remote(remote_name)
@ -86,8 +87,7 @@ pub fn fetch(tx: &mut Transaction, remote_name: &str) -> Result<(), GitFetchErro
})?;
let refspec: &[&str] = &[];
remote.fetch(refspec, None, None)?;
import_refs(tx).map_err(|err| match err {
GitImportError::NotAGitRepo => panic!("git repo somehow became a non-git repo"),
import_refs(tx, git_repo).map_err(|err| match err {
GitImportError::InternalGitError(source) => GitFetchError::InternalGitError(source),
})?;
Ok(())
@ -95,8 +95,6 @@ pub fn fetch(tx: &mut Transaction, remote_name: &str) -> Result<(), GitFetchErro
#[derive(Error, Debug, PartialEq)]
pub enum GitPushError {
#[error("The repo is not backed by a git repo")]
NotAGitRepo,
#[error("No git remote named '{0}'")]
NoSuchRemote(String),
#[error("Push is not fast-forwardable'")]
@ -110,11 +108,11 @@ pub enum GitPushError {
}
pub fn push_commit(
git_repo: &git2::Repository,
commit: &Commit,
remote_name: &str,
remote_branch: &str,
) -> Result<(), GitPushError> {
let git_repo = commit.store().git_repo().ok_or(GitPushError::NotAGitRepo)?;
// Create a temporary ref to work around https://github.com/libgit2/libgit2/issues/3178
let temp_ref_name = format!("refs/jj/git-push/{}", commit.id().hex());
let mut temp_ref = git_repo.reference(

113
lib/tests/test_git.rs
View file

@ -15,7 +15,7 @@
use git2::Oid;
use jujube_lib::commit::Commit;
use jujube_lib::git;
use jujube_lib::git::{GitFetchError, GitImportError, GitPushError};
use jujube_lib::git::{GitFetchError, GitPushError};
use jujube_lib::repo::{ReadonlyRepo, Repo};
use jujube_lib::settings::UserSettings;
use jujube_lib::store::CommitId;
@ -69,9 +69,10 @@ fn test_import_refs() {
std::fs::create_dir(&jj_repo_dir).unwrap();
let repo = ReadonlyRepo::init_external_git(&settings, jj_repo_dir, git_repo_dir);
let git_repo = repo.store().git_repo().unwrap();
let mut tx = repo.start_transaction("test");
let heads_before: HashSet<_> = repo.view().heads().cloned().collect();
jujube_lib::git::import_refs(&mut tx).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx, &git_repo).unwrap_or_default();
let view = tx.as_repo().view();
let heads_after: HashSet<_> = view.heads().cloned().collect();
let expected_heads: HashSet<_> = heads_before
@ -120,7 +121,7 @@ fn test_import_refs_reimport() {
let mut repo = ReadonlyRepo::init_external_git(&settings, jj_repo_dir, git_repo_dir);
let heads_before: HashSet<_> = repo.view().heads().cloned().collect();
let mut tx = repo.start_transaction("test");
jujube_lib::git::import_refs(&mut tx).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx, &git_repo).unwrap_or_default();
tx.commit();
// Delete feature1 and rewrite feature2
@ -138,7 +139,7 @@ fn test_import_refs_reimport() {
Arc::get_mut(&mut repo).unwrap().reload();
let mut tx = repo.start_transaction("test");
jujube_lib::git::import_refs(&mut tx).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx, &git_repo).unwrap_or_default();
let view = tx.as_repo().view();
let heads_after: HashSet<_> = view.heads().cloned().collect();
@ -211,7 +212,7 @@ fn test_import_refs_merge() {
git_ref(&git_repo, "refs/heads/forward-remove", commit1.id());
git_ref(&git_repo, "refs/heads/remove-forward", commit1.id());
let mut tx = repo.start_transaction("initial import");
jujube_lib::git::import_refs(&mut tx).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx, &git_repo).unwrap_or_default();
tx.commit();
Arc::get_mut(&mut repo).unwrap().reload();
@ -224,7 +225,7 @@ fn test_import_refs_merge() {
delete_git_ref(&git_repo, "refs/heads/remove-forward");
git_ref(&git_repo, "refs/heads/add-add", commit3.id());
let mut tx1 = repo.start_transaction("concurrent import 1");
jujube_lib::git::import_refs(&mut tx1).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx1, &git_repo).unwrap_or_default();
tx1.commit();
// The other concurrent operation:
@ -236,7 +237,7 @@ fn test_import_refs_merge() {
git_ref(&git_repo, "refs/heads/remove-forward", commit2.id());
git_ref(&git_repo, "refs/heads/add-add", commit4.id());
let mut tx2 = repo.start_transaction("concurrent import 2");
jujube_lib::git::import_refs(&mut tx2).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx2, &git_repo).unwrap_or_default();
tx2.commit();
// Reload the repo, causing the operations to be merged.
@ -286,13 +287,13 @@ fn test_import_refs_empty_git_repo() {
let git_repo_dir = temp_dir.path().join("source");
let jj_repo_dir = temp_dir.path().join("jj");
git2::Repository::init_bare(&git_repo_dir).unwrap();
let git_repo = git2::Repository::init_bare(&git_repo_dir).unwrap();
std::fs::create_dir(&jj_repo_dir).unwrap();
let repo = ReadonlyRepo::init_external_git(&settings, jj_repo_dir, git_repo_dir);
let heads_before: HashSet<_> = repo.view().heads().cloned().collect();
let mut tx = repo.start_transaction("test");
jujube_lib::git::import_refs(&mut tx).unwrap_or_default();
jujube_lib::git::import_refs(&mut tx, &git_repo).unwrap_or_default();
let view = tx.as_repo().view();
let heads_after: HashSet<_> = view.heads().cloned().collect();
assert_eq!(heads_before, heads_after);
@ -300,20 +301,6 @@ fn test_import_refs_empty_git_repo() {
tx.discard();
}
#[test]
fn test_import_refs_non_git() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let jj_repo_dir = temp_dir.path().join("jj");
std::fs::create_dir(&jj_repo_dir).unwrap();
let repo = ReadonlyRepo::init_local(&settings, jj_repo_dir);
let mut tx = repo.start_transaction("test");
let result = jujube_lib::git::import_refs(&mut tx);
assert_eq!(result, Err(GitImportError::NotAGitRepo));
tx.discard();
}
#[test]
fn test_init() {
let settings = testutils::user_settings();
@ -338,13 +325,15 @@ fn test_fetch_success() {
let source_repo_dir = temp_dir.path().join("source");
let clone_repo_dir = temp_dir.path().join("clone");
let jj_repo_dir = temp_dir.path().join("jj");
let git_repo = git2::Repository::init_bare(&source_repo_dir).unwrap();
let initial_git_commit = empty_git_commit(&git_repo, "refs/heads/main", &[]);
git2::Repository::clone(&source_repo_dir.to_str().unwrap(), &clone_repo_dir).unwrap();
let source_git_repo = git2::Repository::init_bare(&source_repo_dir).unwrap();
let initial_git_commit = empty_git_commit(&source_git_repo, "refs/heads/main", &[]);
let clone_git_repo =
git2::Repository::clone(&source_repo_dir.to_str().unwrap(), &clone_repo_dir).unwrap();
std::fs::create_dir(&jj_repo_dir).unwrap();
ReadonlyRepo::init_external_git(&settings, jj_repo_dir.clone(), clone_repo_dir.clone());
let new_git_commit = empty_git_commit(&git_repo, "refs/heads/main", &[&initial_git_commit]);
let new_git_commit =
empty_git_commit(&source_git_repo, "refs/heads/main", &[&initial_git_commit]);
// The new commit is not visible before git::fetch().
let jj_repo = ReadonlyRepo::load(&settings, jj_repo_dir.clone()).unwrap();
@ -353,41 +342,26 @@ fn test_fetch_success() {
// The new commit is visible after git::fetch().
let mut tx = jj_repo.start_transaction("test");
git::fetch(&mut tx, "origin").unwrap();
git::fetch(&mut tx, &clone_git_repo, "origin").unwrap();
let heads: HashSet<_> = tx.as_repo().view().heads().cloned().collect();
assert!(heads.contains(&commit_id(&new_git_commit)));
tx.discard();
}
#[test]
fn test_fetch_non_git() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let jj_repo_dir = temp_dir.path().join("jj");
std::fs::create_dir(&jj_repo_dir).unwrap();
let jj_repo = ReadonlyRepo::init_local(&settings, jj_repo_dir);
let mut tx = jj_repo.start_transaction("test");
let result = git::fetch(&mut tx, "origin");
assert_eq!(result, Err(GitFetchError::NotAGitRepo));
tx.discard();
}
#[test]
fn test_fetch_no_such_remote() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let source_repo_dir = temp_dir.path().join("source");
let jj_repo_dir = temp_dir.path().join("jj");
git2::Repository::init_bare(&source_repo_dir).unwrap();
let git_repo = git2::Repository::init_bare(&source_repo_dir).unwrap();
std::fs::create_dir(&jj_repo_dir).unwrap();
let jj_repo =
ReadonlyRepo::init_external_git(&settings, jj_repo_dir.clone(), source_repo_dir.clone());
let mut tx = jj_repo.start_transaction("test");
let result = git::fetch(&mut tx, "invalid-remote");
let result = git::fetch(&mut tx, &git_repo, "invalid-remote");
assert!(matches!(result, Err(GitFetchError::NoSuchRemote(_))));
tx.discard();
@ -395,7 +369,6 @@ fn test_fetch_no_such_remote() {
struct PushTestSetup {
source_repo_dir: PathBuf,
clone_repo_dir: PathBuf,
jj_repo: Arc<ReadonlyRepo>,
new_commit: Commit,
}
@ -404,8 +377,8 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
let source_repo_dir = temp_dir.path().join("source");
let clone_repo_dir = temp_dir.path().join("clone");
let jj_repo_dir = temp_dir.path().join("jj");
let git_repo = git2::Repository::init_bare(&source_repo_dir).unwrap();
let initial_git_commit = empty_git_commit(&git_repo, "refs/heads/main", &[]);
let source_repo = git2::Repository::init_bare(&source_repo_dir).unwrap();
let initial_git_commit = empty_git_commit(&source_repo, "refs/heads/main", &[]);
let initial_commit_id = commit_id(&initial_git_commit);
git2::Repository::clone(&source_repo_dir.to_str().unwrap(), &clone_repo_dir).unwrap();
std::fs::create_dir(&jj_repo_dir).unwrap();
@ -417,7 +390,6 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
Arc::get_mut(&mut jj_repo).unwrap().reload();
PushTestSetup {
source_repo_dir,
clone_repo_dir,
jj_repo,
new_commit,
}
@ -428,7 +400,8 @@ fn test_push_commit_success() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let setup = set_up_push_repos(&settings, &temp_dir);
let result = git::push_commit(&setup.new_commit, "origin", "main");
let clone_repo = setup.jj_repo.store().git_repo().unwrap();
let result = git::push_commit(&clone_repo, &setup.new_commit, "origin", "main");
assert_eq!(result, Ok(()));
// Check that the ref got updated in the source repo
@ -443,7 +416,6 @@ fn test_push_commit_success() {
// Check that the ref got updated in the cloned repo. This just tests our
// assumptions about libgit2 because we want the refs/remotes/origin/main
// branch to be updated.
let clone_repo = git2::Repository::open(&setup.clone_repo_dir).unwrap();
let new_target = clone_repo
.find_reference("refs/remotes/origin/main")
.unwrap()
@ -455,11 +427,16 @@ fn test_push_commit_success() {
fn test_push_commit_not_fast_forward() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let mut jj_repo = set_up_push_repos(&settings, &temp_dir).jj_repo;
let new_commit = testutils::create_random_commit(&settings, &jj_repo)
.write_to_new_transaction(&jj_repo, "test");
Arc::get_mut(&mut jj_repo).unwrap().reload();
let result = git::push_commit(&new_commit, "origin", "main");
let mut setup = set_up_push_repos(&settings, &temp_dir);
let new_commit = testutils::create_random_commit(&settings, &setup.jj_repo)
.write_to_new_transaction(&setup.jj_repo, "test");
Arc::get_mut(&mut setup.jj_repo).unwrap().reload();
let result = git::push_commit(
&setup.jj_repo.store().git_repo().unwrap(),
&new_commit,
"origin",
"main",
);
assert_eq!(result, Err(GitPushError::NotFastForward));
}
@ -468,7 +445,12 @@ fn test_push_commit_no_such_remote() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let setup = set_up_push_repos(&settings, &temp_dir);
let result = git::push_commit(&setup.new_commit, "invalid-remote", "main");
let result = git::push_commit(
&setup.jj_repo.store().git_repo().unwrap(),
&setup.new_commit,
"invalid-remote",
"main",
);
assert!(matches!(result, Err(GitPushError::NoSuchRemote(_))));
}
@ -477,16 +459,11 @@ fn test_push_commit_invalid_remote() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let setup = set_up_push_repos(&settings, &temp_dir);
let result = git::push_commit(&setup.new_commit, "http://invalid-remote", "main");
let result = git::push_commit(
&setup.jj_repo.store().git_repo().unwrap(),
&setup.new_commit,
"http://invalid-remote",
"main",
);
assert!(matches!(result, Err(GitPushError::NoSuchRemote(_))));
}
#[test]
fn test_push_commit_non_git() {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, false);
let commit =
testutils::create_random_commit(&settings, &repo).write_to_new_transaction(&repo, "test");
let result = git::push_commit(&commit, "origin", "main");
assert_eq!(result, Err(GitPushError::NotAGitRepo));
}

54
src/commands.rs
View file

@ -59,9 +59,10 @@ use crate::styler::{ColorStyler, Styler};
use crate::template_parser::TemplateParser;
use crate::templater::Template;
use crate::ui::Ui;
use jujube_lib::git::{GitFetchError, GitImportError, GitPushError};
use jujube_lib::git::GitFetchError;
use jujube_lib::index::{HexPrefix, PrefixResolution};
use jujube_lib::operation::Operation;
use jujube_lib::store_wrapper::StoreWrapper;
use jujube_lib::transaction::Transaction;
use jujube_lib::view::merge_views;
@ -591,8 +592,9 @@ fn cmd_init(
if let Some(git_store_str) = sub_matches.value_of("git-store") {
let git_store_path = ui.cwd().join(git_store_str);
repo = ReadonlyRepo::init_external_git(ui.settings(), wc_path, git_store_path);
let git_repo = repo.store().git_repo().unwrap();
let mut tx = repo.start_transaction("import git refs");
git::import_refs(&mut tx).unwrap();
git::import_refs(&mut tx, &git_repo).unwrap();
// TODO: Check out a recent commit. Maybe one with the highest generation
// number.
tx.commit();
@ -2001,6 +2003,15 @@ fn cmd_operation(
Ok(())
}
fn get_git_repo(store: &StoreWrapper) -> Result<git2::Repository, CommandError> {
match store.git_repo() {
None => Err(CommandError::UserError(
"The repo is not backed by a git repo".to_string(),
)),
Some(git_repo) => Ok(git_repo),
}
}
fn cmd_git_fetch(
ui: &mut Ui,
matches: &ArgMatches,
@ -2008,14 +2019,11 @@ fn cmd_git_fetch(
cmd_matches: &ArgMatches,
) -> Result<(), CommandError> {
let repo = get_repo(ui, &matches)?;
let git_repo = get_git_repo(repo.store())?;
let remote_name = cmd_matches.value_of("remote").unwrap();
let mut tx = repo.start_transaction(&format!("fetch from git remote {}", remote_name));
git::fetch(&mut tx, remote_name).map_err(|err| match err {
GitFetchError::NotAGitRepo => CommandError::UserError(
"git push can only be used in repos backed by a git repo".to_string(),
),
err => CommandError::UserError(err.to_string()),
})?;
git::fetch(&mut tx, &git_repo, remote_name)
.map_err(|err| CommandError::UserError(err.to_string()))?;
tx.commit();
Ok(())
}
@ -2036,21 +2044,18 @@ fn cmd_git_clone(
}
let repo = ReadonlyRepo::init_internal_git(ui.settings(), wc_path);
let git_repo = get_git_repo(repo.store())?;
writeln!(
ui,
"Fetching into new repo in {:?}",
repo.working_copy_path()
);
let remote_name = "origin";
repo.store()
.git_repo()
.unwrap()
.remote(remote_name, source)
.unwrap();
git_repo.remote(remote_name, source).unwrap();
let mut tx = repo.start_transaction("fetch from git remote into empty repo");
git::fetch(&mut tx, remote_name).map_err(|err| match err {
GitFetchError::NotAGitRepo | GitFetchError::NoSuchRemote(_) => {
panic!("should't happen as we just created the repo and the git remote")
git::fetch(&mut tx, &git_repo, remote_name).map_err(|err| match err {
GitFetchError::NoSuchRemote(_) => {
panic!("should't happen as we just created the git remote")
}
GitFetchError::InternalGitError(err) => {
CommandError::UserError(format!("Fetch failed: {:?}", err))
@ -2068,16 +2073,13 @@ fn cmd_git_push(
cmd_matches: &ArgMatches,
) -> Result<(), CommandError> {
let mut repo = get_repo(ui, &matches)?;
let git_repo = get_git_repo(repo.store())?;
let mut_repo = Arc::get_mut(&mut repo).unwrap();
let commit = resolve_revision_arg(ui, mut_repo, cmd_matches)?;
let remote_name = cmd_matches.value_of("remote").unwrap();
let branch_name = cmd_matches.value_of("branch").unwrap();
git::push_commit(&commit, remote_name, branch_name).map_err(|err| match err {
GitPushError::NotAGitRepo => CommandError::UserError(
"git push can only be used in repos backed by a git repo".to_string(),
),
err => CommandError::UserError(err.to_string()),
})?;
git::push_commit(&git_repo, &commit, remote_name, branch_name)
.map_err(|err| CommandError::UserError(err.to_string()))?;
Ok(())
}
@ -2088,13 +2090,9 @@ fn cmd_git_refresh(
_cmd_matches: &ArgMatches,
) -> Result<(), CommandError> {
let repo = get_repo(ui, &matches)?;
let git_repo = get_git_repo(repo.store())?;
let mut tx = repo.start_transaction("import git refs");
git::import_refs(&mut tx).map_err(|err| match err {
GitImportError::NotAGitRepo => CommandError::UserError(
"git refresh can only be used in repos backed by a git repo".to_string(),
),
err => CommandError::UserError(err.to_string()),
})?;
git::import_refs(&mut tx, &git_repo).map_err(|err| CommandError::UserError(err.to_string()))?;
tx.commit();
Ok(())
}