Skip to content

test(git): cover worktrees, untracked files, and the OWNERSHIP-deletion gap#199

Merged
bestdan merged 1 commit into
bestdan:mainfrom
azack:azack/hook-detection-test-coverage
Jun 18, 2026
Merged

test(git): cover worktrees, untracked files, and the OWNERSHIP-deletion gap#199
bestdan merged 1 commit into
bestdan:mainfrom
azack:azack/hook-detection-test-coverage

Conversation

@azack

@azack azack commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #193 (now merged). Adds the test coverage that work was missing — exercised manually against the branch before merge, now locked in as tests against main.

What this adds

src-only — the compiled *.test.js are gitignored, so there are no dist changes.

Worktreesget-files-to-push-with-status.test.ts

  • getFilesToPushWithStatus called from inside a linked git worktree, no remote → full main...HEAD range with statuses.
  • same, with a remote → only the unpushed file.

Confirms detection and the origin/<branch> remote resolution work when .git is a file rather than a directory (the case the suite didn't cover).

Untracked filesget-all-changed-files-with-status.test.ts

  • untracked working-tree files are ignored in push mode (only the tracked add is detected).

OWNERSHIP-deletion gapget-all-changed-files-with-status.test.ts + is-codeowners-relevant.test.ts

  • characterizes that a committed OWNERSHIP deletion is stripped by --diff-filter=ACMR, so it never reaches isCodeownersRelevant and codeowners is skipped for that push — leaving a directory potentially orphaned.
  • isCodeownersRelevant would flag a deleted OWNERSHIP by name (added test), so the gap is in detection (D is filtered out upstream), not the gate.
  • an it.todo marks the behavior to close (surface ownership-file deletions so codeowners can re-validate).

Verification

  • Changed test files: 20 passed + 1 todo against current main.
  • prettier --check, eslint, tsc --noEmit: clean.

…on gap

Adds coverage the codeowners early-exit work (bestdan#193) was missing:

- getFilesToPushWithStatus: detection from inside a linked git worktree,
  both with and without a remote (mirrors the existing non-worktree cases).
- getAllChangedFilesWithStatus: untracked working-tree files are ignored
  in push mode.
- isCodeownersRelevant + detection: characterize that a committed OWNERSHIP
  deletion is stripped by --diff-filter=ACMR and never reaches the gate, with
  an it.todo marking the gap to close.

src-only; compiled *.test.js are gitignored.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several unit tests to verify Git utility behaviors, including handling untracked files, identifying relevant codeowners changes, and retrieving files to push within linked worktrees. The review feedback highlights a potential issue in the test cleanup blocks where a failure in one rmSync call could prevent subsequent temporary directories from being deleted. It is recommended to wrap these cleanup operations in nested try...finally blocks to guarantee that all temporary directories are reliably removed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +117 to +120
} finally {
rmSync(tempDir, { recursive: true, force: true });
rmSync(worktreeParent, { recursive: true, force: true });
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If rmSync(tempDir) fails (e.g., due to file locking issues on Windows), the finally block will throw and prevent rmSync(worktreeParent) from executing, leaving orphaned temporary directories. Wrapping the cleanups in nested try...finally blocks ensures both directories are always cleaned up.

Suggested change
} finally {
rmSync(tempDir, { recursive: true, force: true });
rmSync(worktreeParent, { recursive: true, force: true });
}
} finally {
try {
rmSync(tempDir, { recursive: true, force: true });
} finally {
rmSync(worktreeParent, { recursive: true, force: true });
}
}

Comment on lines +152 to +156
} finally {
rmSync(tempDir, { recursive: true, force: true });
rmSync(remoteDir, { recursive: true, force: true });
rmSync(worktreeParent, { recursive: true, force: true });
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similarly, if any of the rmSync calls fail, the subsequent cleanups will be skipped. Wrapping them in nested try...finally blocks ensures that all temporary directories (tempDir, remoteDir, and worktreeParent) are reliably cleaned up.

      } finally {
        try {
          rmSync(tempDir, { recursive: true, force: true });
        } finally {
          try {
            rmSync(remoteDir, { recursive: true, force: true });
          } finally {
            rmSync(worktreeParent, { recursive: true, force: true });
          }
        }
      }

@bestdan bestdan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thank you!

@bestdan bestdan merged commit aea2c34 into bestdan:main Jun 18, 2026
7 checks passed
@azack azack deleted the azack/hook-detection-test-coverage branch June 18, 2026 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants