From 4a03b94d65b7819db0e20a743a87d2de6029cf7f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 24 Nov 2022 11:03:53 -0800 Subject: [PATCH] git: on export, skip failed refs instead of failing whole export Since we now write a (partial) view object of the exported branches to disk (since 79044743209d), we can safely skip exporting some branches. We already skip conflicted branches. This commit makes us also skip branches that we fail to write to the backing Git repo, instead of failing the whole operation (after possibly updating some Git refs). I made the `export_refs()` function return the branches that failed. We should probably make that a struct later and have a separate field for branches that we skipped due to conflicts. Closes #493. --- lib/src/git.rs | 31 ++++++++++++++++++++--------- lib/tests/test_git.rs | 46 +++++++++++++++++++++++++++---------------- 2 files changed, 51 insertions(+), 26 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 41b2465b4..afd7dd0f5 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -177,12 +177,13 @@ pub enum GitExportError { /// Reflect changes between two Jujutsu repo views in the underlying Git repo. /// Returns a stripped-down repo view of the state we just exported, to be used -/// as `old_view` next time. +/// as `old_view` next time. Also returns a list of names of branches that +/// failed to export. fn export_changes( mut_repo: &mut MutableRepo, old_view: &View, git_repo: &git2::Repository, -) -> Result { +) -> Result<(op_store::View, Vec), GitExportError> { let new_view = mut_repo.view(); let old_branches: HashSet<_> = old_view.branches().keys().cloned().collect(); let new_branches: HashSet<_> = new_view.branches().keys().cloned().collect(); @@ -228,9 +229,16 @@ fn export_changes( } } let mut exported_view = old_view.store_view().clone(); + let mut failed_branches = vec![]; for (branch_name, new_target) in branches_to_update { let git_ref_name = format!("refs/heads/{}", branch_name); - git_repo.reference(&git_ref_name, new_target, true, "export from jj")?; + if git_repo + .reference(&git_ref_name, new_target, true, "export from jj") + .is_err() + { + failed_branches.push(branch_name); + continue; + } exported_view.branches.insert( branch_name.clone(), BranchTarget { @@ -248,21 +256,25 @@ fn export_changes( for branch_name 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) { - git_ref.delete()?; + if git_ref.delete().is_err() { + failed_branches.push(branch_name); + continue; + } } exported_view.branches.remove(&branch_name); mut_repo.remove_git_ref(&git_ref_name); } - Ok(exported_view) + Ok((exported_view, failed_branches)) } /// Reflect changes made in the Jujutsu repo since last export in the underlying /// Git repo. The exported view is recorded in the repo -/// (`.jj/repo/git_export_view`). +/// (`.jj/repo/git_export_view`). Returns the names of any branches that failed +/// to export. pub fn export_refs( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, -) -> Result<(), GitExportError> { +) -> Result, GitExportError> { upgrade_old_export_state(mut_repo.base_repo()); let last_export_path = mut_repo.base_repo().repo_path().join("git_export_view"); @@ -275,7 +287,8 @@ pub fn export_refs( op_store::View::default() }; let last_export_view = View::new(last_export_store_view); - let new_export_store_view = export_changes(mut_repo, &last_export_view, git_repo)?; + let (new_export_store_view, failed_branches) = + export_changes(mut_repo, &last_export_view, git_repo)?; if let Ok(mut last_export_file) = OpenOptions::new() .write(true) .create(true) @@ -285,7 +298,7 @@ pub fn export_refs( simple_op_store::write_thrift(&thrift_view, &mut last_export_file) .map_err(|err| GitExportError::WriteStateError(err.to_string()))?; } - Ok(()) + Ok(failed_branches) } fn upgrade_old_export_state(repo: &Arc) { diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index d1259fbd7..a2795375c 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -422,7 +422,7 @@ fn test_export_refs_no_detach() { mut_repo.rebase_descendants(&test_data.settings).unwrap(); // Do an initial export to make sure `main` is considered - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(jj_id(&commit1))) @@ -449,10 +449,10 @@ fn test_export_refs_no_op() { git::import_refs(mut_repo, &git_repo).unwrap(); mut_repo.rebase_descendants(&test_data.settings).unwrap(); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); // The export should be a no-op since nothing changed on the jj side since last // export - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(jj_id(&commit1))) @@ -484,7 +484,7 @@ fn test_export_refs_branch_changed() { let mut_repo = tx.mut_repo(); git::import_refs(mut_repo, &git_repo).unwrap(); mut_repo.rebase_descendants(&test_data.settings).unwrap(); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); let new_commit = create_random_commit(&test_data.settings, &test_data.repo) .set_parents(vec![jj_id(&commit)]) @@ -494,7 +494,7 @@ fn test_export_refs_branch_changed() { RefTarget::Normal(new_commit.id().clone()), ); mut_repo.remove_local_branch("delete-me"); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(new_commit.id().clone())) @@ -527,7 +527,7 @@ fn test_export_refs_current_branch_changed() { let mut_repo = tx.mut_repo(); git::import_refs(mut_repo, &git_repo).unwrap(); mut_repo.rebase_descendants(&test_data.settings).unwrap(); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); let new_commit = create_random_commit(&test_data.settings, &test_data.repo) .set_parents(vec![jj_id(&commit1)]) @@ -536,7 +536,7 @@ fn test_export_refs_current_branch_changed() { "main".to_string(), RefTarget::Normal(new_commit.id().clone()), ); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(new_commit.id().clone())) @@ -565,7 +565,7 @@ fn test_export_refs_unborn_git_branch() { let mut_repo = tx.mut_repo(); git::import_refs(mut_repo, &git_repo).unwrap(); mut_repo.rebase_descendants(&test_data.settings).unwrap(); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); let new_commit = create_random_commit(&test_data.settings, &test_data.repo).write_to_repo(mut_repo); @@ -573,7 +573,7 @@ fn test_export_refs_unborn_git_branch() { "main".to_string(), RefTarget::Normal(new_commit.id().clone()), ); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(new_commit.id().clone())) @@ -625,7 +625,7 @@ fn test_export_import_sequence() { mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit_b.id().clone())); // Export the branch to git - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( mut_repo.get_git_ref("refs/heads/main"), Some(RefTarget::Normal(commit_b.id().clone())) @@ -668,7 +668,7 @@ fn test_export_conflicts() { "feature".to_string(), RefTarget::Normal(commit_a.id().clone()), ); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); // Create a conflict and export. It should not be exported, but other changes // should be. @@ -680,7 +680,7 @@ fn test_export_conflicts() { adds: vec![commit_b.id().clone(), commit_c.id().clone()], }, ); - assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(())); + assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![])); assert_eq!( git_repo .find_reference("refs/heads/feature") @@ -717,19 +717,31 @@ fn test_export_partial_failure() { // `main/sub` will conflict with `main` in Git, at least when using loose ref // storage mut_repo.set_local_branch("main/sub".to_string(), target); - // TODO: this should succeed - assert!(git::export_refs(mut_repo, &git_repo).is_err()); + assert_eq!( + git::export_refs(mut_repo, &git_repo), + Ok(vec!["".to_string(), "main/sub".to_string()]) + ); // The `main` branch should have succeeded but the other should have failed assert!(git_repo.find_reference("refs/heads/").is_err()); - assert!(git_repo.find_reference("refs/heads/main").is_err()); + assert_eq!( + git_repo + .find_reference("refs/heads/main") + .unwrap() + .target() + .unwrap(), + git_id(&commit_a) + ); assert!(git_repo.find_reference("refs/heads/main/sub").is_err()); // Now remove the `main` branch and make sure that the `main/sub` gets exported // even though it didn't change - // TODO: this should succeed mut_repo.remove_local_branch("main"); - assert!(git::export_refs(mut_repo, &git_repo).is_err()); + // TODO: main/sub should not have failed + assert_eq!( + git::export_refs(mut_repo, &git_repo), + Ok(vec!["".to_string(), "main/sub".to_string()]) + ); assert!(git_repo.find_reference("refs/heads/").is_err()); assert!(git_repo.find_reference("refs/heads/main").is_err()); assert!(git_repo.find_reference("refs/heads/main/sub").is_err());