From ce0bbbd811f3fbcc899619b8a37308b2bf79565a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 20 Oct 2023 19:25:21 +0900 Subject: [PATCH 1/5] repo: inline merge_single_ref() from view MutableRepo handles merging of the other kind of refs internally, and the merge function is short enough to inline. I also removed early returns since most callers provide non-identical ref targets, and merge_ref_targets() should be cheap if the inputs can be trivially merged. --- lib/src/repo.rs | 18 ++++++------------ lib/src/view.rs | 19 +------------------ 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index dbeca242df..3c1b726703 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1156,12 +1156,7 @@ impl MutableRepo { .map(|(name, diff)| (RefName::GitRef(name.to_owned()), diff)), ); for (ref_name, (base_target, other_target)) in changed_refs { - self.view.get_mut().merge_single_ref( - self.index.as_index(), - &ref_name, - base_target, - other_target, - ); + self.merge_single_ref(&ref_name, base_target, other_target); } let changed_remote_branches = @@ -1233,12 +1228,11 @@ impl MutableRepo { base_target: &RefTarget, other_target: &RefTarget, ) { - self.view.get_mut().merge_single_ref( - self.index.as_index(), - ref_name, - base_target, - other_target, - ); + let view = self.view.get_mut(); + let index = self.index.as_index(); + let self_target = view.get_ref(ref_name); + let new_target = merge_ref_targets(index, self_target, base_target, other_target); + view.set_ref_target(ref_name, new_target); } } diff --git a/lib/src/view.rs b/lib/src/view.rs index 6d34f4578d..7471b6fe58 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -20,11 +20,10 @@ use std::fmt; use itertools::Itertools; use crate::backend::CommitId; -use crate::index::Index; use crate::op_store::{ BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, RemoteRefState, WorkspaceId, }; -use crate::refs::{merge_ref_targets, TrackingRefPair}; +use crate::refs::TrackingRefPair; use crate::str_util::StringPattern; use crate::{op_store, refs}; @@ -367,20 +366,4 @@ impl View { pub fn store_view_mut(&mut self) -> &mut op_store::View { &mut self.data } - - pub fn merge_single_ref( - &mut self, - index: &dyn Index, - ref_name: &RefName, - base_target: &RefTarget, - other_target: &RefTarget, - ) { - if base_target != other_target { - let self_target = self.get_ref(ref_name); - let new_target = merge_ref_targets(index, self_target, base_target, other_target); - if new_target != *self_target { - self.set_ref_target(ref_name, new_target); - } - } - } } From ad093ecdabbfbe73b68d6b8fc5cd8fff5ea6df0a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 20 Oct 2023 20:15:58 +0900 Subject: [PATCH 2/5] git: expand RefName match arms in import_refs() This prepares for the removal of merge_single_refs(). --- lib/src/git.rs | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 634b7ce525..0d74055673 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -271,22 +271,30 @@ pub fn import_some_refs( default_remote_ref_state_for(ref_name, git_settings) }, }; - if let RefName::RemoteBranch { branch, remote } = ref_name { - if new_remote_ref.is_tracking() { - let local_ref_name = RefName::LocalBranch(branch.clone()); - mut_repo.merge_single_ref(&local_ref_name, base_target, &new_remote_ref.target); - } - // Remote-tracking branch is the last known state of the branch in the remote. - // It shouldn't diverge even if we had inconsistent view. - mut_repo.set_remote_branch(branch, remote, new_remote_ref); - } else { - if new_remote_ref.is_tracking() { - mut_repo.merge_single_ref(ref_name, base_target, &new_remote_ref.target); - } - if let RefName::LocalBranch(branch) = ref_name { + match ref_name { + RefName::LocalBranch(branch) => { + if new_remote_ref.is_tracking() { + mut_repo.merge_single_ref(ref_name, base_target, &new_remote_ref.target); + } // Update Git-tracking branch like the other remote branches. mut_repo.set_remote_branch(branch, REMOTE_NAME_FOR_LOCAL_GIT_REPO, new_remote_ref); } + RefName::RemoteBranch { branch, remote } => { + if new_remote_ref.is_tracking() { + let local_ref_name = RefName::LocalBranch(branch.clone()); + mut_repo.merge_single_ref(&local_ref_name, base_target, &new_remote_ref.target); + } + // Remote-tracking branch is the last known state of the branch in the remote. + // It shouldn't diverge even if we had inconsistent view. + mut_repo.set_remote_branch(branch, remote, new_remote_ref); + } + RefName::Tag(_) => { + if new_remote_ref.is_tracking() { + mut_repo.merge_single_ref(ref_name, base_target, &new_remote_ref.target); + } + // TODO: If we add Git-tracking tag, it will be updated here. + } + RefName::GitRef(_) => unreachable!(), } } From 12ae0bfdea37ce32a7a4cc77ce821dae1c83b7b2 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 20 Oct 2023 19:44:29 +0900 Subject: [PATCH 3/5] repo: add merge methods per ref kind, remove RefName indirection Since local/remote branches are now of different types, it doesn't make much sense to dispatch merging through RefName. Let's add merge_() methods instead. --- lib/src/git.rs | 9 +++--- lib/src/repo.rs | 71 +++++++++++++++++++++++++++++----------------- lib/src/rewrite.rs | 10 ++----- lib/src/view.rs | 54 ++--------------------------------- 4 files changed, 54 insertions(+), 90 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 0d74055673..a4d4fcad27 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -274,23 +274,22 @@ pub fn import_some_refs( match ref_name { RefName::LocalBranch(branch) => { if new_remote_ref.is_tracking() { - mut_repo.merge_single_ref(ref_name, base_target, &new_remote_ref.target); + mut_repo.merge_local_branch(branch, base_target, &new_remote_ref.target); } // Update Git-tracking branch like the other remote branches. mut_repo.set_remote_branch(branch, REMOTE_NAME_FOR_LOCAL_GIT_REPO, new_remote_ref); } RefName::RemoteBranch { branch, remote } => { if new_remote_ref.is_tracking() { - let local_ref_name = RefName::LocalBranch(branch.clone()); - mut_repo.merge_single_ref(&local_ref_name, base_target, &new_remote_ref.target); + mut_repo.merge_local_branch(branch, base_target, &new_remote_ref.target); } // Remote-tracking branch is the last known state of the branch in the remote. // It shouldn't diverge even if we had inconsistent view. mut_repo.set_remote_branch(branch, remote, new_remote_ref); } - RefName::Tag(_) => { + RefName::Tag(name) => { if new_remote_ref.is_tracking() { - mut_repo.merge_single_ref(ref_name, base_target, &new_remote_ref.target); + mut_repo.merge_tag(name, base_target, &new_remote_ref.target); } // TODO: If we add Git-tracking tag, it will be updated here. } diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 3c1b726703..2330e3814f 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -58,7 +58,7 @@ use crate::store::Store; use crate::submodule_store::SubmoduleStore; use crate::transaction::Transaction; use crate::tree::TreeMergeError; -use crate::view::{RefName, View}; +use crate::view::View; use crate::{backend, dag_walk, op_store}; pub trait Repo { @@ -1008,6 +1008,19 @@ impl MutableRepo { self.view_mut().set_local_branch_target(name, target); } + pub fn merge_local_branch( + &mut self, + name: &str, + base_target: &RefTarget, + other_target: &RefTarget, + ) { + let view = self.view.get_mut(); + let index = self.index.as_index(); + let self_target = view.get_local_branch(name); + let new_target = merge_ref_targets(index, self_target, base_target, other_target); + view.set_local_branch_target(name, new_target); + } + pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> RemoteRef { self.view .with_ref(|v| v.get_remote_branch(name, remote_name).clone()) @@ -1035,10 +1048,9 @@ impl MutableRepo { /// Merges the specified remote branch in to local branch, and starts /// tracking it. pub fn track_remote_branch(&mut self, name: &str, remote_name: &str) { - let local_ref_name = RefName::LocalBranch(name.to_owned()); let mut remote_ref = self.get_remote_branch(name, remote_name); let base_target = remote_ref.tracking_target(); - self.merge_single_ref(&local_ref_name, base_target, &remote_ref.target); + self.merge_local_branch(name, base_target, &remote_ref.target); remote_ref.state = RemoteRefState::Tracking; self.set_remote_branch(name, remote_name, remote_ref); } @@ -1066,6 +1078,14 @@ impl MutableRepo { self.view_mut().set_tag_target(name, target); } + pub fn merge_tag(&mut self, name: &str, base_target: &RefTarget, other_target: &RefTarget) { + let view = self.view.get_mut(); + let index = self.index.as_index(); + let self_target = view.get_tag(name); + let new_target = merge_ref_targets(index, self_target, base_target, other_target); + view.set_tag_target(name, new_target); + } + pub fn get_git_ref(&self, name: &str) -> RefTarget { self.view.with_ref(|v| v.get_git_ref(name).clone()) } @@ -1074,6 +1094,14 @@ impl MutableRepo { self.view_mut().set_git_ref_target(name, target); } + fn merge_git_ref(&mut self, name: &str, base_target: &RefTarget, other_target: &RefTarget) { + let view = self.view.get_mut(); + let index = self.index.as_index(); + let self_target = view.get_git_ref(name); + let new_target = merge_ref_targets(index, self_target, base_target, other_target); + view.set_git_ref_target(name, new_target); + } + pub fn git_head(&self) -> RefTarget { self.view.with_ref(|v| v.git_head().clone()) } @@ -1147,16 +1175,20 @@ impl MutableRepo { self.view_mut().add_head(added_head); } - let changed_refs = itertools::chain!( - diff_named_ref_targets(base.local_branches(), other.local_branches()) - .map(|(name, diff)| (RefName::LocalBranch(name.to_owned()), diff)), - diff_named_ref_targets(base.tags(), other.tags()) - .map(|(name, diff)| (RefName::Tag(name.to_owned()), diff)), - diff_named_ref_targets(base.git_refs(), other.git_refs()) - .map(|(name, diff)| (RefName::GitRef(name.to_owned()), diff)), - ); - for (ref_name, (base_target, other_target)) in changed_refs { - self.merge_single_ref(&ref_name, base_target, other_target); + let changed_local_branches = + diff_named_ref_targets(base.local_branches(), other.local_branches()); + for (name, (base_target, other_target)) in changed_local_branches { + self.merge_local_branch(name, base_target, other_target); + } + + let changed_tags = diff_named_ref_targets(base.tags(), other.tags()); + for (name, (base_target, other_target)) in changed_tags { + self.merge_tag(name, base_target, other_target); + } + + let changed_git_refs = diff_named_ref_targets(base.git_refs(), other.git_refs()); + for (name, (base_target, other_target)) in changed_git_refs { + self.merge_git_ref(name, base_target, other_target); } let changed_remote_branches = @@ -1221,19 +1253,6 @@ impl MutableRepo { } } } - - pub fn merge_single_ref( - &mut self, - ref_name: &RefName, - base_target: &RefTarget, - other_target: &RefTarget, - ) { - let view = self.view.get_mut(); - let index = self.index.as_index(); - let self_target = view.get_ref(ref_name); - let new_target = merge_ref_targets(index, self_target, base_target, other_target); - view.set_ref_target(ref_name, new_target); - } } impl Repo for MutableRepo { diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 07794ecde0..d1d489b33c 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -31,7 +31,6 @@ use crate::revset::{RevsetExpression, RevsetIteratorExt}; use crate::settings::UserSettings; use crate::store::Store; use crate::tree::TreeMergeError; -use crate::view::RefName; #[instrument(skip(repo))] pub fn merge_commit_trees( @@ -320,12 +319,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } let (old_target, new_target) = DescendantRebaser::ref_target_update(old_commit_id.clone(), new_commit_ids); - for branch_name in branch_updates { - self.mut_repo.merge_single_ref( - &RefName::LocalBranch(branch_name), - &old_target, - &new_target, - ); + for branch_name in &branch_updates { + self.mut_repo + .merge_local_branch(branch_name, &old_target, &new_target); } } diff --git a/lib/src/view.rs b/lib/src/view.rs index 7471b6fe58..85c583ad04 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -20,13 +20,12 @@ use std::fmt; use itertools::Itertools; use crate::backend::CommitId; -use crate::op_store::{ - BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, RemoteRefState, WorkspaceId, -}; +use crate::op_store::{BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, WorkspaceId}; use crate::refs::TrackingRefPair; use crate::str_util::StringPattern; use crate::{op_store, refs}; +// TODO: move to git module? #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Debug)] pub enum RefName { LocalBranch(String), @@ -129,30 +128,6 @@ impl View { self.data.public_head_ids.remove(head_id); } - pub fn get_ref(&self, name: &RefName) -> &RefTarget { - match &name { - RefName::LocalBranch(name) => self.get_local_branch(name), - RefName::RemoteBranch { branch, remote } => { - &self.get_remote_branch(branch, remote).target - } - RefName::Tag(name) => self.get_tag(name), - RefName::GitRef(name) => self.get_git_ref(name), - } - } - - /// Sets reference of the specified kind to point to the given target. If - /// the target is absent, the reference will be removed. - pub fn set_ref_target(&mut self, name: &RefName, target: RefTarget) { - match name { - RefName::LocalBranch(name) => self.set_local_branch_target(name, target), - RefName::RemoteBranch { branch, remote } => { - self.set_remote_branch_target(branch, remote, target) - } - RefName::Tag(name) => self.set_tag_target(name, target), - RefName::GitRef(name) => self.set_git_ref_target(name, target), - } - } - /// Returns true if any local or remote branch of the given `name` exists. #[must_use] pub fn has_branch(&self, name: &str) -> bool { @@ -270,31 +245,6 @@ impl View { } } - /// Sets remote-tracking branch to point to the given target. If the target - /// is absent, the branch will be removed. - /// - /// If the branch already exists, its tracking state won't be changed. - fn set_remote_branch_target(&mut self, name: &str, remote_name: &str, target: RefTarget) { - if target.is_present() { - let remote_view = self - .data - .remote_views - .entry(remote_name.to_owned()) - .or_default(); - if let Some(remote_ref) = remote_view.branches.get_mut(name) { - remote_ref.target = target; - } else { - let remote_ref = RemoteRef { - target, - state: RemoteRefState::New, - }; - remote_view.branches.insert(name.to_owned(), remote_ref); - } - } else if let Some(remote_view) = self.data.remote_views.get_mut(remote_name) { - remote_view.branches.remove(name); - } - } - /// Iterates local/remote branch `(name, remote_ref)`s of the specified /// remote in lexicographical order. pub fn local_remote_branches<'a>( From 1554677c2ea27a01670c5d651b688b0abc93d235 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 20 Oct 2023 20:21:32 +0900 Subject: [PATCH 4/5] git: move RefName enum from view module It's no longer used by View API. --- lib/src/git.rs | 23 +++++++++++++++++++++-- lib/src/view.rs | 21 --------------------- lib/tests/test_git.rs | 3 +-- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index a4d4fcad27..a6e3403b17 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -17,8 +17,8 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::default::Default; use std::io::Read; -use std::iter; use std::path::PathBuf; +use std::{fmt, iter}; use git2::Oid; use itertools::Itertools; @@ -33,13 +33,32 @@ use crate::refs::BranchPushUpdate; use crate::repo::{MutableRepo, Repo}; use crate::revset::{self, RevsetExpression}; use crate::settings::GitSettings; -use crate::view::{RefName, View}; +use crate::view::View; /// Reserved remote name for the backing Git repo. pub const REMOTE_NAME_FOR_LOCAL_GIT_REPO: &str = "git"; /// Ref name used as a placeholder to unset HEAD without a commit. const UNBORN_ROOT_REF_NAME: &str = "refs/jj/root"; +#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Debug)] +pub enum RefName { + LocalBranch(String), + RemoteBranch { branch: String, remote: String }, + Tag(String), + GitRef(String), +} + +impl fmt::Display for RefName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + RefName::LocalBranch(name) => write!(f, "{name}"), + RefName::RemoteBranch { branch, remote } => write!(f, "{branch}@{remote}"), + RefName::Tag(name) => write!(f, "{name}"), + RefName::GitRef(name) => write!(f, "{name}"), + } + } +} + #[derive(Error, Debug)] pub enum GitImportError { #[error("Failed to read Git HEAD target commit {id}: {err}", id=id.hex())] diff --git a/lib/src/view.rs b/lib/src/view.rs index 85c583ad04..f49592714a 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -15,7 +15,6 @@ #![allow(missing_docs)] use std::collections::{BTreeMap, HashMap, HashSet}; -use std::fmt; use itertools::Itertools; @@ -25,26 +24,6 @@ use crate::refs::TrackingRefPair; use crate::str_util::StringPattern; use crate::{op_store, refs}; -// TODO: move to git module? -#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Debug)] -pub enum RefName { - LocalBranch(String), - RemoteBranch { branch: String, remote: String }, - Tag(String), - GitRef(String), -} - -impl fmt::Display for RefName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - RefName::LocalBranch(name) => write!(f, "{name}"), - RefName::RemoteBranch { branch, remote } => write!(f, "{branch}@{remote}"), - RefName::Tag(name) => write!(f, "{name}"), - RefName::GitRef(name) => write!(f, "{name}"), - } - } -} - #[derive(PartialEq, Eq, Debug, Clone)] pub struct View { data: op_store::View, diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index cb5a636468..9c0dc83901 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -28,14 +28,13 @@ use jj_lib::commit_builder::CommitBuilder; use jj_lib::git; use jj_lib::git::{ FailedRefExport, FailedRefExportReason, GitBranchPushTargets, GitFetchError, GitImportError, - GitPushError, GitRefUpdate, SubmoduleConfig, + GitPushError, GitRefUpdate, RefName, SubmoduleConfig, }; use jj_lib::git_backend::GitBackend; use jj_lib::op_store::{BranchTarget, RefTarget, RemoteRef, RemoteRefState}; use jj_lib::refs::BranchPushUpdate; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; use jj_lib::settings::{GitSettings, UserSettings}; -use jj_lib::view::RefName; use jj_lib::workspace::Workspace; use maplit::{btreemap, hashset}; use tempfile::TempDir; From ef330e418bbdd622689ba4a8f28188f3d10911ca Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 20 Oct 2023 20:25:26 +0900 Subject: [PATCH 5/5] git: remove RefName::GitRef variant which isn't used anymore --- lib/src/git.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index a6e3403b17..e925b507c3 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -45,7 +45,6 @@ pub enum RefName { LocalBranch(String), RemoteBranch { branch: String, remote: String }, Tag(String), - GitRef(String), } impl fmt::Display for RefName { @@ -54,7 +53,6 @@ impl fmt::Display for RefName { RefName::LocalBranch(name) => write!(f, "{name}"), RefName::RemoteBranch { branch, remote } => write!(f, "{branch}@{remote}"), RefName::Tag(name) => write!(f, "{name}"), - RefName::GitRef(name) => write!(f, "{name}"), } } } @@ -117,14 +115,13 @@ fn to_git_ref_name(parsed_ref: &RefName) -> Option { 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()), } } fn to_remote_branch<'a>(parsed_ref: &'a RefName, remote_name: &str) -> Option<&'a str> { match parsed_ref { RefName::RemoteBranch { branch, remote } => (remote == remote_name).then_some(branch), - RefName::LocalBranch(..) | RefName::Tag(..) | RefName::GitRef(..) => None, + RefName::LocalBranch(..) | RefName::Tag(..) => None, } } @@ -312,7 +309,6 @@ pub fn import_some_refs( } // TODO: If we add Git-tracking tag, it will be updated here. } - RefName::GitRef(_) => unreachable!(), } } @@ -469,7 +465,6 @@ fn default_remote_ref_state_for(ref_name: &RefName, git_settings: &GitSettings) RemoteRefState::New } } - RefName::GitRef(_) => unreachable!(), } }