From ef550a9d6ddad1581056185dc50014b04c4a2564 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 3 Sep 2023 16:53:11 -0700 Subject: [PATCH] git: include reason for each failed ref export --- cli/src/cli_util.rs | 14 +++-- lib/src/git.rs | 115 +++++++++++++++++++++++++++++++----------- lib/tests/test_git.rs | 44 +++++++++------- 3 files changed, 119 insertions(+), 54 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 713d594df..e9e44e98a 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -32,7 +32,9 @@ use indexmap::IndexSet; use itertools::Itertools; use jj_lib::backend::{BackendError, ChangeId, CommitId, MergedTreeId, ObjectId}; use jj_lib::commit::Commit; -use jj_lib::git::{GitConfigParseError, GitExportError, GitImportError, GitRemoteManagementError}; +use jj_lib::git::{ + FailedRefExport, GitConfigParseError, GitExportError, GitImportError, GitRemoteManagementError, +}; use jj_lib::git_backend::GitBackend; use jj_lib::gitignore::GitIgnoreFile; use jj_lib::hex_util::to_reverse_hex; @@ -55,7 +57,6 @@ use jj_lib::revset::{ use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::transaction::Transaction; use jj_lib::tree::TreeMergeError; -use jj_lib::view::RefName; use jj_lib::working_copy::{ CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, SnapshotOptions, TreeStateError, WorkingCopy, @@ -1727,13 +1728,16 @@ pub fn print_checkout_stats(ui: &mut Ui, stats: CheckoutStats) -> Result<(), std Ok(()) } -pub fn print_failed_git_export(ui: &Ui, failed_branches: &[RefName]) -> Result<(), std::io::Error> { +pub fn print_failed_git_export( + ui: &Ui, + failed_branches: &[FailedRefExport], +) -> Result<(), std::io::Error> { if !failed_branches.is_empty() { writeln!(ui.warning(), "Failed to export some branches:")?; let mut formatter = ui.stderr_formatter(); - for branch_ref in failed_branches { + for failed_ref_export in failed_branches { formatter.write_str(" ")?; - write!(formatter.labeled("branch"), "{branch_ref}")?; + write!(formatter.labeled("branch"), "{}", failed_ref_export.name)?; formatter.write_str("\n")?; } drop(formatter); diff --git a/lib/src/git.rs b/lib/src/git.rs index 554562b3a..6aa2fb8dd 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -81,10 +81,11 @@ fn parse_git_ref(ref_name: &str) -> Option { fn to_git_ref_name(parsed_ref: &RefName) -> Option { match parsed_ref { - RefName::LocalBranch(branch) => (branch != "HEAD").then(|| format!("refs/heads/{branch}")), - RefName::RemoteBranch { branch, remote } => { - (branch != "HEAD").then(|| format!("refs/remotes/{remote}/{branch}")) + RefName::LocalBranch(branch) => { + (!branch.is_empty() && branch != "HEAD").then(|| format!("refs/heads/{branch}")) } + RefName::RemoteBranch { branch, remote } => (!branch.is_empty() && branch != "HEAD") + .then(|| format!("refs/remotes/{remote}/{branch}")), RefName::Tag(tag) => Some(format!("refs/tags/{tag}")), RefName::GitRef(name) => Some(name.to_owned()), } @@ -379,6 +380,33 @@ pub enum GitExportError { InternalGitError(#[from] git2::Error), } +/// A ref we failed to export to Git, along with the reason it failed. +#[derive(Debug, PartialEq)] +pub struct FailedRefExport { + pub name: RefName, + pub reason: FailedRefExportReason, +} + +/// The reason we failed to export a ref to Git. +#[derive(Debug, PartialEq)] +pub enum FailedRefExportReason { + /// The name is not allowed in Git. + InvalidGitName, + /// The ref was in a conflicted state from the last import. A re-import + /// should fix it. + ConflictedOldState, + /// We wanted to delete it, but it had been modified in Git. + DeletedInJjModifiedInGit, + /// We wanted to add it, but Git had added it with a different target + AddedInJjAddedInGit, + /// We wanted to modify it, but Git had deleted it + ModifiedInJjDeletedInGit, + /// Failed to delete the ref from the Git repo + FailedToDelete(git2::Error), + /// Failed to set the ref in the Git repo + FailedToSet(git2::Error), +} + /// Export changes to branches made in the Jujutsu repo compared to our last /// seen view of the Git repo in `mut_repo.view().git_refs()`. Returns a list of /// refs that failed to export. @@ -394,7 +422,7 @@ pub enum GitExportError { pub fn export_refs( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, -) -> Result, GitExportError> { +) -> Result, GitExportError> { export_some_refs(mut_repo, git_repo, |_| true) } @@ -402,7 +430,7 @@ pub fn export_some_refs( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, git_ref_filter: impl Fn(&RefName) -> bool, -) -> Result, GitExportError> { +) -> Result, GitExportError> { // First find the changes we want need to make without modifying mut_repo let mut branches_to_update = BTreeMap::new(); let mut branches_to_delete = BTreeMap::new(); @@ -446,7 +474,10 @@ pub fn export_some_refs( view.get_git_ref(&name) } else { // Invalid branch name in Git sense - failed_branches.push(jj_known_ref); + failed_branches.push(FailedRefExport { + name: jj_known_ref, + reason: FailedRefExportReason::InvalidGitName, + }); continue; }; if new_branch == old_branch { @@ -457,7 +488,10 @@ pub fn export_some_refs( } else if old_branch.has_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(jj_known_ref); + failed_branches.push(FailedRefExport { + name: jj_known_ref, + reason: FailedRefExportReason::ConflictedOldState, + }); continue; } else { assert!(old_branch.is_absent()); @@ -494,70 +528,91 @@ pub fn export_some_refs( } for (parsed_ref_name, old_oid) in branches_to_delete { let git_ref_name = to_git_ref_name(&parsed_ref_name).unwrap(); - let success = if let Ok(mut git_repo_ref) = git_repo.find_reference(&git_ref_name) { + let reason = if let Ok(mut git_repo_ref) = git_repo.find_reference(&git_ref_name) { if git_repo_ref.target() == Some(old_oid) { // The branch has not been updated by git, so go ahead and delete it - git_repo_ref.delete().is_ok() + git_repo_ref + .delete() + .err() + .map(FailedRefExportReason::FailedToDelete) } else { // The branch was updated by git - false + Some(FailedRefExportReason::DeletedInJjModifiedInGit) } } else { // The branch is already deleted - true + None }; - if success { - mut_repo.set_git_ref_target(&git_ref_name, RefTarget::absent()); + if let Some(reason) = reason { + failed_branches.push(FailedRefExport { + name: parsed_ref_name, + reason, + }); } else { - failed_branches.push(parsed_ref_name); + mut_repo.set_git_ref_target(&git_ref_name, RefTarget::absent()); } } for (parsed_ref_name, (old_oid, new_oid)) in branches_to_update { let git_ref_name = to_git_ref_name(&parsed_ref_name).unwrap(); - let success = match old_oid { + let reason = match old_oid { None => { if let Ok(git_repo_ref) = git_repo.find_reference(&git_ref_name) { // The branch was added in jj and in git. We're good if and only if git // pointed it to our desired target. - git_repo_ref.target() == Some(new_oid) + if git_repo_ref.target() == Some(new_oid) { + None + } else { + Some(FailedRefExportReason::AddedInJjAddedInGit) + } } 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() + .err() + .map(FailedRefExportReason::FailedToSet) } } 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 { + if let Err(err) = git_repo.reference_matching( + &git_ref_name, + new_oid, + true, + old_oid, + "export from jj", + ) { // The reference was probably updated in git if let Ok(git_repo_ref) = git_repo.find_reference(&git_ref_name) { // We still consider this a success if it was updated to our desired target - git_repo_ref.target() == Some(new_oid) + if git_repo_ref.target() == Some(new_oid) { + None + } else { + Some(FailedRefExportReason::FailedToSet(err)) + } } else { // The reference was deleted in git and moved in jj - false + Some(FailedRefExportReason::ModifiedInJjDeletedInGit) } + } else { + // Successfully updated from old_oid to new_oid (unchanged in git) + None } } }; - if success { + if let Some(reason) = reason { + failed_branches.push(FailedRefExport { + name: parsed_ref_name, + reason, + }); + } else { mut_repo.set_git_ref_target( &git_ref_name, RefTarget::normal(CommitId::from_bytes(new_oid.as_bytes())), ); - } else { - failed_branches.push(parsed_ref_name); } } - failed_branches.sort(); + failed_branches.sort_by_key(|failed| failed.name.clone()); Ok(failed_branches) } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 7d25231f2..8e6039c99 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -26,7 +26,10 @@ use jj_lib::backend::{ use jj_lib::commit::Commit; use jj_lib::commit_builder::CommitBuilder; use jj_lib::git; -use jj_lib::git::{GitFetchError, GitImportError, GitPushError, GitRefUpdate, SubmoduleConfig}; +use jj_lib::git::{ + FailedRefExportReason, GitFetchError, GitImportError, GitPushError, GitRefUpdate, + SubmoduleConfig, +}; use jj_lib::git_backend::GitBackend; use jj_lib::op_store::{BranchTarget, RefTarget}; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; @@ -1348,14 +1351,14 @@ 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_target("main/sub", target); - assert_eq!( - git::export_refs(mut_repo, &git_repo), - Ok(vec![ - RefName::LocalBranch("".to_string()), - RefName::LocalBranch("HEAD".to_string()), - RefName::LocalBranch("main/sub".to_string()) - ]) - ); + let failed = git::export_refs(mut_repo, &git_repo).unwrap(); + assert_eq!(failed.len(), 3); + assert_eq!(failed[0].name, RefName::LocalBranch("".to_string())); + assert_matches!(failed[0].reason, FailedRefExportReason::InvalidGitName); + assert_eq!(failed[1].name, RefName::LocalBranch("HEAD".to_string())); + assert_matches!(failed[1].reason, FailedRefExportReason::InvalidGitName); + assert_eq!(failed[2].name, RefName::LocalBranch("main/sub".to_string())); + assert_matches!(failed[2].reason, FailedRefExportReason::FailedToSet(_)); // The `main` branch should have succeeded but the other should have failed assert!(git_repo.find_reference("refs/heads/").is_err()); @@ -1373,13 +1376,12 @@ fn test_export_partial_failure() { // Now remove the `main` branch and make sure that the `main/sub` gets exported // even though it didn't change mut_repo.set_local_branch_target("main", RefTarget::absent()); - assert_eq!( - git::export_refs(mut_repo, &git_repo), - Ok(vec![ - RefName::LocalBranch("".to_string()), - RefName::LocalBranch("HEAD".to_string()), - ]) - ); + let failed = git::export_refs(mut_repo, &git_repo).unwrap(); + assert_eq!(failed.len(), 2); + assert_eq!(failed[0].name, RefName::LocalBranch("".to_string())); + assert_matches!(failed[0].reason, FailedRefExportReason::InvalidGitName); + assert_eq!(failed[1].name, RefName::LocalBranch("HEAD".to_string())); + assert_matches!(failed[1].reason, FailedRefExportReason::InvalidGitName); assert!(git_repo.find_reference("refs/heads/").is_err()); assert!(git_repo.find_reference("refs/heads/HEAD").is_err()); assert!(git_repo.find_reference("refs/heads/main").is_err()); @@ -1471,11 +1473,15 @@ fn test_export_reexport_transitions() { // 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(["ABC", "ABX", "AXB", "XAB"] + git::export_refs(mut_repo, &git_repo) + .unwrap() + .into_iter() + .map(|failed| failed.name) + .collect_vec(), + vec!["ABC", "ABX", "AXB", "XAB"] .into_iter() .map(|s| RefName::LocalBranch(s.to_string())) - .collect_vec()) + .collect_vec() ); for branch in ["AAX", "ABX", "AXA", "AXX"] { assert!(