Merge pull request #10648 from gitbutlerapp/but-new-review-feedback

Address review feedback

Author: Caleb-T-Owens

crates/but-api/src/commands/worktree.rs

@@ -10,12 +10,15 @@ use tracing::instrument;
 #[api_cmd]
 #[cfg_attr(feature = "tauri", tauri::command(async))]
 #[instrument(err(Debug))]
-pub fn worktree_new(project_id: ProjectId, reference: String) -> Result<NewWorktreeOutcome, Error> {
+pub fn worktree_new(
+    project_id: ProjectId,
+    reference: gix::refs::PartialName,
+) -> Result<NewWorktreeOutcome, Error> {
     let project = gitbutler_project::get(project_id)?;
     let guard = project.exclusive_worktree_access();
     let mut ctx = CommandContext::open(&project, AppSettings::load_from_default_path_creating()?)?;
 
-    but_worktrees::new::worktree_new(&mut ctx, guard.read_permission(), &reference)
+    but_worktrees::new::worktree_new(&mut ctx, guard.read_permission(), reference.as_ref())
         .map_err(Into::into)
 }
 

crates/but-db/migrations/2025-10-14-144801_change_worktrees_to_blob/down.sql

@@ -0,0 +1,15 @@
+-- Revert worktrees table path and reference columns back to TEXT
+
+-- Create old table structure with TEXT columns
+CREATE TABLE `worktrees_old`(
+	`path` TEXT NOT NULL PRIMARY KEY,
+	`reference` TEXT NOT NULL,
+	`base` TEXT NOT NULL,
+	`source` TEXT NOT NULL
+);
+
+-- Drop current table
+DROP TABLE worktrees;
+
+-- Rename old table to original name
+ALTER TABLE worktrees_old RENAME TO worktrees;

crates/but-db/migrations/2025-10-14-144801_change_worktrees_to_blob/up.sql

@@ -0,0 +1,16 @@
+-- Change worktrees table path and reference columns to BLOB
+-- SQLite doesn't support ALTER COLUMN, so we need to recreate the table
+
+-- Create new table with BLOB columns
+CREATE TABLE `worktrees_new`(
+	`path` BLOB NOT NULL PRIMARY KEY,
+	`reference` BLOB NOT NULL,
+	`base` TEXT NOT NULL,
+	`source` TEXT NOT NULL
+);
+
+-- Drop old table
+DROP TABLE worktrees;
+
+-- Rename new table to original name
+ALTER TABLE worktrees_new RENAME TO worktrees;

crates/but-db/src/schema.rs

@@ -89,8 +89,8 @@ diesel::table! {
 
 diesel::table! {
     worktrees (path) {
-        path -> Text,
-        reference -> Text,
+        path -> Binary,
+        reference -> Binary,
         base -> Text,
         source -> Text,
     }

crates/but-db/src/worktrees.rs

@@ -12,10 +12,16 @@ use serde::{Deserialize, Serialize};
 #[diesel(table_name = crate::schema::worktrees)]
 #[diesel(check_for_backend(diesel::sqlite::Sqlite))]
 #[diesel(primary_key(path))]
+/// This struct should not be consumed directly. Instead use the
+/// [but_worktrees::Worktree] struct.
 pub struct Worktree {
-    pub path: String,
-    pub reference: String,
+    /// A canonicalized path represented in bytes
+    pub path: Vec<u8>,
+    /// A fully qualified reference serialized from gix::refs::FullNameRef
+    pub reference: Vec<u8>,
+    /// A commit oid serialized as hexidecimal string
     pub base: String,
+    /// JSON stringification of [but_worktrees::WorktreeSource]
     pub source: String,
 }
 
@@ -37,15 +43,15 @@ impl WorktreesHandle<'_> {
         Ok(())
     }
 
-    pub fn get(&mut self, path: &str) -> Result<Option<Worktree>, diesel::result::Error> {
+    pub fn get(&mut self, path: &[u8]) -> Result<Option<Worktree>, diesel::result::Error> {
         let worktree = worktrees
             .filter(crate::schema::worktrees::path.eq(path))
             .first::<Worktree>(&mut self.db.conn)
             .optional()?;
         Ok(worktree)
     }
 
-    pub fn delete(&mut self, path: &str) -> Result<(), diesel::result::Error> {
+    pub fn delete(&mut self, path: &[u8]) -> Result<(), diesel::result::Error> {
         diesel::delete(worktrees.filter(crate::schema::worktrees::path.eq(path)))
             .execute(&mut self.db.conn)?;
         Ok(())

crates/but-worktrees/src/db.rs

@@ -1,6 +1,7 @@
 //! Database operations for worktrees.
 
 use anyhow::Result;
+use bstr::BString;
 use gitbutler_command_context::CommandContext;
 use std::path::Path;
 
@@ -16,7 +17,7 @@ pub fn save_worktree(ctx: &mut CommandContext, worktree: Worktree) -> Result<()>
 #[allow(unused)]
 pub fn get_worktree(ctx: &mut CommandContext, path: &Path) -> Result<Option<Worktree>> {
     let path_str = path.to_string_lossy();
-    let worktree = ctx.db()?.worktrees().get(&path_str)?;
+    let worktree = ctx.db()?.worktrees().get(&gix::path::into_bstr(path))?;
     match worktree {
         Some(w) => Ok(Some(w.try_into()?)),
         None => Ok(None),
@@ -26,8 +27,7 @@ pub fn get_worktree(ctx: &mut CommandContext, path: &Path) -> Result<Option<Work
 /// Delete a worktree from the database.
 #[allow(unused)]
 pub fn delete_worktree(ctx: &mut CommandContext, path: &Path) -> Result<()> {
-    let path_str = path.to_string_lossy();
-    ctx.db()?.worktrees().delete(&path_str)?;
+    ctx.db()?.worktrees().delete(&gix::path::into_bstr(path))?;
     Ok(())
 }
 
@@ -46,11 +46,11 @@ impl TryFrom<but_db::Worktree> for Worktree {
     fn try_from(value: but_db::Worktree) -> Result<Self, Self::Error> {
         let source: WorktreeSource = serde_json::from_str(&value.source)?;
         let base = gix::ObjectId::from_hex(value.base.as_bytes())?;
-        let path = std::path::PathBuf::from(value.path);
+        let path = gix::path::from_byte_slice(&value.path).to_owned();
 
         Ok(Worktree {
             path,
-            reference: value.reference,
+            reference: gix::refs::FullName::try_from(BString::from(value.reference))?,
             base,
             source,
         })
@@ -63,11 +63,10 @@ impl TryFrom<Worktree> for but_db::Worktree {
     fn try_from(value: Worktree) -> Result<Self, Self::Error> {
         let source = serde_json::to_string(&value.source)?;
         let base = value.base.to_hex().to_string();
-        let path = value.path.to_string_lossy().to_string();
 
         Ok(but_db::Worktree {
-            path,
-            reference: value.reference,
+            path: gix::path::into_bstr(&value.path).to_vec(),
+            reference: value.reference.as_bstr().to_vec(),
             base,
             source,
         })

crates/but-worktrees/src/gc.rs

@@ -1,6 +1,5 @@
 use crate::{Worktree, WorktreeHealthStatus, WorktreeSource};
 use anyhow::Result;
-use bstr::BStr;
 use but_workspace::ui::StackHeadInfo;
 
 pub fn get_health(
@@ -9,7 +8,9 @@ pub fn get_health(
     heads: &[StackHeadInfo],
 ) -> Result<WorktreeHealthStatus> {
     if !heads.iter().any(|h| match &worktree.source {
-        WorktreeSource::Branch(b) => h.name == BStr::new(&b),
+        WorktreeSource::Branch(b) => gix::refs::PartialName::try_from(h.name.clone())
+            .map(|n| n == b.clone())
+            .unwrap_or(false),
     }) {
         return Ok(WorktreeHealthStatus::WorkspaceBranchMissing);
     };
@@ -30,7 +31,7 @@ pub fn get_health(
     if !worktree_repo
         .head()?
         .referent_name()
-        .map(|n| n.as_bstr() == worktree.reference)
+        .map(|n| n == worktree.reference.as_ref())
         .unwrap_or(false)
     {
         return Ok(WorktreeHealthStatus::BranchNotCheckedOut);

crates/but-worktrees/src/git.rs

@@ -1,26 +1,25 @@
 use std::path::Path;
 
 use anyhow::{Result, bail};
-use tracing::{Level, event};
 
 pub(crate) fn git_worktree_add(
     project_path: &Path,
     path: &Path,
-    branch_name: &str,
+    branch_name: &gix::refs::PartialNameRef,
     commit: gix::ObjectId,
 ) -> Result<()> {
     let output =
         std::process::Command::from(gix::command::prepare(gix::path::env::exe_invocation()))
             .current_dir(project_path)
             .arg("worktree")
             .arg("add")
-            .args(["-B", branch_name])
+            .args(["-B", &branch_name.to_string()])
             .arg(path.as_os_str())
             .arg(commit.to_string())
             .output()?;
 
-    event!(Level::INFO, "{}", str::from_utf8(&output.stdout)?);
-    event!(Level::ERROR, "{}", str::from_utf8(&output.stderr)?);
+    tracing::info!("{}", str::from_utf8(&output.stdout)?);
+    tracing::error!("{}", str::from_utf8(&output.stderr)?);
 
     if output.status.success() {
         Ok(())

crates/but-worktrees/src/lib.rs

@@ -19,7 +19,7 @@ pub mod new;
 pub enum WorktreeSource {
     /// The worktree was created from a branch. This is the given name of the
     /// branch
-    Branch(String),
+    Branch(gix::refs::PartialName),
 }
 
 /// A worktree entry representing a Git worktree.
@@ -32,7 +32,7 @@ pub struct Worktree {
     pub path: PathBuf,
     /// The git reference this worktree was created from. This is a fully
     /// qualified reference
-    pub reference: String,
+    pub reference: gix::refs::FullName,
     /// The base commit (ObjectId) from which this worktree was created.
     pub base: gix::ObjectId,
     /// The source from which this worktree was created.

crates/but-worktrees/src/new.rs

@@ -1,7 +1,6 @@
 use std::path::PathBuf;
 
 use anyhow::{Context, Result};
-use bstr::BStr;
 use but_graph::VirtualBranchesTomlMetadata;
 use but_workspace::{StacksFilter, stacks_v3};
 use gitbutler_command_context::CommandContext;
@@ -21,7 +20,7 @@ pub struct NewWorktreeOutcome {
 pub fn worktree_new(
     ctx: &mut CommandContext,
     _perm: &WorktreeReadPermission,
-    refname: &str,
+    refname: &gix::refs::PartialNameRef,
 ) -> Result<NewWorktreeOutcome> {
     let repo = ctx.gix_repo_for_merging()?;
     let meta = VirtualBranchesTomlMetadata::from_path(
@@ -31,22 +30,26 @@ pub fn worktree_new(
     let head = stacks
         .into_iter()
         .flat_map(|s| s.heads)
-        .find(|h| h.name == BStr::new(refname))
+        .find(|h| {
+            gix::refs::PartialName::try_from(h.name.clone())
+                .map(|n| n.as_ref() == refname)
+                .unwrap_or(false)
+        })
         .context("Failed to find matching head")?;
 
     // Used as a method of generating the path & refrence name.
     let id = uuid::Uuid::new_v4();
 
     let path = worktree_path(ctx.project(), id);
-    let branch_name = worktree_branch_name(id);
+    let branch_name = gix::refs::PartialName::try_from(format!("gitbutler/worktree/{}", id))?;
 
-    git_worktree_add(&ctx.project().path, &path, &branch_name, head.tip)?;
+    git_worktree_add(&ctx.project().path, &path, branch_name.as_ref(), head.tip)?;
 
     let worktree = Worktree {
         path: path.canonicalize()?,
-        reference: format!("refs/heads/{}", branch_name),
+        reference: gix::refs::FullName::try_from(format!("refs/heads/gitbutler/worktree/{}", id))?,
         base: head.tip,
-        source: WorktreeSource::Branch(head.name.to_string()),
+        source: WorktreeSource::Branch(refname.to_owned()),
     };
     save_worktree(ctx, worktree.clone())?;
 
@@ -56,7 +59,3 @@ pub fn worktree_new(
 fn worktree_path(project: &Project, id: uuid::Uuid) -> PathBuf {
     project.gb_dir().join("worktrees").join(id.to_string())
 }
-
-fn worktree_branch_name(id: uuid::Uuid) -> String {
-    format!("gitbutler/worktree/{}", id)
-}