-
Notifications
You must be signed in to change notification settings - Fork 725
Single-branch support for but branch apply
#11371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Byron is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
a5260b5 to
23bf02d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR ports the but branch apply command to work without the legacy feature flag, making it compatible with the new single-branch mode. The work involves reorganizing tests, adding CI validation for non-legacy builds, and creating new API functions in but-api.
Key changes:
- Add new API functions in
but-apifor branch operations (specificallyapply) - Restructure and reorganize tests for the
branchcommand into separate files (apply.rs,new.rs) - Add CI validation to ensure
butbuilds and tests pass without the legacy feature flag - Update Cargo.toml dependencies to properly gate legacy-specific dependencies
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but/tests/but/utils.rs | Make invoke_bash more flexible by accepting impl AsRef<str> instead of &str |
| crates/but/tests/but/main.rs | Gate id and journey test modules with #[cfg(feature = "legacy")] |
| crates/but/tests/but/command/snapshots/help/no-arg-no-legacy.stdout.term.svg | Add new snapshot for help command output in non-legacy mode |
| crates/but/tests/but/command/mod.rs | Gate commit, rub, and status test modules with #[cfg(feature = "legacy")] |
| crates/but/tests/but/command/help.rs | Add non-legacy version of help test and gate existing tests with legacy feature flag |
| crates/but/tests/but/command/format.rs | Gate legacy-specific test code with #[cfg(feature = "legacy")] |
| crates/but/tests/but/command/branch/new.rs | Extract new command tests from the old branch.rs file into separate module |
| crates/but/tests/but/command/branch/mod.rs | Create branch test module structure with gated submodules for apply and new |
| crates/but/tests/but/command/branch/apply.rs | Extract apply command tests from old branch.rs with comprehensive test coverage |
| crates/but/tests/but/command/branch.rs | Remove file - tests moved to separate apply.rs and new.rs files |
| crates/but/src/args/tests.rs | Gate get_gerrit_flags tests with #[cfg(feature = "legacy")] |
| crates/but-workspace/src/branch/apply.rs | Add into_owned() method to Outcome and add documentation about checkout behavior |
| crates/but-clap/Cargo.toml | Remove unused snapbox dev-dependency |
| crates/but-api/src/lib.rs | Add #![deny(missing_docs)] and new branch module for branch operations |
| crates/but-api/src/legacy/mod.rs | Add #![allow(missing_docs)] for legacy code |
| crates/but-api/src/json.rs | Add module documentation and document ToJsonError trait |
| crates/but-api/src/github.rs | Add #![allow(missing_docs)] for GitHub integration code |
| crates/but-api/src/branch.rs | Add new apply and apply_without_oplog functions for branch application |
| crates/but-api/Cargo.toml | Reorganize dependencies to properly gate legacy-specific crates |
| Cargo.lock | Update lock file to reflect removed snapbox dependency from but-clap |
| .github/workflows/push.yaml | Add CI step to test but without default features (non-legacy mode) |
a5b2db1 to
67da5d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 79 out of 88 changed files in this pull request and generated 4 comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 80 out of 89 changed files in this pull request and generated no new comments.
|
Looks good to me. One thing that i found interesting in the PR title "Single-branch support for but branch apply" - I think I know what you meant but also, per definition the apply function is about adding an additional branch to the workspace, so perhaps 'single branch' may be a misnomer in this case |
krlvi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, for this, LGTM
|
Thanks you!
What if I told you that apply can be used in single branch mode, e.g. while on Single-branch mode for me means that all functions work everywhere, or at least have meaningful results or graceful failure if it's just not possible (like unapplying a branch, that really is only for real workspaces, from what I can tell). |
This allows us to validate the non-legacy mode as well.
…n for apply impl
…ble `but-api` That way we can demonstrate semantics of the oplog/undo system.
While at it, make it clearer from dependencies what's legacy and what not, similar to the `but` (CLI) crate.
In legacy mode, we can use non-legacy code, but not the other way around. So let's split it on the highest level. Also, let's not use feature toggles for the same reason, it's now not possible anymore to call into old code as it's not available anymore.
84bc3c6 to
931b52d
Compare
Ideally, it's clear why tests fail and how to fix them. - add a cache - focus the test-runs to only the crates that harbor ts type definitions - remove CI dependencies as the dependents hang if there was no Rust change, which causes the definitions job to not run.
Port
but branch applyover to non-legacy code, make sure it builds in non-legacy mode as well.Tasks
Next PR
but branch apply#11400Follow-up on #11236.