From 4807312a248864cbbdea96d71dc651c726a3ec6a Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Thu, 5 Nov 2026 14:10:39 +0100 Subject: [PATCH 1/3] fix: count change files that a merge commit adds ChangedChangeFilesSinceRef walked commits with git log --diff-filter=A, which skips merge commits, so a change file added while merging the base back in was never found. It now diffs HEAD against the merge base of the ref, which also drops files added and later removed. --- .changes/merge-added-change-files.md | 8 +++++++ internal/git/git.go | 15 +++++++------ internal/git/git_test.go | 32 ++++++++++++++++++++++++++++ internal/release/check_test.go | 27 +++++++++++++++++++++++ 4 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 .changes/merge-added-change-files.md diff --git a/.changes/merge-added-change-files.md b/.changes/merge-added-change-files.md new file mode 100644 index 0000000..73ca1d0 --- /dev/null +++ b/.changes/merge-added-change-files.md @@ -0,0 +1,8 @@ +--- +kind: fixed +--- +Count change files that a merge commit adds + +## Release Note +The change file check now finds change files that arrive through a merge commit +A pull request whose change file was added while merging the base branch back in, such as during a conflict resolution, was reported as having no change file. Change files are now found by comparing the pull request against its base, so a file added and later removed no longer counts either. diff --git a/internal/git/git.go b/internal/git/git.go index 61223ec..5459921 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -174,16 +174,19 @@ func IsAncestor(root, a, b string) bool { return cmd.Run() == nil } -// ChangedChangeFilesSinceRef returns basenames of .changes/*.md files added -// since ref (exclusive) up to HEAD. When ref is empty, the full history is -// searched. Only files directly in .changes/ count (not the release archive), -// and README.md and reserved "_" files are excluded. +// ChangedChangeFilesSinceRef returns basenames of .changes/*.md files present +// at HEAD but not at the merge base of ref and HEAD. When ref is empty, every +// change file at HEAD counts. Only files directly in .changes/ count (not the +// release archive), and README.md and reserved "_" files are excluded. +// +// It compares trees rather than walking commits, so a file a merge commit +// brings in counts, and one added and later removed does not. func ChangedChangeFilesSinceRef(root, ref string) ([]string, error) { var args []string if ref != "" { - args = []string{"log", ref + "..HEAD", "--diff-filter=A", "--name-only", "--format=", "--", ".changes/"} + args = []string{"diff", "--no-renames", "--diff-filter=A", "--name-only", ref + "...HEAD", "--", ".changes/"} } else { - args = []string{"log", "--diff-filter=A", "--name-only", "--format=", "--", ".changes/"} + args = []string{"ls-tree", "-r", "--name-only", "HEAD", "--", ".changes/"} } out, err := run(root, args...) if err != nil { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 806af5e..5042801 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -245,3 +245,35 @@ func TestChangeFileAdded(t *testing.T) { t.Error("uncommitted change file should report ok=false") } } + +func TestChangedChangeFilesSinceRef_SkipsRemovedFiles(t *testing.T) { + root := newRepo(t) + commitFile(t, root, "seed.txt", "seed") + base, err := CurrentCommit(root) + if err != nil { + t.Fatal(err) + } + + if err := os.MkdirAll(filepath.Join(root, ".changes"), 0755); err != nil { + t.Fatal(err) + } + commitFile(t, root, filepath.Join(".changes", "kept.md"), "---\nkind: fixed\n---\nKept\n") + commitFile(t, root, filepath.Join(".changes", "gone.md"), "---\nkind: fixed\n---\nGone\n") + if err := os.Remove(filepath.Join(root, ".changes", "gone.md")); err != nil { + t.Fatal(err) + } + if err := Add(root, filepath.Join(".changes", "gone.md")); err != nil { + t.Fatal(err) + } + if err := Commit(root, "remove gone.md"); err != nil { + t.Fatal(err) + } + + files, err := ChangedChangeFilesSinceRef(root, base) + if err != nil { + t.Fatal(err) + } + if len(files) != 1 || files[0] != "kept.md" { + t.Errorf("only change files still present at HEAD count, got %v", files) + } +} diff --git a/internal/release/check_test.go b/internal/release/check_test.go index 61b1d98..75e416d 100644 --- a/internal/release/check_test.go +++ b/internal/release/check_test.go @@ -208,3 +208,30 @@ func TestRunCheck_StaleBaseIgnoresBaseAccruedFiles(t *testing.T) { t.Errorf("expected Missing (PR adds no change file), got %q; body:\n%s", v.State, v.Body) } } + +func TestRunCheck_ChangeFileAddedInMergeCommit(t *testing.T) { + root := makeRepo(t, nil) + gitInit(t, root) + base := gitExec(t, root, "rev-parse", "HEAD") + + // The PR branch only gets its change file while merging the base back in, the way a conflict resolution can. + gitExec(t, root, "checkout", "-q", "-b", "pr") + gitExec(t, root, "commit", "--allow-empty", "-m", "pr work") + gitExec(t, root, "checkout", "-q", "master") + gitExec(t, root, "commit", "--allow-empty", "-m", "base work") + gitExec(t, root, "checkout", "-q", "pr") + gitExec(t, root, "merge", "--no-ff", "--no-commit", "master") + if err := os.WriteFile(filepath.Join(root, ".changes", "merged.md"), []byte("---\nkind: added\n---\nMerged feature\n"), 0644); err != nil { + t.Fatal(err) + } + gitExec(t, root, "add", filepath.Join(".changes", "merged.md")) + gitExec(t, root, "commit", "--no-edit") + + v, err := RunCheck(CheckParams{Root: root, BaseBranch: "master", Against: base, Config: releaseBranchConfig}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v.State != StateOK { + t.Errorf("expected ok for a change file added in a merge commit, got %q; body:\n%s", v.State, v.Body) + } +} From e4a063fb4e88c3d9c3c2e4ace16a7f5e62112db1 Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Thu, 5 Nov 2026 14:15:00 +0100 Subject: [PATCH 2/3] feat: credit the commit that created a change file, following renames ChangeFileAdded took the most recent commit that added the file under its current name, so a rename credited the commit that moved it. It now follows renames back to the creating commit, without copy detection so an unrelated file with the same text is never credited. --- .changes/credit-change-file-creator.md | 8 ++++ internal/git/git.go | 57 ++++++++++++++++++++------ internal/git/git_test.go | 39 ++++++++++++++++++ 3 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 .changes/credit-change-file-creator.md diff --git a/.changes/credit-change-file-creator.md b/.changes/credit-change-file-creator.md new file mode 100644 index 0000000..081a02c --- /dev/null +++ b/.changes/credit-change-file-creator.md @@ -0,0 +1,8 @@ +--- +kind: changed +--- +Credit the pull request that created a change file, not the one that last moved it + +## Release Note +Release notes credit the pull request that wrote each change file, even after it was renamed or moved +The PR link was taken from the most recent commit that added the file under its current name, so renaming change files while tidying them credited the tidy-up instead, and the `pr` field had to be set by hand. Renames are now followed back to the commit that created the file. Copies are not followed, so a change file is never credited to an unrelated one with the same text. diff --git a/internal/git/git.go b/internal/git/git.go index 5459921..c35c015 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -287,29 +287,62 @@ func Maintainers(root string) map[string]bool { return set } -// ChangeFileOrigin is the commit that added a change file. +// ChangeFileOrigin is the commit that created a change file. type ChangeFileOrigin struct { SHA string Time int64 // committer time, unix seconds Subject string } -// ChangeFileAdded returns the most recent commit that added .changes/. +// ChangeFileAdded returns the commit that created .changes/, following +// renames: a commit that only moved or renamed it (tidying change files, say) +// is skipped, so the PR that wrote the file is credited instead. // ok is false when the file has not been committed yet (or git fails). func ChangeFileAdded(root, filename string) (origin ChangeFileOrigin, ok bool) { - out, err := run(root, "log", "--diff-filter=A", "--format=%H%x1f%ct%x1f%s", "-n", "1", "--", ".changes/"+filename) - if err != nil || out == "" { - return ChangeFileOrigin{}, false - } - parts := strings.SplitN(out, "\x1f", 3) - if len(parts) != 3 { - return ChangeFileOrigin{}, false + path := ".changes/" + filename + rev := "HEAD" + // Bounded so a pathological history can't loop; real change files move a handful of times at most. + for range 16 { + out, err := run(root, "log", "--diff-filter=A", "--format=%H%x1f%ct%x1f%s", "-n", "1", rev, "--", path) + if err != nil || out == "" { + return ChangeFileOrigin{}, false + } + parts := strings.SplitN(out, "\x1f", 3) + if len(parts) != 3 { + return ChangeFileOrigin{}, false + } + t, err := strconv.ParseInt(parts[1], 10, 64) + if err != nil { + return ChangeFileOrigin{}, false + } + origin = ChangeFileOrigin{SHA: parts[0], Time: t, Subject: parts[2]} + + // The path filter makes a rename look like an add; ask the whole commit whether it was one. + // Renames only, not copies: change files share boilerplate, and a copy match would credit an unrelated file. + from := renamedFrom(root, origin.SHA, path) + if from == "" { + return origin, true + } + path = from + rev = origin.SHA + "^" } - t, err := strconv.ParseInt(parts[1], 10, 64) + return origin, true +} + +// renamedFrom returns the path that commit sha renamed to path, or "" when sha didn't rename it. +func renamedFrom(root, sha, path string) string { + out, err := run(root, "diff-tree", "-r", "--root", "--no-commit-id", "-M", "--diff-filter=R", "--name-status", sha) if err != nil { - return ChangeFileOrigin{}, false + return "" } - return ChangeFileOrigin{SHA: parts[0], Time: t, Subject: parts[2]}, true + for line := range strings.SplitSeq(out, "\n") { + // "R\t\t" + fields := strings.Split(line, "\t") + if len(fields) == 3 && fields[2] == path { + return fields[1] + } + } + return "" } var ( diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 5042801..298c8be 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -246,6 +246,45 @@ func TestChangeFileAdded(t *testing.T) { } } +func TestChangeFileAdded_FollowsRenames(t *testing.T) { + root := newRepo(t) + if err := os.MkdirAll(filepath.Join(root, ".changes"), 0755); err != nil { + t.Fatal(err) + } + body := "---\nkind: added\n---\nA feature with a title long enough to stay similar\n\n## Release Note\nSomething users notice\n" + if err := os.WriteFile(filepath.Join(root, ".changes", "old.md"), []byte(body), 0644); err != nil { + t.Fatal(err) + } + if err := Add(root, ".changes/old.md"); err != nil { + t.Fatal(err) + } + if err := Commit(root, "feat: write the change file (#12)"); err != nil { + t.Fatal(err) + } + + // A later commit renames it and touches the text, as a tidy-up does. + if _, err := run(root, "mv", ".changes/old.md", ".changes/new.md"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, ".changes", "new.md"), []byte(body+"More detail\n"), 0644); err != nil { + t.Fatal(err) + } + if err := Add(root, ".changes/new.md"); err != nil { + t.Fatal(err) + } + if err := Commit(root, "chore: tidy the change files (#20)"); err != nil { + t.Fatal(err) + } + + origin, ok := ChangeFileAdded(root, "new.md") + if !ok { + t.Fatal("expected the renamed change file to be found") + } + if origin.Subject != "feat: write the change file (#12)" { + t.Errorf("expected the commit that created the file, got %q", origin.Subject) + } +} + func TestChangedChangeFilesSinceRef_SkipsRemovedFiles(t *testing.T) { root := newRepo(t) commitFile(t, root, "seed.txt", "seed") From b5faccac11800e873835dd4e7b57e220b9831489 Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Thu, 5 Nov 2026 14:41:53 +0100 Subject: [PATCH 3/3] fix: harden change file origin lookup Match change file names as literal pathspecs so glob characters cannot pick up another file's commit. Parse rename paths with -z so quoted names are followed, report a failed rename lookup as not found instead of crediting the rename, and drop the hop cap since each hop moves to a strict ancestor. Reword the release note: a delete and near-identical add in one commit still reads as a rename. --- .changes/credit-change-file-creator.md | 2 +- internal/git/git.go | 32 ++++++++++++--------- internal/git/git_test.go | 40 ++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/.changes/credit-change-file-creator.md b/.changes/credit-change-file-creator.md index 081a02c..425d8f2 100644 --- a/.changes/credit-change-file-creator.md +++ b/.changes/credit-change-file-creator.md @@ -5,4 +5,4 @@ Credit the pull request that created a change file, not the one that last moved ## Release Note Release notes credit the pull request that wrote each change file, even after it was renamed or moved -The PR link was taken from the most recent commit that added the file under its current name, so renaming change files while tidying them credited the tidy-up instead, and the `pr` field had to be set by hand. Renames are now followed back to the commit that created the file. Copies are not followed, so a change file is never credited to an unrelated one with the same text. +The PR link was taken from the most recent commit that added the file under its current name, so renaming change files while tidying them credited the tidy-up instead, and the `pr` field had to be set by hand. Renames are now followed back to the commit that created the file. Copies are not followed, so a new change file that only shares text with an existing one keeps its own credit. A commit that deletes one change file and adds another with near-identical text still reads as a rename, since git cannot tell the two apart. diff --git a/internal/git/git.go b/internal/git/git.go index c35c015..8c2de03 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -301,9 +301,10 @@ type ChangeFileOrigin struct { func ChangeFileAdded(root, filename string) (origin ChangeFileOrigin, ok bool) { path := ".changes/" + filename rev := "HEAD" - // Bounded so a pathological history can't loop; real change files move a handful of times at most. - for range 16 { - out, err := run(root, "log", "--diff-filter=A", "--format=%H%x1f%ct%x1f%s", "-n", "1", rev, "--", path) + // Each hop moves rev to a strict ancestor, so the walk always ends. + for { + // Literal pathspecs: a name like "[ab].md" must not glob-match "a.md". + out, err := run(root, "--literal-pathspecs", "log", "--diff-filter=A", "--format=%H%x1f%ct%x1f%s", "-n", "1", rev, "--", path) if err != nil || out == "" { return ChangeFileOrigin{}, false } @@ -319,30 +320,33 @@ func ChangeFileAdded(root, filename string) (origin ChangeFileOrigin, ok bool) { // The path filter makes a rename look like an add; ask the whole commit whether it was one. // Renames only, not copies: change files share boilerplate, and a copy match would credit an unrelated file. - from := renamedFrom(root, origin.SHA, path) + from, err := renamedFrom(root, origin.SHA, path) + if err != nil { + return ChangeFileOrigin{}, false + } if from == "" { return origin, true } path = from rev = origin.SHA + "^" } - return origin, true } // renamedFrom returns the path that commit sha renamed to path, or "" when sha didn't rename it. -func renamedFrom(root, sha, path string) string { - out, err := run(root, "diff-tree", "-r", "--root", "--no-commit-id", "-M", "--diff-filter=R", "--name-status", sha) +func renamedFrom(root, sha, path string) (string, error) { + // -z keeps paths raw; without it git quotes names with unusual bytes. + out, err := run(root, "diff-tree", "-r", "-z", "--root", "--no-commit-id", "-M", "--diff-filter=R", "--name-status", sha) if err != nil { - return "" + return "", err } - for line := range strings.SplitSeq(out, "\n") { - // "R\t\t" - fields := strings.Split(line, "\t") - if len(fields) == 3 && fields[2] == path { - return fields[1] + // Records are "R\0\0\0". + fields := strings.Split(strings.TrimRight(out, "\x00"), "\x00") + for i := 0; i+2 < len(fields); i += 3 { + if fields[i+2] == path { + return fields[i+1], nil } } - return "" + return "", nil } var ( diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 298c8be..d0dfa19 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -316,3 +316,43 @@ func TestChangedChangeFilesSinceRef_SkipsRemovedFiles(t *testing.T) { t.Errorf("only change files still present at HEAD count, got %v", files) } } + +func TestChangeFileAdded_LiteralPathspec(t *testing.T) { + root := newRepo(t) + if err := os.MkdirAll(filepath.Join(root, ".changes"), 0755); err != nil { + t.Fatal(err) + } + commitFile(t, root, ".changes/[ab].md", "glob-like name") + commitFile(t, root, ".changes/a.md", "unrelated") + + origin, ok := ChangeFileAdded(root, "[ab].md") + if !ok { + t.Fatal("expected the change file to be found") + } + if origin.Subject != "add .changes/[ab].md" { + t.Errorf("glob characters matched another file: got %q", origin.Subject) + } +} + +func TestChangeFileAdded_FollowsRenameFromQuotedPath(t *testing.T) { + root := newRepo(t) + if err := os.MkdirAll(filepath.Join(root, ".changes"), 0755); err != nil { + t.Fatal(err) + } + body := "---\nkind: added\n---\nA feature with a title long enough to stay similar\n" + commitFile(t, root, ".changes/blåbær.md", body) + if _, err := run(root, "mv", ".changes/blåbær.md", ".changes/plain.md"); err != nil { + t.Fatal(err) + } + if err := Commit(root, "chore: rename"); err != nil { + t.Fatal(err) + } + + origin, ok := ChangeFileAdded(root, "plain.md") + if !ok { + t.Fatal("expected the renamed change file to be found") + } + if origin.Subject != "add .changes/blåbær.md" { + t.Errorf("expected the commit that created the file, got %q", origin.Subject) + } +}