Hook to check stale file references - #174
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new check-stale-references pre-commit hook intended to detect references to deleted/renamed files (by full repo-relative path and by basename) in the remaining tracked files, helping prevent broken cross-references after refactors.
Changes:
- Introduces
dev_tools.check_stale_referencesimplementation and a console entrypoint (check-stale-references). - Adds test coverage for pattern-building, detection, and output formatting.
- Wires the hook into
.pre-commit-hooks.yamland documents it inREADME.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
dev_tools/check_stale_references.py |
Implements stale-reference detection and printing for use as a pre-commit hook. |
tests/test_check_stale_references.py |
Adds unit tests for matching behavior and reporting. |
pyproject.toml |
Registers the new console script and adjusts Ruff test ignores. |
.pre-commit-hooks.yaml |
Adds the new check-stale-references hook definition. |
README.md |
Documents the new hook in the Tools section and TOC. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…oundaries - Replace Python file I/O with git grep for performance - Extract _run_git() helper using subprocess.run(check=True) - Simplify D/R branch in get_deleted_paths (both use parts[1]) - Add __str__ to StaleReference, remove print_stale_references - Tighten regex boundaries to [\w.-] to reject foo.hpp.bak etc. - Remove unused get_repo_root and get_tracked_files
| matches: list[tuple[str, int, str]] = [] | ||
| for line in result.stdout.splitlines(): | ||
| # git grep output: file:line_number:matched_line | ||
| file, line_no, text = line.split(":", 2) | ||
| matches.append((file, int(line_no), text)) | ||
| return matches |
There was a problem hiding this comment.
We can extract this to a list comprehension by extracting another method out of this. That'd be overkill imo.
|
|
||
|
|
||
| def build_path_pattern(deleted_path: str) -> str: | ||
| """Build a PCRE pattern for a deleted path that matches references to it. |
There was a problem hiding this comment.
PCRE support in git is optionally compiled in, but it seems all widely used distros ship with a binary that includes this. Even the tiny alpine image has it:
$ docker run --rm -it alpine:latest sh
$ apk add git
$ apk info -R git # already shows the libpcre2-8.so.0 dependency
$ git clone https://github.com/hofbi/dev-tools.git
$ cd dev-tools
$ git grep -P 'test'
<some output showing that it works>If we don't want to rely on the presence of PRCE in git, we'd have to revert this commit, i.e., do the boundary check from line 59 ourselves in python because only PCRE supports that in git grep.
| For path "a/b/c.txt", matches (with non-word/dot/dash boundaries): | ||
| - /?a/b/c.txt (full path, optional leading /) | ||
| - (../)*b/c.txt (intermediate suffix with optional ../ prefix) | ||
| - (../)*c.txt (basename with optional ../ prefix) |
There was a problem hiding this comment.
This has some chance of raising false positives for common filenames such as utils.py. Similarly, there might be conflict with some language includes such as cpp's #include <mylib/foo.hpp>. Should we guard against these? We could
- Require more than 2 path segments
- Skip known import/include syntax
- Ditch the partial path extraction logic altogether and only check full paths
- Require the basename to be 'unusual enough' (my least preferred option)
There was a problem hiding this comment.
I think we should either look at full paths or relative paths starting with ./ or ../.
There was a problem hiding this comment.
So you're saying don't make prefixes ./ or ../ optional, but require presence of at least one of the two for partial paths?
There was a problem hiding this comment.
Correct. Only consider absolute paths as you described them or relative paths starting with ./ or ../. This should help us to skip most C++ headers and other paths you listed as false positives.
There was a problem hiding this comment.
I think this will cause a huge number of false negatives, i.e., paths that are ignored despite being references. Aren't you worried about that? Or is your idea for this hook to have it kind of opt-in via the required relative or absolute prefix?
There was a problem hiding this comment.
What would you suggest? Go with the current setup for now, see what issues we find, and improve based on that?
| """ | ||
| segments = deleted_path.split("/") | ||
| alternatives: list[str] = [] | ||
| for i in range(len(segments)): |
There was a problem hiding this comment.
Isn't this what enumerate is made for?
Closes #162