fix(but): checkout to default target as fallback#12553
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 fixes a bug in the but teardown command where it would fail with "No active branches found" error when attempting to teardown on a fresh repository with no active branches (e.g., after but setup && but teardown). The fix adds a fallback to check out the default target branch (e.g., "main" from "origin/main") when no active branches exist.
Changes:
- Modified teardown logic to fall back to the default target branch when no active stacks are found
- Added test coverage for the fallback behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/but/src/command/legacy/teardown.rs | Changed from erroring on empty stacks to falling back to the default target branch name |
| crates/but/tests/but/command/teardown.rs | Added test to verify teardown checks out the target branch when there are no active branches |
| .map(|h| h.name.to_string()) | ||
| .ok_or_else(|| anyhow::anyhow!("Stack has no branches"))? | ||
| } else { | ||
| gitbutler_stack::VirtualBranchesHandle::new(ctx.project_data_dir()) |
There was a problem hiding this comment.
Could you try using ctx.ws.get()?. It has all kinds of information, and while the target there is optional, it would typically be set.
Thank you
There was a problem hiding this comment.
Thanks! I was looking around for alternative ways to get that information, this seemed "heavy".
While I've got you on the line, I feel like we should also have "Stack has no branches" situations fall back on the default target. Don't you think? That seems like a completely broken state to me, but all the more reason not to leave the user stuck in a broken GitButler workspace. Right now it just errors out.
There was a problem hiding this comment.
The ws data structure should always have one stack, but it might be unnamed.
To my mind, to actually do it right we should go back to the branch that the user used before switching onto gitbutler/workspace. One could check the reflog for HEAD to figure this out.
Meantime, doing better here without being perfect seems acceptable.
There was a problem hiding this comment.
What was I thinking - of course stacks can be empty. That's a completely acceptable state. In single-branch mode, i.e. when main is checked out, it won't ever be empty, but in full workspace mode, it can be.
There was a problem hiding this comment.
What was I thinking - of course stacks can be empty.
Sorry, that's not what I meant. This state is the entire reason this PR is necessary, as if you have no active branches in your workspace there are also no stacks (e.g. just after running but setup) and then we just fail the teardown.
What I was considering is the case where we have stacks (i.e. the if branch above), where there is a possibility that said stack has no branches. At least in terms of code, that can happen. How that would happen in reality I'm not sure, it seems like an error state.
Somewhat simplified, the current code flow of this conditional is as follows:
- There is at least one stack?
- Yes: There is at least one branch in that stack?
- Yes: Check out to that branch
- No: Error (<---- this is where I feel we should also try to check out to default target instead of erroring)
- No: There is a default target?
- Yes: Check out to default target
- No: Error (<----- can we end up here? Is there a case where we don't have a default target?)
- Yes: There is at least one branch in that stack?
Or maybe, as you say, we should just completely abandon the above and just utilize the reflog instead. Find the last branch and check out to that. But what if that branch has been deleted since switching to the gitbutler workspace? Perhaps then we should check out to the default target. So it would go something like this:
- Previously checked out branch exists?
- Yes: Checkout to that branch
- No: Checkout to default target
WDYT?
There was a problem hiding this comment.
No: Error (<---- this is where I feel we should also try to check out to default target instead of erroring)
I also think we should fall back here, it's then the only branch name we have. And if the code should be perfect, it would have to deal with its absence which is perfectly fine.
Git also has the notion of the default branch name, which is configured and can be queried. That one can be checked out if nothing else helps, I think.
As disclaimer: my comment is very technical and doesn't optimise for UX, it optimises for "find a reasonable branch name" 😅.
🧢 Changes
Makes
but teardowncheck out to the default target as a fallback if there are no active branches.Previously it would error out like so if you just setup/teardown on a fresh repository:
TODO
This is a naive fix for this very particular issue, just wanted to see if it'd pass CI. Also want to handle broken states (e.g. top stack has no active branches).