Skip to content

Commit

Permalink
git: when exporting, don't overwrite changes made by git
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@martinvonz
martinvonz committed Dec 3, 2022
1 parent a98ed62 commit 3979236
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 53 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
125 changes: 88 additions & 37 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -188,27 +189,38 @@ 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`
continue;
}
}
} else {
branches_to_delete.insert(branch_name.clone());
branches_to_delete.insert(branch_name.clone(), old_oid.unwrap());
}
}
// TODO: Also check other worktrees' HEAD.
Expand All @@ -217,52 +229,91 @@ 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())?;
}
}
}
}
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
}
} 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);
}
exported_view.branches.remove(&branch_name);
mut_repo.remove_git_ref(&git_ref_name);
}
for (branch_name, new_target) in branches_to_update {
for (branch_name, (old_oid, new_oid)) 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()
{
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))
}
Expand Down
34 changes: 19 additions & 15 deletions lib/tests/test_git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()));

Expand Down Expand Up @@ -797,18 +801,22 @@ 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}"))
.is_err(),
"{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}"))
Expand All @@ -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}"))
Expand All @@ -829,29 +836,26 @@ 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! {
"refs/heads/AAX".to_string() => RefTarget::Normal(commit_a.id().clone()),
"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()),
}
);
Expand Down

0 comments on commit 3979236

Please sign in to comment.