diff --git a/candy/plugin-task/nested_work_test.go b/candy/plugin-task/nested_work_test.go new file mode 100644 index 0000000..65384d1 --- /dev/null +++ b/candy/plugin-task/nested_work_test.go @@ -0,0 +1,140 @@ +package task + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/opencharly/spec/spec" +) + +// nestedWorkFixture builds the tree opencharly/plugin-task#54 is about: an umbrella +// that pins `outer` at O1, `outer` that pins its own `inner` at L1, and BOTH levels +// initialized — the shape `git clone --recurse-submodules` produces. +// +// `outer`'s main is at O2, the commit that records `inner` at L2, so the bump ROLLS +// outer and the reconcile then moves the nested checkout from L1 to L2. The nested +// checkout is given L2 locally, the way a real nested remote would serve it. +// +// It returns the umbrella, the nested checkout's directory, and the SHAs the +// assertions speak about. +func nestedWorkFixture(t *testing.T) (umb, innerCo, o1, l1, l2 string) { + t.Helper() + base, leafBare, _, L1, L2 := bumpOrigins(t) + + outer := filepath.Join(base, "outer-src") + gitIn(t, base, "init", "-q", "-b", "main", "outer-src") + writeFile(t, filepath.Join(outer, "o"), "o") + gitIn(t, outer, "add", "-A") + gitIn(t, outer, "commit", "-qm", "o") + gitIn(t, outer, "-c", "protocol.file.allow=always", "submodule", "add", "-q", leafBare, "inner") + gitIn(t, outer, "config", "-f", ".gitmodules", "submodule.inner.branch", "main") + gitIn(t, outer, "add", ".gitmodules") + gitIn(t, outer, "commit", "-qm", "inner=main") + gitIn(t, outer, "update-index", "--add", "--cacheinfo", "160000,"+L1+",inner") + gitIn(t, outer, "commit", "-qm", "inner=L1") + o1 = gitIn(t, outer, "rev-parse", "HEAD") + // Advance inner to L2 inside outer: that is the commit the bump rolls outer to. + gitIn(t, outer, "-c", "protocol.file.allow=always", "submodule", "update", "--init", "inner") + gitIn(t, filepath.Join(outer, "inner"), "fetch", "-q", "origin") + gitIn(t, filepath.Join(outer, "inner"), "switch", "--quiet", "--detach", L2) + gitIn(t, outer, "add", "inner") + gitIn(t, outer, "commit", "-qm", "inner=L2") + outerBare := filepath.Join(base, "outer.git") + gitIn(t, base, "clone", "-q", "--bare", outer, outerBare) + + umb = filepath.Join(base, "umb-nested-work") + gitIn(t, base, "init", "-q", "-b", "main", "umb-nested-work") + writeFile(t, filepath.Join(umb, "u"), "u") + gitIn(t, umb, "add", "-A") + gitIn(t, umb, "commit", "-qm", "u") + gitIn(t, umb, "-c", "protocol.file.allow=always", "submodule", "add", "-q", outerBare, "outer") + gitIn(t, umb, "config", "-f", ".gitmodules", "submodule.outer.branch", "main") + gitIn(t, umb, "add", ".gitmodules") + gitIn(t, umb, "commit", "-qm", "branch=main") + gitIn(t, umb, "update-index", "--add", "--cacheinfo", "160000,"+o1+",outer") + gitIn(t, umb, "commit", "-qm", "stale pin") + gitIn(t, umb, "-c", "protocol.file.allow=always", "submodule", "update", "--init", "outer") + outerCo := filepath.Join(umb, "outer") + gitIn(t, outerCo, "-c", "protocol.file.allow=always", "submodule", "update", "--init", "inner") + innerCo = filepath.Join(outerCo, "inner") + gitIn(t, innerCo, "-c", "protocol.file.allow=always", "fetch", "-q", "origin", L2) + return umb, innerCo, o1, L1, L2 +} + +// TestGitSubmodules_BumpRefusesToDropNestedWork is opencharly/plugin-task#54's gate. +// +// THE DEFECT: the reconcile moves the nested checkouts with `git submodule update +// --recursive`, and that update takes a nested checkout off the commit it is at — even +// when that commit carries work NOTHING ELSE REFERENCES — with exit 0 and nothing +// reported. The operator's commit survives only in the reflog. MEASURED against git +// 2.56: the same update refuses a DIRTY nested checkout loudly, so the loss is not +// git's doing but the update's blind spot. +// +// THE ASSERTION: the bump REFUSES, it NAMES the nested checkout, and it leaves the +// tree exactly as it was — the nested checkout still holds the operator's commit, the +// superproject's pin was NOT staged, and the submodule's own checkout was not switched. +// A bump that refuses is the only outcome that loses nothing. +// +// Before the fix this FAILS: the bump PASSES and the nested checkout is moved off the +// operator's commit, with nothing reported. +func TestGitSubmodules_BumpRefusesToDropNestedWork(t *testing.T) { + umb, innerCo, o1, _, _ := nestedWorkFixture(t) + + // The operator's work: a commit in the nested checkout that NO ref references — + // the shape the update drops. + writeFile(t, filepath.Join(innerCo, "work"), "work") + gitIn(t, innerCo, "add", "-A") + gitIn(t, innerCo, "commit", "-qm", "local work") + work := gitIn(t, innerCo, "rev-parse", "HEAD") + if out := gitIn(t, innerCo, "for-each-ref", "--contains", work, "--format=%(refname)"); out != "" { + t.Fatalf("fixture: the local commit must be referenced by no ref, got %q", out) + } + + st, msg := runMaintenanceVerbIn(umb, "git-submodules", map[string]any{"mode": "bump"}) + if st != spec.StatusFail { + t.Fatalf("the bump must REFUSE a nested checkout whose commit the update would drop, got %s: %s", st, msg) + } + if !strings.Contains(msg, "outer/inner") { + t.Fatalf("the refusal must NAME the nested checkout, got: %s", msg) + } + if got := gitIn(t, innerCo, "rev-parse", "HEAD"); got != work { + t.Fatalf("the nested checkout must still hold the operator's commit %s, got %s", work, got) + } + // The tree is untouched: the pin was not staged and the submodule was not switched. + if got := gitlink(umb, "outer"); got != o1 { + t.Fatalf("the refusal must leave the superproject's pin at %s, got %s", o1, got) + } + if got := gitIn(t, filepath.Join(umb, "outer"), "rev-parse", "HEAD"); got != o1 { + t.Fatalf("the refusal must leave the submodule's checkout at %s, got %s", o1, got) + } +} + +// TestGitSubmodules_BumpKeepsNestedWorkThatABranchHolds is the DISCRIMINATING half of +// the gate: the same work, on a BRANCH, is not lost by the update — the branch still +// holds the commit after the checkout moved — so the bump must go through. +// +// Without this half the gate would also pass a verb that refuses every nested +// checkout, which is not a fix but a freeze (R7: a test that cannot fail is invalid). +func TestGitSubmodules_BumpKeepsNestedWorkThatABranchHolds(t *testing.T) { + umb, innerCo, _, _, L2 := nestedWorkFixture(t) + + writeFile(t, filepath.Join(innerCo, "work"), "work") + gitIn(t, innerCo, "add", "-A") + gitIn(t, innerCo, "commit", "-qm", "local work") + work := gitIn(t, innerCo, "rev-parse", "HEAD") + gitIn(t, innerCo, "branch", "keep") + + st, msg := runMaintenanceVerbIn(umb, "git-submodules", map[string]any{"mode": "bump"}) + if st != spec.StatusPass { + t.Fatalf("the bump must go through when a branch holds the local commit, got %s: %s", st, msg) + } + // The work survived the move. + if got := gitIn(t, innerCo, "rev-parse", "keep"); got != work { + t.Fatalf("the branch must still hold the operator's commit %s, got %s", work, got) + } + // And the nested checkout followed the pin the new commit records. + if got := gitIn(t, innerCo, "rev-parse", "HEAD"); got != L2 { + t.Fatalf("the nested checkout must be at the recorded pin %s, got %s", L2, got) + } +} diff --git a/candy/plugin-task/prune.go b/candy/plugin-task/prune.go index b9251d5..bd7e78e 100644 --- a/candy/plugin-task/prune.go +++ b/candy/plugin-task/prune.go @@ -279,7 +279,7 @@ func branchMerged(repoDir, branch, tip, base string, prs map[string][]prInfo, gh // strictly smaller, never larger). func pruneRepos(projDir string) []string { repos := []string{projDir} - subs, err := submodulePaths(projDir) + subs, err := gitmodulesPaths(projDir) if err != nil { return repos } diff --git a/candy/plugin-task/verbs.go b/candy/plugin-task/verbs.go index 11fd9b1..5406936 100644 --- a/candy/plugin-task/verbs.go +++ b/candy/plugin-task/verbs.go @@ -130,9 +130,12 @@ func hostCapture(ctx context.Context, dir, script string) (string, string, int) return stdout.String(), stderr.String(), exit } -// submodulePaths lists every `submodule..path` in .gitmodules (sorted). -func submodulePaths(projDir string) ([]string, error) { - out, _, exit := hostCapture(context.Background(), projDir, +// gitmodulesPaths lists every `submodule..path` in the .gitmodules OF `dir` +// (sorted). `dir` is the directory the .gitmodules is read from, so the same reader +// serves the PROJECT's own .gitmodules and the one a submodule declares for its own +// nested checkouts (opencharly/plugin-task#54) — one reader, one shape. +func gitmodulesPaths(dir string) ([]string, error) { + out, _, exit := hostCapture(context.Background(), dir, "git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' | awk '{print $2}'") if exit != 0 { return nil, fmt.Errorf("read .gitmodules (exit %d)", exit) @@ -523,7 +526,7 @@ func (g pinGraph) edgeSources(paths []string) []edgeRef { // advance, because a sync over a tree it could not fully advance must never report // success. func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status, string) { - paths, err := submodulePaths(projDir) + paths, err := gitmodulesPaths(projDir) if err != nil { return spec.StatusFail, err.Error() } @@ -1397,20 +1400,37 @@ func submoduleAt(projDir, path string) (abs, gitArgs string, err error) { // clone — the exact failure sync-gitlinks.sh documents), detaches, and stages. // It refuses an uninitialized submodule (charly#768) and drives the checkout // through submoduleAt's walk-up-safe `--git-dir`/`--work-tree` form. +// +// The fetch and the switch are TWO commands, because the NESTED CHECKOUTS ARE JUDGED +// between them (opencharly/plugin-task#54): the reconcile moves them, and a move that +// drops the operator's work must be refused BEFORE this checkout is switched and +// before the pin is staged — so a refusal leaves the tree exactly as it was. func pinSubmodule(projDir, path, ref string) (spec.Status, string) { - _, gitArgs, aerr := submoduleAt(projDir, path) + abs, gitArgs, aerr := submoduleAt(projDir, path) if aerr != nil { return spec.StatusFail, aerr.Error() } fetch := fmt.Sprintf( "set -e; git %s fetch --quiet origin; "+ - "if ! git %s cat-file -e %s^{commit} 2>/dev/null; then git %s fetch --quiet origin %s; fi; "+ - "git %s switch --quiet --detach %s", - gitArgs, gitArgs, shellQuote(ref), gitArgs, shellQuote(ref), gitArgs, shellQuote(ref)) + "if ! git %s cat-file -e %s^{commit} 2>/dev/null; then git %s fetch --quiet origin %s; fi", + gitArgs, gitArgs, shellQuote(ref), gitArgs, shellQuote(ref)) _, stderr, exit := hostCapture(context.Background(), projDir, fetch) if exit != 0 { return spec.StatusFail, fmt.Sprintf("pin %s -> %s failed: %s", path, ref, strings.TrimSpace(stderr)) } + // JUDGE THE NESTED CHECKOUTS, before this checkout moves and before the pin is + // staged. The reconcile below moves them with `git submodule update --recursive`, + // and that move does not ask: it takes a nested checkout off the commit it is at + // even when that commit carries work nothing else references. A bump that drops + // the operator's work is worse than a bump that refuses, so the work is judged + // here and a refusal stops the whole bump with the tree untouched. + if st, msg := judgeNestedLevel(abs, gitArgs, ref, path); st != spec.StatusPass { + return st, msg + } + switchCmd := fmt.Sprintf("git %s switch --quiet --detach %s", gitArgs, shellQuote(ref)) + if _, stderr, exit := hostCapture(context.Background(), projDir, switchCmd); exit != 0 { + return spec.StatusFail, fmt.Sprintf("pin %s -> %s failed: %s", path, ref, strings.TrimSpace(stderr)) + } // Stage the gitlink in the SUPERPROJECT (a separate repo from the submodule, // hence a separate git invocation, run with the project dir as its cwd). if _, stderr, exit := hostCapture(context.Background(), projDir, @@ -1424,7 +1444,9 @@ func pinSubmodule(projDir, path, ref string) (spec.Status, string) { // `git-submodules verify` then fails twice over, on a change this very bump just // made. The reconciliation is recursive (a nested checkout can itself nest) and a // failure to complete it is a hard error: a bump that leaves the tree dirty is - // worse than a bump that refuses. + // worse than a bump that refuses. What the move would DESTROY was judged before + // the switch (judgeNestedLevel), so this step only ever moves checkouts whose + // work survives the move. if st, msg := reconcileNestedCheckouts(projDir, path); st != spec.StatusPass { return st, msg } @@ -1460,10 +1482,166 @@ func advancePinInIndex(projDir, path, sha string) (spec.Status, string) { return spec.StatusPass, "" } +// treeGitlinks maps every GITLINK path in `commit`'s tree to the commit it records. +// +// A gitlink is the tree entry of a submodule: mode 160000, and the SHA it carries is the +// commit the submodule is pinned at. +// +// `ok` is false when the tree could not be read here (an object this repo does not +// have) — the caller then falls back to what the checkout declares, and says so in its +// own report. `ok` is true for a tree that records NO submodule at all: that is a fact +// about the tree, not a missing object, and the two must not be folded together. +func treeGitlinks(gitArgs, commit string) (links map[string]string, ok bool) { + links = map[string]string{} + if commit == "" { + return links, false + } + out, _, exit := hostCapture(context.Background(), "", + fmt.Sprintf("git %s ls-tree %s", gitArgs, shellQuote(commit))) + if exit != 0 { + return links, false + } + for _, line := range strings.Split(out, "\n") { + // ls-tree prints " SP SP TAB "; the path is the + // LAST field and may contain spaces, so it is cut on the TAB, not split. + meta, path, cut := strings.Cut(line, "\t") + if !cut { + continue + } + f := strings.Fields(meta) + if len(f) != 3 || f[0] != "160000" { + continue + } + links[path] = f[2] + } + return links, true +} + +// judgeNestedLevel judges every nested checkout under `dir` that the reconcile would +// MOVE, and recurses into them. +// +// `target` is the commit the checkout at `dir` will be AT once the pin lands: the +// gitlinks THAT commit records are what each nested checkout is moved to. It is "" when +// the commit cannot be read here (an object this repo does not have) — then the paths +// come from the checkout's OWN .gitmodules and only the ref arm of the judgement +// decides, which is the arm that catches work being dropped. +// +// The judgement runs BEFORE anything moves (opencharly/plugin-task#54), so a refusal +// leaves the tree exactly as it was. +func judgeNestedLevel(dir, gitArgs, target, label string) (spec.Status, string) { + links, ok := treeGitlinks(gitArgs, target) + paths := make([]string, 0, len(links)) + for p := range links { + paths = append(paths, p) + } + if !ok { + // The target's tree could not be read here: fall back to the paths the + // checkout DECLARES, which is what the update consults when the target's tree + // is not available to it either. + paths, _ = gitmodulesPaths(dir) + } + sort.Strings(paths) + for _, p := range paths { + if !submoduleInitialized(dir, p) { + // No checkout: the update never initializes one (no `--init`), so there is + // nothing to move and nothing to lose. + continue + } + rec := links[p] + if st, msg := judgeNestedCheckout(dir, p, rec, label+"/"+p); st != spec.StatusPass { + return st, msg + } + // RECURSE: that checkout will be at `rec`, and ITS nested checkouts are moved + // to the gitlinks `rec` records. + _, ngitArgs, aerr := submoduleAt(dir, p) + if aerr != nil { + continue + } + if st, msg := judgeNestedLevel(filepath.Join(dir, p), ngitArgs, rec, label+"/"+p); st != spec.StatusPass { + return st, msg + } + } + return spec.StatusPass, "" +} + +// judgeNestedCheckout decides whether the reconcile may MOVE the initialized nested +// checkout at `dir/path`, whose recorded gitlink is `rec`. +// +// THE QUESTION IS ONE: after the move, will the commit the checkout is at still be +// reachable? git's `submodule update --recursive` moves a nested checkout onto the +// recorded gitlink WITHOUT asking, and a commit that nothing else references is +// dropped from HEAD and survives only in the reflog — the operator's work, gone, with +// exit 0 and nothing reported. +// +// MEASURED (git 2.56, four shapes, a real umbrella -> outer -> inner fixture): +// +// nested checkout at the update the local commit +// ----------------------------------------------------------------------- +// the recorded pin, behind exit 0 nothing to lose +// the recorded pin + a commit on a branch exit 0 kept by that branch +// the recorded pin + an ORPHAN commit exit 0 DROPPED, reported by nothing +// the recorded pin + a dirty file exit 1 git refuses, loudly +// +// So the checkout may be moved when the commit it is at will still be reachable: when +// the move goes FORWARD onto it (it is an ancestor of the recorded gitlink) or when a +// ref in that checkout still contains it (the commit survives the move on that ref). +// Otherwise the bump REFUSES, naming the path. +// +// The ancestry arm is decisive only when the recorded commit is present locally; when +// it is not, the ref arm decides, and the refusal says the pin was unreadable. +func judgeNestedCheckout(dir, path, rec, label string) (spec.Status, string) { + _, gitArgs, aerr := submoduleAt(dir, path) + if aerr != nil { + return spec.StatusFail, aerr.Error() + } + head, _, exit := hostCapture(context.Background(), dir, + fmt.Sprintf("git %s rev-parse HEAD", gitArgs)) + if exit != 0 { + // No HEAD (an unborn branch): there is no commit to lose. + return spec.StatusPass, "" + } + head = strings.TrimSpace(head) + if rec == head { + // Already AT the recorded gitlink: the move is a no-op for it. + return spec.StatusPass, "" + } + // The move goes FORWARD onto this commit: the update only adds history, so the + // commit it is at survives the move. + if rec != "" { + if _, _, exit := hostCapture(context.Background(), dir, + fmt.Sprintf("git %s merge-base --is-ancestor %s %s", + gitArgs, shellQuote(head), shellQuote(rec))); exit == 0 { + return spec.StatusPass, "" + } + } + // A ref in that checkout still contains the commit, so the move cannot lose it. + out, _, exit := hostCapture(context.Background(), dir, + fmt.Sprintf("git %s for-each-ref --contains %s --format=%s", + gitArgs, shellQuote(head), shellQuote("%(refname)"))) + if exit == 0 && strings.TrimSpace(out) != "" { + return spec.StatusPass, "" + } + return spec.StatusFail, fmt.Sprintf( + "the nested checkout %s carries a commit the update would drop — its HEAD %s is in no branch, "+ + "tag or remote-tracking ref of that checkout%s, so the update would move it off that commit "+ + "and leave it in the reflog alone; push it, branch it, or stash it, then re-run", + label, head, pinNote(rec)) +} + +// pinNote names the recorded pin in a refusal, and says so when it could not be read +// here — the two are different facts, and a refusal that hides either sends the reader +// looking for the wrong cause. +func pinNote(rec string) string { + if rec == "" { + return " (the recorded pin could not be read here, so only the checkout's own refs could decide)" + } + return fmt.Sprintf(" (the recorded pin is %s)", rec) +} + // reconcileNestedCheckouts brings a just-pinned submodule's OWN nested checkouts back // onto the gitlinks its new commit records — the state `verify` demands. // -// GUARDED twice over, and both guards are load-bearing: +// GUARDED three times over, and every guard is load-bearing: // - a submodule with no `.gitmodules` is UNTOUCHED — a plain file test, no git call, no // network, no new failure mode for the majority of submodules; // - the update is `--recursive` WITHOUT `--init`: it moves the nested checkouts that are @@ -1472,7 +1650,12 @@ func advancePinInIndex(projDir, path, sha string) (spec.Status, string) { // tidiness: `--init` would fetch nested remotes on every bump (including file-protocol // fixtures, which would need a protocol loosening this verb must not bake in), and a // bump is not the place to grow the checkout. The recursion is still there because a -// nested checkout can itself nest. +// nested checkout can itself nest; +// - what the update would DESTROY is judged BEFORE it runs (judgeNestedLevel, called +// from pinSubmodule before the switch): the update moves a nested checkout off a +// commit that carries work nothing else references, silently and with exit 0 +// (opencharly/plugin-task#54), so the work is judged up front and a checkout that +// carries it is never moved. // // A reconcile that cannot complete is a hard error, not a warning: a bump that leaves the // tree dirty is worse than a bump that refuses (this is the defect itself). diff --git a/candy/plugin-task/verbs_test.go b/candy/plugin-task/verbs_test.go index 3b4ab5a..8b4082e 100644 --- a/candy/plugin-task/verbs_test.go +++ b/candy/plugin-task/verbs_test.go @@ -877,7 +877,7 @@ func newBumpUmbrella(t *testing.T, base, pinSHA, leafBare, doomedBare string, na // `git add`, the mechanism reproduced in the issue — stranded the whole remainder. func TestGitSubmodules_BumpContinuesPastFailure(t *testing.T) { base, leafBare, doomedBare, L1, L2 := bumpOrigins(t) - // Path order is SORTED (submodulePaths), so `doomed` is visited FIRST and `zebra` + // Path order is SORTED (gitmodulesPaths), so `doomed` is visited FIRST and `zebra` // sorts AFTER it — exactly the ordering that used to strand the rest. umb := newBumpUmbrella(t, base, L1, leafBare, doomedBare, "doomed", "zebra") // The failing path's failure is set up AFTER initialization: its origin is GONE,