From 2a2a24f847972a42dc7b84a36db1def79f19b91e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 17 Mar 2026 18:37:50 +0800 Subject: [PATCH 1/6] Limit VBH in `but-api` to `legacy_meta()` for stacks/details v3 Co-authored-by: GPT 5.4 --- crates/but-api/src/legacy/rules.rs | 17 ++++------------- crates/but-api/src/legacy/workspace.rs | 22 ++++++++++++++++------ etc/plans/consistent-data.md | 7 +++++-- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/crates/but-api/src/legacy/rules.rs b/crates/but-api/src/legacy/rules.rs index 0d494f9609..8e122daddb 100644 --- a/crates/but-api/src/legacy/rules.rs +++ b/crates/but-api/src/legacy/rules.rs @@ -40,19 +40,10 @@ pub fn update_workspace_rule( #[but_api] #[instrument(err(Debug))] pub fn list_workspace_rules(ctx: &mut Context) -> Result> { - let repo = ctx.clone_repo_for_merging_non_persisting()?; - - let in_workspace = { - let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3( - &repo, - &meta, - but_workspace::legacy::StacksFilter::InWorkspace, - None, - &mut cache, - ) - }? + let in_workspace = crate::legacy::workspace::stacks_v3_from_ctx( + ctx, + but_workspace::legacy::StacksFilter::InWorkspace, + )? .iter() .filter_map(|s| s.id) .collect::>(); diff --git a/crates/but-api/src/legacy/workspace.rs b/crates/but-api/src/legacy/workspace.rs index 811132f2b1..90d105e531 100644 --- a/crates/but-api/src/legacy/workspace.rs +++ b/crates/but-api/src/legacy/workspace.rs @@ -6,7 +6,10 @@ use but_core::{RepositoryExt, sync::RepoExclusive}; use but_ctx::Context; use but_hunk_assignment::HunkAssignmentRequest; use but_settings::AppSettings; -use but_workspace::{commit_engine, commit_engine::StackSegmentId, legacy::ui::StackEntry}; +use but_workspace::{ + commit_engine::{self, StackSegmentId}, + legacy::{StacksFilter, ui::StackEntry}, +}; use gitbutler_branch_actions::{BranchManagerExt, update_workspace_commit}; use gitbutler_commit::commit_ext::CommitExt; use gitbutler_oplog::{ @@ -52,7 +55,7 @@ mod json { #[instrument(err(Debug))] pub fn head_info(ctx: &but_ctx::Context) -> Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut cache = ctx.cache.get_cache_mut()?; but_workspace::head_info( &repo, @@ -72,10 +75,18 @@ pub fn stacks( ctx: &Context, filter: Option, ) -> Result> { + stacks_v3_from_ctx(ctx, filter.unwrap_or_default()) +} +/// +/// Return stack information for the repository that `ctx` refers to using legacy metadata. +pub(crate) fn stacks_v3_from_ctx( + ctx: &Context, + filter: StacksFilter, +) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.legacy_meta()?; let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, filter.unwrap_or_default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, filter, None, &mut cache) } #[cfg(unix)] @@ -83,13 +94,12 @@ pub fn stacks( #[instrument(err(Debug))] pub fn show_graph_svg(ctx: &Context) -> Result<()> { let repo = ctx.open_isolated_repo()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut graph = but_graph::Graph::from_head( &repo, &meta, but_graph::init::Options { collect_tags: true, - extra_target_commit_id: meta.data().default_target.as_ref().map(|t| t.sha), ..but_graph::init::Options::limited() }, )?; @@ -263,7 +273,7 @@ pub fn branch_details( ) -> Result { let mut details = { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let ref_name: gix::refs::FullName = match remote.as_deref() { None => { format!("refs/heads/{branch_name}") diff --git a/etc/plans/consistent-data.md b/etc/plans/consistent-data.md index 22d2f3cda8..a3f57f3444 100644 --- a/etc/plans/consistent-data.md +++ b/etc/plans/consistent-data.md @@ -54,7 +54,7 @@ The `but_ctx::Context` is the solution to this problem, and is passed around as ### 1. Sync `.git/gitbutler/virtual-branches.toml` with database representation -- [ ] [in progress](https://github.com/gitbutlerapp/gitbutler/issues/12075) +- [x] https://github.com/gitbutlerapp/gitbutler/issues/12075 ### 2. Port legacy virtual-branches consumers to `ctx.ws` @@ -69,9 +69,12 @@ The crate list below started from direct `VirtualBranchesHandle` references and callers that only touch the same legacy state via `ctx.virtual_branches()` or the legacy read/write helpers. -#### but-api (1) +#### but-api (2) + +`legacy_meta()` and `legacy_meta_mut()`, needed for stacks/details V3 and vb.toml reconciliation. - [ ] `crates/but-api/src/legacy/workspace.rs` +- [ ] `crates/but-api/src/legacy/meta.rs` #### but-claude (1) From a312e33d6b1a03ba777805c07ab3d4e8d74dad6b Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Tue, 17 Mar 2026 19:27:43 +0800 Subject: [PATCH 2/6] Make VirtualBrnachesState require mutation That way it's clear when mutations happens. Co-authored-by: Sebastian Thiel --- crates/but-action/src/lib.rs | 2 +- crates/but-action/src/simple.rs | 8 +++--- .../src/legacy/commit_engine/mod.rs | 2 +- crates/but-workspace/src/legacy/head.rs | 2 +- .../legacy/tree_manipulation/split_branch.rs | 4 +-- .../gitbutler-branch-actions/src/actions.rs | 4 +-- crates/gitbutler-branch-actions/src/base.rs | 6 ++--- .../src/branch_manager/branch_creation.rs | 12 ++++----- .../src/branch_manager/branch_removal.rs | 2 +- .../src/branch_upstream_integration.rs | 4 +-- .../src/integration.rs | 4 +-- .../src/move_branch.rs | 14 +++++----- .../src/move_commits.rs | 9 ++++--- .../gitbutler-branch-actions/src/reorder.rs | 4 +-- crates/gitbutler-branch-actions/src/squash.rs | 4 +-- .../src/undo_commit.rs | 4 +-- .../src/upstream_integration.rs | 4 +-- .../gitbutler-branch-actions/src/virtual.rs | 6 ++--- .../tests/branch-actions/driverless.rs | 3 ++- crates/gitbutler-oplog/src/oplog.rs | 6 ++--- crates/gitbutler-stack/src/stack.rs | 19 ++++++------- crates/gitbutler-stack/src/state.rs | 20 +++++++------- crates/gitbutler-stack/tests/mod.rs | 27 ++++++++++--------- crates/gitbutler-testsupport/src/lib.rs | 2 +- etc/plans/consistent-data.md | 17 ++++++++++++ 25 files changed, 105 insertions(+), 84 deletions(-) diff --git a/crates/but-action/src/lib.rs b/crates/but-action/src/lib.rs index 09b09ebd5c..7a74b59e3d 100644 --- a/crates/but-action/src/lib.rs +++ b/crates/but-action/src/lib.rs @@ -101,7 +101,7 @@ pub fn handle_changes( fn default_target_setting_if_none( ctx: &Context, - vb_state: &VirtualBranchesHandle, + vb_state: &mut VirtualBranchesHandle, ) -> anyhow::Result { if let Ok(default_target) = vb_state.get_default_target() { return Ok(default_target); diff --git a/crates/but-action/src/simple.rs b/crates/but-action/src/simple.rs index 09daa88c21..b1c1f92d87 100644 --- a/crates/but-action/src/simple.rs +++ b/crates/but-action/src/simple.rs @@ -33,8 +33,8 @@ pub(crate) fn handle_changes( let mut guard = ctx.exclusive_worktree_access(); let perm = guard.write_permission(); - let vb_state = &VirtualBranchesHandle::new(ctx.project_data_dir()); - default_target_setting_if_none(ctx, vb_state)?; // Create a default target if none exists. + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + default_target_setting_if_none(ctx, &mut vb_state)?; // Create a default target if none exists. let snapshot_before = ctx.create_snapshot( SnapshotDetails::new(OperationKind::AutoHandleChangesBefore), @@ -45,7 +45,7 @@ pub(crate) fn handle_changes( ctx, change_summary, external_prompt.clone(), - vb_state, + &mut vb_state, perm, exclusive_stack, ); @@ -73,7 +73,7 @@ fn handle_changes_simple_inner( ctx: &mut Context, change_summary: &str, external_prompt: Option, - vb_state: &VirtualBranchesHandle, + vb_state: &mut VirtualBranchesHandle, perm: &mut RepoExclusive, exclusive_stack: Option, ) -> anyhow::Result { diff --git a/crates/but-workspace/src/legacy/commit_engine/mod.rs b/crates/but-workspace/src/legacy/commit_engine/mod.rs index 79187121fa..8083f654d6 100644 --- a/crates/but-workspace/src/legacy/commit_engine/mod.rs +++ b/crates/but-workspace/src/legacy/commit_engine/mod.rs @@ -413,7 +413,7 @@ pub fn create_commit_and_update_refs_with_project( context_lines: u32, _perm: &mut RepoExclusive, ) -> anyhow::Result { - let vbh = VirtualBranchesHandle::new(project_data_dir); + let mut vbh = VirtualBranchesHandle::new(project_data_dir); let mut vb = vbh.read_file()?; let frame = match maybe_stackid { None => { diff --git a/crates/but-workspace/src/legacy/head.rs b/crates/but-workspace/src/legacy/head.rs index 4fbf246274..265fb7a607 100644 --- a/crates/but-workspace/src/legacy/head.rs +++ b/crates/but-workspace/src/legacy/head.rs @@ -47,7 +47,7 @@ pub fn remerged_workspace_tree_v2( ctx: &Context, repo: &gix::Repository, ) -> Result<(gix::ObjectId, Vec, gix::ObjectId)> { - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let target = vb_state .get_default_target() .context("failed to get target")?; diff --git a/crates/but-workspace/src/legacy/tree_manipulation/split_branch.rs b/crates/but-workspace/src/legacy/tree_manipulation/split_branch.rs index 28c8fdb59a..b4468ccbcc 100644 --- a/crates/but-workspace/src/legacy/tree_manipulation/split_branch.rs +++ b/crates/but-workspace/src/legacy/tree_manipulation/split_branch.rs @@ -157,7 +157,7 @@ pub fn split_into_dependent_branch( file_changes_to_split_off: &[String], perm: &mut RepoExclusive, ) -> Result { - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let source_stack = vb_state.get_stack_in_workspace(stack_id)?; let merge_base = source_stack.merge_base(ctx)?; @@ -234,7 +234,7 @@ pub fn split_into_dependent_branch( Some(source_branch_name), )?; - source_stack.set_stack_head(&vb_state, &repo, new_head.id().detach())?; + source_stack.set_stack_head(&mut vb_state, &repo, new_head.id().detach())?; source_stack.set_heads_from_rebase_output(ctx, source_result.clone().references)?; let move_changes_result = MoveChangesResult { diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index 28573a2c0e..d517e94eb1 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/crates/gitbutler-branch-actions/src/actions.rs @@ -61,7 +61,7 @@ pub fn delete_local_branch(ctx: &mut Context, refname: &Refname, given_name: Str let mut guard = ctx.exclusive_worktree_access(); ctx.verify(guard.write_permission())?; let repo = &*ctx.repo.get()?; - let handle = ctx.virtual_branches(); + let mut handle = ctx.virtual_branches(); let stack = handle.list_all_stacks()?.into_iter().find(|stack| { stack .source_refname @@ -364,7 +364,7 @@ pub fn fetch_from_remotes(ctx: &Context, askpass: Option) -> Result Result { /// there wasn't enough repository state to infer a safe target. #[instrument(skip(ctx), err(Debug))] pub fn bootstrap_default_target_if_missing(ctx: &Context) -> Result { - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); if vb_state.maybe_get_default_target()?.is_some() { return Ok(false); } @@ -204,7 +204,7 @@ pub(crate) fn set_base_branch( push_remote_name: None, }; - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); vb_state.set_default_target(target.clone())?; // TODO: make sure this is a real branch @@ -281,7 +281,7 @@ pub(crate) fn set_target_push_remote(ctx: &Context, push_remote_name: &str) -> R .context("failed to get remote name")? .to_string() .into(); - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); vb_state.set_default_target(target)?; Ok(()) diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs index 98ca84945e..397c7c91e7 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs @@ -56,7 +56,7 @@ impl BranchManager<'_> { create: &BranchCreateRequest, perm: &mut RepoExclusive, ) -> Result { - let vb_state = self.ctx.virtual_branches(); + let mut vb_state = self.ctx.virtual_branches(); let default_target = vb_state.get_default_target()?; let mut all_stacks = vb_state @@ -188,7 +188,7 @@ impl BranchManager<'_> { } }; - let vb_state = self.ctx.virtual_branches(); + let mut vb_state = self.ctx.virtual_branches(); let default_target = vb_state.get_default_target()?; @@ -240,8 +240,8 @@ impl BranchManager<'_> { branch.set_pr_number(self.ctx, head, Some(pr_number))?; } branch.set_stack_head( - &vb_state, - &(&*git2_repo).to_isolated_gix_repo()?, + &mut vb_state, + &(&*git2_repo).to_gix_repo()?, head_commit.id().to_gix(), )?; self.ctx.add_branch_reference(&branch)?; @@ -273,7 +273,7 @@ impl BranchManager<'_> { ) -> Result<(String, Vec)> { let git2_repo = &*self.ctx.git2_repo.get()?; - let vb_state = self.ctx.virtual_branches(); + let mut vb_state = self.ctx.virtual_branches(); let default_target = vb_state.get_default_target()?; let mut stack = vb_state.get_stack_in_workspace(stack_id)?; @@ -350,7 +350,7 @@ impl BranchManager<'_> { let output = rebase.rebase(&*self.ctx.cache.get_cache()?)?; let new_head = git2_repo.find_commit(output.top_commit.to_git2())?; - stack.set_stack_head(&vb_state, &repo, new_head.id().to_gix())?; + stack.set_stack_head(&mut vb_state, &repo, new_head.id().to_gix())?; stack.set_heads_from_rebase_output(self.ctx, output.references)?; } diff --git a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs index 2cd0bd6f68..9185da7ed3 100644 --- a/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs +++ b/crates/gitbutler-branch-actions/src/branch_manager/branch_removal.rs @@ -22,7 +22,7 @@ impl BranchManager<'_> { assigned_diffspec: Vec, safe_checkout: bool, ) -> Result { - let vb_state = self.ctx.virtual_branches(); + let mut vb_state = self.ctx.virtual_branches(); let mut stack = vb_state.get_stack(stack_id)?; // We don't want to try unapplying branches which are marked as not in workspace by the new metric diff --git a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs b/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs index d6828a914f..2eb1367ec7 100644 --- a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs +++ b/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs @@ -101,7 +101,7 @@ pub fn integrate_branch_with_steps( ) -> Result<()> { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; let repo = ctx.repo.get()?; - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let mut source_stack = vb_state.get_stack_in_workspace(stack_id)?; let merge_base = source_stack.merge_base(ctx)?; @@ -163,7 +163,7 @@ pub fn integrate_branch_with_steps( let result = rebase.rebase(&*ctx.cache.get_cache()?)?; let head = result.top_commit.to_git2(); - source_stack.set_stack_head(&vb_state, &repo, head.to_gix())?; + source_stack.set_stack_head(&mut vb_state, &repo, head.to_gix())?; let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?; update_uncommitted_changes(ctx, old_workspace, new_workspace, perm)?; source_stack.set_heads_from_rebase_output(ctx, result.references)?; diff --git a/crates/gitbutler-branch-actions/src/integration.rs b/crates/gitbutler-branch-actions/src/integration.rs index 0b914a4de6..70ace506fc 100644 --- a/crates/gitbutler-branch-actions/src/integration.rs +++ b/crates/gitbutler-branch-actions/src/integration.rs @@ -257,7 +257,7 @@ fn verify_head_is_clean(ctx: &Context, perm: &mut RepoExclusive) -> Result<()> { let gix_repo = git2_repo.to_gix_repo()?; let head_commit_id = gix_repo.head_id()?.detach(); - let vb_handle = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_handle = VirtualBranchesHandle::new(ctx.project_data_dir()); let default_target = vb_handle .get_default_target() .context("failed to get default target")?; @@ -336,7 +336,7 @@ fn verify_head_is_clean(ctx: &Context, perm: &mut RepoExclusive) -> Result<()> { ))?; head = rebased_commit_oid.to_gix(); - new_branch.set_stack_head(&vb_handle, &gix_repo, head)?; + new_branch.set_stack_head(&mut vb_handle, &gix_repo, head)?; } Ok(()) } diff --git a/crates/gitbutler-branch-actions/src/move_branch.rs b/crates/gitbutler-branch-actions/src/move_branch.rs index 0bc69e3ad5..beef3ebbfe 100644 --- a/crates/gitbutler-branch-actions/src/move_branch.rs +++ b/crates/gitbutler-branch-actions/src/move_branch.rs @@ -31,7 +31,7 @@ pub(crate) fn move_branch( ) -> Result { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; let repo = ctx.repo.get()?; - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?; let source_merge_base = source_stack.merge_base(ctx)?; @@ -50,7 +50,7 @@ pub(crate) fn move_branch( source_stack_id, subject_branch_name, &repo, - &vb_state, + &mut vb_state, source_stack, source_merge_base, )?; @@ -61,7 +61,7 @@ pub(crate) fn move_branch( target_branch_name, subject_branch_name, &repo, - &vb_state, + &mut vb_state, destination_stack, destination_merge_base, subject_branch_steps, @@ -88,7 +88,7 @@ pub(crate) fn tear_off_branch( ) -> Result { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; let repo = ctx.repo.get()?; - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?; let source_merge_base = source_stack.merge_base(ctx)?; @@ -98,7 +98,7 @@ pub(crate) fn tear_off_branch( source_stack_id, subject_branch_name, &repo, - &vb_state, + &mut vb_state, source_stack, source_merge_base, )?; @@ -151,7 +151,7 @@ fn inject_branch_steps_into_destination( target_branch_name: &str, subject_branch_name: &str, repo: &gix::Repository, - vb_state: &VirtualBranchesHandle, + vb_state: &mut VirtualBranchesHandle, destination_stack: gitbutler_stack::Stack, destination_merge_base: gix::ObjectId, subject_branch_steps: Vec, @@ -200,7 +200,7 @@ fn extract_and_rebase_source_branch( source_stack_id: StackId, subject_branch_name: &str, repository: &gix::Repository, - vb_state: &VirtualBranchesHandle, + vb_state: &mut VirtualBranchesHandle, source_stack: gitbutler_stack::Stack, source_merge_base: gix::ObjectId, ) -> Result<(Vec, Vec), anyhow::Error> { diff --git a/crates/gitbutler-branch-actions/src/move_commits.rs b/crates/gitbutler-branch-actions/src/move_commits.rs index b14eb0868b..daca88e697 100644 --- a/crates/gitbutler-branch-actions/src/move_commits.rs +++ b/crates/gitbutler-branch-actions/src/move_commits.rs @@ -19,7 +19,7 @@ pub(crate) fn move_commit( source_stack_id: StackId, ) -> Result> { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); let repo = ctx.repo.get()?; let applied_stacks = vb_state @@ -43,7 +43,7 @@ pub(crate) fn move_commit( take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?; - move_commit_to_destination_stack(&vb_state, ctx, destination_stack, subject_commit_oid)?; + move_commit_to_destination_stack(&mut vb_state, ctx, destination_stack, subject_commit_oid)?; let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?; // Even if this fails, it's not actionable @@ -92,13 +92,14 @@ fn take_commit_from_source_stack( let output = rebase.rebase(&*ctx.cache.get_cache()?)?; source_stack.set_heads_from_rebase_output(ctx, output.references)?; - source_stack.set_stack_head(&ctx.virtual_branches(), &repo, output.top_commit)?; + let mut vb_state = ctx.virtual_branches(); + source_stack.set_stack_head(&mut vb_state, &repo, output.top_commit)?; Ok(None) } /// Move the commit to the destination stack. fn move_commit_to_destination_stack( - vb_state: &VirtualBranchesHandle, + vb_state: &mut VirtualBranchesHandle, ctx: &Context, mut destination_stack: gitbutler_stack::Stack, commit_id: gix::ObjectId, diff --git a/crates/gitbutler-branch-actions/src/reorder.rs b/crates/gitbutler-branch-actions/src/reorder.rs index 5038dff39f..faa0b34905 100644 --- a/crates/gitbutler-branch-actions/src/reorder.rs +++ b/crates/gitbutler-branch-actions/src/reorder.rs @@ -28,7 +28,7 @@ pub fn reorder_stack( perm: &mut RepoExclusive, ) -> Result { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; - let state = ctx.virtual_branches(); + let mut state = ctx.virtual_branches(); let git2_repo = &*ctx.git2_repo.get()?; let mut stack = state.get_stack(stack_id)?; let current_order = commits_order(ctx, &stack)?; @@ -62,7 +62,7 @@ pub fn reorder_stack( let new_head = output.top_commit.to_git2(); // Ensure the stack head is set to the new oid after rebasing - stack.set_stack_head(&state, &repo, new_head.to_gix())?; + stack.set_stack_head(&mut state, &repo, new_head.to_gix())?; stack.set_heads_from_rebase_output(ctx, output.references.clone())?; diff --git a/crates/gitbutler-branch-actions/src/squash.rs b/crates/gitbutler-branch-actions/src/squash.rs index f2186d5dc2..75470dfba1 100644 --- a/crates/gitbutler-branch-actions/src/squash.rs +++ b/crates/gitbutler-branch-actions/src/squash.rs @@ -63,7 +63,7 @@ fn do_squash_commits( ) -> Result { let new_commit_oid = { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); let stack = vb_state.get_stack_in_workspace(stack_id)?; let repo = ctx.repo.get()?; @@ -264,7 +264,7 @@ fn do_squash_commits( let new_stack_head = output.top_commit.to_git2(); - stack.set_stack_head(&vb_state, &repo, new_stack_head.to_gix())?; + stack.set_stack_head(&mut vb_state, &repo, new_stack_head.to_gix())?; let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?; update_uncommitted_changes(ctx, old_workspace, new_workspace, perm)?; diff --git a/crates/gitbutler-branch-actions/src/undo_commit.rs b/crates/gitbutler-branch-actions/src/undo_commit.rs index 29b0af6701..739d5ccf48 100644 --- a/crates/gitbutler-branch-actions/src/undo_commit.rs +++ b/crates/gitbutler-branch-actions/src/undo_commit.rs @@ -26,7 +26,7 @@ pub(crate) fn undo_commit( commit_to_remove: gix::ObjectId, _perm: &mut RepoExclusive, ) -> Result { - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); let mut stack = vb_state.get_stack_in_workspace(stack_id)?; @@ -50,7 +50,7 @@ pub(crate) fn undo_commit( let output = rebase.rebase(&*ctx.cache.get_cache()?)?; let new_head = output.top_commit.to_git2(); - stack.set_stack_head(&vb_state, &repo, new_head.to_gix())?; + stack.set_stack_head(&mut vb_state, &repo, new_head.to_gix())?; stack.set_heads_from_rebase_output(ctx, output.references)?; diff --git a/crates/gitbutler-branch-actions/src/upstream_integration.rs b/crates/gitbutler-branch-actions/src/upstream_integration.rs index fcc66ab3c5..2cfb5aa49d 100644 --- a/crates/gitbutler-branch-actions/src/upstream_integration.rs +++ b/crates/gitbutler-branch-actions/src/upstream_integration.rs @@ -454,7 +454,7 @@ pub(crate) fn integrate_upstream( let repo = ctx.repo.get()?; let context = UpstreamIntegrationContext::open(ctx, target_commit_oid, permission, &repo, review_map)?; - let virtual_branches_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut virtual_branches_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let default_target = virtual_branches_state.get_default_target()?; let mut deleted_branches = vec![]; @@ -606,7 +606,7 @@ pub(crate) fn integrate_upstream( } } - stack.set_stack_head(&virtual_branches_state, &repo, *head)?; + stack.set_stack_head(&mut virtual_branches_state, &repo, *head)?; let delete_local_refs = resolutions .iter() diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index 3228c59305..ae95cbbaa8 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -37,7 +37,7 @@ impl From for crate::author::Author { } pub fn update_stack(ctx: &Context, update: &BranchUpdateRequest) -> Result { - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); let mut stack = vb_state.get_stack_in_workspace(update.id.context("BUG(opt-stack-id)")?)?; if let Some(order) = update.order { @@ -175,7 +175,7 @@ pub(crate) fn update_commit_message( if message.is_empty() { bail!("commit message can not be empty"); } - let vb_state = ctx.virtual_branches(); + let mut vb_state = ctx.virtual_branches(); let default_target = vb_state.get_default_target()?; let repo = ctx.repo.get()?; @@ -208,7 +208,7 @@ pub(crate) fn update_commit_message( rebase.rebase(&*ctx.cache.get_cache()?)? }; - stack.set_stack_head(&vb_state, &repo, output.top_commit)?; + stack.set_stack_head(&mut vb_state, &repo, output.top_commit)?; stack.set_heads_from_rebase_output(ctx, output.references)?; crate::integration::update_workspace_commit_with_vb_state(&vb_state, ctx, false) diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/driverless.rs b/crates/gitbutler-branch-actions/tests/branch-actions/driverless.rs index e52c8ef898..a62f77f64d 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/driverless.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/driverless.rs @@ -199,7 +199,8 @@ fn write_workspace_metadata(repo: &gix::Repository, stacks: &[StackSpec<'_>]) -> meta.set_changed_to_necessitate_write(); meta.write_unreconciled()?; - VirtualBranchesHandle::new(repo.gitbutler_storage_path()?).set_default_target(Target { + let mut handle = VirtualBranchesHandle::new(repo.gitbutler_storage_path()?); + handle.set_default_target(Target { branch: "refs/remotes/origin/main".parse()?, remote_url: ".".to_owned(), sha: repo.rev_parse_single("refs/remotes/origin/main")?.detach(), diff --git a/crates/gitbutler-oplog/src/oplog.rs b/crates/gitbutler-oplog/src/oplog.rs index ea63b5f8b7..09a333e21a 100644 --- a/crates/gitbutler-oplog/src/oplog.rs +++ b/crates/gitbutler-oplog/src/oplog.rs @@ -369,7 +369,7 @@ fn get_workdir_tree( pub fn prepare_snapshot(ctx: &Context, _shared_access: &RepoShared) -> Result { let git2_repo = ctx.git2_repo.get()?; - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); // grab the target commit let default_target_commit = @@ -409,7 +409,7 @@ pub fn prepare_snapshot(ctx: &Context, _shared_access: &RepoShared) -> Result Result<()> { self.ensure_initialized()?; (self.heads, _) = remove_head(self.heads.clone(), branch_name, &*ctx.repo.get()?)?; - let state = branch_state(ctx); + let mut state = branch_state(ctx); state.set_stack(self.clone()) } @@ -425,7 +425,7 @@ impl Stack { return Ok(()); // noop } - let state = branch_state(ctx); + let mut state = branch_state(ctx); let mut updated_heads = self.heads.clone(); // Handle name updates @@ -453,7 +453,7 @@ impl Stack { /// TODO: is there a performance implication of this? pub fn sync_heads_with_references( &mut self, - state: &VirtualBranchesHandle, + state: &mut VirtualBranchesHandle, gix_repo: &gix::Repository, ) -> Result<()> { if self @@ -474,7 +474,7 @@ impl Stack { /// - the tree of the stack to the new tree (if provided) pub fn set_stack_head( &mut self, - state: &VirtualBranchesHandle, + state: &mut VirtualBranchesHandle, gix_repo: &gix::Repository, commit_id: gix::ObjectId, ) -> Result<()> { @@ -483,7 +483,7 @@ impl Stack { fn set_stack_head_inner( &mut self, - state: Option<&VirtualBranchesHandle>, + state: Option<&mut VirtualBranchesHandle>, gix_repo: &gix::Repository, commit_id: gix::ObjectId, ) -> Result<()> { @@ -515,7 +515,7 @@ impl Stack { let mut deleted_branches = vec![]; - let state = branch_state(ctx); + let mut state = branch_state(ctx); for head in self.heads.iter_mut() { let full_name = head.full_name()?; if for_archival.iter().any(|reference| match reference { @@ -588,7 +588,7 @@ impl Stack { project_data_dir: &Path, new_heads: HashMap, ) -> Result<()> { - let state = branch_state_from_project_data_dir(project_data_dir); + let mut state = branch_state_from_project_data_dir(project_data_dir); // same heads, just different commits if self @@ -640,7 +640,8 @@ impl Stack { match self.heads.iter_mut().find(|r| r.name() == branch_name) { Some(head) => { head.pr_number = new_pr_number; - branch_state(ctx).set_stack(self.clone()) + let mut state = branch_state(ctx); + state.set_stack(self.clone()) } None => bail!( "Series {} does not exist on stack {}", diff --git a/crates/gitbutler-stack/src/state.rs b/crates/gitbutler-stack/src/state.rs index 33d584a8b5..12a24bf71f 100644 --- a/crates/gitbutler-stack/src/state.rs +++ b/crates/gitbutler-stack/src/state.rs @@ -123,7 +123,7 @@ impl VirtualBranchesHandle { /// Persists the default target for the given repository. /// /// Errors if the file cannot be read or written. - pub fn set_default_target(&self, target: Target) -> Result<()> { + pub fn set_default_target(&mut self, target: Target) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.default_target = Some(target); self.write_file(&virtual_branches)?; @@ -151,7 +151,7 @@ impl VirtualBranchesHandle { /// Sets the state of the given virtual branch. /// /// Errors if the file cannot be read or written. - pub fn set_stack(&self, stack: Stack) -> Result<()> { + pub fn set_stack(&mut self, stack: Stack) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.branches.insert(stack.id, stack); self.write_file(&virtual_branches)?; @@ -161,7 +161,7 @@ impl VirtualBranchesHandle { /// Marks a particular branch as not in the workspace /// /// Errors if the file cannot be read or written. - pub fn mark_as_not_in_workspace(&self, id: StackId) -> Result<()> { + pub fn mark_as_not_in_workspace(&mut self, id: StackId) -> Result<()> { let mut stack = self.get_stack(id)?; stack.in_workspace = false; self.set_stack(stack)?; @@ -267,7 +267,7 @@ impl VirtualBranchesHandle { } /// Write the given `virtual_branches` back to disk in one go. - pub fn write_file(&self, virtual_branches: &VirtualBranches) -> Result<()> { + pub fn write_file(&mut self, virtual_branches: &VirtualBranches) -> Result<()> { let _ = self.ensure_vb_storage_in_sync()?; let legacy = virtual_branches_legacy_types::VirtualBranches::from(virtual_branches.clone()); but_meta::legacy_storage::write_virtual_branches_and_sync(&self.file_path, &legacy) @@ -281,11 +281,11 @@ impl VirtualBranchesHandle { /// Import TOML into DB and refresh sync metadata. /// /// This is primarily used for oplog restore, where TOML was restored externally. - pub fn import_toml_into_db_for_restore(&self) -> Result<()> { + pub fn import_toml_into_db_for_restore(&mut self) -> Result<()> { but_meta::legacy_storage::import_toml_into_db(&self.file_path) } - pub fn update_ordering(&self) -> Result<()> { + pub fn update_ordering(&mut self) -> Result<()> { let succeeded = self .list_stacks_in_workspace()? .iter() @@ -304,7 +304,7 @@ impl VirtualBranchesHandle { } } - pub fn next_order_index(&self) -> Result { + pub fn next_order_index(&mut self) -> Result { self.update_ordering()?; let order = self .list_stacks_in_workspace()? @@ -317,7 +317,7 @@ impl VirtualBranchesHandle { Ok(order) } - pub fn delete_branch_entry(&self, branch_id: &StackId) -> Result<()> { + pub fn delete_branch_entry(&mut self, branch_id: &StackId) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.branches.remove(branch_id); self.write_file(&virtual_branches)?; @@ -329,7 +329,7 @@ impl VirtualBranchesHandle { /// 2. They have no regular commits /// /// Also collects branches with a head oid pointing to a commit that can't be found in the repo - pub fn garbage_collect(&self, repo: &gix::Repository) -> Result<()> { + pub fn garbage_collect(&mut self, repo: &gix::Repository) -> Result<()> { let target = self.get_default_target()?; let stacks_not_in_workspace = self .list_all_stacks()? @@ -379,7 +379,7 @@ impl VirtualBranchesHandle { /// /// This function will return `Ok(None)` if there is no default target. pub fn upsert_last_pushed_base( - &self, + &mut self, repository: &gix::Repository, ) -> Result> { let mut virtual_branches = self.read_file()?; diff --git a/crates/gitbutler-stack/tests/mod.rs b/crates/gitbutler-stack/tests/mod.rs index 43d319d098..7eb62acc79 100644 --- a/crates/gitbutler-stack/tests/mod.rs +++ b/crates/gitbutler-stack/tests/mod.rs @@ -429,7 +429,7 @@ fn push_series_success() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let test_ctx = test_ctx(&ctx)?; - let state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut state = VirtualBranchesHandle::new(ctx.project_data_dir()); let mut target = state.get_default_target()?; target.push_remote_name = Some("origin".into()); state.set_default_target(target)?; @@ -444,7 +444,7 @@ fn update_name_after_push() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; - let state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut state = VirtualBranchesHandle::new(ctx.project_data_dir()); let mut target = state.get_default_target()?; target.push_remote_name = Some("origin".into()); state.set_default_target(target)?; @@ -578,11 +578,11 @@ fn list_series_two_heads_different_commit() -> Result<()> { fn set_stack_head_commit_invalid() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let repo = ctx.repo.get()?; let result = test_ctx .stack - .set_stack_head(&vb_state, &repo, git2::Oid::zero().to_gix()); + .set_stack_head(&mut vb_state, &repo, git2::Oid::zero().to_gix()); assert!(result.is_err()); Ok(()) } @@ -592,11 +592,11 @@ fn set_stack_head() -> Result<()> { let (ctx, _temp_dir) = command_ctx("multiple-commits")?; let mut test_ctx = test_ctx(&ctx)?; let commit = test_ctx.other_commits.last().unwrap(); - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let repo = ctx.repo.get()?; let result = test_ctx .stack - .set_stack_head(&vb_state, &repo, commit.id().to_gix()); + .set_stack_head(&mut vb_state, &repo, commit.id().to_gix()); assert!(result.is_ok()); let branches = test_ctx.stack.branches(); assert_eq!( @@ -814,7 +814,8 @@ fn seed_metadata(repo: &gix::Repository, name: &str) -> Result<()> { sha: repo.rev_parse_single("refs/remotes/origin/main")?.detach(), push_remote_name: Some("origin".to_owned()), }; - VirtualBranchesHandle::new(repo.gitbutler_storage_path()?).set_default_target(target)?; + let mut handle = VirtualBranchesHandle::new(repo.gitbutler_storage_path()?); + handle.set_default_target(target)?; Ok(()) } @@ -963,7 +964,7 @@ fn storage_sync_recreates_toml_when_missing() -> Result<()> { #[test] fn storage_sync_db_mutation_always_updates_toml_mirror() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; let toml_path = tmp.path().join("virtual_branches.toml"); fs::remove_file(&toml_path)?; @@ -993,7 +994,7 @@ fn storage_sync_db_mutation_always_updates_toml_mirror() -> Result<()> { #[test] fn storage_sync_newer_toml_overwrites_db() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; handle.set_default_target(stack_target( "main", @@ -1019,7 +1020,7 @@ fn storage_sync_newer_toml_overwrites_db() -> Result<()> { #[test] fn storage_sync_equal_mtime_and_changed_hash_overwrites_db() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; handle.set_default_target(stack_target( "main", @@ -1052,7 +1053,7 @@ fn storage_sync_equal_mtime_and_changed_hash_overwrites_db() -> Result<()> { #[test] fn storage_sync_older_toml_does_not_overwrite_db() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; handle.set_default_target(stack_target( "main", @@ -1096,7 +1097,7 @@ fn storage_sync_older_toml_does_not_overwrite_db() -> Result<()> { #[test] fn storage_sync_invalid_newer_toml_is_rewritten_from_db() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; handle.set_default_target(stack_target( "main", @@ -1130,7 +1131,7 @@ fn storage_sync_invalid_newer_toml_is_rewritten_from_db() -> Result<()> { #[test] fn storage_sync_restore_import_helper_imports_toml_into_db() -> Result<()> { let tmp = tempfile::tempdir()?; - let handle = VirtualBranchesHandle::new(tmp.path()); + let mut handle = VirtualBranchesHandle::new(tmp.path()); let _ = handle.read_file()?; handle.set_default_target(stack_target( "db-main", diff --git a/crates/gitbutler-testsupport/src/lib.rs b/crates/gitbutler-testsupport/src/lib.rs index f7b9e33207..b3816f1334 100644 --- a/crates/gitbutler-testsupport/src/lib.rs +++ b/crates/gitbutler-testsupport/src/lib.rs @@ -35,7 +35,7 @@ pub mod virtual_branches { use crate::empty_bare_repository; pub fn set_test_target(ctx: &Context) -> anyhow::Result<()> { - let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); + let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); let (remote_repo, _tmp) = empty_bare_repository(); let git2_repo = &*ctx.git2_repo.get()?; let mut remote = git2_repo diff --git a/etc/plans/consistent-data.md b/etc/plans/consistent-data.md index a3f57f3444..950f6c1ffb 100644 --- a/etc/plans/consistent-data.md +++ b/etc/plans/consistent-data.md @@ -200,6 +200,23 @@ Also write the database after _each change_ so it's semantically similar to how Sync the TOML file _on drop_ only, knowing well that this may write data that is going to be rolled back. The TOML sync is only for backward compatibility with older application versions. +- [ ] First migrate every remaining `ctx.legacy_meta()` caller to `ctx.meta()` + - Current baseline: + - `crates/but-action/src/lib.rs` + - `crates/but-action/src/reword.rs` + - `crates/but-cherry-apply/tests/cherry_apply/main.rs` + - `crates/but-claude/src/hooks/mod.rs` + - `crates/but-cursor/src/lib.rs` + - `crates/but-testing/src/command/mod.rs` + - `crates/but-tools/src/workspace.rs` + - `crates/but-workspace/src/legacy/mod.rs` + - `crates/but-worktrees/tests/worktree/main.rs` + - `crates/but/src/command/legacy/mcp_internal/stack.rs` + - `crates/but/src/legacy/commits.rs` + - `crates/gitbutler-branch-actions/src/upstream_integration.rs` + - `crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs` + - `crates/gitbutler-cli/src/command/vbranch.rs` + - `crates/gitbutler-testsupport/src/lib.rs` - [ ] Not started ### 4. Modernize workspace metadata schema From c9781538b06845cd55cf5ffbc8a3e69e931d243d Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Thu, 19 Mar 2026 15:25:58 +0800 Subject: [PATCH 3/6] Remove legacy_meta() requirement for stacks/stack_details v3 That way they don't need direct access to data that soon enough won't exist anymore. In theory this can be postponed until no code exists that relies on that data. Co-authored-by: Sebastian Thiel --- crates/but-api/src/legacy/workspace.rs | 4 +- crates/but-workspace/src/legacy/stacks.rs | 163 ++++++++++++---------- etc/plans/consistent-data.md | 2 +- 3 files changed, 89 insertions(+), 80 deletions(-) diff --git a/crates/but-api/src/legacy/workspace.rs b/crates/but-api/src/legacy/workspace.rs index 90d105e531..f75fe7c1dc 100644 --- a/crates/but-api/src/legacy/workspace.rs +++ b/crates/but-api/src/legacy/workspace.rs @@ -84,7 +84,7 @@ pub(crate) fn stacks_v3_from_ctx( filter: StacksFilter, ) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3(&repo, &meta, filter, None, &mut cache) } @@ -185,7 +185,7 @@ pub fn stack_details( ) -> Result { let mut details = { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta, &mut cache) }?; diff --git a/crates/but-workspace/src/legacy/stacks.rs b/crates/but-workspace/src/legacy/stacks.rs index a3afe4c746..e623c950bb 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -3,9 +3,11 @@ use std::collections::{HashMap, HashSet}; use anyhow::{Context as _, bail}; use bstr::BString; -use but_core::RefMetadata; +use but_core::{ + RefMetadata, + ref_metadata::{StackKind, Workspace}, +}; use but_ctx::Context; -use but_meta::VirtualBranchesTomlMetadata; use gitbutler_commit::commit_ext::{CommitExt, CommitMessageBstr as _}; use gitbutler_stack::{Stack, StackId}; use gix::date::parse::TimeBuf; @@ -25,39 +27,62 @@ use crate::{ ui::{CommitState, StackDetails}, }; -/// Get a stable `StackId` for the given `name`. It's fetched from `meta`, assuming it's backed by a toml file -/// and assuming that `name` is stored there as applied or unapplied branch. -fn id_from_name_v2_to_v3( - name: &gix::refs::FullNameRef, - meta: &VirtualBranchesTomlMetadata, -) -> anyhow::Result { - id_from_name_v2_to_v3_opt(name, meta)?.with_context(|| { - format!( - "{name:?} didn't have a stack-id even though \ - it was supposed to be in virtualbranches.toml" - ) - }) +fn default_workspace_metadata(meta: &impl RefMetadata) -> anyhow::Result> { + ref_info::function::workspace_data_of_default_workspace_branch(meta) } -/// Get a stable `StackId` for the given `name`. It's fetched from `meta`, assuming it's backed by a toml file -/// and assuming that `name` is stored there as applied or unapplied branch. -/// It's `None` if `name` isn't known to the workspace. -fn id_from_name_v2_to_v3_opt( - name: &gix::refs::FullNameRef, - meta: &VirtualBranchesTomlMetadata, -) -> anyhow::Result> { - let ref_meta = meta.branch(name)?; - Ok(ref_meta.stack_id().map(|id| { - id.to_string() - .parse() - .expect("new stack ids are just UUIDs, like the old ones") - })) +/// Build a lookup from workspace branch ref names to their stable stack IDs. +/// +/// The mapping covers both applied and unapplied stacks from the default workspace metadata so +/// callers can attach a V3 [`StackId`] to branch-derived UI entries without reaching into legacy +/// TOML-backed metadata. +fn stack_ids_by_ref_name( + meta: &impl RefMetadata, +) -> anyhow::Result> { + let Some(workspace) = default_workspace_metadata(meta)? else { + return Ok(HashMap::new()); + }; + Ok(workspace + .stacks(StackKind::AppliedAndUnapplied) + .flat_map(|stack| { + stack + .branches + .iter() + .map(move |branch| (branch.ref_name.clone(), stack.id)) + }) + .collect()) +} + +/// Build a reverse lookup from stable stack IDs to the branch refs that currently represent them. +/// +/// Each entry contains every branch ref recorded for the stack in default workspace metadata. This +/// allows callers to find a surviving repository ref for a stack before asking `ref_info()` to +/// reconstruct the current workspace projection for that stack. +fn branch_names_by_stack_id( + meta: &impl RefMetadata, +) -> anyhow::Result>> { + let Some(workspace) = default_workspace_metadata(meta)? else { + return Ok(HashMap::new()); + }; + Ok(workspace + .stacks(StackKind::AppliedAndUnapplied) + .map(|stack| { + ( + stack.id, + stack + .branches + .iter() + .map(|branch| branch.ref_name.clone()) + .collect(), + ) + }) + .collect()) } /// Get the associated forge review information out of the metadata, if any. fn review_id_from_meta( name: &gix::refs::FullNameRef, - meta: &VirtualBranchesTomlMetadata, + meta: &impl RefMetadata, ) -> anyhow::Result> { let pull_request = meta .branch_opt(name)? @@ -91,7 +116,7 @@ pub fn stack_heads_info( fn try_from_stack_v3( repo: &gix::Repository, stack: branch::Stack, - meta: &VirtualBranchesTomlMetadata, + stack_ids_by_ref_name: &HashMap, ) -> anyhow::Result { let name = stack .name() @@ -120,7 +145,7 @@ fn try_from_stack_v3( }) .collect::>()?; Ok(StackEntry { - id: id_from_name_v2_to_v3_opt(name.as_ref(), meta)?, + id: stack_ids_by_ref_name.get(&name).copied(), tip: heads .first() .map(|h| h.tip) @@ -139,7 +164,7 @@ fn try_from_stack_v3( // TODO: See if the UI can migrate to `head_info()` or a variant of it so the information is only called once. pub fn stacks_v3( repo: &gix::Repository, - meta: &VirtualBranchesTomlMetadata, + meta: &impl RefMetadata, filter: StacksFilter, ref_name_override: Option<&gix::refs::FullNameRef>, cache: &mut but_db::CacheHandle, @@ -150,8 +175,9 @@ pub fn stacks_v3( // found while traversing its commits to some base becomes a stack in that very sense. fn unapplied_stacks( repo: &gix::Repository, - meta: &VirtualBranchesTomlMetadata, + meta: &impl RefMetadata, applied_stacks: &[branch::Stack], + stack_ids_by_ref_name: &HashMap, ) -> anyhow::Result> { let mut out = Vec::new(); for item in meta.iter() { @@ -179,7 +205,7 @@ pub fn stacks_v3( .with_context(|| format!("Encountered symbolic reference: {ref_name}"))? .detach(); out.push(StackEntry { - id: id_from_name_v2_to_v3_opt(ref_name.as_ref(), meta)?, + id: stack_ids_by_ref_name.get(&ref_name).copied(), // TODO: this is just a simulation and such a thing doesn't really exist in the V3 world, let's see how it goes. // Thus, we just pass ourselves as first segment, similar to having no other segments. heads: vec![StackHeadInfo { @@ -204,27 +230,31 @@ pub fn stacks_v3( None => head_info(repo, meta, options, cache), Some(ref_name) => ref_info(repo.find_reference(ref_name)?, meta, options, cache), }?; + let stack_ids_by_ref_name = stack_ids_by_ref_name(meta)?; fn into_ui_stacks( repo: &gix::Repository, stacks: Vec, - meta: &VirtualBranchesTomlMetadata, + stack_ids_by_ref_name: &HashMap, ) -> Vec { stacks .into_iter() - .filter_map(|stack| try_from_stack_v3(repo, stack, meta).ok()) + .filter_map(|stack| try_from_stack_v3(repo, stack, stack_ids_by_ref_name).ok()) .collect() } let mut stacks = match filter { - StacksFilter::InWorkspace => into_ui_stacks(repo, info.stacks, meta), + StacksFilter::InWorkspace => into_ui_stacks(repo, info.stacks, &stack_ids_by_ref_name), StacksFilter::All => { - let unapplied_stacks = unapplied_stacks(repo, meta, &info.stacks)?; + let unapplied_stacks = + unapplied_stacks(repo, meta, &info.stacks, &stack_ids_by_ref_name)?; let mut all_stacks = unapplied_stacks; - all_stacks.extend(into_ui_stacks(repo, info.stacks, meta)); + all_stacks.extend(into_ui_stacks(repo, info.stacks, &stack_ids_by_ref_name)); all_stacks } - StacksFilter::Unapplied => unapplied_stacks(repo, meta, &info.stacks)?, + StacksFilter::Unapplied => { + unapplied_stacks(repo, meta, &info.stacks, &stack_ids_by_ref_name)? + } }; let needs_filtering_to_hide_segments_not_checked_out = stacks @@ -257,27 +287,17 @@ pub fn stacks_v3( pub fn stack_details_v3( stack_id: Option, repo: &gix::Repository, - meta: &VirtualBranchesTomlMetadata, + meta: &impl RefMetadata, cache: &mut but_db::CacheHandle, ) -> anyhow::Result { - fn stack_by_id( - head_info: RefInfo, - stack_id: StackId, - alt_stack_id: Option, - meta: &VirtualBranchesTomlMetadata, - ) -> anyhow::Result> { - let stacks_with_id: Vec<_> = head_info + // `ref_info()` resolves stacks relative to an existing ref, so once we know the stack ID we + // first locate any recorded branch ref for that stack and then select the matching stack from + // the resulting workspace projection. + fn stack_by_id(head_info: RefInfo, stack_id: StackId) -> Option { + head_info .stacks .into_iter() - .filter_map(|stack| { - let name = stack.name()?.to_owned(); - Some(id_from_name_v2_to_v3(name.as_ref(), meta).map(|stack_id| (stack_id, stack))) - }) - .collect::>()?; - - Ok(stacks_with_id - .into_iter() - .find_map(|(id, stack)| (id == stack_id || Some(id) == alt_stack_id).then_some(stack))) + .find(|stack| stack.id == Some(stack_id)) } let mut ref_info_options = ref_info::Options { // TODO(perf): make this so it can be enabled for a specific stack-id. @@ -309,31 +329,20 @@ pub fn stack_details_v3( } } Some(stack_id) => { - // Even though it shouldn't be the case, the ids can totally go out of sync. Use both to play it safer. - let (vb_stack, alt_stack_id) = meta - .data() - .branches + let branch_names_by_stack_id = branch_names_by_stack_id(meta)?; + let branch_names = branch_names_by_stack_id + .get(&stack_id) + .with_context(|| format!("Couldn't find {stack_id} in workspace metadata"))?; + let existing_ref = branch_names .iter() - .find_map(|(k, s)| { - if s.id == stack_id { - Some((s, Some(*k))) - } else if *k == stack_id { - Some((s, Some(s.id))) - } else { - None - } - }) + .find_map(|ref_name| repo.find_reference(ref_name.as_ref()).ok()) .with_context(|| { - format!("Couldn't find {stack_id} even when looking at virtual_branches.toml directly") + format!("Couldn't find any refs for stack {stack_id} in the repository") })?; - let full_name = gix::refs::FullName::try_from(format!( - "refs/heads/{shortname}", - shortname = vb_stack.derived_name()? - ))?; - let existing_ref = repo.find_reference(&full_name)?; let ref_info = ref_info(existing_ref, meta, ref_info_options, cache)?; - stack_by_id(ref_info, stack_id, alt_stack_id, meta)? - .with_context(|| format!("Really couldn't find {stack_id} or {alt_stack_id:?} in current HEAD or when searching virtual_branches.toml plainly"))? + stack_by_id(ref_info, stack_id).with_context(|| { + format!("Really couldn't find {stack_id} in the current workspace projection") + })? } }; diff --git a/etc/plans/consistent-data.md b/etc/plans/consistent-data.md index 950f6c1ffb..67ce330a93 100644 --- a/etc/plans/consistent-data.md +++ b/etc/plans/consistent-data.md @@ -73,7 +73,7 @@ read/write helpers. `legacy_meta()` and `legacy_meta_mut()`, needed for stacks/details V3 and vb.toml reconciliation. -- [ ] `crates/but-api/src/legacy/workspace.rs` +- [x] `crates/but-api/src/legacy/workspace.rs` - [ ] `crates/but-api/src/legacy/meta.rs` #### but-claude (1) From 003be313a06f60a7ea09e89cbbbd0fe5e40ea7b3 Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Thu, 19 Mar 2026 16:19:43 +0800 Subject: [PATCH 4/6] Address Copilot review Co-authored-by: Sebastian Thiel --- .../ref_info/with_workspace_commit/legacy.rs | 298 +++++++++++++++++- .../src/command/legacy/mcp_internal/stack.rs | 2 +- crates/but/src/legacy/commits.rs | 4 +- 3 files changed, 299 insertions(+), 5 deletions(-) diff --git a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs index 545cfc5d39..daf9f01f76 100644 --- a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs +++ b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs @@ -267,12 +267,14 @@ mod stacks { } mod stack_details { - use but_testsupport::visualize_commit_graph_all; + use but_testsupport::{graph_workspace, invoke_bash, visualize_commit_graph_all}; use crate::ref_info::{ - stack_details_v3, + head_info, stack_details_v3, + utils::standard_options, with_workspace_commit::{ read_only_in_memory_scenario, + utils::named_writable_scenario, utils::{StackState, add_stack, add_stack_with_segments}, }, }; @@ -493,4 +495,296 @@ mod stack_details { "#); Ok(()) } + + #[test] + fn multi_segment_stack_uses_advanced_tip_ref_to_find_full_stack() -> anyhow::Result<()> { + let (_tmp, repo, mut meta) = named_writable_scenario("ws-ref-ws-commit-one-stack")?; + let stack_id = add_stack_with_segments(&mut meta, 1, "B", StackState::InWorkspace, &["A"]); + + invoke_bash( + r#" + git checkout B + git commit --allow-empty -m B-outside + git checkout gitbutler/workspace + "#, + &repo, + ); + + insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" + * cc0bf57 (B) B-outside + | * 2076060 (HEAD -> gitbutler/workspace) GitButler Workspace Commit + |/ + * d69fe94 B + * 09d8e52 (A) A + * 85efbe4 (origin/main, main) M + "); + + // The raw workspace projection still knows about the advanced tip, but it cannot attach it + // to `refs/heads/B` anymore from `HEAD`, so the top segment is already anonymous here. + // Strangely enough, the worktree projection is absolutely supposed to be able to see that + // if the stack tips are known to the workspace, but it simply doesn't see it here. + let graph = but_graph::Graph::from_head( + &repo, + &meta, + but_graph::init::Options { + ..standard_options().traversal + }, + )?; + let ws = graph.into_workspace()?; + insta::assert_snapshot!(graph_workspace(&ws), @r" + πŸ“•πŸ˜οΈ:0:gitbutler/workspace[🌳] <> βœ“refs/remotes/origin/main on 85efbe4 + └── ≑:5:anon: on 85efbe4 {1} + β”œβ”€β”€ :5:anon: + β”‚ └── Β·d69fe94 (🏘️) + └── πŸ“™:4:A + └── Β·09d8e52 (🏘️) + "); + insta::assert_debug_snapshot!(ws, @r#" + Workspace(πŸ“•πŸ˜οΈ:0:gitbutler/workspace[🌳] <> βœ“refs/remotes/origin/main on 85efbe4) { + id: 0, + kind: Managed { + ref_info: RefInfo { + ref_name: FullName( + "refs/heads/gitbutler/workspace", + ), + worktree: Some( + Main, + ), + }, + }, + stacks: [ + Stack(≑:5:anon: on 85efbe4 {1}) { + segments: [ + StackSegment(:5:anon:) { + commits: [ + "Β·d69fe94 (🏘\u{fe0f})", + ], + commits_on_remote: [], + commits_outside: None, + }, + StackSegment(πŸ“™:4:A) { + commits: [ + "Β·09d8e52 (🏘\u{fe0f})", + ], + commits_on_remote: [], + commits_outside: None, + }, + ], + id: 00000000-0000-0000-0000-000000000001, + }, + ], + metadata: Some( + Workspace { + ref_info: RefInfo { created_at: "2023-01-31 14:55:57 +0000", updated_at: None }, + stacks: [ + WorkspaceStack { + id: 00000000-0000-0000-0000-000000000001, + branches: [ + WorkspaceStackBranch { + ref_name: "refs/heads/B", + archived: false, + }, + WorkspaceStackBranch { + ref_name: "refs/heads/A", + archived: false, + }, + ], + workspacecommit_relation: Merged, + }, + ], + target_ref: "refs/remotes/origin/main", + target_commit_id: Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), + push_remote: None, + }, + ), + target_ref: Some( + TargetRef { + ref_name: FullName( + "refs/remotes/origin/main", + ), + segment_index: NodeIndex(1), + commits_ahead: 0, + }, + ), + extra_target: None, + } + "#); + + // Looking from `HEAD` means traversing the workspace ref, so `B-outside` is not part of the + // traversed graph at all. + // The stack is still recognized from metadata, but the advanced tip can no longer be attached + // to `refs/heads/B`, so the top segment becomes anonymous and there are no `commits_outside` + // to report here. Those are only populated for commits that are visible in the traversal but + // sit above the managed workspace commit. + // This *should not be*, it should detect this case. + let info = head_info(&repo, &meta, standard_options())?; + insta::assert_debug_snapshot!(info, @r#" + RefInfo { + workspace_ref_info: Some( + RefInfo { + ref_name: FullName( + "refs/heads/gitbutler/workspace", + ), + worktree: Some( + Main, + ), + }, + ), + symbolic_remote_names: { + "origin", + }, + stacks: [ + Stack { + id: Some( + 00000000-0000-0000-0000-000000000001, + ), + base: Some( + Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), + ), + segments: [ + ref_info::ui::Segment { + id: NodeIndex(5), + ref_name: "None", + remote_tracking_ref_name: "None", + commits: [ + LocalCommit(d69fe94, "B\n", local), + ], + commits_on_remote: [], + commits_outside: None, + metadata: "None", + push_status: CompletelyUnpushed, + base: "09d8e52", + }, + ref_info::ui::Segment { + id: NodeIndex(4), + ref_name: "β–ΊA", + remote_tracking_ref_name: "None", + commits: [ + LocalCommit(09d8e52, "A\n", local), + ], + commits_on_remote: [], + commits_outside: None, + metadata: Branch, + push_status: CompletelyUnpushed, + base: "85efbe4", + }, + ], + }, + ], + target_ref: Some( + TargetRef { + ref_name: FullName( + "refs/remotes/origin/main", + ), + segment_index: NodeIndex(1), + commits_ahead: 0, + }, + ), + target_commit: Some( + TargetCommit { + commit_id: Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), + segment_index: NodeIndex(2), + }, + ), + extra_target: None, + lower_bound: Some( + NodeIndex(2), + ), + is_managed_ref: true, + is_managed_commit: true, + ancestor_workspace_commit: None, + is_entrypoint: true, + } + "#); + + // Looking up by `stack_id` takes a different path: it first finds a surviving ref for the + // stack, here `refs/heads/B`, and calls `ref_info()` from that ref instead of from `HEAD`. + // From that starting point `B-outside` is no longer "outside" at all, but simply the tip + // commit of the `B` segment, so it shows up in `commits` rather than `commits_outside`. + // The legacy `StackDetails` projection also drops `commits_outside` entirely, so this view + // cannot distinguish the advanced commit from ordinary in-stack commits anymore. + let actual = stack_details_v3(Some(stack_id), &repo, &meta)?; + insta::assert_debug_snapshot!(actual, @r#" + StackDetails { + derived_name: "B", + push_status: CompletelyUnpushed, + branch_details: [ + BranchDetails { + name: "B", + reference: FullName( + "refs/heads/B", + ), + linked_worktree_id: None, + remote_tracking_branch: None, + pr_number: None, + review_id: None, + tip: Sha1(cc0bf57992f34345564a4f616f60dd880cd83377), + base_commit: Sha1(09d8e528cc9381ddc4a7a436d83507b20fc909b0), + push_status: CompletelyUnpushed, + last_updated_at: None, + authors: [ + author , + ], + is_conflicted: false, + commits: [ + Commit(cc0bf57, "B-outside", local), + Commit(d69fe94, "B", local), + ], + upstream_commits: [], + is_remote_head: false, + }, + BranchDetails { + name: "A", + reference: FullName( + "refs/heads/A", + ), + linked_worktree_id: None, + remote_tracking_branch: None, + pr_number: None, + review_id: None, + tip: Sha1(09d8e528cc9381ddc4a7a436d83507b20fc909b0), + base_commit: Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), + push_status: CompletelyUnpushed, + last_updated_at: None, + authors: [ + author , + ], + is_conflicted: false, + commits: [ + Commit(09d8e52, "A", local), + ], + upstream_commits: [], + is_remote_head: false, + }, + BranchDetails { + name: "main", + reference: FullName( + "refs/heads/main", + ), + linked_worktree_id: None, + remote_tracking_branch: Some( + "refs/remotes/origin/main", + ), + pr_number: None, + review_id: None, + tip: Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), + base_commit: Sha1(0000000000000000000000000000000000000000), + push_status: Integrated, + last_updated_at: None, + authors: [ + author , + ], + is_conflicted: false, + commits: [ + Commit(85efbe4, "M", integrated), + ], + upstream_commits: [], + is_remote_head: false, + }, + ], + is_conflicted: false, + } + "#); + Ok(()) + } } diff --git a/crates/but/src/command/legacy/mcp_internal/stack.rs b/crates/but/src/command/legacy/mcp_internal/stack.rs index 2b89b7c2ce..3f3bac27e5 100644 --- a/crates/but/src/command/legacy/mcp_internal/stack.rs +++ b/crates/but/src/command/legacy/mcp_internal/stack.rs @@ -10,7 +10,7 @@ use serde::Serialize; /// This includes information about the branch itself and its commits pub fn branch_details(ref_name: &str, current_dir: &Path) -> anyhow::Result { let ctx = Context::discover(current_dir)?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let repo = ctx.clone_repo_for_merging_non_persisting()?; let ref_name = repo.find_reference(ref_name)?.name().to_owned(); diff --git a/crates/but/src/legacy/commits.rs b/crates/but/src/legacy/commits.rs index ec3f8f554b..535a4110f5 100644 --- a/crates/but/src/legacy/commits.rs +++ b/crates/but/src/legacy/commits.rs @@ -7,14 +7,14 @@ use but_workspace::{ pub fn stacks(ctx: &Context) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) } pub fn stack_details(ctx: &Context, stack_id: StackId) -> anyhow::Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let meta = ctx.legacy_meta()?; + let meta = ctx.meta()?; let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stack_details_v3(Some(stack_id), &repo, &meta, &mut cache) } From bf02fd020a8e6087db22075a7cd5769dc978b6fc Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Thu, 19 Mar 2026 17:24:28 +0800 Subject: [PATCH 5/6] Fix outside commit detection of the WP Co-authored-by: Sebastian Thiel --- crates/but-graph/src/init/post.rs | 28 +++++++++- crates/but-graph/tests/fixtures/scenarios.sh | 16 +++++- .../tests/graph/init/with_workspace.rs | 52 +++++++++++++++++-- .../ref_info/with_workspace_commit/legacy.rs | 43 ++++++++------- 4 files changed, 115 insertions(+), 24 deletions(-) diff --git a/crates/but-graph/src/init/post.rs b/crates/but-graph/src/init/post.rs index 4b88ea94db..dcc9716281 100644 --- a/crates/but-graph/src/init/post.rs +++ b/crates/but-graph/src/init/post.rs @@ -933,6 +933,30 @@ impl Graph { if !is_stack_tip { self[sidx].sibling_segment_id = Some(named_sid); } else { + let named_is_direct_parent = self + .inner + .neighbors_directed(sidx, Direction::Incoming) + .any(|n| n == named_sid); + let named_direct_parent_has_outside_commits = named_is_direct_parent && { + let mut has_outside_commits = false; + self.visit_all_segments_including_start_until( + named_sid, + Direction::Outgoing, + |segment| { + let prune = true; + if segment + .commits + .iter() + .any(|c| c.flags.contains(CommitFlags::InWorkspace)) + { + return prune; + } + has_outside_commits |= !segment.commits.is_empty(); + !has_outside_commits + }, + ); + has_outside_commits + }; let named_is_direct_ws_child = self .inner .neighbors_directed(ws_sidx, Direction::Outgoing) @@ -941,7 +965,9 @@ impl Graph { .inner .neighbors_directed(sidx, Direction::Outgoing) .any(|n| unique_ws_segment_ids.contains(&n)); - if !named_is_direct_ws_child && !has_ws_segments_below { + if named_direct_parent_has_outside_commits + || (!named_is_direct_ws_child && !has_ws_segments_below) + { self[sidx].sibling_segment_id = Some(named_sid); } } diff --git a/crates/but-graph/tests/fixtures/scenarios.sh b/crates/but-graph/tests/fixtures/scenarios.sh index 0aeee1d105..cb1600a716 100644 --- a/crates/but-graph/tests/fixtures/scenarios.sh +++ b/crates/but-graph/tests/fixtures/scenarios.sh @@ -291,6 +291,20 @@ mkdir ws create_workspace_commit_once B A ) + git init advanced-stack-tip-outside-workspace + (cd advanced-stack-tip-outside-workspace + commit M + setup_target_to_match_main + git checkout -b A + commit A + git checkout -b B + commit B + create_workspace_commit_once B + git checkout B + commit B-outside + git checkout gitbutler/workspace + ) + git init reproduce-11459 (cd reproduce-11459 commit M1 @@ -1507,4 +1521,4 @@ EOF git checkout -b gitbutler/workspace git commit --allow-empty -m "GitButler Workspace Commit" ) -) \ No newline at end of file +) diff --git a/crates/but-graph/tests/graph/init/with_workspace.rs b/crates/but-graph/tests/graph/init/with_workspace.rs index 7da1b293dc..c2a4a21d2b 100644 --- a/crates/but-graph/tests/graph/init/with_workspace.rs +++ b/crates/but-graph/tests/graph/init/with_workspace.rs @@ -157,6 +157,51 @@ fn reproduce_11483() -> anyhow::Result<()> { Ok(()) } +#[test] +fn workspace_projection_with_advanced_stack_tip() -> anyhow::Result<()> { + let (repo, mut meta) = read_only_in_memory_scenario("ws/advanced-stack-tip-outside-workspace")?; + add_stack_with_segments(&mut meta, 1, "B", StackState::InWorkspace, &["A"]); + + insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" + * cc0bf57 (B) B-outside + | * 2076060 (HEAD -> gitbutler/workspace) GitButler Workspace Commit + |/ + * d69fe94 B + * 09d8e52 (A) A + * 85efbe4 (origin/main, main) M + "); + + let graph = Graph::from_head(&repo, &*meta, standard_options())?.validated()?; + insta::assert_snapshot!(graph_tree(&graph), @r" + + β”œβ”€β”€ πŸ‘‰πŸ“•β–Ίβ–Ίβ–Ί:0[0]:gitbutler/workspace[🌳] + β”‚ └── Β·2076060 (βŒ‚|🏘|01) + β”‚ └── β–Ί:5[1]:anon: β†’:3: + β”‚ └── Β·d69fe94 (βŒ‚|🏘|01) + β”‚ └── πŸ“™β–Ί:4[2]:A + β”‚ └── Β·09d8e52 (βŒ‚|🏘|01) + β”‚ └── β–Ί:2[3]:main <> origin/main β†’:1: + β”‚ └── Β·85efbe4 (βŒ‚|🏘|βœ“|11) + β”œβ”€β”€ β–Ί:1[0]:origin/main β†’:2: + β”‚ └── β†’:2: (main β†’:1:) + └── πŸ“™β–Ί:3[0]:B + └── Β·cc0bf57 (βŒ‚) + └── β†’:5: + "); + let ws = &graph.into_workspace()?; + insta::assert_snapshot!(graph_workspace(ws), @r" + πŸ“•πŸ˜οΈ:0:gitbutler/workspace[🌳] <> βœ“refs/remotes/origin/main on 85efbe4 + └── β‰‘πŸ“™:5:B β†’:3: on 85efbe4 {1} + β”œβ”€β”€ πŸ“™:5:B β†’:3: + β”‚ β”œβ”€β”€ Β·cc0bf57* + β”‚ └── Β·d69fe94 (🏘️) + └── πŸ“™:4:A + └── Β·09d8e52 (🏘️) + "); + + Ok(()) +} + #[test] fn no_overzealous_stacks_due_to_workspace_metadata() -> anyhow::Result<()> { // NOTE: Was supposed to reproduce #11459, but it found another issue instead. @@ -4997,7 +5042,7 @@ fn branch_ahead_of_workspace() -> anyhow::Result<()> { β”œβ”€β”€ πŸ‘‰πŸ“•β–Ίβ–Ίβ–Ί:0[0]:gitbutler/workspace[🌳] β”‚ └── Β·fe6ba62 (βŒ‚|🏘|00001) - β”‚ β”œβ”€β”€ β–Ί:19[3]:anon: + β”‚ β”œβ”€β”€ β–Ί:19[3]:anon: β†’:4: β”‚ β”‚ └── Β·a62b0de (βŒ‚|🏘|βœ“|00011) β”‚ β”‚ └── β–Ί:21[4]:anon: β†’:5: β”‚ β”‚ └── Β·120a217 (βŒ‚|🏘|βœ“|00111) @@ -5080,8 +5125,9 @@ fn branch_ahead_of_workspace() -> anyhow::Result<()> { β”‚ β”œβ”€β”€ Β·ff75b80* β”‚ β”œβ”€β”€ Β·91bc3fc (🏘️|βœ“) β”‚ └── Β·cf9330f (🏘️|βœ“) - └── ≑:19:anon: on fafd9d0 {0} - β”œβ”€β”€ :19:anon: + └── β‰‘πŸ“™:19:A β†’:4: on fafd9d0 {0} + β”œβ”€β”€ πŸ“™:19:A β†’:4: + β”‚ β”œβ”€β”€ Β·c83f258* β”‚ └── Β·a62b0de (🏘️|βœ“) └── πŸ“™:21:A-middle <> origin/A-middle β†’:5: β”œβ”€β”€ Β·27c2545* diff --git a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs index daf9f01f76..8ccccadde1 100644 --- a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs +++ b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs @@ -519,10 +519,9 @@ mod stack_details { * 85efbe4 (origin/main, main) M "); - // The raw workspace projection still knows about the advanced tip, but it cannot attach it - // to `refs/heads/B` anymore from `HEAD`, so the top segment is already anonymous here. - // Strangely enough, the worktree projection is absolutely supposed to be able to see that - // if the stack tips are known to the workspace, but it simply doesn't see it here. + // The raw workspace projection can now link the advanced tip back to `refs/heads/B` even + // though the extra commit sits outside the workspace commit. That sibling link records the + // outside commit separately from the in-workspace `B` commit. let graph = but_graph::Graph::from_head( &repo, &meta, @@ -533,8 +532,9 @@ mod stack_details { let ws = graph.into_workspace()?; insta::assert_snapshot!(graph_workspace(&ws), @r" πŸ“•πŸ˜οΈ:0:gitbutler/workspace[🌳] <> βœ“refs/remotes/origin/main on 85efbe4 - └── ≑:5:anon: on 85efbe4 {1} - β”œβ”€β”€ :5:anon: + └── β‰‘πŸ“™:5:B β†’:3: on 85efbe4 {1} + β”œβ”€β”€ πŸ“™:5:B β†’:3: + β”‚ β”œβ”€β”€ Β·cc0bf57* β”‚ └── Β·d69fe94 (🏘️) └── πŸ“™:4:A └── Β·09d8e52 (🏘️) @@ -553,14 +553,18 @@ mod stack_details { }, }, stacks: [ - Stack(≑:5:anon: on 85efbe4 {1}) { + Stack(β‰‘πŸ“™:5:B β†’:3: on 85efbe4 {1}) { segments: [ - StackSegment(:5:anon:) { + StackSegment(πŸ“™:5:B β†’:3:) { commits: [ "Β·d69fe94 (🏘\u{fe0f})", ], commits_on_remote: [], - commits_outside: None, + commits_outside: Some( + [ + "Β·cc0bf57", + ], + ), }, StackSegment(πŸ“™:4:A) { commits: [ @@ -610,13 +614,10 @@ mod stack_details { } "#); - // Looking from `HEAD` means traversing the workspace ref, so `B-outside` is not part of the - // traversed graph at all. - // The stack is still recognized from metadata, but the advanced tip can no longer be attached - // to `refs/heads/B`, so the top segment becomes anonymous and there are no `commits_outside` - // to report here. Those are only populated for commits that are visible in the traversal but - // sit above the managed workspace commit. - // This *should not be*, it should detect this case. + // Looking from `HEAD` still traverses the workspace ref, but the projection now preserves a + // sibling link back to `refs/heads/B`. That lets `head_info()` keep the original branch name + // and surface the advanced `B-outside` commit via `commits_outside` even though it is not + // part of the managed workspace commit history itself. let info = head_info(&repo, &meta, standard_options())?; insta::assert_debug_snapshot!(info, @r#" RefInfo { @@ -644,14 +645,18 @@ mod stack_details { segments: [ ref_info::ui::Segment { id: NodeIndex(5), - ref_name: "None", + ref_name: "β–ΊB", remote_tracking_ref_name: "None", commits: [ LocalCommit(d69fe94, "B\n", local), ], commits_on_remote: [], - commits_outside: None, - metadata: "None", + commits_outside: Some( + [ + Commit(cc0bf57, "B-outside\n"), + ], + ), + metadata: Branch, push_status: CompletelyUnpushed, base: "09d8e52", }, From 587c1bf5877a0e71c3d3819b3cc0538b4fbb088f Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Thu, 19 Mar 2026 22:07:42 +0800 Subject: [PATCH 6/6] Prefer HEAD projection in legacy stack_details_v3() Make legacy stack_details_v3() try the current HEAD projection first for stack_id lookups, and only fall back to ref-based reconstruction when that stack is no longer visible from HEAD. This keeps legacy stack details aligned with the workspace projection in cases where a stack tip was advanced outside the workspace commit, while still preserving the old fallback for stacks that cannot be found from HEAD. Also document that legacy BranchDetails/StackDetails currently discard Segment::commits_outside, and emit a warning when those commits are dropped during legacy conversion. Co-authored-by: Sebastian Thiel --- crates/but-workspace/src/legacy/stacks.rs | 52 ++++++++++++------- crates/but-workspace/src/ui/mod.rs | 5 ++ .../ref_info/with_workspace_commit/legacy.rs | 40 +++----------- packages/but-sdk/src/generated/index.d.ts | 14 ++++- .../core/src/generated/workspace/index.ts | 5 ++ 5 files changed, 64 insertions(+), 52 deletions(-) diff --git a/crates/but-workspace/src/legacy/stacks.rs b/crates/but-workspace/src/legacy/stacks.rs index e623c950bb..fe2a71ec86 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -290,9 +290,8 @@ pub fn stack_details_v3( meta: &impl RefMetadata, cache: &mut but_db::CacheHandle, ) -> anyhow::Result { - // `ref_info()` resolves stacks relative to an existing ref, so once we know the stack ID we - // first locate any recorded branch ref for that stack and then select the matching stack from - // the resulting workspace projection. + // Prefer the current `HEAD` projection if it can still see the requested stack, and only fall + // back to resolving from a surviving ref when that stack is no longer reachable from `HEAD`. fn stack_by_id(head_info: RefInfo, stack_id: StackId) -> Option { head_info .stacks @@ -329,20 +328,27 @@ pub fn stack_details_v3( } } Some(stack_id) => { - let branch_names_by_stack_id = branch_names_by_stack_id(meta)?; - let branch_names = branch_names_by_stack_id - .get(&stack_id) - .with_context(|| format!("Couldn't find {stack_id} in workspace metadata"))?; - let existing_ref = branch_names - .iter() - .find_map(|ref_name| repo.find_reference(ref_name.as_ref()).ok()) - .with_context(|| { - format!("Couldn't find any refs for stack {stack_id} in the repository") - })?; - let ref_info = ref_info(existing_ref, meta, ref_info_options, cache)?; - stack_by_id(ref_info, stack_id).with_context(|| { - format!("Really couldn't find {stack_id} in the current workspace projection") - })? + if let Some(stack) = stack_by_id( + head_info(repo, meta, ref_info_options.clone(), cache)?, + stack_id, + ) { + stack + } else { + let branch_names_by_stack_id = branch_names_by_stack_id(meta)?; + let branch_names = branch_names_by_stack_id + .get(&stack_id) + .with_context(|| format!("Couldn't find {stack_id} in workspace metadata"))?; + let existing_ref = branch_names + .iter() + .find_map(|ref_name| repo.find_reference(ref_name.as_ref()).ok()) + .with_context(|| { + format!("Couldn't find any refs for stack {stack_id} in the repository") + })?; + let ref_info = ref_info(existing_ref, meta, ref_info_options, cache)?; + stack_by_id(ref_info, stack_id).with_context(|| { + format!("Really couldn't find {stack_id} in the current workspace projection") + })? + } } }; @@ -400,7 +406,7 @@ impl ui::BranchDetails { commits_on_remote: commits_unique_in_remote_tracking_branch, remote_tracking_ref_name, // There is nothing equivalent - commits_outside: _, + commits_outside, metadata, push_status, is_entrypoint: _, @@ -410,6 +416,16 @@ impl ui::BranchDetails { let ref_info = ref_info .clone() .context("Can't handle a stack yet whose tip isn't pointed to by a ref")?; + if let Some(commits_outside) = commits_outside + .as_ref() + .filter(|commits| !commits.is_empty()) + { + tracing::warn!( + ignored_outside_commits = commits_outside.len(), + stack_segment_ref = %ref_info.ref_name, + "Legacy StackDetails drops commits_outside for this stack segment" + ); + } let (updated_at, review_id, pr_number) = metadata .clone() .map(|meta| { diff --git a/crates/but-workspace/src/ui/mod.rs b/crates/but-workspace/src/ui/mod.rs index 0a4505529a..253da2ab1d 100644 --- a/crates/but-workspace/src/ui/mod.rs +++ b/crates/but-workspace/src/ui/mod.rs @@ -304,6 +304,11 @@ pub struct BranchDetails { /// Whether the branch is conflicted. pub is_conflicted: bool, /// The commits contained in the branch, excluding the upstream commits. + /// + /// Note that legacy stack details currently do not expose + /// [`crate::ref_info::Segment::commits_outside`], so commits that only appear there are + /// omitted from this list rather than represented separately. + /// It's also unclear how to recover from there. pub commits: Vec, /// The commits that are only at the remote. pub upstream_commits: Vec, diff --git a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs index 8ccccadde1..a90bd2264c 100644 --- a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs +++ b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs @@ -702,12 +702,12 @@ mod stack_details { } "#); - // Looking up by `stack_id` takes a different path: it first finds a surviving ref for the - // stack, here `refs/heads/B`, and calls `ref_info()` from that ref instead of from `HEAD`. - // From that starting point `B-outside` is no longer "outside" at all, but simply the tip - // commit of the `B` segment, so it shows up in `commits` rather than `commits_outside`. - // The legacy `StackDetails` projection also drops `commits_outside` entirely, so this view - // cannot distinguish the advanced commit from ordinary in-stack commits anymore. + // Looking up by `stack_id` now prefers the current `HEAD` projection if it can still see + // that stack, and only falls back to resolving from a surviving ref when `HEAD` cannot. + // That keeps the stack anchored in the same workspace view as `head_info()`, so `B` stays + // the top segment instead of being re-anchored from `refs/heads/B`. + // Legacy `StackDetails` still has no dedicated `commits_outside` field and continues to + // discard those commits entirely, so `B-outside` is intentionally omitted here. let actual = stack_details_v3(Some(stack_id), &repo, &meta)?; insta::assert_debug_snapshot!(actual, @r#" StackDetails { @@ -723,7 +723,7 @@ mod stack_details { remote_tracking_branch: None, pr_number: None, review_id: None, - tip: Sha1(cc0bf57992f34345564a4f616f60dd880cd83377), + tip: Sha1(d69fe9427ac4a2422ab953acba483f804e8098ef), base_commit: Sha1(09d8e528cc9381ddc4a7a436d83507b20fc909b0), push_status: CompletelyUnpushed, last_updated_at: None, @@ -732,7 +732,6 @@ mod stack_details { ], is_conflicted: false, commits: [ - Commit(cc0bf57, "B-outside", local), Commit(d69fe94, "B", local), ], upstream_commits: [], @@ -761,31 +760,6 @@ mod stack_details { upstream_commits: [], is_remote_head: false, }, - BranchDetails { - name: "main", - reference: FullName( - "refs/heads/main", - ), - linked_worktree_id: None, - remote_tracking_branch: Some( - "refs/remotes/origin/main", - ), - pr_number: None, - review_id: None, - tip: Sha1(85efbe4d5a663bff0ed8fb5fbc38a72be0592f55), - base_commit: Sha1(0000000000000000000000000000000000000000), - push_status: Integrated, - last_updated_at: None, - authors: [ - author , - ], - is_conflicted: false, - commits: [ - Commit(85efbe4, "M", integrated), - ], - upstream_commits: [], - is_remote_head: false, - }, ], is_conflicted: false, } diff --git a/packages/but-sdk/src/generated/index.d.ts b/packages/but-sdk/src/generated/index.d.ts index 5328823f01..4f70cbfb20 100644 --- a/packages/but-sdk/src/generated/index.d.ts +++ b/packages/but-sdk/src/generated/index.d.ts @@ -3,6 +3,11 @@ /** Just like [apply_only()], but will create an oplog entry as well on success. */ export declare function apply(projectId: string, existingBranch: string): Promise +/** + * Persist hunk-to-commit assignments for the current workspace. + * + * `assignments` is a list of hunk assignment requests produced by the UI. + */ export declare function assignHunk(projectId: string, assignments: Array): Promise export declare function branchDetails(projectId: string, branchName: string, remote: string | null): Promise @@ -210,7 +215,14 @@ export type BranchDetails = { authors: Array; /** Whether the branch is conflicted. */ isConflicted: boolean; - /** The commits contained in the branch, excluding the upstream commits. */ + /** + * The commits contained in the branch, excluding the upstream commits. + * + * Note that legacy stack details currently do not expose + * [`crate::ref_info::Segment::commits_outside`], so commits that only appear there are + * omitted from this list rather than represented separately. + * It's also unclear how to recover from there. + */ commits: Array; /** The commits that are only at the remote. */ upstreamCommits: Array; diff --git a/packages/core/src/generated/workspace/index.ts b/packages/core/src/generated/workspace/index.ts index a150fc18d7..0a9f790b82 100644 --- a/packages/core/src/generated/workspace/index.ts +++ b/packages/core/src/generated/workspace/index.ts @@ -76,6 +76,11 @@ export type BranchDetails = { isConflicted: boolean; /** * The commits contained in the branch, excluding the upstream commits. + * + * Note that legacy stack details currently do not expose + * [`crate::ref_info::Segment::commits_outside`], so commits that only appear there are + * omitted from this list rather than represented separately. + * It's also unclear how to recover from there. */ commits: Array; /**