From b9f6055759ce1039e43b34093f2dedb01635eab1 Mon Sep 17 00:00:00 2001 From: Simon Franken Date: Thu, 22 Oct 2026 12:42:20 +0200 Subject: [PATCH] Restore keeps staged pins; offer the committed pin for a staged one Staged pins survive a restore at every depth and are the targets of the submodules below. For a submodule with a staged pin, x asks whether to restore to the staged or the committed pin, the latter unstaging it. Staged pins of a repository that moves count as a loss. Co-Authored-By: Claude Opus 5.5 --- docs/actions.md | 61 ++++++++++---- docs/decisions.md | 9 ++ docs/requirements.md | 6 +- docs/ux-guide.md | 5 +- internal/submodule/git/restore.go | 110 ++++++++++++++++++++----- internal/submodule/git/restore_test.go | 103 +++++++++++++++++++---- internal/submodule/provider.go | 15 +++- internal/tui/app.go | 11 ++- internal/tui/restore.go | 71 +++++++++++++--- internal/tui/restore_test.go | 40 ++++++++- 10 files changed, 359 insertions(+), 72 deletions(-) diff --git a/docs/actions.md b/docs/actions.md index 8d085df..b57a355 100644 --- a/docs/actions.md +++ b/docs/actions.md @@ -24,38 +24,65 @@ the tip in the submodule, then `s`". Makes the selected repository, and everything below it, exactly what git records. It works on any submodule and on the superproject (header selected). Restoring the superproject gives the exact project that the -current root commit specifies. +current root commit specifies, with the pins you have staged. ### Target | Selected | Target commit | Branch | |---|---|---| | Superproject | `HEAD` | stays on the current branch | -| Submodule | The pin in the parent's **index** (what the UI shows as *Pinned*) | see [Branches](#branches) | -| Nested submodule, during the recursion | The gitlink in the restored parent's commit | as for a submodule | +| Submodule | The pin in the parent's **index** (what the UI shows as *Pinned*); with a staged pin you choose, see below | see [Branches](#branches) | +| Nested submodule, during the recursion | The gitlink in the restored parent's index | as for a submodule | -Restoring the superproject resets its index too, so **staged pins are -discarded**: afterwards every pin is the one in `HEAD`. Restoring a submodule -does not touch the parent's index, so a staged pin of that submodule stays -and is the target. +**Staged pins are kept, at every depth.** A staged pin is a decision, not +drift. Each repository's files are reset to its `HEAD`, but its staged pins +stay in its index, and the submodules below are restored to them. So a +nested submodule with a staged pin and uncommitted changes keeps the pin +and is reset to it; its changes are discarded (and listed in the question). + +**Choosing the pin.** If the selected submodule has a staged pin itself, `x` +first asks which pin to restore to: + +``` +Restore libs/staged to s staged pin · c committed pin (unstages it) any other key cancels +``` + +- `s`: the staged pin, which stays staged. +- `c`: the committed pin (the gitlink in the parent's `HEAD`). The parent's + index is set back to it, which is how a pin is unstaged in submarine. + +The loss check then runs for the chosen pin (the two can differ: a staged +commit that exists only locally is lost by `c`, not by `s`). + +Two exceptions, where a staged pin is not kept: + +- **The repository moves.** If a repository is off its pin, restore checks + out another commit; staged pins in its own index belonged to the old one. + They are counted as a loss (`2 staged pins`), so the question appears. Its + submodules follow the target commit's tree. +- **An added, not yet committed submodule** is no staged pin: its + `.gitmodules` entry is reset with the other files. Its directory then + counts as an untracked file. ### Postconditions After a successful restore of repository R with target T: -1. **R's HEAD is T.** The index and the worktree equal T's tree. No - modified, added, removed or conflicted files remain. +1. **R's HEAD is T.** The index and the worktree equal T's tree, except for + the staged pins that were kept. No modified, added, removed or conflicted + files remain. 2. **No in-progress operation** remains in R: merge, rebase, cherry-pick, revert. 3. **No untracked files or directories** remain in R. This includes leftover directories of submodules that T no longer has (*untracked repository*). Ignored files are kept. -4. **Every submodule that T records is present and initialized**, even if +4. **Every submodule that R's index records is present and initialized**, even if it was never initialized, is inactive (`submodule..active`) or has `update = none / merge / rebase` configured. Its URL comes from T's `.gitmodules` (synced into the config). 5. **Each of those submodules is restored the same way**, with its gitlink - in T as its target, recursively at any depth. + in R's index (T's, or a kept staged pin) as its target, recursively at + any depth. So the subtree shows `in sync` everywhere, except for states restore does not change: `unpushed`, behind/ahead of the ref, and `detached HEAD on pin`. @@ -85,6 +112,7 @@ Restore discards work. The things it can lose: | Untracked files | `changed` (untracked) | | Conflicts and in-progress operations | `conflict`; merge, rebase, cherry-pick, revert state | | Commits | A detached HEAD off the pin whose commits are on no branch, tag or remote ref (only the reflog still has them) | +| Staged pins | Staged pins in a repository that restore moves to another commit (see [Target](#target)) | | Repositories | An untracked directory that is a repository with its **own** `.git` directory (a clone that was never added as a submodule). Its commits may exist nowhere else. A removed submodule whose git directory is absorbed into `.git/modules` loses only its worktree. Its objects stay | Nothing else is lost. Branches, stashes, remote refs, ignored files and @@ -148,15 +176,19 @@ cat-file / fetch --all # only if T is missing locally rebase|cherry-pick|revert --quit # whichever is in progress; --quit, not --abort: no ref moves checkout --force --detach T # only if HEAD is not T, so a branch is never moved reset --hard # also ends a merge +update-index --cacheinfo … # put the staged pins back (only if HEAD did not move) clean -ffd # -ff: untracked repositories too; ignored files stay submodule sync submodule update --init --checkout --force -- -# then every gitlink of T's tree (ls-tree -r T) is restored the same way +# then every gitlink of the index (ls-files -s) is restored the same way ``` `--checkout` overrides `submodule..update`, and the explicit paths override `submodule..active`, so no child module is skipped. +With `c` the parent's index gets the committed pin first (`update-index +--cacheinfo` in the parent). + `RestoreLoss` (`lossOf`) follows the repositories on disk: per repository `status` (without submodules), the in-progress state files, and `rev-list --count HEAD --not --branches --tags --remotes` if HEAD is not the @@ -197,9 +229,8 @@ staging only looks at the submodule's own worktree. - If the checked-out commit is not pushed, it is staged anyway. The row then shows `unpushed` (Danger), which is the warning. - There is no confirmation: nothing is lost. -- Undo: there is no unstage action. `git restore --staged ` in the - parent undoes it. Restoring the superproject also discards every staged - pin, together with everything else. +- Undo: restore the submodule and choose `c` (committed pin), see + [Target](#target). ### Implementation note diff --git a/docs/decisions.md b/docs/decisions.md index fea0fb9..b061ffb 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -336,6 +336,15 @@ the same for every submodule of the target tree, initializing missing ones. clone a missing submodule and fetch a missing commit, non-interactively and with a timeout, because "no child module missing" cannot be met offline. - Key `x`, not `R`, which sits next to `r` (reload). +- *Staged pins are kept, at every depth.* The first version reset the + superproject's index and so dropped staged pins silently (they were not + counted as a loss). A staged pin is a decision, not drift: each + repository's staged pins are put back after its reset and are the targets + of its submodules. Unstaging gets a place instead: for a submodule with a + staged pin, `x` asks *staged* (`s`) or *committed* (`c`, unstages it). That + asks only where the intent is ambiguous and needs no extra key. Staged pins + of a repository that moves to another commit can't be carried over and + count as a loss. **Consequences.** `Restorer` is a new optional provider interface; the mock does not implement it. Restore is idempotent and can be repeated after a failure. It takes the time of its slowest clone; there is no per-repository diff --git a/docs/requirements.md b/docs/requirements.md index 8054525..5294c01 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -132,8 +132,10 @@ commit hashes. It doesn't answer the questions users actually have: - **F56** `x` restores the selection (a submodule, or the superproject when the header is selected): its worktree becomes exactly what git records, and - so do all submodules below it, recursively. It asks first, but only if - something would be lost. See [actions.md](actions.md#restore). + so do all submodules below it, recursively. Staged pins are kept at every + depth. For a submodule with a staged pin it asks whether to restore to the + staged or the committed pin (the latter unstages it). It asks for + confirmation only if something would be lost. See [actions.md](actions.md#restore). Stage pin is drafted in [actions.md](actions.md#stage-pin). diff --git a/docs/ux-guide.md b/docs/ux-guide.md index d05a8a9..223bc5c 100644 --- a/docs/ux-guide.md +++ b/docs/ux-guide.md @@ -281,13 +281,14 @@ the term*. | Phase | Line | |---|---| +| the submodule has a staged pin | yellow `Restore src/app to`, `s staged pin · c committed pin (unstages it)`, right-aligned `any other key cancels` | | reading what is lost | spinner, muted `checking src/app…` | | something would be lost | yellow `Restore src/app?`, red `Discards 7 changed files in 3 repositories, 2 commits on no branch, 1 untracked repository.`, right-aligned `y restore · n cancel` | | running | spinner, yellow `restoring src/app…` | | failed | red `restore of src/app failed: `, until the next key | -`y` restores, any other key cancels; the keys are not passed on while the -question is open. Without a loss there is no question. Success flashes +`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`. 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 5a293aa..cd6e17d 100644 --- a/internal/submodule/git/restore.go +++ b/internal/submodule/git/restore.go @@ -28,8 +28,8 @@ type gitlink struct{ path, sha string } // RestoreLoss reports what Restore(path) would discard. It walks the // repositories on disk, not the loaded tree. -func (p *Provider) RestoreLoss(ctx context.Context, path string) (submodule.Loss, error) { - dir, target, err := p.restoreTarget(ctx, path) +func (p *Provider) RestoreLoss(ctx context.Context, path string, to submodule.RestoreTarget) (submodule.Loss, error) { + dir, target, err := p.restoreTarget(ctx, path, to) if err != nil { return submodule.Loss{}, err } @@ -40,9 +40,10 @@ func (p *Provider) RestoreLoss(ctx context.Context, path string) (submodule.Loss } // Restore makes path ("" is the superproject) and all submodules below it -// exactly what git records, see docs/actions.md. A missing submodule is -// cloned and missing commits are fetched; git never prompts. -func (p *Provider) Restore(ctx context.Context, path string) error { +// exactly what git records, see docs/actions.md. Staged pins are kept. With +// RestoreCommitted, a submodule's own staged pin is unstaged first. A missing +// submodule is cloned and missing commits are fetched; git never prompts. +func (p *Provider) Restore(ctx context.Context, path string, to submodule.RestoreTarget) error { if path == "" { return p.restoreRepo(ctx, p.root, "HEAD") } @@ -52,10 +53,15 @@ func (p *Provider) Restore(ctx context.Context, path string) error { if !ok { return fmt.Errorf("unknown submodule %q", path) } - pin, err := indexGitlink(ctx, in) + pin, err := pinOf(ctx, in, to) if err != nil { return err } + if to == submodule.RestoreCommitted { + if _, err := p.exec(ctx, in.superDir, nil, "update-index", "--cacheinfo", "160000,"+pin+","+in.relPath); err != nil { + return err + } + } if !initialized(in.dir) { if err := p.initSubmodules(ctx, in.superDir, []string{in.relPath}); err != nil { return fmt.Errorf("%s: %w", path, err) @@ -65,9 +71,9 @@ func (p *Provider) Restore(ctx context.Context, path string) error { } // restoreTarget returns the directory and the commit path is restored to: -// HEAD for the superproject, else the gitlink in its parent's index. The -// directory is "" if the submodule is not initialized. -func (p *Provider) restoreTarget(ctx context.Context, path string) (dir, target string, err error) { +// HEAD for the superproject, else the pin to. The directory is "" if the +// submodule is not initialized. +func (p *Provider) restoreTarget(ctx context.Context, path string, to submodule.RestoreTarget) (dir, target string, err error) { if path == "" { return p.root, "HEAD", nil } @@ -77,7 +83,7 @@ func (p *Provider) restoreTarget(ctx context.Context, path string) (dir, target if !ok { return "", "", fmt.Errorf("unknown submodule %q", path) } - pin, err := indexGitlink(ctx, in) + pin, err := pinOf(ctx, in, to) if err != nil { return "", "", err } @@ -87,7 +93,19 @@ func (p *Provider) restoreTarget(ctx context.Context, path string) (dir, target return in.dir, pin, nil } -func indexGitlink(ctx context.Context, in info) (string, error) { +// pinOf returns the gitlink of a submodule in its parent's index (staged) +// or HEAD (committed). +func pinOf(ctx context.Context, in info, to submodule.RestoreTarget) (string, error) { + if to == submodule.RestoreCommitted { + out, _, err := try(ctx, in.superDir, "ls-tree", "HEAD", "--", in.relPath) + if err != nil { + return "", err + } + if pin := parseGitlink(out); pin != "" { + return pin, nil + } + return "", errors.New("has no committed pin (added, but not committed yet)") + } out, err := run(ctx, in.superDir, "ls-files", "-s", "--", in.relPath) if err != nil { return "", err @@ -102,8 +120,9 @@ func indexGitlink(ctx context.Context, in info) (string, error) { // --- restore ----------------------------------------------------------------- // restoreRepo restores the initialized repository at dir to target (a commit -// or "HEAD") and then every submodule that target records. Branches are never -// moved: a checkout that is not on target is detached first. +// or "HEAD") and then every submodule its index records. Branches are never +// moved: a checkout that is not on target is detached first. If the checkout +// stays on target, its staged pins are kept and are the submodules' targets. func (p *Provider) restoreRepo(ctx context.Context, dir, target string) error { env := p.networkEnv() g := func(args ...string) (string, error) { @@ -119,14 +138,22 @@ func (p *Provider) restoreRepo(ctx context.Context, dir, target string) error { if gitDir, _, err := gitDirs(dir); err == nil { p.clearOperations(ctx, dir, gitDir) } + var keep []gitlink if head, err := g("rev-parse", "--verify", "-q", "HEAD"); err != nil || head != sha { if _, err := g("checkout", "--force", "--detach", sha); err != nil { return err } + } else if keep, err = stagedPins(ctx, dir); err != nil { + return err } if _, err := g("reset", "--hard", "-q"); err != nil { return err } + for _, l := range keep { + if _, err := g("update-index", "--cacheinfo", "160000,"+l.sha+","+l.path); err != nil { + return err + } + } // -ff also removes untracked repositories; ignored files stay if _, err := g("clean", "-ffdq"); err != nil { return err @@ -135,7 +162,7 @@ func (p *Provider) restoreRepo(ctx context.Context, dir, target string) error { return err } - links, err := treeGitlinks(ctx, dir, sha) + links, err := indexGitlinks(ctx, dir) if err != nil { return err } @@ -276,6 +303,31 @@ func indexGitlinks(ctx context.Context, dir string) ([]gitlink, error) { return links, nil } +// stagedPins lists the gitlinks of the index of dir that differ from HEAD's, +// i.e. staged pins of submodules that HEAD has. A submodule that was added +// but not committed is not one: its .gitmodules entry is not kept either. +func stagedPins(ctx context.Context, dir string) ([]gitlink, error) { + index, err := indexGitlinks(ctx, dir) + if err != nil { + return nil, err + } + tree, err := treeGitlinks(ctx, dir, "HEAD") + if err != nil { + return nil, err + } + committed := map[string]string{} + for _, l := range tree { + committed[l.path] = l.sha + } + var staged []gitlink + for _, l := range index { + if c, ok := committed[l.path]; ok && c != l.sha { + staged = append(staged, l) + } + } + return staged, nil +} + // --- loss -------------------------------------------------------------------- // lossOf adds up what a restore of the repository at dir to target @@ -341,22 +393,38 @@ func (p *Provider) lossOf(ctx context.Context, dir, target string) submodule.Los unknown() } } - } - if children, err = indexGitlinks(ctx, dir); err != nil { - unknown() - } - if target != "" { - if links, err := treeGitlinks(ctx, dir, target); err == nil { + // staged pins are kept, unless the checkout moves away from + // the commit they were staged on; then the submodules follow + // the target's tree + staged, err := stagedPins(ctx, dir) + if err != nil { + unknown() + } + var links []gitlink + switch { + case !moves: + links, err = indexGitlinks(ctx, dir) + case target != "": + own.StagedPins = len(staged) + links, err = treeGitlinks(ctx, dir, target) + default: + own.StagedPins = len(staged) + } + if err == nil && links != nil { targets = map[string]string{} for _, l := range links { targets[l.path] = l.sha } } } + + if children, err = indexGitlinks(ctx, dir); err != nil { + unknown() + } }() - if own.Files+own.Operations+own.Commits+own.Repositories+own.Unknown > 0 { + if own.Files+own.Operations+own.Commits+own.Repositories+own.StagedPins+own.Unknown > 0 { own.Modules = 1 } diff --git a/internal/submodule/git/restore_test.go b/internal/submodule/git/restore_test.go index fa4633a..0c84340 100644 --- a/internal/submodule/git/restore_test.go +++ b/internal/submodule/git/restore_test.go @@ -24,12 +24,16 @@ func out(t *testing.T, dir string, args ...string) string { func exists(path string) bool { _, err := os.Stat(path); return err == nil } // assertExact checks that the superproject at dir and every submodule below -// it is initialized, on its pin and has no change at all. +// it is initialized, on its pin (staged ones included) and has no change +// besides staged pins. func assertExact(t *testing.T, dir string) { t.Helper() - if s := out(t, dir, "status", "--porcelain"); s != "" { + if s := out(t, dir, "diff", "--name-only", "--ignore-submodules=dirty"); s != "" { // worktree vs index t.Errorf("%s not clean:\n%s", dir, s) } + if s := out(t, dir, "ls-files", "--others", "--exclude-standard"); s != "" { + t.Errorf("%s has untracked files:\n%s", dir, s) + } raw, err := exec.Command("git", "-C", dir, "submodule", "status", "--recursive").Output() // the prefix is a leading space if err != nil { t.Fatal(err) @@ -43,9 +47,13 @@ func assertExact(t *testing.T, dir string) { continue } f := strings.Fields(line) - if s := out(t, filepath.Join(dir, f[1]), "status", "--porcelain"); s != "" { + sub := filepath.Join(dir, f[1]) + if s := out(t, sub, "diff", "--name-only", "--ignore-submodules=dirty"); s != "" { t.Errorf("%s not clean:\n%s", f[1], s) } + if s := out(t, sub, "ls-files", "--others", "--exclude-standard"); s != "" { + t.Errorf("%s has untracked files:\n%s", f[1], s) + } } } @@ -72,7 +80,7 @@ func TestRestoreSuperprojectMakesEverythingExact(t *testing.T) { conflict := filepath.Join(super, "libs", "conflict") mainTip := out(t, conflict, "rev-parse", "main") - if err := p.Restore(ctx, ""); err != nil { + if err := p.Restore(ctx, "", submodule.RestoreStaged); err != nil { t.Fatal(err) } assertExact(t, super) @@ -88,8 +96,9 @@ func TestRestoreSuperprojectMakesEverythingExact(t *testing.T) { if !exists(filepath.Join(super, "libs", "missing", ".git")) { t.Error("libs/missing was not initialized") } - if out(t, super, "diff", "--cached", "--name-only") != "" { - t.Error("staged pin survived a superproject restore") + // the fixture stages libs/staged and libs/conflict + if got := out(t, super, "diff", "--cached", "--name-only"); got != "libs/conflict\nlibs/staged" { + t.Errorf("staged = %q, want the staged pins kept", got) } } @@ -100,7 +109,7 @@ func TestRestoreSubmoduleKeepsStagedPinAsTarget(t *testing.T) { // the fixture staged the checked-out commit; move the checkout away git(t, filepath.Join(super, "libs", "staged"), "checkout", "-q", "--detach", "HEAD~1") - if err := p.Restore(context.Background(), "libs/staged"); err != nil { + if err := p.Restore(context.Background(), "libs/staged", submodule.RestoreStaged); err != nil { t.Fatal(err) } if got := out(t, filepath.Join(super, "libs", "staged"), "rev-parse", "HEAD"); got != staged { @@ -118,7 +127,7 @@ func TestRestoreLoss(t *testing.T) { loss := func(path string) submodule.Loss { t.Helper() - l, err := p.RestoreLoss(ctx, path) + l, err := p.RestoreLoss(ctx, path, submodule.RestoreStaged) if err != nil { t.Fatal(err) } @@ -191,7 +200,7 @@ func TestRestoreRemovesLeftoversAndEndsRebase(t *testing.T) { t.Fatal("setup: no rebase in progress") } - if err := p.Restore(ctx, ""); err != nil { + if err := p.Restore(ctx, "", submodule.RestoreStaged); err != nil { t.Fatal(err) } for _, gone := range []string{"stray", "libs/topic"} { @@ -205,9 +214,7 @@ func TestRestoreRemovesLeftoversAndEndsRebase(t *testing.T) { if got := out(t, v, "rev-parse", "y"); got != yTip { t.Errorf("branch y moved: %s -> %s", yTip, got) } - if out(t, super, "status", "--porcelain") != "" { - t.Errorf("superproject not clean:\n%s", out(t, super, "status", "--porcelain")) - } + assertExact(t, super) } func TestRestoreClonesAMissingSubmodule(t *testing.T) { @@ -219,7 +226,7 @@ func TestRestoreClonesAMissingSubmodule(t *testing.T) { if err := os.RemoveAll(filepath.Join(super, ".git", "modules", "libs", "missing")); err != nil { t.Fatal(err) } - if err := p.Restore(context.Background(), "libs/missing"); err != nil { + if err := p.Restore(context.Background(), "libs/missing", submodule.RestoreStaged); err != nil { t.Fatal(err) } if !exists(filepath.Join(super, "libs", "missing", ".git")) { @@ -238,7 +245,7 @@ func TestRestoreReportsAnUnavailablePin(t *testing.T) { bogus := "0123456789abcdef0123456789abcdef01234567" git(t, super, "update-index", "--cacheinfo", "160000,"+bogus+",src/core") _ = core - err := p.Restore(context.Background(), "src/core") + err := p.Restore(context.Background(), "src/core", submodule.RestoreStaged) if err == nil || !strings.Contains(err.Error(), "not available") { t.Fatalf("err = %v, want a message about the unavailable commit", err) } @@ -251,7 +258,7 @@ func TestRestoreIgnoresUpdateNoneAndInactive(t *testing.T) { git(t, super, "config", "submodule.libs/missing.active", "false") git(t, super, "config", "submodule.active", "src") - if err := p.Restore(context.Background(), ""); err != nil { + if err := p.Restore(context.Background(), "", submodule.RestoreStaged); err != nil { t.Fatal(err) } if !exists(filepath.Join(super, "libs", "missing", ".git")) { @@ -259,3 +266,69 @@ func TestRestoreIgnoresUpdateNoneAndInactive(t *testing.T) { } assertExact(t, super) } + +func TestRestoreKeepsNestedStagedPinAndResetsToIt(t *testing.T) { + p, root := ownFixture(t) + super := filepath.Join(root, "super") + log := filepath.Join(super, "libs", "log") + nested := filepath.Join(log, "modules", "nested") + + // stage a new pin of nested in libs/log, then change nested again + git(t, nested, "commit", "-q", "-am", "Nested work") + staged := out(t, nested, "rev-parse", "HEAD") + git(t, log, "add", "modules/nested") + os.WriteFile(filepath.Join(nested, "README"), []byte("more\n"), 0o644) + + if l, err := p.RestoreLoss(context.Background(), "", submodule.RestoreStaged); err != nil || l.StagedPins != 0 || l.Files == 0 { + t.Fatalf("loss %+v, %v: want the change, not the staged pin", l, err) + } + if err := p.Restore(context.Background(), "", submodule.RestoreStaged); err != nil { + t.Fatal(err) + } + if got := out(t, log, "diff", "--cached", "--name-only"); got != "modules/nested" { + t.Errorf("libs/log staged = %q, want the nested pin", got) + } + if got := out(t, nested, "rev-parse", "HEAD"); got != staged { + t.Errorf("nested HEAD = %s, want the staged pin %s", got, staged) + } + assertExact(t, super) +} + +func TestRestoreCommittedUnstagesThePin(t *testing.T) { + p, root := ownFixture(t) + super := filepath.Join(root, "super") + committed := strings.Fields(out(t, super, "ls-tree", "HEAD", "libs/staged"))[2] + + if err := p.Restore(context.Background(), "libs/staged", submodule.RestoreCommitted); err != nil { + t.Fatal(err) + } + if got := out(t, super, "diff", "--cached", "--name-only"); strings.Contains(got, "libs/staged") { + t.Errorf("still staged: %q", got) + } + if got := out(t, filepath.Join(super, "libs", "staged"), "rev-parse", "HEAD"); got != committed { + t.Errorf("HEAD = %s, want the committed pin %s", got, committed) + } +} + +func TestRestoreLossCountsStagedPinsOfAMovingRepository(t *testing.T) { + p, root := ownFixture(t) + super := filepath.Join(root, "super") + log := filepath.Join(super, "libs", "log") + nested := filepath.Join(log, "modules", "nested") + + // libs/log moves on (off pin, on its branch) and stages a nested pin + git(t, nested, "commit", "-q", "-am", "Nested work") + git(t, log, "add", "modules/nested") + git(t, log, "commit", "-q", "--allow-empty", "-m", "Log work") + git(t, nested, "commit", "-q", "--allow-empty", "-m", "More nested work") + git(t, log, "add", "modules/nested") + + l, err := p.RestoreLoss(context.Background(), "libs/log", submodule.RestoreStaged) + if err != nil { + t.Fatal(err) + } + // the nested commits are on a detached HEAD that moves back: lost too + if l.StagedPins != 1 || l.Commits != 2 { + t.Errorf("loss %+v, want 1 staged pin and the 2 detached nested commits", l) + } +} diff --git a/internal/submodule/provider.go b/internal/submodule/provider.go index cca2672..c9c2759 100644 --- a/internal/submodule/provider.go +++ b/internal/submodule/provider.go @@ -40,10 +40,19 @@ type Locator interface { type Restorer interface { // RestoreLoss reports what Restore would discard, read from the disk at // this moment. - RestoreLoss(ctx context.Context, path string) (Loss, error) - Restore(ctx context.Context, path string) error + RestoreLoss(ctx context.Context, path string, to RestoreTarget) (Loss, error) + Restore(ctx context.Context, path string, to RestoreTarget) error } +// RestoreTarget is the pin a submodule is restored to. It doesn't matter for +// the superproject, whose target is always HEAD. +type RestoreTarget int + +const ( + RestoreStaged RestoreTarget = iota // the pin in the parent's index: a staged pin is kept and used + RestoreCommitted // the pin in the parent's HEAD: a staged pin is unstaged +) + // Loss is what a restore discards. The zero Loss means nothing is lost. type Loss struct { Files int // changed, conflicted and untracked files and directories @@ -51,6 +60,7 @@ type Loss struct { Operations int // merges, rebases, cherry-picks, reverts in progress Commits int // commits that no branch, tag or remote ref reaches Repositories int // untracked repositories with their own .git directory + StagedPins int // staged pins of repositories that move to another commit Unknown int // repositories whose state could not be read } @@ -65,6 +75,7 @@ func (l Loss) Add(o Loss) Loss { Operations: l.Operations + o.Operations, Commits: l.Commits + o.Commits, Repositories: l.Repositories + o.Repositories, + StagedPins: l.StagedPins + o.StagedPins, Unknown: l.Unknown + o.Unknown, } } diff --git a/internal/tui/app.go b/internal/tui/app.go index 9c7a95a..6efeed4 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -159,7 +159,7 @@ func (m *Model) startFetch() tea.Cmd { // busy reports whether something shows a spinner. func (m Model) busy() bool { - if m.repo == nil || m.fetch.Running || (m.restore != nil && m.restore.phase != restoreAsking) { + if m.repo == nil || m.fetch.Running || (m.restore != nil && (m.restore.phase == restoreChecking || m.restore.phase == restoreRunning)) { return true } loading := m.repo.Root != nil && m.repo.Root.Status == nil && m.repo.Root.Err == nil @@ -331,8 +331,13 @@ func (m Model) changed(msg changeMsg) (tea.Model, tea.Cmd) { func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { k := m.keys - if m.restore != nil && m.restore.phase == restoreAsking { - return m.answerRestore(msg) + if m.restore != nil { + switch m.restore.phase { + case restoreChoosing: + return m.choosePin(msg) + case restoreAsking: + return m.answerRestore(msg) + } } if m.notice != "" { m.notice = "" diff --git a/internal/tui/restore.go b/internal/tui/restore.go index e4fe2e1..a1ab739 100644 --- a/internal/tui/restore.go +++ b/internal/tui/restore.go @@ -15,7 +15,8 @@ import ( type restorePhase int const ( - restoreChecking restorePhase = iota // reading what would be lost + restoreChoosing restorePhase = iota // staged or committed pin? + restoreChecking // reading what would be lost restoreAsking // waiting for y / n restoreRunning ) @@ -24,6 +25,7 @@ const ( type restoreState struct { path, name string // name is how the footer calls it phase restorePhase + to submodule.RestoreTarget loss submodule.Loss } @@ -40,10 +42,11 @@ type restoreDoneMsg struct { } // startRestore checks what restoring the selection would discard. If nothing -// would be lost it restores at once, else it asks first. +// would be lost it restores at once, else it asks first. A submodule with a +// staged pin first asks which pin to restore to. func (m Model) startRestore() (tea.Model, tea.Cmd) { s := m.tree.Selected() - r, ok := m.provider.(submodule.Restorer) + _, ok := m.provider.(submodule.Restorer) var flash string switch { case s == nil: @@ -60,15 +63,45 @@ func (m Model) startRestore() (tea.Model, tea.Cmd) { } m.restore = &restoreState{path: s.Path, name: restoreName(s)} + if !s.IsRoot() && s.Status != nil && s.Status.PinStaged { + m.restore.phase = restoreChoosing + m.layout() + return m, nil + } + return m.checkRestore() +} + +// checkRestore reads what the restore would discard. +func (m Model) checkRestore() (tea.Model, tea.Cmd) { + m.restore.phase = restoreChecking m.layout() - ctx, path := m.ctx, s.Path + r := m.provider.(submodule.Restorer) + ctx, path, to := m.ctx, m.restore.path, m.restore.to check := func() tea.Msg { - loss, err := r.RestoreLoss(ctx, path) + loss, err := r.RestoreLoss(ctx, path, to) return lossMsg{path, loss, err} } return m, tea.Batch(check, m.tick()) } +// choosePin handles a key while the restore asks for the pin: s the staged +// one, c the committed one (which unstages it), anything else cancels. +func (m Model) choosePin(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch { + case msg.Type == tea.KeyCtrlC: + return m, tea.Quit + case msg.String() == "s": + m.restore.to = submodule.RestoreStaged + return m.checkRestore() + case msg.String() == "c": + m.restore.to = submodule.RestoreCommitted + return m.checkRestore() + } + m.restore = nil + m.layout() + return m, nil +} + // lossChecked reacts to the check: restore, or ask. func (m Model) lossChecked(msg lossMsg) (tea.Model, tea.Cmd) { if m.restore == nil || m.restore.path != msg.path || m.restore.phase != restoreChecking { @@ -107,8 +140,8 @@ func (m Model) runRestore() (tea.Model, tea.Cmd) { m.restore.phase = restoreRunning m.layout() r := m.provider.(submodule.Restorer) - ctx, path := m.ctx, m.restore.path - run := func() tea.Msg { return restoreDoneMsg{path, r.Restore(ctx, path)} } + ctx, path, to := m.ctx, m.restore.path, m.restore.to + run := func() tea.Msg { return restoreDoneMsg{path, r.Restore(ctx, path, to)} } return m, tea.Batch(run, m.tick()) } @@ -117,7 +150,7 @@ func (m Model) runRestore() (tea.Model, tea.Cmd) { func (m Model) restoreDone(msg restoreDoneMsg) (tea.Model, tea.Cmd) { name := "the superproject" if m.restore != nil { - name = m.restore.name + name = m.restore.name + m.restore.toText() } m.restore = nil var cmds []tea.Cmd @@ -131,6 +164,15 @@ func (m Model) restoreDone(msg restoreDoneMsg) (tea.Model, tea.Cmd) { return m, tea.Batch(cmds...) } +// toText is " to the committed pin" if that was chosen; the staged pin is the +// usual target and needs no words. +func (r *restoreState) toText() string { + if r.to == submodule.RestoreCommitted { + return " to the committed pin" + } + return "" +} + func restoreName(s *submodule.Submodule) string { if s == nil || s.IsRoot() { return "the superproject" @@ -142,13 +184,19 @@ func restoreName(s *submodule.Submodule) string { func (m Model) restoreView() string { r := m.restore left, right := "", "" + key := func(k string) string { return theme.Fg(m.theme.Text).Bold(true).Render(k) } + muted := func(s string) string { return theme.Fg(m.theme.Muted).Render(s) } switch r.phase { + case restoreChoosing: + left = theme.Fg(m.theme.Warn).Render("Restore "+r.name+" to") + " " + + key("s") + muted(" staged pin · ") + key("c") + muted(" committed pin (unstages it)") + right = muted("any other key cancels") case restoreChecking: left = theme.Fg(m.theme.Primary).Render(m.spinner.View()) + " " + theme.Fg(m.theme.Muted).Render("checking "+r.name+"…") case restoreRunning: - left = theme.Fg(m.theme.Primary).Render(m.spinner.View()) + " " + theme.Fg(m.theme.Warn).Render("restoring "+r.name+"…") + left = theme.Fg(m.theme.Primary).Render(m.spinner.View()) + " " + theme.Fg(m.theme.Warn).Render("restoring "+r.name+r.toText()+"…") case restoreAsking: - left = theme.Fg(m.theme.Warn).Render("Restore "+r.name+"?") + " " + + left = theme.Fg(m.theme.Warn).Render("Restore "+r.name+r.toText()+"?") + " " + theme.Fg(m.theme.Danger).Render("Discards "+lossText(r.loss)+".") right = theme.Fg(m.theme.Text).Bold(true).Render("y") + theme.Fg(m.theme.Muted).Render(" restore · ") + theme.Fg(m.theme.Text).Bold(true).Render("n") + theme.Fg(m.theme.Muted).Render(" cancel") @@ -192,6 +240,9 @@ func lossText(l submodule.Loss) string { if l.Repositories > 0 { parts = append(parts, plural(l.Repositories, "untracked repository", "untracked repositories")) } + if l.StagedPins > 0 { + parts = append(parts, plural(l.StagedPins, "staged pin", "staged pins")) + } if l.Unknown > 0 { parts = append(parts, plural(l.Unknown, "repository of unknown state", "repositories of unknown state")) } diff --git a/internal/tui/restore_test.go b/internal/tui/restore_test.go index 5e1de90..f69ad35 100644 --- a/internal/tui/restore_test.go +++ b/internal/tui/restore_test.go @@ -25,6 +25,7 @@ func TestLossText(t *testing.T) { {submodule.Loss{Files: 1, Modules: 1}, "1 changed file"}, {submodule.Loss{Files: 7, Modules: 3, Commits: 2, Repositories: 1}, "7 changed files in 3 repositories, 2 commits on no branch, 1 untracked repository"}, {submodule.Loss{Operations: 1, Modules: 1, Unknown: 2}, "1 operation in progress, 2 repositories of unknown state"}, + {submodule.Loss{StagedPins: 2, Modules: 1}, "2 staged pins"}, } for _, c := range cases { if got := lossText(c.in); got != c.want { @@ -38,14 +39,16 @@ type fakeRestorer struct { submodule.Provider loss submodule.Loss restored []string + to []submodule.RestoreTarget } -func (f *fakeRestorer) RestoreLoss(context.Context, string) (submodule.Loss, error) { +func (f *fakeRestorer) RestoreLoss(context.Context, string, submodule.RestoreTarget) (submodule.Loss, error) { return f.loss, nil } -func (f *fakeRestorer) Restore(_ context.Context, path string) error { +func (f *fakeRestorer) Restore(_ context.Context, path string, to submodule.RestoreTarget) error { f.restored = append(f.restored, path) + f.to = append(f.to, to) return nil } @@ -151,3 +154,36 @@ func TestRestoreWithoutRestorerSaysSo(t *testing.T) { t.Fatal("the mock can't restore") } } + +func TestRestoreOfAStagedPinAsksWhichPin(t *testing.T) { + m, f := newRestoreModel(t, submodule.Loss{}) + mm := m.(Model) + var staged *submodule.Submodule + submodule.Walk(mm.repo.Submodules, func(s *submodule.Submodule) { + if staged == nil && s.Children == nil { + staged = s + } + }) + staged.Status = &submodule.Status{PinStaged: true, Pin: submodule.PinInfo{Pushed: true}} + mm.tree.Select(staged.Path) + if mm.tree.Selected() != staged { + t.Fatalf("setup: selected %v", mm.tree.Selected()) + } + m = mm + + m, _ = press(m, "x") + if m.(Model).restore == nil || m.(Model).restore.phase != restoreChoosing { + t.Fatalf("want the pin question, got %+v", m.(Model).restore) + } + m, cmd := press(m, "c") + var next tea.Cmd + for _, msg := range run(cmd) { + if l, ok := msg.(lossMsg); ok { + m, next = m.Update(l) + } + } + feed(m, next) + if len(f.to) != 1 || f.to[0] != submodule.RestoreCommitted || f.restored[0] != staged.Path { + t.Fatalf("restored %q to %v, want %s to the committed pin", f.restored, f.to, staged.Path) + } +}