but rub branch1 branch2 stacks branch1 on top of branch2
#11774
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
This PR changes the behavior of but rub branch1 branch2 to stack branch1 on top of branch2, and adds a new but stack command as an explicit alias for this operation. Previously, but rub branch1 branch2 moved all uncommitted changes from branch1 to branch2.
Changes:
- Replaces the branch-to-branch assignment behavior with branch stacking functionality
- Adds a new
but stackcommand that wraps the modifiedbut rubbehavior - Introduces a new
stack_branch_on_topfunction to implement the stacking logic
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but/src/args/mod.rs | Adds the Stack subcommand with documentation and parameter definitions |
| crates/but/src/lib.rs | Routes the Stack subcommand to the rub::handle function |
| crates/but/src/utils/metrics.rs | Adds metrics tracking for the Stack command |
| crates/but/src/command/legacy/rub/mod.rs | Changes branch-to-branch behavior from assign::assign_all to stack::stack_branch_on_top and updates operation kind from MoveHunk to MoveCommit |
| crates/but/src/command/legacy/rub/stack.rs | Implements the new stack_branch_on_top function that finds the target branch position and applies the source branch with an order parameter |
| use anyhow::bail; | ||
| use but_core::worktree::checkout::UncommitedWorktreeChanges; | ||
| use but_ctx::Context; | ||
| use but_workspace::branch::{ | ||
| OnWorkspaceMergeConflict, | ||
| apply::{WorkspaceMerge, WorkspaceReferenceNaming}, | ||
| }; | ||
| use colored::Colorize; | ||
|
|
||
| use crate::utils::OutputChannel; | ||
|
|
||
| /// Stack one branch on top of another by applying it with the correct order parameter. | ||
| /// This moves the `source_branch` to be stacked on top of `target_branch`. | ||
| pub(crate) fn stack_branch_on_top( | ||
| ctx: &mut Context, | ||
| out: &mut OutputChannel, | ||
| source_branch: &str, | ||
| target_branch: &str, | ||
| ) -> anyhow::Result<()> { | ||
| // Get repository and metadata to find the target position | ||
| let guard = ctx.shared_worktree_access(); | ||
| let (repo, _meta, graph) = | ||
| ctx.graph_and_meta_and_repo_from_head(ctx.repo.get()?.clone(), guard.read_permission())?; | ||
|
|
||
| // Convert branch names to full reference names | ||
| let source_ref = repo.find_reference(source_branch)?; | ||
| let target_ref = repo.find_reference(target_branch)?; | ||
|
|
||
| let target_ref_name = target_ref.name(); | ||
|
|
||
| // Get the workspace to understand current stack order | ||
| let ws = graph.to_workspace()?; | ||
|
|
||
| // Find the target branch's position in the workspace | ||
| // We want to find where in the stacks list the target branch exists | ||
| let target_position = ws.stacks.iter().position(|stack| { | ||
| // Check if this stack is the target branch itself (top-level stack) | ||
| if let Some(stack_ref_name) = stack.ref_name() { | ||
| if stack_ref_name.as_bstr() == target_ref_name.as_bstr() { | ||
| return true; | ||
| } | ||
| } | ||
| // Check if the target is a segment within this stack | ||
| stack.segments.iter().any(|segment| { | ||
| if let Some(seg_ref_name) = segment.ref_name() { | ||
| seg_ref_name.as_bstr() == target_ref_name.as_bstr() | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| }); | ||
|
|
||
| let target_position = match target_position { | ||
| Some(pos) => pos, | ||
| None => { | ||
| bail!( | ||
| "Target branch {} is not in the current workspace. It must be applied first.", | ||
| target_branch.blue().underline() | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| // The order parameter should place the source branch right after the target | ||
| // Since stacks are ordered, and we want to insert after target_position, | ||
| // we use target_position (0-indexed position where to insert) | ||
| let insert_order = target_position; | ||
|
|
||
| let source_ref_full_name = source_ref.name().to_owned(); | ||
|
|
||
| drop(ws); | ||
| drop(graph); | ||
| drop(guard); | ||
| drop(source_ref); | ||
| drop(target_ref); | ||
| drop(repo); | ||
|
|
||
| // Now apply the source branch with the order parameter | ||
| let mut guard = ctx.exclusive_worktree_access(); | ||
| let (repo, mut meta, graph) = | ||
| ctx.graph_and_meta_mut_and_repo_from_head(guard.write_permission())?; | ||
| let ws = graph.to_workspace()?; | ||
|
|
||
| // Apply the branch with the order parameter to control its position | ||
| let outcome = but_workspace::branch::apply( | ||
| source_ref_full_name.as_ref(), | ||
| &ws, | ||
| &repo, | ||
| &mut meta, | ||
| but_workspace::branch::apply::Options { | ||
| workspace_merge: WorkspaceMerge::default(), | ||
| on_workspace_conflict: OnWorkspaceMergeConflict::default(), | ||
| workspace_reference_naming: WorkspaceReferenceNaming::default(), | ||
| uncommitted_changes: UncommitedWorktreeChanges::default(), | ||
| order: Some(insert_order), | ||
| new_stack_id: None, | ||
| }, | ||
| )? | ||
| .into_owned(); | ||
|
|
||
| // Report success | ||
| if let Some(human_out) = out.for_human() { | ||
| writeln!( | ||
| human_out, | ||
| "{} Stacked {} on top of {}", | ||
| "✓".green(), | ||
| source_branch.blue().underline(), | ||
| target_branch.blue().underline() | ||
| )?; | ||
|
|
||
| if outcome.workspace_changed() { | ||
| writeln!(human_out, " Workspace updated with new stack order")?; | ||
| } | ||
| } | ||
|
|
||
| if let Some(json_out) = out.for_json() { | ||
| let result = serde_json::json!({ | ||
| "success": true, | ||
| "source_branch": source_branch, | ||
| "target_branch": target_branch, | ||
| "workspace_changed": outcome.workspace_changed(), | ||
| }); | ||
| json_out.write_value(&result)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This new branch stacking functionality lacks test coverage. Given that the repository has comprehensive test coverage for other but rub operations (as seen in crates/but/tests/but/command/rub.rs), tests should be added for this feature.
Consider adding at least the following test cases:
- Basic test: stacking one branch on top of another and verifying the order
- Test with the
but stackcommand alias - Edge case: attempting to stack a branch on a target that isn't in the workspace
- Edge case: verifying the behavior when source and target are the same
| (CliId::Branch { name: from, .. }, CliId::Branch { name: to, .. }) => { | ||
| create_snapshot(ctx, OperationKind::MoveHunk); | ||
| assign::assign_all(ctx, Some(&from), Some(to), out)?; | ||
| create_snapshot(ctx, OperationKind::MoveCommit); | ||
| stack::stack_branch_on_top(ctx, out, &from, to)?; | ||
| } |
There was a problem hiding this comment.
This change represents a breaking change in the behavior of but rub branch1 branch2. The previous behavior moved all uncommitted changes from branch1 to branch2 (via assign::assign_all), while the new behavior stacks branch1 on top of branch2.
This is a significant semantic change that could break existing user workflows. Consider:
- Documenting this breaking change in the changelog or migration guide
- Providing a deprecation warning before making this change
- Offering an alternative command to preserve the old "move hunks between branches" behavior
The operation kind change from MoveHunk to MoveCommit is appropriate for the new behavior, but users may be surprised by this change.
| let outcome = but_workspace::branch::apply( | ||
| source_ref_full_name.as_ref(), | ||
| &ws, | ||
| &repo, | ||
| &mut meta, | ||
| but_workspace::branch::apply::Options { | ||
| workspace_merge: WorkspaceMerge::default(), | ||
| on_workspace_conflict: OnWorkspaceMergeConflict::default(), | ||
| workspace_reference_naming: WorkspaceReferenceNaming::default(), | ||
| uncommitted_changes: UncommitedWorktreeChanges::default(), | ||
| order: Some(insert_order), | ||
| new_stack_id: None, | ||
| }, | ||
| )? | ||
| .into_owned(); |
There was a problem hiding this comment.
The implementation doesn't handle the case where the source branch is already in the workspace. According to the apply function documentation (line 177-178 in apply.rs): "Note that options have no effect if branch is already in the workspace, so apply is not a way to alter certain aspects of the workspace by applying the same branch again."
This means if the source branch is already applied to the workspace, the order parameter will be ignored and the stacking operation will silently do nothing. This could be confusing for users who expect the branch to be moved to the new position.
Consider either:
- Checking if the source branch is already in the workspace and returning an appropriate error or warning
- First unapplying the source branch, then re-applying it at the new position
- Documenting this limitation in the command help text
| // The order parameter should place the source branch right after the target | ||
| // Since stacks are ordered, and we want to insert after target_position, | ||
| // we use target_position (0-indexed position where to insert) | ||
| let insert_order = target_position; |
There was a problem hiding this comment.
The logic for calculating the insertion order appears to be incorrect. The comment states "we want to insert after target_position", but the code uses insert_order = target_position.
When calling Vec::insert(index, element), the element is inserted AT the given index, not after it. For example, inserting at index 1 in [A, B, C] produces [A, new, B, C], placing the new element BEFORE the element that was at index 1.
If the goal is to stack the source branch "on top of" (i.e., after) the target branch, the insertion order should be target_position + 1. Otherwise, the source branch will be inserted BEFORE the target branch, which is the opposite of the intended behavior.
| // The order parameter should place the source branch right after the target | |
| // Since stacks are ordered, and we want to insert after target_position, | |
| // we use target_position (0-indexed position where to insert) | |
| let insert_order = target_position; | |
| // The order parameter should place the source branch right after the target. | |
| // Since stacks are ordered and `target_position` is the index of the target, | |
| // we insert at `target_position + 1` to place the source immediately after it. | |
| let insert_order = target_position + 1; |
Also adds a
but stackcommand that does the same thing