From 39792368ba1136ce1e191ad897b216c3e11d9663 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 2 Dec 2022 15:09:07 -0800 Subject: [PATCH] git: when exporting, don't overwrite changes made by git This fixes the bugs shown by the tests added in the previous patch by checking that the git branches we're about to update have not been updated by git since our last export. If they have, we fail those branches. The user can then re-import from the git repo and resolve any conflicts before exporting again. I had to update the `test_export_import_sequence` to make it pass. That shows a new bug, which I'll fix next. The problem is that the exported view doesn't get updated on import, so we would try to export changes compared to an earlier export, even though we actually knew (because of the `jj git import`) that the state in git had changed. --- CHANGELOG.md | 6 +- lib/src/git.rs | 131 +++++++++++++++++++++++++++++------------- lib/tests/test_git.rs | 34 ++++++----- 3 files changed, 115 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa9f9d601..f2341df59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,12 +85,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * (#463) A bug in the export of branches to Git caused spurious conflicted branches. This typically occurred when running in a working copy colocated with Git (created by running `jj init --git-dir=.`). - + * (#493) When exporting branches to Git, we used to fail if some branches could not be exported (e.g. because Git doesn't allow a branch called `main` and another branch called `main/sub`). We now print a warning about these branches instead. +* If you had modified branches in jj and also modified branches in conflicting + ways in Git, `jj git export` used to overwrite the changes you made in Git. + We now print a warning about these branches instead. + * `jj edit root` now fails gracefully. * `jj git import` used to abandon a commit if Git branches and tags referring diff --git a/lib/src/git.rs b/lib/src/git.rs index 1f9b6b997..df4c184b8 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::collections::{BTreeMap, HashSet}; use std::default::Default; use std::fs::OpenOptions; use std::io::Read; @@ -179,6 +179,7 @@ pub enum GitExportError { /// Returns a stripped-down repo view of the state we just exported, to be used /// as `old_view` next time. Also returns a list of names of branches that /// failed to export. +// TODO: Also indicate why we failed to export these branches fn export_changes( mut_repo: &mut MutableRepo, old_view: &View, @@ -188,19 +189,30 @@ fn export_changes( let old_branches: HashSet<_> = old_view.branches().keys().cloned().collect(); let new_branches: HashSet<_> = new_view.branches().keys().cloned().collect(); let mut branches_to_update = BTreeMap::new(); - let mut branches_to_delete = BTreeSet::new(); + let mut branches_to_delete = BTreeMap::new(); // First find the changes we want need to make without modifying mut_repo + let mut failed_branches = vec![]; for branch_name in old_branches.union(&new_branches) { let old_branch = old_view.get_local_branch(branch_name); let new_branch = new_view.get_local_branch(branch_name); if new_branch == old_branch { continue; } + let old_oid = match old_branch { + None => None, + Some(RefTarget::Normal(id)) => Some(Oid::from_bytes(id.as_bytes()).unwrap()), + Some(RefTarget::Conflict { .. }) => { + // The old git ref should only be a conflict if there were concurrent import + // operations while the value changed. Don't overwrite these values. + failed_branches.push(branch_name.clone()); + continue; + } + }; if let Some(new_branch) = new_branch { match new_branch { RefTarget::Normal(id) => { - branches_to_update - .insert(branch_name.clone(), Oid::from_bytes(id.as_bytes()).unwrap()); + let new_oid = Oid::from_bytes(id.as_bytes()); + branches_to_update.insert(branch_name.clone(), (old_oid, new_oid.unwrap())); } RefTarget::Conflict { .. } => { // Skip conflicts and leave the old value in `exported_view` @@ -208,7 +220,7 @@ fn export_changes( } } } else { - branches_to_delete.insert(branch_name.clone()); + branches_to_delete.insert(branch_name.clone(), old_oid.unwrap()); } } // TODO: Also check other worktrees' HEAD. @@ -217,11 +229,12 @@ fn export_changes( (head_ref.symbolic_target(), head_ref.peel_to_commit()) { if let Some(branch_name) = head_git_ref.strip_prefix("refs/heads/") { - let detach_head = if let Some(new_target) = branches_to_update.get(branch_name) { - *new_target != current_git_commit.id() - } else { - branches_to_delete.contains(branch_name) - }; + let detach_head = + if let Some((_old_oid, new_oid)) = branches_to_update.get(branch_name) { + *new_oid != current_git_commit.id() + } else { + branches_to_delete.contains_key(branch_name) + }; if detach_head { git_repo.set_head_detached(current_git_commit.id())?; } @@ -229,40 +242,78 @@ fn export_changes( } } let mut exported_view = old_view.store_view().clone(); - let mut failed_branches = vec![]; - for branch_name in branches_to_delete { + for (branch_name, old_oid) in branches_to_delete { let git_ref_name = format!("refs/heads/{}", branch_name); - if let Ok(mut git_ref) = git_repo.find_reference(&git_ref_name) { - if git_ref.delete().is_err() { - failed_branches.push(branch_name); - continue; + let success = if let Ok(mut git_ref) = git_repo.find_reference(&git_ref_name) { + if git_ref.target() == Some(old_oid) { + // The branch has not been updated by git, so go ahead and delete it + git_ref.delete().is_ok() + } else { + // The branch was updated by git + false } - } - exported_view.branches.remove(&branch_name); - mut_repo.remove_git_ref(&git_ref_name); - } - for (branch_name, new_target) in branches_to_update { - let git_ref_name = format!("refs/heads/{}", branch_name); - if git_repo - .reference(&git_ref_name, new_target, true, "export from jj") - .is_err() - { + } else { + // The branch is already deleted + true + }; + if success { + exported_view.branches.remove(&branch_name); + mut_repo.remove_git_ref(&git_ref_name); + } else { + failed_branches.push(branch_name); + } + } + for (branch_name, (old_oid, new_oid)) in branches_to_update { + let git_ref_name = format!("refs/heads/{}", branch_name); + let success = match old_oid { + None => { + if let Ok(git_ref) = git_repo.find_reference(&git_ref_name) { + // The branch was added in jj and in git. Iff git already pointed it to our + // desired target, we're good + git_ref.target() == Some(new_oid) + } else { + // The branch was added in jj but still doesn't exist in git, so add it + git_repo + .reference(&git_ref_name, new_oid, true, "export from jj") + .is_ok() + } + } + Some(old_oid) => { + // The branch was modified in jj. We can use libgit2's API for updating under a + // lock. + if git_repo + .reference_matching(&git_ref_name, new_oid, true, old_oid, "export from jj") + .is_ok() + { + // Successfully updated from old_oid to new_oid (unchanged in git) + true + } else { + // The reference was probably updated in git + if let Ok(git_ref) = git_repo.find_reference(&git_ref_name) { + // Iff it was updated to our desired target, we still consider it a success + git_ref.target() == Some(new_oid) + } else { + // The reference was deleted in git + false + } + } + } + }; + if success { + exported_view.branches.insert( + branch_name.clone(), + BranchTarget { + local_target: Some(RefTarget::Normal(CommitId::from_bytes(new_oid.as_bytes()))), + remote_targets: Default::default(), + }, + ); + mut_repo.set_git_ref( + git_ref_name, + RefTarget::Normal(CommitId::from_bytes(new_oid.as_bytes())), + ); + } else { failed_branches.push(branch_name); - continue; } - exported_view.branches.insert( - branch_name.clone(), - BranchTarget { - local_target: Some(RefTarget::Normal(CommitId::from_bytes( - new_target.as_bytes(), - ))), - remote_targets: Default::default(), - }, - ); - mut_repo.set_git_ref( - git_ref_name, - RefTarget::Normal(CommitId::from_bytes(new_target.as_bytes())), - ); } Ok((exported_view, failed_branches)) } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 2aa250309..9c7f21067 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -16,6 +16,7 @@ use std::path::PathBuf; use std::sync::Arc; use git2::Oid; +use itertools::Itertools; use jujutsu_lib::backend::CommitId; use jujutsu_lib::commit::Commit; use jujutsu_lib::git; @@ -585,6 +586,9 @@ fn test_export_import_sequence() { Some(RefTarget::Normal(commit_a.id().clone())) ); + // TODO: This export shouldn't be necessary for the next one to succeed + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); + // Modify the branch in jj to point to B mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit_b.id().clone())); @@ -797,9 +801,14 @@ fn test_export_reexport_transitions() { // TODO: The branches that we made conflicting changes to should have failed to // export. They should have been unchanged in git and in // mut_repo.view().git_refs(). - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); - // TODO: AXB should have remained at B from git - for branch in ["AAX", "AXA", "AXB", "AXX"] { + assert_eq!( + git::export_refs(mut_repo, &git_repo), + Ok(["AXB", "ABC", "ABX", "XAB"] + .into_iter() + .map(String::from) + .collect_vec()) + ); + for branch in ["AAX", "ABX", "AXA", "AXX"] { assert!( git_repo .find_reference(&format!("refs/heads/{branch}")) @@ -807,8 +816,7 @@ fn test_export_reexport_transitions() { "{branch} should not exist" ); } - // TODO: XAB should have remained at B from git - for branch in ["XAA", "XAB", "XAX", "XXA"] { + for branch in ["XAA", "XAX", "XXA"] { assert_eq!( git_repo .find_reference(&format!("refs/heads/{branch}")) @@ -818,8 +826,7 @@ fn test_export_reexport_transitions() { "{branch} should point to commit A" ); } - // TODO: ABX should have remained deleted from git - for branch in ["AAB", "ABA", "AAB", "ABB", "ABX"] { + for branch in ["AAB", "ABA", "AAB", "ABB", "AXB", "XAB"] { assert_eq!( git_repo .find_reference(&format!("refs/heads/{branch}")) @@ -829,18 +836,15 @@ fn test_export_reexport_transitions() { "{branch} should point to commit B" ); } - // TODO: ABC should have remained at C from git let branch = "ABC"; assert_eq!( git_repo .find_reference(&format!("refs/heads/{branch}")) .unwrap() .target(), - Some(git_id(&commit_b)), - "{branch} should point to commit B", + Some(git_id(&commit_c)), + "{branch} should point to commit C" ); - // TODO: ABC should remain at A, ABX should remain at A, AXB should remain at A, - // XAB should remain missing. assert_eq!( *mut_repo.view().git_refs(), btreemap! { @@ -848,10 +852,10 @@ fn test_export_reexport_transitions() { "refs/heads/AAB".to_string() => RefTarget::Normal(commit_a.id().clone()), "refs/heads/ABA".to_string() => RefTarget::Normal(commit_b.id().clone()), "refs/heads/ABB".to_string() => RefTarget::Normal(commit_b.id().clone()), - "refs/heads/ABC".to_string() => RefTarget::Normal(commit_b.id().clone()), - "refs/heads/ABX".to_string() => RefTarget::Normal(commit_b.id().clone()), + "refs/heads/ABC".to_string() => RefTarget::Normal(commit_a.id().clone()), + "refs/heads/ABX".to_string() => RefTarget::Normal(commit_a.id().clone()), + "refs/heads/AXB".to_string() => RefTarget::Normal(commit_a.id().clone()), "refs/heads/XAA".to_string() => RefTarget::Normal(commit_a.id().clone()), - "refs/heads/XAB".to_string() => RefTarget::Normal(commit_a.id().clone()), "refs/heads/XAX".to_string() => RefTarget::Normal(commit_a.id().clone()), } );