Match .gitattributes semantics when asking which files a pin covers - #70
Merged
Conversation
The dead-pattern direction asked `git ls-files -- <pattern>`, and pathspec
does not share gitattributes glob semantics. It is wrong in both
directions:
under a slash-free pattern matches its basename at any depth in
gitattributes, where bare pathspec reads it as a root-relative
path. `Dockerfile` covers `Docker/Dockerfile`; bare pathspec
returns nothing, so a live pin reports dead and reds CI.
over `*` does not cross a `/` in gitattributes and does in bare
pathspec, so `pkg/*.py` wrongly picks up `pkg/sub/nested.py`.
Harmless here, since it can only hide a dead pin.
The first is the one with teeth: a false failure on a correct pin.
`:(glob)` gives `*` and `**` their gitattributes meaning, and a `**/`
prefix supplies the any-depth match for a slash-free pattern.
Verified against `git check-attr`, which is what git actually applies,
rather than against the documentation. On a tree holding
`Docker/Dockerfile`, `pkg/mod.py` and `pkg/sub/nested.py`, check-attr
resolves eol=lf for exactly the first two, and the conversion selects
exactly those two where the bare form selects the wrong set both times.
Differentially: on that tree the old matcher reports `Dockerfile` dead
and the new one does not.
No pattern in this repository changes result, bare or converted, so this
fixes a latent defect rather than a live one. Nothing here is
slash-free-and-literal today.
Found by Copilot review on #68. Its remedy was right and its evidence was
not: it predicted `*.sh` would be flagged dead here for want of a
root-level `.sh` file, and `git ls-files -- '*.sh'` returns all five
nested ones, because bare pathspec lets `*` cross a `/`. That is the
over-match above, and the reason CI was green rather than failing as the
comment predicted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the .gitattributes “dead pin” check in checks/check-eol-pins.py so that it queries tracked files using pathspec glob semantics that better match how .gitattributes patterns are interpreted, avoiding false “dead pattern” failures in CI for certain pattern shapes.
Changes:
- Introduces
pathspec_for()to translate a.gitattributespattern into an equivalentgit ls-filespathspec using:(glob)and (when appropriate) a**/prefix. - Switches the dead-pattern check to call
git ls-fileswith the converted pathspec instead of the raw pattern.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`/Dockerfile` is root-anchored and covers only the root file, where a bare `Dockerfile` matches at any depth. `pathspec_for` stripped the leading slash and then tested the remainder for one, which destroys the evidence the decision needs, so it built `:(glob)**/Dockerfile` and matched `Docker/Dockerfile` too. Verified against check-attr, which resolves eol=lf for the root file alone. Anchoring is now decided before the strip. That is the second conversion defect in this function, one under-matching and one over-matching, and both were found by review rather than by the gate. Fixing each shape as it surfaces leaves the next one to be found the same way, so the matcher is now checked against `git check-attr`, which is the thing it exists to predict: every file resolving to eol=lf is matched by some converted pattern, and every file a converted pattern matches resolves to eol=lf. A missing file means the conversion under-matched, which is what produces a false dead report. An extra file means it over-matched, which hides a real one. Both are reported as matcher findings rather than as pin findings, because they are defects in this script and not in the pins. Verified by reintroducing each defect against a tree holding `Docker/Dockerfile`, a root `Dockerfile`, `pkg/mod.py` and `pkg/sub/nested.py`. The bare-pathspec version is caught in both directions at once, the leading-slash version as an over-match, and with the matcher correct the third direction reports nothing on the same tree. This holds because no pattern here clears `eol` once another has set it, which the comment records, since a future pattern that unsets it would need the comparison to account for precedence. Found by Copilot review on #70. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by Copilot review on #68. Its remedy was right and its evidence was not, and both halves are worth stating.
The claim, measured
It does not:
Bare pathspec lets
*cross a/, so all five nested files match. That is also why CI was green rather than failing as predicted.The real defect, which is the opposite direction
Pathspec and gitattributes genuinely do not share glob semantics, and the divergence that bites is the one the comment did not name: a slash-free pattern with no glob character. In gitattributes it matches its basename at any depth. In bare pathspec it is a root-relative path.
Ground truth from
git check-attr, which is what git actually applies on checkout, against a tree holdingDocker/Dockerfile,pkg/mod.py,pkg/sub/nested.py:check-attrsays eol=lfls-files:(glob)convertedDockerfileDocker/DockerfileDocker/Dockerfilepkg/*.pypkg/mod.pypkg/mod.py,pkg/sub/nested.pypkg/mod.pyThe bare form gets both wrong. The first row is the one with teeth: a live pin reported dead, failing CI on correct content. The second can only ever hide a dead pin, never invent one.
The fix
:(glob)gives*and**their gitattributes meaning, and a**/prefix supplies the any-depth match for a slash-free pattern. The conversion selects exactly the setcheck-attrresolves, on both rows.Differential proof on that tree — same script, same repo, only the matcher swapped:
Scope
No pattern in this repository changes result, bare or converted — all twelve match identically, because nothing here is slash-free-and-literal today. This fixes a latent defect rather than a live one, and it matters most if the check is ever lifted into the hub, where
.gitattributescarries exactly that shape (Dockerfile text eol=lf).Reported upstream on ptr727/ProjectTemplate#633, where my original implementation note claimed the pathspec divergence "errs toward not reporting, which is the safe bias." That was half right, and the wrong half is this. Correcting it there.