Ensure stale branch are not taken into account by --include-paths (#104)

* Ensure stale branch are not taken into account by --include-paths

* Address review: drop #62 links and clarify parent-parsing/stale-merge comments

Author: RomainCscn

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", () => {

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 };
 }
 
 /**
@@ -387,7 +394,7 @@ export function ensureCommitAvailable(sha: string, cwd: string = process.cwd()):
 const RUN_LOG_MAX_BUFFER = 256 * 1024 * 1024;
 
 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",
@@ -399,14 +406,54 @@ 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.
  *
  * `--full-history` (only when `includePaths` is set): a non-evil merge's
  * 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 <sha>
  * -- <paths>` walks back from `<sha>` to the first ancestor matching the
@@ -437,14 +484,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) {

src/types.ts

@@ -68,6 +68,7 @@ export type CommitContext = {
   sha: string;
   branchName?: string | null;
   message?: string | null;
+  parents?: string[];
 };
 
 export type GitInfo = {