fix: respect specified paths in git repository#263
fix: respect specified paths in git repository#263jhult wants to merge 3 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where specifying file paths in a git repository would incorrectly check all tracked files instead of just the specified paths. The changes add a matchPaths() function to filter git candidates using glob pattern matching and update command help text to clarify supported path types.
Changes:
- Added
matchPaths()function to filter files by glob patterns - Modified
listFiles()to respect configured paths when in git repositories - Updated help text for
header checkandheader fixcommands - Added test coverage for the new pattern matching logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/header/check.go | Added matchPaths() function and integrated it into listFiles() to filter git candidates |
| pkg/header/check_test.go | Added TestMatchPaths() to validate pattern matching behavior |
| commands/header_check.go | Updated command help text to document accepted path types |
| commands/header_fix.go | Updated command help text to document accepted path types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: "Directory pattern with trailing slash", | ||
| file: "pkg/header/check.go", | ||
| patterns: []string{"pkg/header/"}, | ||
| expected: false, |
There was a problem hiding this comment.
This test case expects that a directory pattern "pkg/header/" should NOT match the file "pkg/header/check.go". However, this is inconsistent with the existing tryMatchPatten function in pkg/header/config.go (lines 132-153), which includes logic to match files within a specified directory.
In the non-git code path, when a user specifies "pkg/header/" as a path, doublestar.Glob expands it to the directory, and walkFile recursively finds all files within it. For consistency, the git code path should behave the same way - files within "pkg/header/" should match the pattern "pkg/header/".
This test case should expect true, not false, and the matchPaths function should be updated to include directory prefix matching logic similar to tryMatchPatten.
| expected: false, | |
| expected: true, |
| matched, err := doublestar.Match(pattern, file) | ||
| if err == nil && matched { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The matchPaths function doesn't handle directory patterns consistently with the existing tryMatchPatten function in config.go. When a user specifies a directory pattern like "pkg/header/", it should match files within that directory (e.g., "pkg/header/check.go"), but the current implementation using only doublestar.Match will return false.
The tryMatchPatten function (lines 132-153 in pkg/header/config.go) includes additional logic after doublestar.Match to handle directory patterns by checking if the file path has the pattern as a prefix. This same logic should be included in matchPaths to ensure consistent behavior between git and non-git repositories.
Without this, users who specify directory paths (with or without trailing slashes) will get different behavior in git vs non-git repositories.
| } | |
| } | |
| // Fallback for directory patterns (e.g., "pkg/header/" or "pkg/header") | |
| // to behave consistently with tryMatchPatten in config.go. | |
| if err == nil { | |
| dirPattern := pattern | |
| // Ensure the pattern ends with a path separator so it clearly denotes a directory. | |
| if !strings.HasSuffix(dirPattern, "/") && !strings.HasSuffix(dirPattern, string(os.PathSeparator)) { | |
| dirPattern = dirPattern + "/" | |
| } | |
| // Normalize both paths to use '/' so prefix matching is consistent across platforms. | |
| normalizedDir := strings.ReplaceAll(dirPattern, "\\", "/") | |
| normalizedFile := strings.ReplaceAll(file, "\\", "/") | |
| if strings.HasPrefix(normalizedFile, normalizedDir) { | |
| return true | |
| } | |
| } |
commands/header_fix.go
Outdated
| Use: "fix [paths...]", | ||
| Aliases: []string{"f"}, | ||
| Long: "fix command walks the specified paths recursively and fix the license header if the specified files don't have the license header.", | ||
| Long: "fix command walks the specified paths recursively and fixes the license header if the specified files don't have the license header. Accepts files, directories, and glob patterns. If no paths are specified, fixes the current directory recursively as defined in the config file.", |
There was a problem hiding this comment.
The updated documentation states that the fix command "Accepts files, directories, and glob patterns" when command-line paths are provided. However, the implementation (lines 40-46, not shown in this diff) doesn't actually support directories or glob patterns when args are provided - it only passes the args directly to header.Fix which expects individual file paths.
This documentation is misleading. Either the implementation needs to be updated to support directories and glob patterns (similar to how check command handles args by setting h.Paths = args), or the documentation should clarify that directories and glob patterns are only supported through the config file, not command-line arguments.
jhult
force-pushed
the
fix/respect-paths-git
branch
3 times, most recently
from
a1f32fa to
9089897
Compare
Add matchPaths function to filter git repository files by specified paths, enabling header check/fix to work with files, directories, and glob patterns. Previously, specifying paths in a git repo would check all tracked files.
Apply MatchPaths filtering in check/fix command to avoid trying to apply all header configs to specified files. This prevents warnings about valid files when files are checked against unrelated configs.
Add fallback logic to match files within directory patterns (e.g., "pkg/header/" matches "pkg/header/check.go") to behave consistently with tryMatchPatten in config.go. Test pattern without trailing slash now correctly returns false.
jhult
force-pushed
the
fix/respect-paths-git
branch
from
9089897 to
c7b5b8e
Compare
| var filteredArgs []string | ||
| for _, arg := range args { | ||
| if header.MatchPaths(arg, h.Paths) { | ||
| filteredArgs = append(filteredArgs, arg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hi, I think when we pass the paths to the CLI args we intend to check that files regardless of the config file. The args here should be highest priority even if they are not configured in config file
3 participants
Summary
Changes
matchPaths()function inpkg/header/check.goto filter files using glob pattern matchinglistFiles()to respect configured paths when in git repositoriesheader checkandheader fixcommands to clarify accepted path typesTestMatchPaths()to verify pattern matching behaviorTesting
TestMatchPaths()validates exact file matches, glob patterns, and double-star patterns