diff --git a/.changes/credit-change-file-creator.md b/.changes/credit-change-file-creator.md new file mode 100644 index 0000000..425d8f2 --- /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 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/.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..8c2de03 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 { @@ -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/. +// 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" + // 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\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 "", nil } var ( diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 806af5e..d0dfa19 100644 --- a/internal/git/git_test.go +++ b/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) + } +} 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) + } +}