diff --git a/src/git.test.ts b/src/git.test.ts index ff5670a..24dc1f8 100644 --- a/src/git.test.ts +++ b/src/git.test.ts @@ -493,6 +493,15 @@ type TempRepoReleaseBranch = { }; }; +type TempRepoStaleMerge = { + cwd: string; + commits: { + base: string; + staleMerge: string; // Merge of feat/ABC-1-stale — edited app-a/ only, merged after app-b/ landed + subjectMerge: string; // Merge of feat/XYZ-2-impl — edited app-b/, key only on the merge subject (HEAD) + }; +}; + function runGit(command: string, cwd: string): string { return execSync(`git ${command}`, { cwd, @@ -729,6 +738,52 @@ function createTempRepoReleaseBranch(): TempRepoReleaseBranch { return { cwd, commits: { base, headMerge } }; } +/** + * Two PR merges into main, each carrying its issue key only in the branch name + * (no content commit carries a key): + * - feat/ABC-1-stale is rooted at `base`, edits app-a/ only, and is merged + * AFTER app-b/ appears on main — a stale branch never rebased. The merge + * differs from its first parent for app-b/ only because app-b/ advanced on + * main while the branch was open, so `--full-history` keeps it under an app-b + * pathspec even though the branch delivered nothing to app-b/. + * - feat/XYZ-2-impl is rooted at the stale merge and genuinely edits app-b/. + * Its key lives only on the merge subject, so dropping the merge would lose + * the key entirely even though the merge did deliver app-b/ changes. + */ +function createTempRepoStaleMerge(): TempRepoStaleMerge { + const { cwd, base } = initTempRepo({ + prefix: "linear-release-stale-merge-", + dirs: ["app-a", "app-b"], + seedFile: { path: "app-a/file.txt", content: "a0" }, + }); + + runGit(`checkout -b feat/ABC-1-stale ${base}`, cwd); + writeFileSync(join(cwd, "app-a", "file.txt"), "a1"); + runGit("add .", cwd); + runGit('commit -m "rework app-a internals"', cwd); + + runGit("checkout main", cwd); + writeFileSync(join(cwd, "app-b", "file.txt"), "b0"); + runGit("add .", cwd); + runGit('commit -m "add app-b on main"', cwd); + + runGit('merge --no-ff feat/ABC-1-stale -m "Merge pull request #1 from owner/feat/ABC-1-stale"', cwd); + const staleMerge = runGit("rev-parse HEAD", cwd); + runGit("branch -D feat/ABC-1-stale", cwd); + + runGit(`checkout -b feat/XYZ-2-impl ${staleMerge}`, cwd); + writeFileSync(join(cwd, "app-b", "file.txt"), "b1"); + runGit("add .", cwd); + runGit('commit -m "implement the thing"', cwd); + + runGit("checkout main", cwd); + runGit('merge --no-ff feat/XYZ-2-impl -m "Merge pull request #2 from owner/feat/XYZ-2-impl"', cwd); + const subjectMerge = runGit("rev-parse HEAD", cwd); + runGit("branch -D feat/XYZ-2-impl", cwd); + + return { cwd, commits: { base, staleMerge, subjectMerge } }; +} + describe("getCommitContextsBetweenShas", () => { let repo: TempRepo; @@ -1044,8 +1099,9 @@ describe("merge commit handling", () => { const shas = new Set(result.map((c) => c.sha)); expect(shas.has(multiRepo.commits.merge100)).toBe(true); expect(shas.has(multiRepo.commits.merge200)).toBe(true); - // merge300 only touched infra/ — kept by the merges-only scan, then dropped - // by commitTouchesPaths so LIN-300 doesn't leak into a frontend release. + // merge300 only touched infra/, so under the frontend/backend pathspec it + // changed nothing relative to its parents and `--full-history` drops it + // natively — LIN-300 never reaches a frontend release. expect(shas.has(multiRepo.commits.merge300)).toBe(false); expect(shas.has(multiRepo.commits.headMerge)).toBe(true); @@ -1115,6 +1171,63 @@ describe("merge commit handling", () => { expect(branchNames).not.toContain("feature/LIN-300-mobile"); }); }); + + describe("getCommitContextsBetweenShas with stale-branch merges under a path filter", () => { + let repo: TempRepoStaleMerge; + + beforeAll(() => { + repo = createTempRepoStaleMerge(); + }); + + afterAll(() => { + rmSync(repo.cwd, { recursive: true, force: true }); + }); + + it("drops a stale-branch merge that delivered no change to the filtered paths", () => { + // feat/ABC-1-stale edited app-a/ only but merged after app-b/ landed, so + // `--full-history` keeps its merge under the app-b pathspec. The merge + // delivered nothing to app-b/, so its subject key must not be attributed. + const result = getCommitContextsBetweenShas(repo.commits.base, repo.commits.subjectMerge, { + includePaths: ["app-b/**"], + cwd: repo.cwd, + }); + + const shas = new Set(result.map((c) => c.sha)); + expect(shas.has(repo.commits.staleMerge)).toBe(false); + + const branchNames = result.map((c) => c.branchName).filter((b): b is string => !!b); + expect(branchNames).not.toContain("feat/ABC-1-stale"); + }); + + it("retains a merge whose key lives only on the subject when it delivered the filtered paths", () => { + // feat/XYZ-2-impl genuinely edited app-b/ and carries its key only on the + // merge subject, so dropping the merge would lose the key entirely. + const result = getCommitContextsBetweenShas(repo.commits.base, repo.commits.subjectMerge, { + includePaths: ["app-b/**"], + cwd: repo.cwd, + }); + + const shas = new Set(result.map((c) => c.sha)); + expect(shas.has(repo.commits.subjectMerge)).toBe(true); + + const branchNames = result.map((c) => c.branchName).filter((b): b is string => !!b); + expect(branchNames).toContain("feat/XYZ-2-impl"); + }); + + it("still attributes a stale merge to the surface it actually touched", () => { + // The same stale merge DID deliver app-a/ changes, so under an app-a filter + // its subject key is correctly retained — the fix discards leaks, not work. + // And the app-b-only merge must not leak into the app-a surface. + const result = getCommitContextsBetweenShas(repo.commits.base, repo.commits.subjectMerge, { + includePaths: ["app-a/**"], + cwd: repo.cwd, + }); + + const branchNames = result.map((c) => c.branchName).filter((b): b is string => !!b); + expect(branchNames).toContain("feat/ABC-1-stale"); + expect(branchNames).not.toContain("feat/XYZ-2-impl"); + }); + }); }); describe("assertGitAvailable", () => { diff --git a/src/git.ts b/src/git.ts index 893fa4a..18f5f5e 100644 --- a/src/git.ts +++ b/src/git.ts @@ -332,13 +332,20 @@ export function extractBranchNameFromMergeMessage(message: string | null | undef * Prefers branch name from merge message over decorations for issue tracking. */ function parseCommitChunk(chunk: string): CommitContext { - const [sha, rawMessage, rawDecorations] = chunk.split("\x1f"); + const [sha, rawMessage, rawDecorations, rawParents] = chunk.split("\x1f"); // Collapse runs of horizontal whitespace, but keep newlines so downstream // extractors can tell the title from the body and skip nested commit blocks. const message = (rawMessage ?? "").trim().replace(/[ \t]+/g, " "); const branchName = extractBranchNameFromMergeMessage(message) ?? extractBranchName(rawDecorations); - - return { sha: sha.trim(), branchName, message }; + // %P is the parent SHAs, space-separated and empty for a root commit. Keep only + // full 40-char hashes so a root commit yields [] rather than [""], letting + // parents.length reliably tell a merge (2+) from a normal commit (1). + const parents = (rawParents ?? "") + .trim() + .split(/\s+/) + .filter((p) => /^[0-9a-f]{40}$/i.test(p)); + + return { sha: sha.trim(), branchName, message, parents }; } /** @@ -383,7 +390,7 @@ export function ensureCommitAvailable(sha: string, cwd: string = process.cwd()): } function runLog(rangeArgs: string, cwd: string): CommitContext[] { - const output = execSync(`git log --format=%H%x1f%B%x1f%D%x1e ${rangeArgs}`, { + const output = execSync(`git log --format=%H%x1f%B%x1f%D%x1f%P%x1e ${rangeArgs}`, { cwd, stdio: ["ignore", "pipe", "pipe"], encoding: "utf8", @@ -394,6 +401,43 @@ function runLog(rangeArgs: string, cwd: string): CommitContext[] { .map(parseCommitChunk); } +/** + * Whether merge `commit` delivered net changes to the filtered paths, compared to + * its first parent (the branch it was merged into). Non-merges pass through. + * + * Without this, `--full-history` retains a stale branch's merge for paths it never + * touched — they differ across the merge only because the target branch advanced + * while the branch was open — leaking the issue key on the merge subject. The + * first-parent diff is empty for those and non-empty when the merge really + * delivered the paths, so a merge whose issue key lives only on its subject is + * still kept when it actually touched the paths. On an unexpected diff failure, + * keep the merge rather than drop real work. + */ +function mergeDeliversToPaths(commit: CommitContext, pathspec: string, cwd: string): boolean { + const parents = commit.parents ?? []; + if (parents.length <= 1) { + return true; + } + try { + execSync(`git diff --quiet ${parents[0]} ${commit.sha} ${pathspec}`, { + cwd, + stdio: ["ignore", "ignore", "pipe"], + }); + return false; + } catch (e) { + const status = (e as { status?: number }).status; + if (status === 1) { + return true; + } + warn( + `Could not diff merge ${commit.sha.slice(0, 7)} against its first parent; keeping it under the path filter. ${ + e instanceof Error ? e.message : String(e) + }`, + ); + return true; + } +} + /** * Returns commits between two SHAs, optionally filtered by file paths. * @@ -401,7 +445,10 @@ function runLog(rangeArgs: string, cwd: string): CommitContext[] { * tree equals one of its parents' trees, so under a pathspec it's TREESAME * and git's default simplification drops it. That's true of every provider's * merge commit (GitHub, GitLab MR, Bitbucket PR, plain `git merge --no-ff`) - * and would lose the issue keys encoded in their feature-branch names. + * and would lose the issue keys encoded in their feature-branch names. Merges + * it keeps are then passed through `mergeDeliversToPaths`, which drops a + * stale-branch merge that delivered no net change to the filtered paths so its + * subject key isn't attributed to a surface it never touched. * * `--no-walk` (only when `fromSha === toSha`): without it, `git log -1 * -- ` walks back from `` to the first ancestor matching the @@ -432,14 +479,16 @@ export function getCommitContextsBetweenShas( } const inspectingSingleCommit = fromSha === toSha && inspectSingleCommit; + const pathspec = buildPathspecArgs(includePaths); const args = [ includePaths?.length ? "--full-history" : "", inspectingSingleCommit ? `--no-walk ${toSha}` : `${fromSha}..${toSha}`, - buildPathspecArgs(includePaths), + pathspec, ] .filter(Boolean) .join(" "); - const commits = runLog(args, cwd); + const logged = runLog(args, cwd); + const commits = pathspec ? logged.filter((commit) => mergeDeliversToPaths(commit, pathspec, cwd)) : logged; if (commits.length === 0) { if (inspectingSingleCommit) { diff --git a/src/types.ts b/src/types.ts index 12d0868..6f5d5f5 100644 --- a/src/types.ts +++ b/src/types.ts @@ -68,6 +68,7 @@ export type CommitContext = { sha: string; branchName?: string | null; message?: string | null; + parents?: string[]; }; export type GitInfo = {