repo: add merge methods per ref kind, remove RefName indirection

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,30 @@ 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),
+}
+
+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}"),
+        }
+    }
+}
+
 #[derive(Error, Debug)]
 pub enum GitImportError {
     #[error("Failed to read Git HEAD target commit {id}: {err}", id=id.hex())]
@@ -98,14 +115,13 @@ fn to_git_ref_name(parsed_ref: &RefName) -> Option<String> {
         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,
     }
 }
 
@@ -271,22 +287,28 @@ 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_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() {
+                    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(name) => {
+                if new_remote_ref.is_tracking() {
+                    mut_repo.merge_tag(name, base_target, &new_remote_ref.target);
+                }
+                // TODO: If we add Git-tracking tag, it will be updated here.
+            }
         }
     }
 
@@ -443,7 +465,6 @@ fn default_remote_ref_state_for(ref_name: &RefName, git_settings: &GitSettings)
                 RemoteRefState::New
             }
         }
-        RefName::GitRef(_) => unreachable!(),
     }
 }
 

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,21 +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.view.get_mut().merge_single_ref(
-                self.index.as_index(),
-                &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 =
@@ -1226,20 +1253,6 @@ impl MutableRepo {
             }
         }
     }
-
-    pub fn merge_single_ref(
-        &mut self,
-        ref_name: &RefName,
-        base_target: &RefTarget,
-        other_target: &RefTarget,
-    ) {
-        self.view.get_mut().merge_single_ref(
-            self.index.as_index(),
-            ref_name,
-            base_target,
-            other_target,
-        );
-    }
 }
 
 impl Repo for MutableRepo {

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

lib/src/view.rs

@@ -15,38 +15,15 @@
 #![allow(missing_docs)]
 
 use std::collections::{BTreeMap, HashMap, HashSet};
-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::op_store::{BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, WorkspaceId};
+use crate::refs::TrackingRefPair;
 use crate::str_util::StringPattern;
 use crate::{op_store, refs};
 
-#[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,
@@ -130,30 +107,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 {
@@ -271,31 +224,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>(
@@ -367,20 +295,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);
-            }
-        }
-    }
 }