From 5eba305844da282bfd5c47b606eb530ab25862cf Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 31 Oct 2022 09:10:22 -0700 Subject: [PATCH] git: when exporting, skip conflicted branches --- lib/src/git.rs | 27 ++++++++++++++++++++------- lib/tests/test_git.rs | 20 ++++++++++++++++---- src/cli_util.rs | 11 +++-------- tests/test_git_colocated.rs | 11 +++++++---- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 5dc03f7ed..c74ce75da 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -13,6 +13,7 @@ // limitations under the License. use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::default::Default; use std::fs::OpenOptions; use std::io::Read; use std::path::PathBuf; @@ -24,7 +25,7 @@ use thiserror::Error; use crate::backend::CommitId; use crate::commit::Commit; -use crate::op_store::{OperationId, RefTarget}; +use crate::op_store::{BranchTarget, OperationId, RefTarget}; use crate::repo::{MutableRepo, ReadonlyRepo}; use crate::view::{RefName, View}; use crate::{op_store, simple_op_store, simple_op_store_model}; @@ -175,13 +176,16 @@ pub enum GitExportError { } /// Reflect changes between two Jujutsu repo views in the underlying Git repo. -pub fn export_changes( +/// Returns a stripped-down repo view of the state we just exported, to be used +/// as `old_view` next time. +fn export_changes( old_view: &View, new_view: &View, git_repo: &git2::Repository, -) -> Result<(), GitExportError> { +) -> Result { let old_branches: HashSet<_> = old_view.branches().keys().cloned().collect(); let new_branches: HashSet<_> = new_view.branches().keys().cloned().collect(); + let mut exported_view = old_view.store_view().clone(); // First find the changes we want need to make and then make them all at once to // reduce the risk of making some changes before we fail. let mut refs_to_update = BTreeMap::new(); @@ -196,16 +200,25 @@ pub fn export_changes( if let Some(new_branch) = new_branch { match new_branch { RefTarget::Normal(id) => { + exported_view.branches.insert( + branch_name.clone(), + BranchTarget { + local_target: Some(RefTarget::Normal(id.clone())), + remote_targets: Default::default(), + }, + ); refs_to_update.insert( git_ref_name.clone(), Oid::from_bytes(id.as_bytes()).unwrap(), ); } RefTarget::Conflict { .. } => { - return Err(GitExportError::ConflictedBranch(branch_name.to_string())); + // Skip conflicts and leave the old value in `exported_view` + continue; } } } else { + exported_view.branches.remove(branch_name); refs_to_delete.insert(git_ref_name.clone()); } } @@ -232,7 +245,7 @@ pub fn export_changes( git_ref.delete()?; } } - Ok(()) + Ok(exported_view) } /// Reflect changes made in the Jujutsu repo since last export in the underlying @@ -254,13 +267,13 @@ pub fn export_refs( op_store::View::default() }; let last_export_view = View::new(last_export_store_view); - export_changes(&last_export_view, repo.view(), git_repo)?; + let new_export_store_view = export_changes(&last_export_view, repo.view(), git_repo)?; if let Ok(mut last_export_file) = OpenOptions::new() .write(true) .create(true) .open(&last_export_path) { - let thrift_view = simple_op_store_model::View::from(repo.operation().view().store_view()); + let thrift_view = simple_op_store_model::View::from(&new_export_store_view); simple_op_store::write_thrift(&thrift_view, &mut last_export_file) .map_err(|err| GitExportError::WriteStateError(err.to_string()))?; } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 2ac249567..53beaa400 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -19,7 +19,7 @@ use git2::Oid; use jujutsu_lib::backend::CommitId; use jujutsu_lib::commit::Commit; use jujutsu_lib::git; -use jujutsu_lib::git::{GitExportError, GitFetchError, GitPushError, GitRefUpdate}; +use jujutsu_lib::git::{GitFetchError, GitPushError, GitRefUpdate}; use jujutsu_lib::git_backend::GitBackend; use jujutsu_lib::op_store::{BranchTarget, RefTarget}; use jujutsu_lib::repo::ReadonlyRepo; @@ -659,10 +659,22 @@ fn test_export_conflicts() { }, ); test_data.repo = tx.commit(); - // TODO: Make it succeed instead, just skipping the conflicted branch + assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(())); assert_eq!( - git::export_refs(&test_data.repo, &git_repo), - Err(GitExportError::ConflictedBranch("feature".to_string())) + git_repo + .find_reference("refs/heads/feature") + .unwrap() + .target() + .unwrap(), + git_id(&commit_a) + ); + assert_eq!( + git_repo + .find_reference("refs/heads/main") + .unwrap() + .target() + .unwrap(), + git_id(&commit_b) ); } diff --git a/src/cli_util.rs b/src/cli_util.rs index 69b297e04..8bb71f42f 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -165,14 +165,9 @@ impl From for CommandError { impl From for CommandError { fn from(err: GitExportError) -> Self { - match err { - GitExportError::ConflictedBranch(branch_name) => { - user_error(format!("Cannot export conflicted branch '{branch_name}'")) - } - err => CommandError::InternalError(format!( - "Failed to export refs to underlying Git repo: {err}" - )), - } + CommandError::InternalError(format!( + "Failed to export refs to underlying Git repo: {err}" + )) } } diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index ce0d846db..901ac7671 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -168,10 +168,13 @@ fn test_git_colocated_branches() { "test", ) .unwrap(); - // TODO: Shouldn't be a conflict (https://github.com/martinvonz/jj/issues/463) - let stderr = test_env.jj_cmd_failure(&workspace_root, &["st"]); - insta::assert_snapshot!(stderr, @r###" - Error: Cannot export conflicted branch 'master' + // TODO: Shouldn't be a conflict + insta::assert_snapshot!(get_log_output(&test_env, &workspace_root), @r###" + @ 086821b6c35f5fdf07da884b859a14dcf85b5e36 master? + | o 6c0e140886d181602ae7a8e1ac41bc3094842370 master? + |/ + o 230dd059e1b059aefc0da06a2e5a7dbf22362f22 + o 0000000000000000000000000000000000000000 "###); }