From b205576759bd23afd0a36cb579a80fdadfaf51a2 Mon Sep 17 00:00:00 2001 From: Simon Franken Date: Sat, 24 Oct 2026 23:26:32 +0200 Subject: [PATCH] Hard restore with X asks before dropping staged pins X dropped staged pins without a question, including the selected submodule's own pin, which the loss check didn't count. A staged pin is a decision, and X sits one Shift away from x, so dropped staged pins now count as a loss and make X ask (D31, amends D30). Co-Authored-By: Claude Opus 5.5 --- README.md | 6 +++--- docs/actions.md | 7 ++++--- docs/decisions.md | 15 +++++++++++++++ docs/requirements.md | 2 +- docs/ux-guide.md | 5 +++-- internal/submodule/git/restore.go | 25 ++++++++++++++++++++++--- internal/submodule/git/restore_test.go | 23 +++++++++++++++++++++++ internal/tui/restore.go | 8 +------- internal/tui/restore_test.go | 17 ++++++++++++----- 9 files changed, 84 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 54f0902..f678984 100644 --- a/README.md +++ b/README.md @@ -117,9 +117,9 @@ something would be lost, you are asked first: submodule with a staged pin, `x` first asks whether to restore to the staged pin (`s`) or the committed one (`c`, which unstages it). -`X` is a hard restore: it goes to the committed pins without asking which -one, and also drops the staged pins of submodules below the selection, -which `x` keeps where it can. It still asks if work would be lost. +`X` restores to the committed pins without asking which one, and also +drops the staged pins of submodules below the selection, which `x` keeps +where it can. It asks if work or a staged pin would be lost. ### Switch a branch with `b` diff --git a/docs/actions.md b/docs/actions.md index beafc23..1a574f9 100644 --- a/docs/actions.md +++ b/docs/actions.md @@ -87,9 +87,10 @@ own, and for nested repositories that are on their pin. A repository that moves loses its staged pins either way (see above); then `x` → `c` and `X` end the same, but `c` asks first. -The dropped staged pins are counted in the loss (`2 staged pins`), but -alone they don't make restore ask: pressing `X` is the choice to drop them. -If something else would be lost, the question lists them with it. The +The dropped staged pins are counted in the loss (`2 staged pins`), the +selected submodule's own staged pin included, and like any loss they make +restore ask (D31): `Restore libs/staged to the committed pins? Discards 1 +staged pin.` The commits of a dropped pin stay in the submodule; if HEAD leaves them and no ref reaches them, they count as `commits on no branch`, as always. diff --git a/docs/decisions.md b/docs/decisions.md index f74e2c2..cfeb00b 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -593,3 +593,18 @@ staged and committed pin is still useful there. the superproject. `restoreRepo` and `lossOf` take whether to keep staged pins; switch always keeps them. Without a committed pin (an added submodule), `X` uses the index pin. + +## D31 — Hard restore asks before dropping staged pins + +**Context.** D30 let `X` drop staged pins without a question: pressing `X` +was the choice. In a usability walkthrough (see +[notes/usability-plan.md](../notes/usability-plan.md)) `X` on `libs/staged` +unstaged its pin silently. `X` is one Shift away from `x`, and users new to +submodules don't know where to find the dropped commit again. +**Decision.** Amends D30. Dropped staged pins are a loss like any other and +make `X` ask: `Restore libs/staged to the committed pins? Discards 1 staged +pin.` `RestoreLoss` with `RestoreHard` now also counts the selected +submodule's own staged pin (in its parent's index), which `lossOf` doesn't +see. The `c` answer of `x` stays silent: the user picked it in a question. +**Consequences.** `X` asks more often, but only where it changes what is +recorded. Without staged pins or other losses it still runs at once. diff --git a/docs/requirements.md b/docs/requirements.md index 581d34c..3205ef2 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -150,7 +150,7 @@ commit hashes. It doesn't answer the questions users actually have: confirmation only if something would be lost. See [actions.md](actions.md#restore). `X` is a hard restore: it goes to the committed pins at every depth, without the pin question, and drops all staged pins below the selection - too. Dropped staged pins alone don't make it ask. See + too. Dropped staged pins count as a loss, so it asks first. See [actions.md](actions.md#hard-restore). - **F57** `b` opens a popup in the middle of the screen with a search line diff --git a/docs/ux-guide.md b/docs/ux-guide.md index 7d3bc9c..8962c88 100644 --- a/docs/ux-guide.md +++ b/docs/ux-guide.md @@ -363,8 +363,9 @@ the term*. `y` restores, any other key cancels; the keys are not passed on while a question is open. After `c` the name reads `src/app to the committed pin`. -`X` (hard restore) never asks for the pin; its name reads `src/app to the -committed pins`, and dropped staged pins alone don't make it ask. Without a loss there is no question. Success flashes +`X` (restore committed pins) never asks for the pin; its name reads +`src/app to the committed pins`. Staged pins it drops count as a loss +(`Discards 1 staged pin.`), so it asks (D31). Without a loss there is no question. Success flashes `restored src/app` in the footer. For the header the name is `the superproject`. This line takes precedence over the filter line and the problems-only notice. diff --git a/internal/submodule/git/restore.go b/internal/submodule/git/restore.go index b99540f..170d35e 100644 --- a/internal/submodule/git/restore.go +++ b/internal/submodule/git/restore.go @@ -35,10 +35,29 @@ func (p *Provider) RestoreLoss(ctx context.Context, path string, to submodule.Re if err != nil { return submodule.Loss{}, err } - if dir == "" { // not initialized: nothing on disk to lose - return submodule.Loss{}, nil + var loss submodule.Loss + if dir != "" { // not initialized: nothing on disk to lose + loss = p.lossOf(ctx, dir, target, to.KeepsStaged()) } - return p.lossOf(ctx, dir, target, to.KeepsStaged()), nil + if to == submodule.RestoreHard && path != "" && p.pinStaged(ctx, path, target) { + // a hard restore unstages the submodule's own pin too + loss.StagedPins++ + loss.Modules = max(loss.Modules, 1) + } + return loss, nil +} + +// pinStaged reports whether the pin of path in its parent's index differs +// from committed, the pin a hard restore goes to. +func (p *Provider) pinStaged(ctx context.Context, path, committed string) bool { + p.mu.RLock() + in, ok := p.mods[path] + p.mu.RUnlock() + if !ok { + return false + } + staged, err := pinOf(ctx, in, submodule.RestoreStaged) + return err == nil && staged != committed } // Restore makes path ("" is the superproject) and all submodules below it diff --git a/internal/submodule/git/restore_test.go b/internal/submodule/git/restore_test.go index 0290277..fdc65cc 100644 --- a/internal/submodule/git/restore_test.go +++ b/internal/submodule/git/restore_test.go @@ -364,6 +364,29 @@ func TestRestoreHardOfASubmoduleUnstagesItsPin(t *testing.T) { } } +func TestRestoreLossOfAHardRestoreCountsTheOwnStagedPin(t *testing.T) { + p, _ := ownFixture(t) + committed, err := p.RestoreLoss(context.Background(), "libs/staged", submodule.RestoreCommitted) + if err != nil { + t.Fatal(err) + } + hard, err := p.RestoreLoss(context.Background(), "libs/staged", submodule.RestoreHard) + if err != nil { + t.Fatal(err) + } + if hard.StagedPins != committed.StagedPins+1 || hard.Modules == 0 { + t.Errorf("hard loss %+v, want the own staged pin on top of %+v", hard, committed) + } + // without a staged pin, a hard restore unstages nothing of its own + clean, err := p.RestoreLoss(context.Background(), "src/ui", submodule.RestoreHard) + if err != nil { + t.Fatal(err) + } + if clean.StagedPins != 0 { + t.Errorf("loss of src/ui %+v, want no staged pin", clean) + } +} + func TestRestoreLossCountsStagedPinsOfAMovingRepository(t *testing.T) { p, root := ownFixture(t) super := filepath.Join(root, "super") diff --git a/internal/tui/restore.go b/internal/tui/restore.go index abce6a2..3f0ee1d 100644 --- a/internal/tui/restore.go +++ b/internal/tui/restore.go @@ -135,13 +135,7 @@ func (m Model) lossChecked(msg lossMsg) (tea.Model, tea.Cmd) { m.restore.branch = msg.branch } m.restore.loss = msg.loss - // a hard restore is asked for to drop staged pins: alone they are no - // reason to ask, but are listed if something else is lost - lost := msg.loss - if m.restore.to == submodule.RestoreHard { - lost.StagedPins, lost.Modules = 0, 0 - } - if lost.Empty() { + if msg.loss.Empty() { return m.runRestore() } m.restore.phase = restoreAsking diff --git a/internal/tui/restore_test.go b/internal/tui/restore_test.go index 3b718c0..6b6a15c 100644 --- a/internal/tui/restore_test.go +++ b/internal/tui/restore_test.go @@ -189,7 +189,7 @@ func TestRestoreOfAStagedPinAsksWhichPin(t *testing.T) { } } -func TestHardRestoreOfAStagedPinDoesNotAsk(t *testing.T) { +func TestHardRestoreAsksToDropStagedPins(t *testing.T) { m, f := newRestoreModel(t, submodule.Loss{StagedPins: 2, Modules: 2}) mm := m.(Model) var staged *submodule.Submodule @@ -202,15 +202,22 @@ func TestHardRestoreOfAStagedPinDoesNotAsk(t *testing.T) { mm.tree.Select(staged.Path) m = mm - // neither which pin, nor y/n for staged pins alone + // not which pin, but y/n: staged pins alone are a reason to ask m, cmd := press(m, "X") - var next tea.Cmd for _, msg := range run(cmd) { if l, ok := msg.(lossMsg); ok { - m, next = m.Update(l) + m, _ = m.Update(l) } } - feed(m, next) + r := m.(Model).restore + if r == nil || r.phase != restoreAsking { + t.Fatalf("want a question, got %+v", r) + } + if got := lossText(r.loss); got != "2 staged pins" { + t.Errorf("question lists %q, want the staged pins", got) + } + m, cmd = press(m, "y") + feed(m, cmd) if len(f.to) != 1 || f.to[0] != submodule.RestoreHard || f.restored[0] != staged.Path { t.Fatalf("restored %q to %v, want %s hard", f.restored, f.to, staged.Path) }