fix(git-submodules): the bump report counts the pins that MOVED, not the index WRITES (plugin-task#53)

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)
+	}
+}

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,19 +743,31 @@ 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.
 					if st, msg := advancePinInIndex(projDir, upath, sha); st != spec.StatusPass {
 						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.