fix: Find change files added in merges and credit the PR that created them

.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 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.

.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.

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 {
@@ -284,29 +287,66 @@ 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/<filename>.
+// ChangeFileAdded returns the commit that created .changes/<filename>, 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"
+	// 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
+		}
+		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, err := renamedFrom(root, origin.SHA, path)
+		if err != nil {
+			return ChangeFileOrigin{}, false
+		}
+		if from == "" {
+			return origin, true
+		}
+		path = from
+		rev = origin.SHA + "^"
 	}
-	t, err := strconv.ParseInt(parts[1], 10, 64)
+}
+
+// renamedFrom returns the path that commit sha renamed to path, or "" when sha didn't rename it.
+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 ChangeFileOrigin{}, false
+		return "", err
 	}
-	return ChangeFileOrigin{SHA: parts[0], Time: t, Subject: parts[2]}, true
+	// Records are "R<score>\0<from>\0<to>\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 "", nil
 }
 
 var (

internal/git/git_test.go

@@ -245,3 +245,114 @@ func TestChangeFileAdded(t *testing.T) {
 		t.Error("uncommitted change file should report ok=false")
 	}
 }
+
+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")
+	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)
+	}
+}
+
+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)
+	}
+}

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