cli+tui: make repeated but undos work#13695
Merged
Merged
Conversation
e32a85f to
adb6018
Compare
davidpdrsn
commented
May 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the oplog/undo plumbing so repeated but undo walks further back in history (instead of undoing the previous undo), and introduces a lazy snapshot iterator to support traversing the oplog without a pre-known limit. It also attempts to distinguish “undo restores” from explicit but oplog restore operations via new restore/oplog kinds.
Changes:
- Added lazy snapshot traversal (
snapshots_iter) and updated callers/tests away fromlist_snapshots(limit, ...). - Added
RestoreKind/ newOperationKindvariants and wired them through restore APIs. - Added undo-target selection logic (
get_undo_target_snapshot) and new CLI tests for repeated undo.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/gitbutler-oplog/src/oplog.rs | Replaces eager snapshot listing with an iterator; adds RestoreKind; updates restore snapshot recording. |
| crates/gitbutler-oplog/src/lib.rs | Re-exports RestoreKind. |
| crates/gitbutler-oplog/src/entry.rs | Splits restore operation kinds and adds backward-compatible parsing for legacy trailer values. |
| crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/oplog.rs | Migrates tests from list_snapshots to snapshots_iter and updates restore signature. |
| crates/but/tests/but/command/undo.rs | Adds CLI tests for repeated undo and explicit restore undo behavior. |
| crates/but/src/command/legacy/status/tui/mod.rs | Switches undo flow to get_undo_target_snapshot and passes restore kind. |
| crates/but/src/command/legacy/rub/squash.rs | Updates restore signature on squash rollback path. |
| crates/but/src/command/legacy/oplog.rs | Migrates oplog list to iterator; switches undo to get_undo_target_snapshot; updates restore API calls. |
| crates/but-api/src/legacy/oplog.rs | Implements iterator wrapper + undo-target logic; introduces API-level RestoreKind; updates restore API signature. |
davidpdrsn
commented
May 7, 2026
d7b7913 to
2122715
Compare
but undos workbut undos work
but undos workbut undos work
but undos work
slarse
approved these changes
May 8, 2026
Contributor
|
lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, if you ran
but undoand thenbut undoagain, you'd undo the undo. Thats not intended and instead, what we want to do is undo the operation before the previous undo, basically walking backwards in history. This makesbut undodo that.How it works
I find the undo target by walking from newest to oldest through the oplog and counting how many undos we see. For each undo we see, we skip one non-undo entry. When we're at 0, we know the target is the next entry.
Example when all undos are at the top
And with undos mixed with other operations
Undoing
but oplog restoreI want
but oplog restoreto be treated differently frombut undo, i.e., if you runbut oplog restoreand thenbut undo, then we should undo the restore. The restore shouldn't be counted as an undo.This required changing
OperationKind, which previously usedRestoreFromSnapshotfor both undos and restores. I've added a newRestoreFromSnapshotViaUndovariant that is used for undos so we can tell them apart from explicit restores. That means old undo entries will be categorized as restores, but I don't think there is anything we can do about that.but redoWe also want to add
but redoto walk forwards in the oplog. I'm pretty confident we can use the same general strategy to support that as well, but if someone disagrees, I'd like to know 😊 since this introduces data changes. I want to make sure we don't need to make further changes when we dobut redo. I might wait to merge this until I have a workingbut redoimplementation as well.Iterating the oplog
Before, we had
ctx.list_snapshots(), which returned aVec<Snapshot>and required passing a limit on how many oplog entries to retrieve. I've changed that to instead bectx.snapshots_iter(), which returns a lazy iterator. We need this because we don't know specifically how far to walk back to find the undo target, so we cannot provide a limit upfront.One small behavior change here is that when iterating from a specific oplog commit, that commit isn't included, whereas before it would be. I think this behavior is more appropriate for pagination since clients can just pass the last commit and get exactly the next page to render. There was even a TODO about that in the Svelte app.
GUI changes
The new
OperationKinddid require some changes in Svelte land, which I'm really not familiar with, so I would like some extra eyes on that.