move branches, but make it *graphic*#12696
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
de7627b to
8d4a964
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “move branch” implementation that operates on the workspace graph (via the new rebase engine), exposes it through a new API surface, and adds a but branch move CLI command while renaming the previous move-branch endpoint/command as legacy.
Changes:
- Added graph-based branch moving in
but-workspaceand exposed it throughbut-apiandbutCLI. - Extended graph rebase mutation primitives (disconnect now supports optionally skipping reconnection) to enable segment moves.
- Renamed existing UI/backend move-branch wiring to
*_legacyand added fixtures/tests for the new behavior.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/gitbutler-tauri/src/main.rs | Switches Tauri invoke command from move_branch to move_branch_legacy. |
| crates/but/tests/fixtures/scenario/two-stacks-one-single-and-ready-to-mingle-one-double.sh | Adds a CLI test scenario fixture with two stacks to exercise branch moves. |
| crates/but/tests/but/command/branch/move_branch.rs | Adds CLI-level tests for but branch move (by name and CLI IDs). |
| crates/but/tests/but/command/branch/mod.rs | Registers the new move_branch test module. |
| crates/but/src/utils/metrics.rs | Adds metrics mapping for the new branch move subcommand. |
| crates/but/src/lib.rs | Wires branch move into CLI dispatch; refactors branch subcommand handling. |
| crates/but/src/command/mod.rs | Makes command::branch module always available (not only non-legacy builds). |
| crates/but/src/command/legacy/branch/mod.rs | Refactors legacy branch command handling into individual functions. |
| crates/but/src/command/branch/move_branch.rs | Implements the CLI operation that calls workspace graph move + materialization. |
| crates/but/src/command/branch/mod.rs | Exposes move_branch (and keeps apply behind non-legacy feature). |
| crates/but/src/args/metrics.rs | Adds BranchMove as a metrics command name. |
| crates/but/src/args/branch.rs | Adds the branch move clap subcommand and arguments. |
| crates/but-workspace/tests/workspace/branch/move_branch.rs | Adds workspace-level tests covering stack/segment move scenarios. |
| crates/but-workspace/tests/workspace/branch/mod.rs | Registers the new workspace move-branch tests module. |
| crates/but-workspace/tests/fixtures/scenario/ws-ref-ws-commit-single-stack-double-stack.sh | Adds a workspace fixture scenario with two stacks. |
| crates/but-workspace/src/branch/move_branch.rs | Adds the core graph-based branch-move implementation. |
| crates/but-workspace/src/branch/mod.rs | Exports the new move_branch API from the workspace branch module. |
| crates/but-server/src/lib.rs | Renames server route from /move_branch to /move_branch_legacy. |
| crates/but-rebase/tests/rebase/graph_rebase/disconnect.rs | Updates disconnect tests for the new disconnect_segment_from(..., skip_reconnect_step) signature. |
| crates/but-rebase/src/graph_rebase/mutate.rs | Extends disconnect primitive with skip_reconnect_step and adjusts reconnect logic. |
| crates/but-graph/src/api.rs | Adds Graph::find_segment helper for looking up segments by index. |
| crates/but-api/src/legacy/virtual_branches.rs | Renames legacy move_branch API to move_branch_legacy. |
| crates/but-api/src/branch.rs | Adds a new graph-based move_branch API. |
| apps/desktop/src/lib/stacks/stackService.svelte.ts | Updates desktop client to call the renamed legacy move-branch command. |
5911a34 to
23297a7
Compare
| Subcommands::Branch(branch::Platform { cmd }) => match cmd { | ||
| #[cfg(not(feature = "legacy"))] | ||
| None => todo!("implement list and call recursively"), | ||
| #[cfg(feature = "legacy")] | ||
| None => { | ||
| let mut ctx = setup::init_ctx( | ||
| &args, | ||
| InitCtxOptions { | ||
| background_sync: BackgroundSync::Enabled, | ||
| ..Default::default() | ||
| }, | ||
| out, | ||
| )?; | ||
| command::legacy::branch::handle_no_subcommand(&mut ctx, out) | ||
| } | ||
| } | ||
| #[cfg(feature = "legacy")] | ||
| Some(branch::Subcommands::List { | ||
| filter, | ||
| local, | ||
| remote, | ||
| all, | ||
| no_ahead, | ||
| review, | ||
| no_check, | ||
| empty, | ||
| }) => { | ||
| let mut ctx = setup::init_ctx( | ||
| &args, | ||
| InitCtxOptions { | ||
| background_sync: BackgroundSync::Enabled, | ||
| ..Default::default() | ||
| }, | ||
| out, | ||
| )?; | ||
| command::legacy::branch::list_branches( | ||
| &mut ctx, out, filter, local, remote, all, no_ahead, review, no_check, empty, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Regression: the legacy but branch ... paths no longer call .emit_metrics(metrics_ctx) (previously the legacy branch handler did). This will stop emitting telemetry for branch list/show/new/delete and the no-subcommand default; consider wrapping these arms with .emit_metrics(metrics_ctx) (or emitting once after the match).
|
|
||
| use crate::{CliId, IdMap, id::parser::parse_sources, utils::OutputChannel}; | ||
|
|
||
| /// Move a branchon top of another |
There was a problem hiding this comment.
Typo in the doc comment: branchon → branch on (missing space).
| /// Move a branchon top of another | |
| /// Move a branch on top of another |
| pub fn find_segment(&self, segment_idx: SegmentIndex) -> Option<Segment> { | ||
| if self.inner.contains_node(segment_idx) { | ||
| Some(self.inner[segment_idx].clone()) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
find_segment() clones and returns an owned Segment, which can be unnecessarily expensive (segments can contain many commits). Consider returning Option<&Segment> (e.g. via self.inner.node_weight(segment_idx)) and let callers clone only what they need.
| pub fn find_segment(&self, segment_idx: SegmentIndex) -> Option<Segment> { | |
| if self.inner.contains_node(segment_idx) { | |
| Some(self.inner[segment_idx].clone()) | |
| } else { | |
| None | |
| } | |
| pub fn find_segment(&self, segment_idx: SegmentIndex) -> Option<&Segment> { | |
| self.inner.node_weight(segment_idx) |
23297a7 to
9940f96
Compare
| let branch_ref_name_str = &format!("refs/heads/{branch_name}"); | ||
| let target_ref_name_str = &format!("refs/heads/{target_branch_name}"); | ||
|
|
||
| let but_workspace::branch::move_branch::Outcome { rebase } = | ||
| but_workspace::branch::move_branch( | ||
| &ws, | ||
| editor, | ||
| branch_ref_name_str.try_into()?, | ||
| target_ref_name_str.try_into()?, |
There was a problem hiding this comment.
let branch_ref_name_str = &format!(...) (and the same for target_ref_name_str) borrows a temporary String, which won’t live long enough. This should be an owned String (or build the FullNameRef directly) and then pass a stable &str/&String into try_into().
| let branch_ref_name_str = &format!("refs/heads/{branch_name}"); | |
| let target_ref_name_str = &format!("refs/heads/{target_branch_name}"); | |
| let but_workspace::branch::move_branch::Outcome { rebase } = | |
| but_workspace::branch::move_branch( | |
| &ws, | |
| editor, | |
| branch_ref_name_str.try_into()?, | |
| target_ref_name_str.try_into()?, | |
| let branch_ref_name = format!("refs/heads/{branch_name}"); | |
| let target_ref_name = format!("refs/heads/{target_branch_name}"); | |
| let but_workspace::branch::move_branch::Outcome { rebase } = | |
| but_workspace::branch::move_branch( | |
| &ws, | |
| editor, | |
| branch_ref_name.as_str().try_into()?, | |
| target_ref_name.as_str().try_into()?, |
| /// The branch being moved. | ||
| #[clap(value_name = "BRANCH")] | ||
| branch: String, | ||
| /// The branch to move the other on top off. |
There was a problem hiding this comment.
Typo in help text: “on top off” → “on top of”. This text shows up in --help, so it’s worth correcting.
| /// The branch to move the other on top off. | |
| /// The branch to move the other on top of. |
There was a problem hiding this comment.
I am loving how working with the new workspace and rebase engine can yield new mutations quickly!
With that said, I think we should have some tests to show what happens with purely virtual branches, just because I think that with meta this probably won't yield expected results. It's fine, but maybe we can just limit the participating branches to those that aren't empty.
Lastly, I set this PR back to draft as I think the stringy types in but-api must be fixed, so we can avoid falling back to former patterns of an API that is dominated by its JSON nature.
Finally, let me state that I spent only a little time with this, and didn't review anything outside of the api and the workspace, nor did I look at how this mutation is actually done or its logic. I did skim the tests to see what it can do though, and found them very easy to follow 🩷.
| .segments | ||
| .iter() | ||
| .enumerate() |
There was a problem hiding this comment.
I think this is iter().position().
| .base_segment_id | ||
| .and_then(|segment_idx| workspace.graph.find_segment(segment_idx)); | ||
|
|
||
| let parents_to_disconnect = match (stack_base_segment, graph_base_segment) { |
There was a problem hiding this comment.
This wants to be stack_base_segment.or(graph_base_segment) to avoid the duplication below.
There was a problem hiding this comment.
They'r different types, so I won't let me do that :/
Segment and StackSegment
1a67c15 to
23d1bd9
Compare
23d1bd9 to
db952e4
Compare
| for stack_entry in &stacks { | ||
| if stack_entry.heads.iter().all(|b| b.name != *branch_name) { | ||
| // Not found in this stack, | ||
| continue; | ||
| } | ||
| Some(Subcommands::Show { | ||
| branch_id, | ||
| review, | ||
| files, | ||
| ai, | ||
| check, | ||
| }) => { | ||
| show::show(ctx, &branch_id, out, review, files, ai, check)?; | ||
| Ok(()) | ||
|
|
There was a problem hiding this comment.
In delete(), this branch lookup compares a BString (b.name) against *branch_name (a String). This won’t compile and also mixes encodings (bytes vs UTF-8). Compare using bytes (e.g., b.name.as_ref() == branch_name.as_bytes()) or convert b.name with to_str() and compare to branch_name (handling non-UTF8 names appropriately).
|
|
||
| let mut ws_meta = workspace.metadata.clone(); | ||
|
|
||
| let (source_stack, subject_segment) = source; | ||
| let (_, target_segment) = destination; |
There was a problem hiding this comment.
ws_meta is initialized as workspace.metadata.clone() and returned even when it isn’t modified (the only mutation is inside the “empty branch” conditional). Downstream callers treat Some(ws_meta) as “needs to be persisted”, so this can cause redundant metadata writes on every non-empty move. Consider returning None unless the metadata actually changes (e.g., start with let mut ws_meta = None; and only clone+mutate when needed), or update the doc/field name to reflect that it may always mirror existing metadata.
There was a problem hiding this comment.
Yeah, this probably means it should be set to None when it's not changed.
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot!
I have pushed a refactor commit which straightens out the use of RefMetadata and further limits the move-branch functions to only work if it's a traditional managed commits. For everything else one would definitely need tests.
However, it's probably fair to kick that can hard and say that once meta is backed by the DB officially, and but-rebase alters it, move-branch will naturally work in these cases. So no need to put time into it beyond what 95% of the users need right now.
I did not look at:
- tests
- integration into but-api or but-rebase
- basically anything except for
but-workspace, and even there I only dealt with typing/superficial things.
Codex found two issues, one of which is related to a missing edit-mode check. The other one is about possibly missing metrics, both seem worth another look.
[P1] Dispatch legacy branch moves through the guarded path
Repro:
- Build/run the default
butbinary (legacyis enabled by default incrates/but/Cargo.toml). - In a repo with applied GitButler stacks, run
but setup. - Leave open-workspace mode, e.g.
git checkout main. - Run
but branch move A C.
Expected:
This should fail through the legacy guarded path, the same way other legacy branch mutations do, because gitbutler_branch_actions::move_branch enforces ensure_open_workspace_mode(...) and records a MoveBranch snapshot for undo.
Actual:
but branch move now dispatches directly to command::branch::move_branch from crates/but/src/lib.rs, so it bypasses the legacy wrapper. In edit/outside-workspace mode it can still rewrite refs/workspace metadata, and the move is no longer recorded in the oplog.
[P2] Re-wrap branch dispatch in metrics emission
Repro:
- Compare the old and new
Subcommands::Branch(...)dispatch incrates/but/src/lib.rs. - Notice that
match_subcommand()still createsmetrics_ctx. - Notice that the new
Subcommands::Stack { .. }arm and the refactoredSubcommands::Branch(...)arm now return rawResult<()>values instead of calling.emit_metrics(metrics_ctx).
Expected:
but branch ... and but stack ... should emit the same duration/error telemetry as other CLI commands, including the new CommandName::BranchMove mapping added in this patch.
Actual:
No metrics emission happens for these paths anymore, so branch commands silently stop reporting telemetry. That means the newly added BranchMove metric will never fire.
Byron
left a comment
There was a problem hiding this comment.
And some notes on the ominous refactor commit. I made it because I need to test what I feel is right IRL to know it's right.
Also, I get that it might work better for you with comments, so here they are, and it should be fine to drop the commit and redo it based on the gist.
| /// | ||
| /// Returned by [function::move_branch()]. | ||
| #[derive(Debug)] | ||
| pub struct Outcome<WorkspaceMeta> { |
There was a problem hiding this comment.
This is a strong smell, and I noticed that LLMs have a hard time with RefMetadata when I tried just now. Basically everything it comes up with is probably not what you want.
There was a problem hiding this comment.
I don't like this default assumption that LLMs came up with my changes.
This I did on my own. Would you mind explaining why this is a smell, so that I can avoid that next time?
There was a problem hiding this comment.
I don't like this default assumption that LLMs came up with my changes.
Yeah, I will avoid mentioning it. Judging from myself, all I get to do is reviews and refactors these days. And since I am a late-bloomer, I just assume everyone else uses it more :D.
This I did on my own. Would you mind explaining why this is a smell, so that I can avoid that next time?
It makes no sense to use a generic type for a struct for which only one implementation exists. But it doesn't look that way when looking at this Handle type and I really hope that it can be simplified. Something along these lines should be possible once it's finally in the database, at which point there isn't even a need for RefMetadata as trait anymore.
So it's a deal, the trait will go away.
There was a problem hiding this comment.
Yeah, the motivation here was to maintain dynamism with the correct types. I assumed that if we're dealing with the implementation of a trait, then it's feasible to get different workspace handles as well.
Didn't know there was only the one
| /// It should replace the actual workspace metadata to configure moved 'virtual' branches segments, if `Some()`. | ||
| pub ws_meta: Option<but_core::ref_metadata::Workspace>, |
There was a problem hiding this comment.
It's still quite undefined what should happen without metadata, but it seems fair to just ignore it as there is nothing we can do for now (unless one day there is a case for changing this, but I doubt it).
| pub fn move_branch( | ||
| workspace: &but_graph::projection::Workspace, | ||
| mut editor: Editor, | ||
| meta: &mut M, |
There was a problem hiding this comment.
The key here is that but-graph already collects metadata, and thus it should be queried from the Workspace.
This makes handling the metadata update here quite straightforward, but forces the caller to set it if present. Fair enough.
| WorkspaceKind::ManagedMissingWorkspaceCommit { .. } => { | ||
| bail!("Moving branches currently need a workspace commit") |
There was a problem hiding this comment.
Let's be even more conservative here, and opt it in with a barrage of tests.
Maybe that won't be needed if the rebase engine takes care of this, so postponing complexity seems like the way to go.
|
|
||
| let mut ws_meta = workspace.metadata.clone(); | ||
|
|
||
| let (source_stack, subject_segment) = source; | ||
| let (_, target_segment) = destination; |
There was a problem hiding this comment.
Yeah, this probably means it should be set to None when it's not changed.
| .base_segment_id | ||
| .map(|segment_idx| &workspace.graph[segment_idx]); | ||
|
|
||
| let parents_to_disconnect = match (stack_base_segment, graph_base_segment) { |
There was a problem hiding this comment.
I replaced this as it wasn't a match to me. Please double-check, I was lazy and asked my bot to do it 😅.
There was a problem hiding this comment.
Gotcha. I guess this is purely stylistic, though.
Happy to keep talking about this offline some other time.
There was a problem hiding this comment.
I do think if … else is less ambiguous here and I'd prefer it unless exhaustiveness is needed. Using match won't kill me though, it just confuses me.
There was a problem hiding this comment.
Makes sense to me. If else is easier to read 👍
Byron
left a comment
There was a problem hiding this comment.
There are still a couple of legit-looking LLM comments to look into, but besides that I think it's good to go.
Again with the note that I didn't actually try using it.
| fn set_workspace_metadata( | ||
| meta: &mut impl RefMetadata, | ||
| ws: &but_graph::projection::Workspace, | ||
| ws_meta: Option<but_core::ref_metadata::Workspace>, | ||
| ) -> anyhow::Result<()> { | ||
| if let Some((ws_meta, ref_name)) = ws_meta.zip(ws.ref_name()) { | ||
| let mut md = meta.workspace(ref_name)?; | ||
| *md = ws_meta; | ||
| meta.set_workspace(&md)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Yeah, right!
Git + metadata = graph -> workspace. Then move copies the workspace metadata, and alters it. For refreshes of the workspace it wants meta to re-read reference metadata. So meta has to get the new metadata which is associated with a reference.
It is implicit knowledge that ws.ref_name() is the gitbutler/workspace typically, and that's where we want to set the changed metadata in the metadata store (vb.toml + db mirror right now).
*md = ws_meta;This is the interesting line as one can't set the data directly. This is me predicting the future, thinking it will be easier when it's an actual database.
But probably it's just helping with the setup now, and in future all this will go away and be a very dumb assignment.
Use the newly implemented graph mutations to move branches Prefer direct segment access in `but_graph::Graph` After all, we may assume that segment ids exist in the graph. And even if not, we can access the inner petgraph directly thanks to a DeRef implementation.
The original `move_branch` API is not marked as legacy.
graphically as in using graph operations. Add a new non-legacy API to move branches around. Show a more idiomatic usage of the API for `move_branch`
Allow for moving branches on top of other branches in the CLI
Add tests for the move branch functionality
9e1c68e to
441dce2
Compare
|
Addressed the two open issues found by Codex.
TODO: Do we need to check for open workspace even if we're currently checking for being in a managed workspace? |
Implement the new way of moving branches by using the graph and new rebase engine.
Usage of the but command
Tests
fixes GB-1089