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 7904474320), 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

@@ -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<crate::op_store::View, GitExportError> {
+) -> Result<(op_store::View, Vec<String>), 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<Vec<String>, 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<ReadonlyRepo>) {

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());