diff --git a/candy/plugin-task/bump_moved_test.go b/candy/plugin-task/bump_moved_test.go new file mode 100644 index 0000000..22b1570 --- /dev/null +++ b/candy/plugin-task/bump_moved_test.go @@ -0,0 +1,94 @@ +package task + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/opencharly/spec/spec" +) + +// The bump report must count the pins that MOVED, not the pins it WROTE. +// +// The defect (opencharly/plugin-task#53): `advancePinInIndex` records a pin in the +// superproject's index with `git update-index --add --cacheinfo` and returns PASS on +// exit 0, and the caller counted every PASS. So the report's "advancing 406 pin(s) in +// the index" counted INDEX WRITES, and it read as a count of pins that moved. Measured +// on the live umbrella: 406 written, 4 moved — a report that a body repeats verbatim +// would be false by 402. +// +// The control is the SAME tree bumped TWICE, which is the only comparison that +// discriminates: the walk writes the pin both times (it must record where the pin is, +// even when the answer is "where it already was"), so the WRITE count is identical +// and only the MOVE count can differ. +// +// Coverage fails without the change: the pre-change summary carries no moved count at +// all, so neither assertion below can pass on the old tree. +func TestGitSubmodules_BumpCountsMovedPinsNotIndexWrites(t *testing.T) { + base := t.TempDir() + + // A leaf repo with ONE commit, so the remote serves exactly one tip. + leaf := filepath.Join(base, "leaf") + gitIn(t, base, "init", "-q", "-b", "main", filepath.Base(leaf)) + writeFile(t, filepath.Join(leaf, "f"), "L1") + gitIn(t, leaf, "add", "-A") + gitIn(t, leaf, "commit", "-qm", "L1") + leafTip := gitIn(t, leaf, "rev-parse", "HEAD") + leafBare := filepath.Join(base, "leaf.git") + gitIn(t, base, "clone", "-q", "--bare", leaf, leafBare) + + // An umbrella that DECLARES `target` and has NO checkout for it: the pin lives + // in the index (opencharly/plugin-task#37), so the bump advances it there. + umb := filepath.Join(base, "umb") + gitIn(t, base, "init", "-q", "-b", "main", filepath.Base(umb)) + writeFile(t, filepath.Join(umb, "u"), "u") + gitIn(t, umb, "add", "-A") + gitIn(t, umb, "commit", "-qm", "u") + writeFile(t, filepath.Join(umb, ".gitmodules"), + "[submodule \"target\"]\n\tpath = target\n\turl = "+leafBare+"\n\tbranch = main\n") + gitIn(t, umb, "add", ".gitmodules") + gitIn(t, umb, "commit", "-qm", "gitmodules") + // The recorded pin is ONE COMMIT BEHIND: the index holds a sha the remote does + // not serve, so the bump's write moves it. + head := gitIn(t, umb, "rev-parse", "HEAD") + gitIn(t, umb, "update-index", "--add", "--cacheinfo", "160000,"+head+",target") + gitIn(t, umb, "commit", "-qm", "stale gitlink") + // The directory exists and is EMPTY (no .git): the declared-but-unmaterialized state. + if err := os.MkdirAll(filepath.Join(umb, "target"), 0o755); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(umb, "target", ".git")); err == nil { + t.Fatalf("fixture precondition: target must have no .git") + } + + // BUMP ONE: the pin is behind, so the write MOVES it. + st, msg := runMaintenanceVerbIn(umb, "git-submodules", map[string]any{"mode": "bump"}) + if st != spec.StatusPass { + t.Fatalf("bump over an unmaterialized path must PASS, got %s: %s", st, msg) + } + if !strings.Contains(msg, "advancing 1 leaf pin(s) in the index (1 moved)") { + t.Fatalf("the report must name the pin it advanced AND that the pin moved: %q", msg) + } + if !strings.Contains(msg, "1 pin(s) moved") { + t.Fatalf("the report must say how many pins moved: %q", msg) + } + if got := gitlink(umb, "target"); got != leafTip { + t.Fatalf("the pin did not advance to the remote's tip: %s != %s", got, leafTip) + } + + // BUMP TWO: the SAME tree, whose pin is now current. The walk still WRITES the + // pin — it must record where the pin is — so the touched count is unchanged and + // the MOVED count is zero. This is the discrimination: a report that counted + // writes would print the same number twice. + st, msg = runMaintenanceVerbIn(umb, "git-submodules", map[string]any{"mode": "bump"}) + if st != spec.StatusPass { + t.Fatalf("the second bump over a current pin must PASS, got %s: %s", st, msg) + } + if !strings.Contains(msg, "advancing 1 leaf pin(s) in the index (0 moved)") { + t.Fatalf("the write count must not change and the moved count must fall to zero: %q", msg) + } + if !strings.Contains(msg, "0 pin(s) moved") { + t.Fatalf("a bump over a current pin must report ZERO moved pins: %q", msg) + } +} diff --git a/candy/plugin-task/verbs.go b/candy/plugin-task/verbs.go index 0625a7a..04929b7 100644 --- a/candy/plugin-task/verbs.go +++ b/candy/plugin-task/verbs.go @@ -578,6 +578,24 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status // — costs that ONE pin instead of every pin sorted after it. Every failure is // reported together at the end, so the run's own output names all of them. var done, pinned, inIndex, failures []string + // MOVED, not WRITTEN (opencharly/plugin-task#53). + // + // The three groups above are WRITES: a pin that was already current is still + // written, because the walk must record where the pin is even when the answer + // is "where it already was". So the counts name how many pins the wave TOUCHED, + // and the counters below name how many of them actually MOVED — the value the + // superproject's index records for that path is a DIFFERENT one from the one it + // recorded before. A report that conflates the two tells the reader a wave + // moved 400 pins when it moved 4. + // + // The groups PARTITION the paths the walk touched — every touched path is in + // exactly one of them, so the three moved counters SUM to the moved total: + // done — phase-1 paths WITH a checkout, rolled to their default-branch HEAD + // inIndex — phase-1 paths with NO checkout, advanced in the index + // pinned — the pin_map entries, each advanced in the index or rolled onto + // its twin, and counted HERE and not in the two groups above, so the groups + // stay disjoint and the sum is the truth. + rolledMoved, indexMoved, pinnedMoved := 0, 0, 0 // pinned_from may be an EXTERNAL checkout (`../src`) rather than one of this // project's OWN submodules; only a pinned_from that IS one of `paths` is rolled // by phase 1, so only that case can be judged "did not roll". An external @@ -658,17 +676,28 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status "the remote for %q does not serve refs/heads/%s, so its pin cannot be advanced", p, branch)) continue } + // The pin the index ALREADY records, read BEFORE the write: a write + // that records the value the index already holds moved nothing. + recorded := gitlink(projDir, p) if st, msg := advancePinInIndex(projDir, p, sha); st != spec.StatusPass { failures = append(failures, msg) continue } inIndex = append(inIndex, p) + // The write records EXACTLY `sha`, so the comparison against the + // value the index carried before it IS the change. + if recorded != sha { + indexMoved++ + } // A source whose own pin advanced here still has no TREE to read a // twin from - an unmaterialized source has no checkout at all - and // the state already says so: it is classified by the CHECKOUT // before phase 1 (opencharly/plugin-task#39). continue } + // The pin the index records BEFORE the roll; the roll stages the + // checkout's new HEAD, so the value is read again after it. + recorded := gitlink(projDir, p) if st, msg := pinSubmodule(projDir, p, "origin/"+branch); st != spec.StatusPass { failures = append(failures, msg) continue @@ -677,6 +706,11 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status pinnedFromState = "rolled" } done = append(done, p) + // A checkout that was already at the branch tip, and whose pin the + // index already recorded, is a roll that moved nothing. + if gitlink(projDir, p) != recorded { + rolledMoved++ + } } // Phase 2: pin each pin_map entry from the (now-current) pinned_from gitlinks. // @@ -709,6 +743,8 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status failures = append(failures, fmt.Sprintf("no gitlink for %q in %q", cpath, in.PinnedFrom)) continue } + // The pin the index records BEFORE the move. + recorded := gitlink(projDir, upath) if !submoduleInitialized(projDir, upath) { // UNMATERIALIZED edge: the pin moves in the index, to the twin // the source's CURRENT tree records. @@ -716,12 +752,22 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status failures = append(failures, msg) continue } - inIndex = append(inIndex, upath) } else if st, msg := pinSubmodule(projDir, upath, sha); st != spec.StatusPass { failures = append(failures, msg) continue } pinned = append(pinned, upath) + // BOTH branches record EXACTLY `sha` in the index — the index write, + // or the roll staged onto the checkout that now sits at `sha` — so one + // comparison against the value the index carried before answers + // whether the pin MOVED. + // + // A pin_map entry is counted HERE and not in the index-advance group: + // the groups partition the paths the walk touched, so the moved + // counters sum to the moved total instead of counting an edge twice. + if recorded != sha { + pinnedMoved++ + } } } sort.Strings(done) @@ -732,12 +778,28 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status // rolled/pinned name lists are long enough (422 names on a full umbrella) that appending the // diagnostic after them is how it gets truncated away before a reader sees it. A message that // reports what went wrong must lead with it. + // + // Every group carries TWO numbers: how many pins it TOUCHED and how many of + // them MOVED (opencharly/plugin-task#53). The touched count is a WRITE count — + // a pin that was already current is still written — and the moved count is + // the one an operator quotes, so the two must never be one number. + // + // The three groups are a PARTITION, and they say so: the two leaf clauses + // count LEAF pins (the plugin's own vocabulary — the currency report says + // "upstream leaf/leaves" and "derived-from edge(s)" too), the pin_map clause + // counts the EDGES, and the three moved counts SUM to the moved total. + moved := rolledMoved + indexMoved + pinnedMoved + pinnedNames := strings.Join(pinned, ", ") + if pinnedNames == "" { + pinnedNames = "none" + } if len(failures) > 0 { - return spec.StatusFail, fmt.Sprintf("%d pin(s) FAILED: %s; rolled %d submodule(s) to their default-branch HEAD; advanced %d pin(s) in the index; pinned %d pin_map entr(y|ies): %s", - len(failures), strings.Join(failures, "; "), len(done), len(inIndex), len(pinned), strings.Join(pinned, ", ")) + return spec.StatusFail, fmt.Sprintf("%d pin(s) FAILED: %s; rolled %d leaf submodule(s) to their default-branch HEAD (%d moved); advanced %d leaf pin(s) in the index (%d moved); pinned %d pin_map entr(y|ies) (%d moved): %s; %d pin(s) moved", + len(failures), strings.Join(failures, "; "), + len(done), rolledMoved, len(inIndex), indexMoved, len(pinned), pinnedMoved, pinnedNames, moved) } - return spec.StatusPass, fmt.Sprintf("pinned %d pin_map entr(y|ies) (%s) after rolling %d submodule(s) to their default-branch HEAD and advancing %d pin(s) in the index", - len(pinned), strings.Join(pinned, ", "), len(done), len(inIndex)) + return spec.StatusPass, fmt.Sprintf("pinned %d pin_map entr(y|ies) (%s; %d moved) after rolling %d leaf submodule(s) to their default-branch HEAD (%d moved) and advancing %d leaf pin(s) in the index (%d moved); %d pin(s) moved", + len(pinned), pinnedNames, pinnedMoved, len(done), rolledMoved, len(inIndex), indexMoved, moved) case "verify": // Half 1 — the derived EDGE: every pin_map entry must equal pinned_from's twin.