Follow-up cleanup of Branch-Listing with gix

Cargo.toml

@@ -33,8 +33,9 @@ members = [
 resolver = "2"
 
 [workspace.dependencies]
+bstr = { version = "1.10.0", features = ["serde"] }
 # Add the `tracing` or `tracing-detail` features to see more of gitoxide in the logs. Useful to see which programs it invokes.
-gix = { git = "https://github.com/Byron/gitoxide", rev = "29898e3010bd3332418c683f2ac96aff5c8e72fa", default-features = false, features = [] }
+gix = { git = "https://github.com/Byron/gitoxide", rev = "7dff44754e0fdc369f92221468fb953bad9be60a", default-features = false, features = ["serde"] }
 git2 = { version = "0.18.3", features = [
     "vendored-openssl",
     "vendored-libgit2",

crates/gitbutler-branch-actions/Cargo.toml

@@ -26,7 +26,7 @@ gitbutler-fs.workspace = true
 gitbutler-diff.workspace = true
 gitbutler-operating-modes.workspace = true
 serde = { workspace = true, features = ["std"] }
-bstr = "1.10.0"
+bstr.workspace = true
 diffy = "0.4.0"
 hex = "0.4.3"
 regex = "1.10"

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

@@ -31,9 +31,9 @@ pub struct BaseBranch {
     pub remote_url: String,
     pub push_remote_name: Option<String>,
     pub push_remote_url: String,
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub base_sha: git2::Oid,
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub current_sha: git2::Oid,
     pub behind: usize,
     pub upstream_commits: Vec<RemoteCommit>,

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;
@@ -33,9 +35,12 @@ pub fn list_branches(
     for reference in platform.all()?.filter_map(Result::ok) {
         // Loosely match on branch names
         if let Some(branch_names) = &filter_branch_names {
-            let has_matching_name = branch_names
-                .iter()
-                .any(|branch_name| reference.name().as_bstr().ends_with_str(&branch_name.0));
+            let has_matching_name = branch_names.iter().any(|branch_name| {
+                reference
+                    .name()
+                    .as_bstr()
+                    .ends_with_str(branch_name.as_bstr())
+            });
 
             if !has_matching_name {
                 continue;
@@ -189,9 +194,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);
     }
@@ -203,11 +212,10 @@ fn branch_group_to_branch(
         in_workspace: branch.in_workspace,
     });
 
-    // TODO(ST): keep the type alive, don't reduce to BString
-    let mut remotes: Vec<BString> = Vec::new();
+    let mut remotes: Vec<gix::remote::Name<'static>> = Vec::new();
     for branch in remote_branches.iter() {
         if let Some(remote_name) = branch.remote_name(gix::remote::Direction::Fetch) {
-            remotes.push(remote_name.as_bstr().into());
+            remotes.push(remote_name.to_owned());
         }
     }
 
@@ -293,7 +301,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) => {
@@ -302,25 +310,24 @@ 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)
     }
 }
 
 /// Determines if a branch should be listed in the UI.
 /// 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.as_bytes())
 }
 
 /// A filter that can be applied to the branch listing
@@ -347,8 +354,8 @@ pub struct BranchListing {
     pub name: BranchIdentity,
     /// This is a list of remotes that this branch can be found on (e.g. `origin`, `upstream` etc.),
     /// by collecting remotes from all local branches with the same identity that have a tracking setup.
-    #[serde(serialize_with = "gitbutler_serde::serde::as_string_lossy_vec")]
-    pub remotes: Vec<BString>,
+    #[serde(serialize_with = "gitbutler_serde::as_string_lossy_vec_remote_name")]
+    pub remotes: Vec<gix::remote::Name<'static>>,
     /// The branch may or may not have a virtual branch associated with it.
     pub virtual_branch: Option<VirtualBranchReference>,
     /// Timestamp in milliseconds since the branch was last updated.
@@ -370,52 +377,25 @@ pub struct BranchListing {
 /// Represents a "commit author" or "signature", based on the data from the git history
 #[derive(Debug, Clone, Serialize, PartialEq, Eq, Hash)]
 pub struct Author {
-    // TODO(ST): use `BString` here to not degenerate information
     /// The name of the author as configured in the git config
-    pub name: Option<String>,
+    pub name: Option<BString>,
     /// The email of the author as configured in the git config
-    pub email: Option<String>,
-}
-
-/// 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())
-    }
+    pub email: Option<BString>,
 }
 
 impl From<git2::Signature<'_>> for Author {
     fn from(value: git2::Signature) -> Self {
-        let name = value.name().map(str::to_string);
-        let email = value.email().map(str::to_string);
+        let name = value.name().map(str::to_string).map(Into::into);
+        let email = value.email().map(str::to_string).map(Into::into);
         Author { name, email }
     }
 }
 
 impl From<gix::actor::SignatureRef<'_>> for Author {
     fn from(value: gix::actor::SignatureRef<'_>) -> Self {
         Author {
-            name: Some(value.name.to_string()),
-            email: Some(value.email.to_string()),
+            name: Some(value.name.to_owned()),
+            email: Some(value.email.to_owned()),
         }
     }
 }
@@ -436,9 +416,13 @@ pub struct VirtualBranchReference {
 /// a list of enriched branch data in the form of `BranchData`.
 pub fn get_branch_listing_details(
     ctx: &CommandContext,
-    branch_names: impl IntoIterator<Item = impl Into<BranchIdentity>>,
+    branch_names: impl IntoIterator<Item = impl TryInto<BranchIdentity>>,
 ) -> Result<Vec<BranchListingDetails>> {
-    let branch_names: Vec<_> = branch_names.into_iter().map(Into::into).collect();
+    let branch_names: Vec<_> = branch_names
+        .into_iter()
+        .map(TryInto::try_into)
+        .filter_map(Result::ok)
+        .collect();
     let repo = ctx.repository();
     let branches = list_branches(ctx, None, Some(branch_names.clone()))?;
     let default_target = ctx
@@ -536,10 +520,10 @@ pub struct BranchEntry {
     /// The name of the branch (e.g. `main`, `feature/branch`)
     pub name: String,
     /// The head commit of the branch
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     head: git2::Oid,
     /// The commit base of the branch
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     base: git2::Oid,
     /// The list of commits associated with the branch
     pub commits: Vec<CommitEntry>,
@@ -568,18 +552,17 @@ pub struct RemoteBranchEntry {
 #[serde(rename_all = "camelCase")]
 pub struct CommitEntry {
     /// The commit sha that it can be referenced by
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub id: git2::Oid,
     /// If the commit is referencing a specific change, this is its change id
     pub change_id: Option<String>,
     /// The commit message
-    #[serde(serialize_with = "gitbutler_serde::serde::as_string_lossy")]
     pub description: BString,
     /// The timestamp of the commit in milliseconds
     pub created_at: u128,
     /// The author of the commit
     pub authors: Vec<Author>,
     /// The parent commits of the commit
-    #[serde(with = "gitbutler_serde::serde::oid_vec")]
+    #[serde(with = "gitbutler_serde::oid_vec")]
     pub parent_ids: Vec<git2::Oid>,
 }

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

@@ -21,16 +21,15 @@ use crate::{
 #[derive(Debug, PartialEq, Clone, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct VirtualBranchCommit {
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub id: git2::Oid,
-    #[serde(serialize_with = "gitbutler_serde::serde::as_string_lossy")]
     pub description: BString,
     pub created_at: u128,
     pub author: Author,
     pub is_remote: bool,
     pub files: Vec<VirtualBranchFile>,
     pub is_integrated: bool,
-    #[serde(with = "gitbutler_serde::serde::oid_vec")]
+    #[serde(with = "gitbutler_serde::oid_vec")]
     pub parent_ids: Vec<git2::Oid>,
     pub branch_id: BranchId,
     pub change_id: Option<String>,

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

@@ -24,7 +24,6 @@ use serde::Serialize;
 #[serde(rename_all = "camelCase")]
 pub struct VirtualBranchHunk {
     pub id: String,
-    #[serde(serialize_with = "gitbutler_serde::serde::as_string_lossy")]
     pub diff: BString,
     pub modified_at: u128,
     pub file_path: PathBuf,
@@ -50,7 +49,7 @@ pub struct VirtualBranchHunk {
 #[serde(rename_all = "camelCase")]
 pub struct HunkLock {
     pub branch_id: BranchId,
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub commit_id: git2::Oid,
 }
 

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/src/remote.rs

@@ -23,7 +23,7 @@ use crate::author::Author;
 #[derive(Debug, Clone, Serialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct RemoteBranch {
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub sha: git2::Oid,
     pub name: Refname,
     pub upstream: Option<RemoteRefname>,
@@ -36,26 +36,25 @@ pub struct RemoteBranch {
 #[derive(Debug, Clone, Serialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct RemoteBranchData {
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub sha: git2::Oid,
     pub name: Refname,
     pub upstream: Option<RemoteRefname>,
     pub behind: u32,
     pub commits: Vec<RemoteCommit>,
-    #[serde(with = "gitbutler_serde::serde::oid_opt", default)]
+    #[serde(with = "gitbutler_serde::oid_opt", default)]
     pub fork_point: Option<git2::Oid>,
 }
 
 #[derive(Debug, Clone, PartialEq, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct RemoteCommit {
     pub id: String,
-    #[serde(serialize_with = "gitbutler_serde::serde::as_string_lossy")]
     pub description: BString,
     pub created_at: u128,
     pub author: Author,
     pub change_id: Option<String>,
-    #[serde(with = "gitbutler_serde::serde::oid_vec")]
+    #[serde(with = "gitbutler_serde::oid_vec")]
     pub parent_ids: Vec<git2::Oid>,
 }
 

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

@@ -68,13 +68,13 @@ pub struct VirtualBranch {
     pub updated_at: u128,
     pub selected_for_changes: bool,
     pub allow_rebasing: bool,
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub head: git2::Oid,
     /// The merge base between the target branch and the virtual branch
-    #[serde(with = "gitbutler_serde::serde::oid")]
+    #[serde(with = "gitbutler_serde::oid")]
     pub merge_base: git2::Oid,
     /// The fork point between the target branch and the virtual branch
-    #[serde(with = "gitbutler_serde::serde::oid_opt", default)]
+    #[serde(with = "gitbutler_serde::oid_opt", default)]
     pub fork_point: Option<git2::Oid>,
 }
 

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

@@ -137,8 +137,8 @@ fn one_branch_on_integration_multiple_remotes() -> Result<()> {
 
 mod util {
     use anyhow::Result;
-    use bstr::BString;
-    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.
@@ -154,7 +154,7 @@ mod util {
     impl Default for ExpectedBranchListing<'static> {
         fn default() -> Self {
             ExpectedBranchListing {
-                identity: "<invalid identity - should always be specified".into(),
+                identity: "invalid-identity-should-always-be-specified".into(),
                 remotes: vec![],
                 virtual_branch_given_name: None,
                 virtual_branch_in_workspace: false,
@@ -182,7 +182,7 @@ mod util {
             expected
                 .remotes
                 .into_iter()
-                .map(BString::from)
+                .map(|name| gix::remote::Name::Symbol(name.into()))
                 .collect::<Vec<_>>(),
             "remotes: {msg}"
         );