From 9a5669a6f7515fe0c5f151b6e72ab0d5ff522e5f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 19:37:12 +0900 Subject: [PATCH 1/6] git: remove stale TODO comment about FailedRefExportReason --- lib/src/git.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index a9bb03a961..292858f2f5 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -420,7 +420,6 @@ pub enum FailedRefExportReason { /// We do not export tags and other refs at the moment, since these aren't /// supposed to be modified by JJ. For them, the Git state is considered /// authoritative. -// TODO: Also indicate why we failed to export these branches pub fn export_refs( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, From 5399677396a4a89e3acc58b9ebb921a51e78a374 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 16:04:09 +0900 Subject: [PATCH 2/6] git: extract helper that exports single deleted ref --- lib/src/git.rs | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 292858f2f5..f81dfe7560 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -538,22 +538,7 @@ 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 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() - .err() - .map(FailedRefExportReason::FailedToDelete) - } else { - // The branch was updated by git - Some(FailedRefExportReason::DeletedInJjModifiedInGit) - } - } else { - // The branch is already deleted - None - }; - if let Some(reason) = reason { + if let Err(reason) = delete_git_ref(git_repo, &git_ref_name, old_oid) { failed_branches.push(FailedRefExport { name: parsed_ref_name, reason, @@ -626,6 +611,27 @@ pub fn export_some_refs( Ok(failed_branches) } +fn delete_git_ref( + git_repo: &git2::Repository, + git_ref_name: &str, + old_oid: Oid, +) -> Result<(), FailedRefExportReason> { + 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() + .map_err(FailedRefExportReason::FailedToDelete)?; + } else { + // The branch was updated by git + return Err(FailedRefExportReason::DeletedInJjModifiedInGit); + } + } else { + // The branch is already deleted + } + Ok(()) +} + #[derive(Debug, Error)] pub enum GitRemoteManagementError { #[error("No git remote named '{0}'")] From c2c8595a404b1d970ef1af0006becf480777f730 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 16:16:37 +0900 Subject: [PATCH 3/6] git: extract helper that exports single updated ref --- lib/src/git.rs | 94 +++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index f81dfe7560..fcfc3cf7c6 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -549,53 +549,7 @@ pub fn export_some_refs( } 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 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. - 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") - .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 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 - 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 - Some(FailedRefExportReason::ModifiedInJjDeletedInGit) - } - } else { - // Successfully updated from old_oid to new_oid (unchanged in git) - None - } - } - }; - if let Some(reason) = reason { + if let Err(reason) = update_git_ref(git_repo, &git_ref_name, old_oid, new_oid) { failed_branches.push(FailedRefExport { name: parsed_ref_name, reason, @@ -632,6 +586,52 @@ fn delete_git_ref( Ok(()) } +fn update_git_ref( + git_repo: &git2::Repository, + git_ref_name: &str, + old_oid: Option, + new_oid: Oid, +) -> Result<(), FailedRefExportReason> { + 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. + if git_repo_ref.target() != Some(new_oid) { + return Err(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") + .map_err(FailedRefExportReason::FailedToSet)?; + } + } + Some(old_oid) => { + // The branch was modified in jj. We can use libgit2's API for updating under a + // lock. + 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 + if git_repo_ref.target() != Some(new_oid) { + return Err(FailedRefExportReason::FailedToSet(err)); + } + } else { + // The reference was deleted in git and moved in jj + return Err(FailedRefExportReason::ModifiedInJjDeletedInGit); + } + } else { + // Successfully updated from old_oid to new_oid (unchanged in + // git) + } + } + } + Ok(()) +} + #[derive(Debug, Error)] pub enum GitRemoteManagementError { #[error("No git remote named '{0}'")] From a1051bddfc213df9d0bb7ff8ab629c13d68adbc8 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 16:48:37 +0900 Subject: [PATCH 4/6] git: do not export new ref if racy process created one with the same name Since we've checked the ref doesn't exist in this code path, I think it's better to not overwrite the existing ref. --- lib/src/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index fcfc3cf7c6..9949e9930f 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -603,7 +603,7 @@ fn update_git_ref( } 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") + .reference(git_ref_name, new_oid, false, "export from jj") .map_err(FailedRefExportReason::FailedToSet)?; } } From 3dbc10603a6c93d137e4e7f9f34f03a5c05e9e69 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 15:38:25 +0900 Subject: [PATCH 5/6] git: extract calculation step from export_some_refs() export_some_refs() is big, let's split it to functions of reasonable size. --- lib/src/git.rs | 123 ++++++++++++++++++++++++++++++------------------- 1 file changed, 76 insertions(+), 47 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 9949e9930f..8651a9bec9 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -409,6 +409,13 @@ pub enum FailedRefExportReason { FailedToSet(git2::Error), } +#[derive(Debug)] +struct RefsToExport { + branches_to_update: BTreeMap, Oid)>, + branches_to_delete: BTreeMap, + failed_branches: Vec, +} + /// 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. @@ -432,12 +439,73 @@ pub fn export_some_refs( git_repo: &git2::Repository, git_ref_filter: impl Fn(&RefName) -> bool, ) -> Result, GitExportError> { - // First find the changes we want need to make without modifying mut_repo + let RefsToExport { + branches_to_update, + branches_to_delete, + mut failed_branches, + } = diff_refs_to_export( + mut_repo.view(), + mut_repo.store().root_commit_id(), + git_ref_filter, + ); + + // TODO: Also check other worktrees' HEAD. + if let Ok(head_ref) = git_repo.find_reference("HEAD") { + if let (Some(head_git_ref), Ok(current_git_commit)) = + (head_ref.symbolic_target(), head_ref.peel_to_commit()) + { + if let Some(parsed_ref) = parse_git_ref(head_git_ref) { + let detach_head = + if let Some((_old_oid, new_oid)) = branches_to_update.get(&parsed_ref) { + *new_oid != current_git_commit.id() + } else { + branches_to_delete.contains_key(&parsed_ref) + }; + if detach_head { + git_repo.set_head_detached(current_git_commit.id())?; + } + } + } + } + for (parsed_ref_name, old_oid) in branches_to_delete { + let git_ref_name = to_git_ref_name(&parsed_ref_name).unwrap(); + if let Err(reason) = delete_git_ref(git_repo, &git_ref_name, old_oid) { + failed_branches.push(FailedRefExport { + name: parsed_ref_name, + reason, + }); + } else { + 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(); + if let Err(reason) = update_git_ref(git_repo, &git_ref_name, old_oid, new_oid) { + 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())), + ); + } + } + failed_branches.sort_by_key(|failed| failed.name.clone()); + Ok(failed_branches) +} + +/// Calculates diff of branches to be exported. +fn diff_refs_to_export( + view: &View, + root_commit_id: &CommitId, + git_ref_filter: impl Fn(&RefName) -> bool, +) -> RefsToExport { let mut branches_to_update = BTreeMap::new(); let mut branches_to_delete = BTreeMap::new(); let mut failed_branches = vec![]; - let root_commit_target = RefTarget::normal(mut_repo.store().root_commit_id().clone()); - let view = mut_repo.view(); + let root_commit_target = RefTarget::normal(root_commit_id.clone()); let jj_repo_iter_all_branches = view.branches().iter().flat_map(|(branch, target)| { itertools::chain( target @@ -518,51 +586,12 @@ pub fn export_some_refs( branches_to_delete.insert(jj_known_ref, old_oid.unwrap()); } } - // TODO: Also check other worktrees' HEAD. - if let Ok(head_ref) = git_repo.find_reference("HEAD") { - if let (Some(head_git_ref), Ok(current_git_commit)) = - (head_ref.symbolic_target(), head_ref.peel_to_commit()) - { - if let Some(parsed_ref) = parse_git_ref(head_git_ref) { - let detach_head = - if let Some((_old_oid, new_oid)) = branches_to_update.get(&parsed_ref) { - *new_oid != current_git_commit.id() - } else { - branches_to_delete.contains_key(&parsed_ref) - }; - if detach_head { - git_repo.set_head_detached(current_git_commit.id())?; - } - } - } - } - for (parsed_ref_name, old_oid) in branches_to_delete { - let git_ref_name = to_git_ref_name(&parsed_ref_name).unwrap(); - if let Err(reason) = delete_git_ref(git_repo, &git_ref_name, old_oid) { - failed_branches.push(FailedRefExport { - name: parsed_ref_name, - reason, - }); - } else { - 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(); - if let Err(reason) = update_git_ref(git_repo, &git_ref_name, old_oid, new_oid) { - 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())), - ); - } + + RefsToExport { + branches_to_update, + branches_to_delete, + failed_branches, } - failed_branches.sort_by_key(|failed| failed.name.clone()); - Ok(failed_branches) } fn delete_git_ref( From beea985dc65a7a651dbe9cda8e3431475b2f8d82 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 5 Sep 2023 17:39:45 +0900 Subject: [PATCH 6/6] git: rename local variables in diff_refs_to_export() old/new_branch sounds like a branch name, but it's a RefTarget. And jj_known_refs_passing_filter may contain "new" ref names. --- lib/src/git.rs | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 8651a9bec9..30cd6aa1bc 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -521,69 +521,69 @@ fn diff_refs_to_export( }), ) }); - let jj_known_refs_passing_filter: HashSet<_> = view + let all_ref_names_passing_filter: HashSet<_> = view .git_refs() .keys() .filter_map(|name| parse_git_ref(name)) .chain(jj_repo_iter_all_branches) .filter(git_ref_filter) .collect(); - for jj_known_ref in jj_known_refs_passing_filter { - let new_branch = match &jj_known_ref { + for ref_name in all_ref_names_passing_filter { + let new_target = match &ref_name { RefName::LocalBranch(branch) => view.get_local_branch(branch), RefName::RemoteBranch { remote, branch } => { - // Currently, the only situation where this case occurs *and* new_branch != - // old_branch is after a `jj branch forget`. So, in practice, for - // remote-tracking branches either `new_branch == old_branch` or - // `new_branch == None`. + // Currently, the only situation where this case occurs *and* new_target != + // old_target is after a `jj branch forget`. So, in practice, for + // remote-tracking branches either `new_target == old_target` or + // `new_target == None`. view.get_remote_branch(branch, remote) } _ => continue, }; - let old_branch = if let Some(name) = to_git_ref_name(&jj_known_ref) { + let old_target = if let Some(name) = to_git_ref_name(&ref_name) { view.get_git_ref(&name) } else { // Invalid branch name in Git sense failed_branches.push(FailedRefExport { - name: jj_known_ref, + name: ref_name, reason: FailedRefExportReason::InvalidGitName, }); continue; }; - if new_branch == old_branch { + if new_target == old_target { continue; } - if *new_branch == root_commit_target { + if *new_target == root_commit_target { // Git doesn't have a root commit failed_branches.push(FailedRefExport { - name: jj_known_ref, + name: ref_name, reason: FailedRefExportReason::OnRootCommit, }); continue; } - let old_oid = if let Some(id) = old_branch.as_normal() { + let old_oid = if let Some(id) = old_target.as_normal() { Some(Oid::from_bytes(id.as_bytes()).unwrap()) - } else if old_branch.has_conflict() { + } else if old_target.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(FailedRefExport { - name: jj_known_ref, + name: ref_name, reason: FailedRefExportReason::ConflictedOldState, }); continue; } else { - assert!(old_branch.is_absent()); + assert!(old_target.is_absent()); None }; - if let Some(id) = new_branch.as_normal() { + if let Some(id) = new_target.as_normal() { let new_oid = Oid::from_bytes(id.as_bytes()); - branches_to_update.insert(jj_known_ref, (old_oid, new_oid.unwrap())); - } else if new_branch.has_conflict() { + branches_to_update.insert(ref_name, (old_oid, new_oid.unwrap())); + } else if new_target.has_conflict() { // Skip conflicts and leave the old value in git_refs continue; } else { - assert!(new_branch.is_absent()); - branches_to_delete.insert(jj_known_ref, old_oid.unwrap()); + assert!(new_target.is_absent()); + branches_to_delete.insert(ref_name, old_oid.unwrap()); } }