From f23edc647295fa1bd5510d2427f386a6b1019527 Mon Sep 17 00:00:00 2001 From: Rick Liu Date: Wed, 22 Jul 2026 23:37:13 +0100 Subject: [PATCH] fix(diff): head each deferred diff with its module and target Now that diffs print together at the end instead of inline with the operation logs, they lost the surrounding context that told you which module produced a change and which repo it belonged to. Restore it: - quick mode: DiffCollector carries the module name and a target label (rendered repo URL + branch, or the target dir) with each diff and prints a header before it, deduped across consecutive same-target diffs. - full mode: printTargetDiffs heads each target's git diff with the module (from the numbered clone dir), the origin remote URL, and the base branch read from the clone itself. Adds spec rule DF4 and header assertions to the DF1/DF2 tests. Co-Authored-By: Claude Opus 4.8 --- cmd/diff.go | 69 ++++++++++++++++++++++++++++++++++++++-- cmd/diff_test.go | 9 ++++++ pkg/action/action.go | 76 ++++++++++++++++++++++++++++++++++++++------ pkg/action/diff.go | 12 ++++--- pkg/module/module.go | 47 +++++++++++++++++++-------- specs/module.md | 7 ++++ 6 files changed, 189 insertions(+), 31 deletions(-) diff --git a/cmd/diff.go b/cmd/diff.go index 97e8b56..7f927ed 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -168,8 +168,9 @@ func runDiffFull(ctx context.Context, source string, paramMap map[string]string, // Returns the number of repos that had changes. func printTargetDiffs(root string, w io.Writer) (int, error) { repos := gitRepoDirs(root) + color := isTerminalWriter(w) colorFlag := "--color=never" - if isTerminalWriter(w) { + if color { colorFlag = "--color=always" } @@ -187,7 +188,7 @@ func printTargetDiffs(root string, w io.Writer) (int, error) { continue } changed++ - fmt.Fprintf(w, "\n# %s\n", filepath.Base(dir)) + fmt.Fprint(w, targetDiffHeader(dir, base, color)) fmt.Fprint(w, out) if !strings.HasSuffix(out, "\n") { fmt.Fprintln(w) @@ -196,6 +197,70 @@ func printTargetDiffs(root string, w io.Writer) (int, error) { return changed, nil } +// targetDiffHeader renders the "which module / which repo" banner above a +// target's git diff: the module name (from the numbered clone dir), the origin +// remote URL, and the base branch — all read from the clone itself. +func targetDiffHeader(dir, base string, color bool) string { + module := moduleFromDir(filepath.Base(dir)) + branch := strings.TrimPrefix(base, "refs/remotes/origin/") + repo := "" + if url, err := runGit(dir, "remote", "get-url", "origin"); err == nil { + repo = strings.TrimSpace(url) + } + if branch != "" && branch != "HEAD" { + if repo != "" { + repo += " (" + branch + ")" + } else { + repo = branch + } + } + return diffHeaderLine(module, repo, color) +} + +// moduleFromDir strips the "NN-" execution-order prefix local mode adds to a +// clone directory, leaving the module name. +func moduleFromDir(name string) string { + i := 0 + for i < len(name) && name[i] >= '0' && name[i] <= '9' { + i++ + } + if i > 0 && i < len(name) && name[i] == '-' { + return name[i+1:] + } + return name +} + +// diffHeaderLine renders a module/target banner matching the quick-mode diff +// header: an inverted module chip followed by a muted target label. +func diffHeaderLine(module, target string, color bool) string { + var b strings.Builder + b.WriteByte('\n') + switch { + case !color: + if module != "" { + b.WriteString("[" + module + "]") + } + if target != "" { + if module != "" { + b.WriteByte(' ') + } + b.WriteString(target) + } + default: + if module != "" { + b.WriteString("\033[7m " + module + " \033[0m") + } + if target != "" { + if module != "" { + b.WriteByte(' ') + } + b.WriteString("\033[38;5;244m" + target + "\033[0m") + } + } + b.WriteByte('\n') + return b.String() +} + // gitRepoDirs returns root (if a git repo) followed by its immediate git-repo // subdirectories, in directory-name order — matching the numbered subdirs that // local mode clones into. diff --git a/cmd/diff_test.go b/cmd/diff_test.go index 4a8eb73..9cee0ca 100644 --- a/cmd/diff_test.go +++ b/cmd/diff_test.go @@ -119,6 +119,11 @@ spec: if !strings.Contains(out, "-hello") || !strings.Contains(out, "+HELLO") { t.Errorf("expected shell-op change (notes.txt) in diff, got:\n%s", out) } + // DF4: the diff must carry a header identifying the module and target repo, + // so it stays legible away from the surrounding operation logs. + if !strings.Contains(out, "demo") || !strings.Contains(out, upstream) { + t.Errorf("expected module/repo header in diff, got:\n%s", out) + } } // DF2: --quick simulates the run (dry-run) — it prints newFiles/patch unified @@ -156,6 +161,10 @@ spec: if !strings.Contains(out, "new content") { t.Errorf("expected rendered content in quick diff, got:\n%s", out) } + // Quick diffs carry a module header for context, just like full mode. + if !strings.Contains(out, "quick-demo") { + t.Errorf("expected module header in quick diff, got:\n%s", out) + } if _, err := os.Stat(filepath.Join(targetDir, "new.txt")); !os.IsNotExist(err) { t.Error("--quick must not write files to the target") } diff --git a/pkg/action/action.go b/pkg/action/action.go index 8f8d753..271b7e8 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log/slog" + "strings" ) // PRResult records one pull/merge request created during a run. @@ -44,34 +45,85 @@ func (s *RunSummary) Print(w io.Writer) { } } +// diffEntry is one captured file diff plus the context needed to read it once +// diffs are printed together at the end: which module produced it and which +// target (repo) it applies to. +type diffEntry struct { + module string + target string + text string // uncolored unified diff +} + // DiffCollector accumulates file diffs across a whole run, shared by parent // and child module executions. Execution is sequential, so no locking is // needed. Diffs are held rather than written inline as each file is merged, // so the whole set can be printed once at the very end of the run — after the -// per-operation logs — where they are easy to read. +// per-operation logs. Each diff carries a module/target header so it stays +// legible out of the surrounding log context. type DiffCollector struct { - // Diffs holds each captured file diff as uncolored unified-diff text. - Diffs []string + entries []diffEntry } -// Add records one file diff. Safe to call on a nil collector. -func (c *DiffCollector) Add(diff string) { +// Add records one file diff along with the module and target it belongs to. +// Safe to call on a nil collector. +func (c *DiffCollector) Add(module, target, diff string) { if c == nil { return } - c.Diffs = append(c.Diffs, diff) + c.entries = append(c.entries, diffEntry{module: module, target: target, text: diff}) } // Print writes all collected diffs to w, colorized when w is a terminal. -// Nothing is written when the collector is nil or empty. +// A module/target header is written before each diff (deduplicated across a +// run of diffs from the same module and target). Nothing is written when the +// collector is nil or empty. func (c *DiffCollector) Print(w io.Writer) { - if c == nil || len(c.Diffs) == 0 { + if c == nil || len(c.entries) == 0 { return } color := isTerminalWriter(w) - for _, diff := range c.Diffs { - fmt.Fprint(w, colorizeDiff(diff, color)) + var lastModule, lastTarget string + for i, e := range c.entries { + if i == 0 || e.module != lastModule || e.target != lastTarget { + fmt.Fprint(w, diffHeader(e.module, e.target, color)) + lastModule, lastTarget = e.module, e.target + } + fmt.Fprint(w, colorizeDiff(e.text, color)) + } +} + +// diffHeader renders the "which module / which repo" banner shown above a diff. +// Returns just a leading blank line when there is no context to show. +func diffHeader(module, target string, color bool) string { + if module == "" && target == "" { + return "\n" + } + var b strings.Builder + b.WriteByte('\n') + switch { + case !color: + if module != "" { + b.WriteString("[" + module + "]") + } + if target != "" { + if module != "" { + b.WriteByte(' ') + } + b.WriteString(target) + } + default: + if module != "" { + b.WriteString(diffColorInvert + " " + module + " " + diffColorReset) + } + if target != "" { + if module != "" { + b.WriteByte(' ') + } + b.WriteString(diffColorMuted + target + diffColorReset) + } } + b.WriteByte('\n') + return b.String() } // ExecutionContext holds runtime state shared across actions. @@ -82,6 +134,10 @@ type ExecutionContext struct { ModuleDir string // TargetDir is the path to the target repository working directory. TargetDir string + // TargetLabel is a human-readable identity for the target (repo URL and + // branch, or the target dir), shown as a header above collected diffs so + // they stay legible out of the surrounding log context. May be empty. + TargetLabel string // Params are the resolved template parameters. Params map[string]string // Excludes are glob patterns for files/dirs to exclude from template walking. diff --git a/pkg/action/diff.go b/pkg/action/diff.go index 113aac5..7794c0e 100644 --- a/pkg/action/diff.go +++ b/pkg/action/diff.go @@ -10,10 +10,12 @@ import ( // ANSI color codes for diff output. const ( - diffColorReset = "\033[0m" - diffColorRed = "\033[31m" - diffColorGreen = "\033[32m" - diffColorCyan = "\033[36m" + diffColorReset = "\033[0m" + diffColorRed = "\033[31m" + diffColorGreen = "\033[32m" + diffColorCyan = "\033[36m" + diffColorInvert = "\033[7m" + diffColorMuted = "\033[38;5;244m" // mid gray — matches the log handler's muted tone ) // printDiff computes a unified diff between old and new content and hands it to @@ -54,7 +56,7 @@ func printDiff(execCtx *ExecutionContext, path, oldContent, newContent string) { return } - execCtx.Diffs.Add(text) + execCtx.Diffs.Add(execCtx.ModuleName, execCtx.TargetLabel, text) } // colorizeDiff applies ANSI colors to unified diff lines. diff --git a/pkg/module/module.go b/pkg/module/module.go index 6cdaede..1def6da 100644 --- a/pkg/module/module.go +++ b/pkg/module/module.go @@ -162,19 +162,38 @@ func evalParamCommand(name, command, moduleDir string, logger *slog.Logger) (str // NewExecutionContext creates an ExecutionContext for this module. func (m *Module) NewExecutionContext(targetDir string, opts RunOptions) *action.ExecutionContext { return &action.ExecutionContext{ - ModuleName: m.Config.Metadata.Name, - ModuleDir: m.Dir, - TargetDir: targetDir, - Params: m.Params, - Excludes: m.Config.Spec.Excludes, - Includes: m.Config.Spec.Includes, - DryRun: opts.DryRun, - LocalRun: opts.LocalRun, - ShowDiff: opts.ShowDiff, - Diffs: opts.Diffs, - GitAuthor: opts.GitAuthor, - GitEmail: opts.GitEmail, - Summary: opts.Summary, - Logger: m.Logger, + ModuleName: m.Config.Metadata.Name, + ModuleDir: m.Dir, + TargetDir: targetDir, + TargetLabel: m.targetLabel(targetDir), + Params: m.Params, + Excludes: m.Config.Spec.Excludes, + Includes: m.Config.Spec.Includes, + DryRun: opts.DryRun, + LocalRun: opts.LocalRun, + ShowDiff: opts.ShowDiff, + Diffs: opts.Diffs, + GitAuthor: opts.GitAuthor, + GitEmail: opts.GitEmail, + Summary: opts.Summary, + Logger: m.Logger, } } + +// targetLabel is a human-readable identity for the module's target, used as a +// header above collected diffs: the rendered repo URL and branch when the +// module has a target spec, otherwise the target directory it runs against. +func (m *Module) targetLabel(targetDir string) string { + t := m.Config.Spec.Target + if t == nil { + return targetDir + } + url, err := tmpl.RenderString(t.URL, m.Params) + if err != nil || url == "" { + return targetDir + } + if branch, err := tmpl.RenderString(t.Branch, m.Params); err == nil && branch != "" { + return url + " (" + branch + ")" + } + return url +} diff --git a/specs/module.md b/specs/module.md index e2f30ae..9b9e8c2 100644 --- a/specs/module.md +++ b/specs/module.md @@ -922,6 +922,13 @@ Full mode clones into a workspace: a temporary directory that is removed once th diff is printed, unless `--target-path` is supplied, in which case the clones are kept there for inspection. +#### DF4: Diffs are headed with module and target + +Because diffs print together at the end — away from the per-operation logs — +each diff is preceded by a header naming the module that produced it and the +target it applies to (repo URL and branch, or the target directory). Consecutive +diffs sharing the same module and target reuse a single header. + --- ## Validation Rules