Use BranchIdentity type instead of given_name().

This is a change in perception, as `given_name()` was the name of a virtual-branch,
which is also used to correlate other similarly named branches.

Thus, it's now more than just that, and for lack of a better word it's called
the 'identity' of a branch.

It's something very specific and shouldn't accidentally be used wrongly, hence
the strong typing.

crates/gitbutler-branch-actions/src/branch.rs

@@ -2,7 +2,9 @@ use crate::VirtualBranchesExt;
 use anyhow::{Context, Result};
 use bstr::{BStr, BString, ByteSlice};
 use core::fmt;
-use gitbutler_branch::{Branch as GitButlerBranch, BranchId, ReferenceExtGix, Target};
+use gitbutler_branch::{
+    Branch as GitButlerBranch, BranchId, BranchIdentity, ReferenceExtGix, Target,
+};
 use gitbutler_command_context::CommandContext;
 use gitbutler_reference::normalize_branch_name;
 use gix::prelude::ObjectIdExt;
@@ -189,9 +191,13 @@ fn branch_group_to_branch(
     };
 
     if virtual_branch.is_none()
-        && local_branches
-            .iter()
-            .any(|b| b.name().given_name(remotes).as_deref().ok() == Some(target.branch.branch()))
+        && local_branches.iter().any(|b| {
+            b.name()
+                .identity(remotes)
+                .as_deref()
+                .ok()
+                .map_or(false, |identity| identity == target.branch.branch())
+        })
     {
         return Ok(None);
     }
@@ -292,7 +298,7 @@ impl GroupBranch<'_> {
     fn identity(&self, remotes: &BTreeSet<Cow<'_, BStr>>) -> Option<BranchIdentity> {
         match self {
             GroupBranch::Local(branch) | GroupBranch::Remote(branch) => {
-                branch.name().given_name(remotes).ok()
+                branch.name().identity(remotes).ok()
             }
             // The identity of a Virtual branch is derived from the source refname, upstream or the branch given name, in that order
             GroupBranch::Virtual(branch) => {
@@ -301,10 +307,10 @@ impl GroupBranch<'_> {
                 let rich_name = branch.name.clone();
                 let rich_name = normalize_branch_name(&rich_name).ok()?;
                 let identity = name_from_source.unwrap_or(name_from_upstream.unwrap_or(&rich_name));
-                Some(identity.to_string())
+                Some(identity.into())
             }
         }
-        .map(BranchIdentity)
+        .map(BranchIdentity::from)
     }
 }
 
@@ -312,14 +318,13 @@ impl GroupBranch<'_> {
 /// This excludes the target branch as well as gitbutler specific branches.
 fn should_list_git_branch(identity: &BranchIdentity) -> bool {
     // Exclude gitbutler technical branches (not useful for the user)
-    let is_technical = [
-        "gitbutler/integration",
-        "gitbutler/target",
-        "gitbutler/oplog",
-        "HEAD",
-    ]
-    .contains(&&*identity.0);
-    !is_technical
+    const TECHNICAL_IDENTITIES: &[&[u8]] = &[
+        b"gitbutler/integration",
+        b"gitbutler/target",
+        b"gitbutler/oplog",
+        b"HEAD",
+    ];
+    !TECHNICAL_IDENTITIES.contains(&identity.0.as_bytes())
 }
 
 /// A filter that can be applied to the branch listing
@@ -375,32 +380,6 @@ pub struct Author {
     pub email: Option<BString>,
 }
 
-/// The identity of a branch as to allow to group similar branches together.
-///
-/// * For *local* branches, it is what's left without the standard prefix, like `refs/heads`, e.g. `main`
-///   for `refs/heads/main` or `feat/one` for `refs/heads/feat/one`.
-/// * For *remote* branches, it is what's without the prefix and remote name, like `main` for `refs/remotes/origin/main`.
-///   or `feat/one` for `refs/remotes/my/special/remote/feat/one`.
-/// * For virtual branches, it's either the above if there is a `source_refname` or an `upstream`, or it's the normalized
-///   name of the virtual branch.
-#[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash, Ord, PartialOrd)]
-pub struct BranchIdentity(String);
-
-/// Facilitate obtaining this type from the UI - otherwise it would be better not to have it as it should be
-/// a particular thing, not any string.
-impl From<String> for BranchIdentity {
-    fn from(value: String) -> Self {
-        BranchIdentity(value)
-    }
-}
-
-/// Also not for testing.
-impl From<&str> for BranchIdentity {
-    fn from(value: &str) -> Self {
-        BranchIdentity(value.into())
-    }
-}
-
 impl From<git2::Signature<'_>> for Author {
     fn from(value: git2::Signature) -> Self {
         let name = value.name().map(str::to_string).map(Into::into);

crates/gitbutler-branch-actions/src/lib.rs

@@ -41,6 +41,6 @@ mod commit;
 mod hunk;
 
 pub use branch::{
-    get_branch_listing_details, list_branches, Author, BranchIdentity, BranchListing,
-    BranchListingDetails, BranchListingFilter,
+    get_branch_listing_details, list_branches, Author, BranchListing, BranchListingDetails,
+    BranchListingFilter,
 };

crates/gitbutler-branch-actions/tests/virtual_branches/list.rs

@@ -137,7 +137,8 @@ fn one_branch_on_integration_multiple_remotes() -> Result<()> {
 
 mod util {
     use anyhow::Result;
-    use gitbutler_branch_actions::{BranchIdentity, BranchListing, BranchListingFilter};
+    use gitbutler_branch::BranchIdentity;
+    use gitbutler_branch_actions::{BranchListing, BranchListingFilter};
     use gitbutler_command_context::CommandContext;
 
     /// A flattened and simplified mirror of `BranchListing` for comparing the actual and expected data.

crates/gitbutler-branch/src/branch.rs

@@ -1,7 +1,9 @@
 use anyhow::Result;
+use bstr::{BStr, BString, ByteSlice};
 use gitbutler_id::id::Id;
 use gitbutler_reference::{normalize_branch_name, Refname, RemoteRefname, VirtualRefname};
 use serde::{Deserialize, Serialize};
+use std::ops::Deref;
 
 use crate::ownership::BranchOwnershipClaims;
 
@@ -135,3 +137,41 @@ pub struct BranchCreateRequest {
     pub order: Option<usize>,
     pub selected_for_changes: Option<bool>,
 }
+
+/// The identity of a branch as to allow to group similar branches together.
+///
+/// * For *local* branches, it is what's left without the standard prefix, like `refs/heads`, e.g. `main`
+///   for `refs/heads/main` or `feat/one` for `refs/heads/feat/one`.
+/// * For *remote* branches, it is what's without the prefix and remote name, like `main` for `refs/remotes/origin/main`.
+///   or `feat/one` for `refs/remotes/my/special/remote/feat/one`.
+/// * For virtual branches, it's either the above if there is a `source_refname` or an `upstream`, or it's the normalized
+///   name of the virtual branch.
+#[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash, Ord, PartialOrd)]
+pub struct BranchIdentity(
+    /// The identity is always a valid reference name, full or partial.
+    // TODO(ST): make this a partial reference name
+    pub  BString,
+);
+
+impl Deref for BranchIdentity {
+    type Target = BStr;
+
+    fn deref(&self) -> &Self::Target {
+        self.0.as_bstr()
+    }
+}
+
+/// Facilitate obtaining this type from the UI - otherwise it would be better not to have it as it should be
+/// a particular thing, not any string.
+impl From<String> for BranchIdentity {
+    fn from(value: String) -> Self {
+        BranchIdentity(value.into())
+    }
+}
+
+/// Also not for testing.
+impl From<&str> for BranchIdentity {
+    fn from(value: &str) -> Self {
+        BranchIdentity(value.into())
+    }
+}

crates/gitbutler-branch/src/lib.rs

@@ -1,7 +1,7 @@
 mod branch;
 
 use anyhow::Context;
-pub use branch::{Branch, BranchCreateRequest, BranchId, BranchUpdateRequest};
+pub use branch::{Branch, BranchCreateRequest, BranchId, BranchIdentity, BranchUpdateRequest};
 use bstr::ByteSlice;
 mod branch_ext;
 pub use branch_ext::BranchExt;

crates/gitbutler-branch/src/reference_ext.rs

@@ -1,3 +1,4 @@
+use crate::BranchIdentity;
 use anyhow::{Context, Result};
 use bstr::{BStr, ByteSlice};
 use gix::refs::Category;
@@ -19,15 +20,15 @@ pub trait ReferenceExt {
 
 // TODO(ST): replace the original with this one.
 pub trait ReferenceExtGix {
-    /// Fetches a branches name without the remote name attached
+    /// Produces a name by removing all prefixes, leaving only its actual name. All known
+    /// `remotes` are needed to be able to strip remote names.
     ///
-    /// refs/heads/my-branch -> my-branch
-    /// refs/remotes/origin/my-branch -> my-branch
-    /// refs/remotes/Byron/gitbutler/my-branch -> my-branch (where the remote is Byron/gitbutler)
+    /// Here are some examples:
     ///
-    /// An ideal implementation wouldn't require us to list all the references,
-    /// but there doesn't seem to be a libgit2 solution to this.
-    fn given_name(&self, remotes: &BTreeSet<Cow<'_, BStr>>) -> Result<String>;
+    /// `refs/heads/my-branch` -> `my-branch`
+    /// `refs/remotes/origin/my-branch` -> `my-branch`
+    /// `refs/remotes/Byron/gitbutler/my-branch` -> `my-branch` (where the remote is `Byron/gitbutler`)
+    fn identity(&self, remotes: &BTreeSet<Cow<'_, BStr>>) -> Result<BranchIdentity>;
 }
 
 impl<'repo> ReferenceExt for git2::Reference<'repo> {
@@ -65,13 +66,12 @@ impl<'repo> ReferenceExt for git2::Reference<'repo> {
 }
 
 impl ReferenceExtGix for &gix::refs::FullNameRef {
-    // TODO: make this `identity()`, and use `BranchIdentity` type.
-    fn given_name(&self, remotes: &BTreeSet<Cow<'_, BStr>>) -> Result<String> {
+    fn identity(&self, remotes: &BTreeSet<Cow<'_, BStr>>) -> Result<BranchIdentity> {
         let (category, shorthand_name) = self
             .category_and_short_name()
             .context("Branch could not be categorized")?;
         if !matches!(category, Category::RemoteBranch) {
-            return Ok(shorthand_name.to_string());
+            return Ok(BranchIdentity(shorthand_name.into()));
         }
 
         let longest_remote = remotes
@@ -91,7 +91,6 @@ impl ReferenceExtGix for &gix::refs::FullNameRef {
             ))?
             .into();
 
-        // TODO(correctness): this should be `BString`, but can't change it yet.
-        Ok(shorthand_name.to_string())
+        Ok(BranchIdentity(shorthand_name.into()))
     }
 }

crates/gitbutler-reference/src/refname/remote.rs

@@ -6,9 +6,11 @@ use super::error::Error;
 
 #[derive(Debug, Clone, Hash, PartialEq, Eq)]
 pub struct Refname {
-    // contains name of the remote, e.x. "origin" or "upstream"
+    /// contains name of the remote, e.x. "origin" or "upstream"
     remote: String,
-    // contains name of the branch, e.x. "master" or "main"
+    /// contains name of the branch, e.x. "master" or "main"
+    // TODO(ST): use `BString` for this, or maybe figure out if there could
+    //           be better abstractions for `Refname`, or a better name for the type.
     branch: String,
 }