Add glob-pattern support for --exclude and integrate matcher into watcher/syncer#6
Draft
Add glob-pattern support for --exclude and integrate matcher into watcher/syncer#6
--exclude and integrate matcher into watcher/syncer#6Conversation
🤖 Pull request artifacts
|
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Exclude as "exclude.Matcher"
participant Watcher
participant Syncer
participant FS as "Filesystem / fsnotify"
CLI->>Exclude: New(cfg.Excludes, cfg.Source)
Exclude-->>CLI: matcher / error
CLI->>Watcher: New(rootPath, matcher, logger)
CLI->>Syncer: New(rootPath, client, matcher, logger)
FS->>Watcher: fsnotify event (path)
Watcher->>Exclude: MatchRule(fullpath)
Exclude-->>Watcher: matched? (rule)
alt matched
Watcher-->>Watcher: log debug + skip
else not matched
Watcher->>Syncer: notify event/path
Syncer->>Exclude: MatchRule(absPath) or MatchRule(relPath)
Exclude-->>Syncer: matched?
alt matched
Syncer-->>Syncer: log debug + skip/return
else not matched
Syncer->>FS: upload/remove/reconcile actions
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
internal/syncer/syncer.go (1)
159-169: SimplifyisExcluded– fallback is unnecessary.The matcher's
normalizeInputPathalready handles both absolute and relative paths (converting absolute paths to relative internally). CallingMatchRuletwice with bothabsPathandrelPathis redundant—passing justabsPathshould suffice.♻️ Suggested simplification
func (s *Syncer) isExcluded(absPath, relPath string, isDir bool) (bool, string) { if s.matcher == nil { return false, "" } - if matched, rule := s.matcher.MatchRule(absPath, isDir); matched { - return true, rule - } - - return s.matcher.MatchRule(relPath, isDir) + return s.matcher.MatchRule(absPath, isDir) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 159 - 169, The isExcluded method redundantly calls s.matcher.MatchRule twice; simplify by only using the absolute path once. Keep the nil check for s.matcher, then call s.matcher.MatchRule(absPath, isDir) and return its (matched, rule) result directly instead of calling MatchRule(relPath...). Update the function body in Syncer.isExcluded to remove the fallback call and return the single MatchRule result.internal/exclude/matcher.go (3)
61-61: UnusedisDirparameter.The
isDirparameter is ignored (blank identifier_ bool). If directory-specific matching behavior is planned (e.g., patterns ending with/), consider implementing it; otherwise, remove the parameter from the API to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` at line 61, The MatchRule method currently declares an unused isDir parameter (signature: Matcher.MatchRule(filePath string, _ bool) (bool, string)); remove the unused boolean parameter from the method signature (change to Matcher.MatchRule(filePath string) (bool, string)) and update all callers and any tests to use the new signature, or alternatively implement directory-aware logic inside Matcher.MatchRule to respect patterns ending with '/' by using the isDir value — pick one approach consistently and adjust references to the Matcher.MatchRule symbol accordingly.
12-16: Consider simplifying therulestruct.Both
literalandpatternfields are always assigned the same normalized value (lines 44-45). You could consolidate into a singlevaluefield to reduce redundancy.♻️ Suggested simplification
type rule struct { - literal string - pattern string + value string isPattern bool }Then update usages accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 12 - 16, The rule struct has redundant fields literal and pattern that are always the same; replace them with a single field (e.g., value string) and keep isPattern bool to indicate whether value is a glob/pattern or a literal, then update every place that constructs or reads rule.literal or rule.pattern (e.g., parsers/constructors and matchers) to use rule.value instead and preserve the existing isPattern semantics so matching logic in functions that reference rule.literal/pattern continues to work correctly.
125-127: Consider documenting the intentional omission of[and].The
hasMetafunction only checks for*and?, meaning bracket expressions like[abc]are treated as literals. This appears intentional (per the tests), but a brief comment would clarify this design choice for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 125 - 127, The hasMeta function currently checks only for '*' and '?' via strings.ContainsAny(pattern, "*?") and intentionally treats bracket expressions like '[' and ']' as literal characters; add a brief comment above hasMeta explaining this design decision (i.e., that '[' and ']' are intentionally omitted so character classes are not considered meta) and reference the function name hasMeta and the pattern parameter so future maintainers understand why bracket expressions are not treated as meta patterns.internal/watcher/watcher.go (1)
227-230: Consider threading context throughaddRecursive.Using
context.Background()here loses the ability to respect cancellation during initial directory traversal and prevents structured logging context from propagating. Consider passing the context from the caller.♻️ Suggested change
-func (w *Watcher) addRecursive(path string) error { - if w.isExcluded(context.Background(), path) { +func (w *Watcher) addRecursive(ctx context.Context, path string) error { + if w.isExcluded(ctx, path) { return nil }Then update callers to pass
ctx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/watcher/watcher.go` around lines 227 - 230, The call to context.Background() in addRecursive loses cancellation and logging context; change the signature of Watcher.addRecursive to accept a context (e.g., func (w *Watcher) addRecursive(ctx context.Context, path string) error) and use that ctx when calling w.isExcluded and any other context-using helpers, then update all callers of addRecursive to pass through their ctx (or derive one) so cancellation and structured logging propagate correctly.docs/implementation-plan-exclude-patterns.md (5)
54-60: Clarify matching order and literal-vs-pattern precedence.The statement "Every
--excludeentry is treated as: 1. literal path match... 2. glob pattern match" is ambiguous. Does this mean:
- Both checks are performed for every rule (literal first, then pattern)?
- Or that each rule is classified as either literal OR pattern?
If both are attempted, what's the performance impact? If a rule contains glob metacharacters, should literal matching be skipped? Clarify the evaluation order and short-circuit behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 54 - 60, Clarify that each --exclude entry is evaluated deterministically: first test for an exact literal path equality (short-circuit and treat as a match if true), and only if the literal check fails then evaluate as a glob pattern match; explicitly note that entries containing glob metacharacters still undergo the literal check first but that implementations MAY skip the literal check for strings that are explicitly flagged/parsed as patterns to avoid extra cost, and document that a successful literal match prevents any subsequent glob evaluation (short-circuit) and that directory literal matches exclude all descendants.
43-67: Add cross-platform and edge-case considerations.The proposed behavior should explicitly address:
- Case sensitivity: Should pattern matching be case-sensitive? This varies by platform (Windows is case-insensitive, Unix is case-sensitive). Specify the intended behavior or document that it follows the underlying filesystem.
- Symbolic links: How should symbolic links be handled? Should they be followed for matching, or matched by their link name?
- Absolute paths: What happens if a user provides an absolute path in a pattern (e.g.,
/home/user/project/file.txt)? Should these be rejected, normalized, or supported?- Directory matching scope (line 59): Explicitly state whether "directory match excludes all descendants" applies to both literal matches (e.g.,
node_modules) and pattern matches (e.g.,**/tmp).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 43 - 67, Update the "Proposed behavior" section to explicitly cover cross-platform and edge cases: state the intended case-sensitivity behavior (e.g., normalize decision or "follows underlying filesystem"), define symbolic link handling (e.g., whether matching follows symlinks or matches the link name), specify how absolute paths in patterns are treated (reject, normalize to source-root-relative, or support as-is), and clarify that the rule "Directory match excludes all descendants" in "Matching rules" applies to both literal matches and glob pattern matches (give examples like `node_modules` and `**/tmp` to illustrate). Ensure these clarifications appear alongside the existing "Pattern syntax" and "Matching rules" text so readers can find them with the same headings and examples (e.g., `--exclude=.git/**`, `--exclude=**/*.tmp`).
106-115: Expand test coverage to include additional edge cases.The minimum test cases are solid. Consider adding:
- Case sensitivity: Test behavior with patterns like
*.TMPvs*.tmpacross platforms.- Malformed patterns: Test validation of invalid patterns (e.g., unbalanced brackets if using extended globs later).
- Broad patterns: Test performance implications of overly broad patterns like
**/*or*.- Empty/whitespace rules: Test behavior with empty strings or whitespace-only exclude rules.
- Boundary cases: Test patterns matching the source root itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 106 - 115, Add additional unit tests around the exclude-pattern matcher to cover edge cases: add tests for case-sensitivity (e.g., patterns like "*.TMP" vs "*.tmp"), malformed patterns (invalid glob syntax), broad patterns ("**/*" and "*" to observe matching and performance), empty or whitespace-only rules, and patterns that match the source root itself; target the existing exclude matcher tests (the test file covering the exclude logic / the function that evaluates excludes e.g., isExcluded or matchesExclude) and assert normalized path handling (Windows separators) and explicit validation errors for invalid patterns where applicable.
150-155: Optionally enhance acceptance criteria with validation tooling.The acceptance criteria are clear and measurable. Consider adding:
- Optional enhancement: "Users can run a
--validate-excludesor--list-excludedcommand to preview which files would be excluded without performing actual sync, reducing risk of unintended exclusions."This would strengthen the UX and make troubleshooting easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 150 - 155, Add an optional acceptance criterion that describes a dry-run/preview validation tool (e.g., a CLI flag like --validate-excludes or --list-excluded) that allows users to preview which files would be excluded without performing create/update/delete actions; update the "Acceptance criteria" section to include this bullet, clarify it is optional/enhancement, and ensure it specifies expected behavior (preview output, non-destructive, usable with glob and literal excludes).
137-147: Consider additional risks for completeness.The identified risks are good. Consider adding:
Unintended critical file exclusion: Users might write overly broad patterns (e.g.,
*.log) that accidentally exclude important files.
- Mitigation: Provide a
--dry-runor--list-excludedflag to preview what would be excluded before actual sync.Literal filenames with glob metacharacters: If a user has a file literally named
test[1].txtand wants to exclude it, the current plan treats all rules as potential patterns. A ruletest[1].txtmight be interpreted as a character class pattern rather than a literal filename.
- Mitigation: Document escaping mechanism (if supported by doublestar) or recommend a prefix syntax like
literal:to force literal matching.Production rollback: If severe issues arise post-release, what's the rollback strategy?
- Mitigation: Consider a feature flag or environment variable to temporarily disable pattern matching and fall back to literal-only mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 137 - 147, Add three additional risk items under the "Risks and mitigations" section: 4) Unintended critical file exclusion — advise adding a preview flag like --dry-run or --list-excluded to show what would be excluded before syncing; 5) Literal filenames with glob metacharacters — document escaping rules for doublestar or introduce/describe a literal: prefix to force literal matching; and 6) Production rollback — specify a rollback strategy such as a feature flag or environment variable to disable pattern matching and fall back to literal-only mode in emergencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 6: Update the doublestar dependency from v4.8.1 to v4.10.0 in go.mod by
changing the module version string (the line containing
"github.com/bmatcuk/doublestar/v4 v4.8.1"), then run `go get
github.com/bmatcuk/doublestar/v4@v4.10.0` followed by `go mod tidy` to refresh
the lockfile and verify module resolution; finally run the test suite/build to
ensure no breaking changes affected code that uses doublestar functions.
In `@internal/exclude/matcher_test.go`:
- Around line 87-95: The test loop contains a redundant per-iteration copy `tt
:= tt` causing a copyloopvar lint; remove that line and use the ranged variable
`tt` directly in the t.Run closure, leaving the call to matcher.Match(tt.path,
false) and the t.Fatalf unchanged so each subtest uses the correct tt values
(i.e., update the loop that iterates over tests and the t.Run anonymous function
to drop the extra `tt := tt`).
- Around line 39-47: The test loop contains a redundant loop-variable copy `tt
:= tt` inside the range over `tests`; remove that assignment so each subtest
uses the iteration-scoped `tt` directly. Update the for loop that ranges over
`tests` and its t.Run closure (which calls matcher.Match) by deleting the `tt :=
tt` line to satisfy Go 1.22+ semantics and the linter warning.
In `@internal/syncer/syncer.go`:
- Around line 110-111: Replace path.Join with filepath.Join when constructing
childAbsPath and childRelPath (these variables are created using absPath and
relPath) so filesystem separators follow the OS semantics; update the same
usages of path.Join elsewhere in this function (the later construction of
sibling/descendant paths around where childAbsPath/childRelPath are set) to
filepath.Join to avoid mixing path and filepath operations.
In `@README.md`:
- Around line 217-222: The "Proposed next implementation" heading and its text
now conflict with the checklist state showing the `--exclude` patterns feature
as completed; update the README's "Proposed next implementation" section
(heading and the bullet referencing
`docs/implementation-plan-exclude-patterns.md`) to either (a) mark it as
completed and move or rename the section to something like "Completed: Support
for patterns in the `--exclude` option" with a brief status note, or (b) remove
the section entirely to avoid duplication; ensure you reference the same heading
text "Proposed next implementation" and the linked file
`docs/implementation-plan-exclude-patterns.md` when making the change.
---
Nitpick comments:
In `@docs/implementation-plan-exclude-patterns.md`:
- Around line 54-60: Clarify that each --exclude entry is evaluated
deterministically: first test for an exact literal path equality (short-circuit
and treat as a match if true), and only if the literal check fails then evaluate
as a glob pattern match; explicitly note that entries containing glob
metacharacters still undergo the literal check first but that implementations
MAY skip the literal check for strings that are explicitly flagged/parsed as
patterns to avoid extra cost, and document that a successful literal match
prevents any subsequent glob evaluation (short-circuit) and that directory
literal matches exclude all descendants.
- Around line 43-67: Update the "Proposed behavior" section to explicitly cover
cross-platform and edge cases: state the intended case-sensitivity behavior
(e.g., normalize decision or "follows underlying filesystem"), define symbolic
link handling (e.g., whether matching follows symlinks or matches the link
name), specify how absolute paths in patterns are treated (reject, normalize to
source-root-relative, or support as-is), and clarify that the rule "Directory
match excludes all descendants" in "Matching rules" applies to both literal
matches and glob pattern matches (give examples like `node_modules` and `**/tmp`
to illustrate). Ensure these clarifications appear alongside the existing
"Pattern syntax" and "Matching rules" text so readers can find them with the
same headings and examples (e.g., `--exclude=.git/**`, `--exclude=**/*.tmp`).
- Around line 106-115: Add additional unit tests around the exclude-pattern
matcher to cover edge cases: add tests for case-sensitivity (e.g., patterns like
"*.TMP" vs "*.tmp"), malformed patterns (invalid glob syntax), broad patterns
("**/*" and "*" to observe matching and performance), empty or whitespace-only
rules, and patterns that match the source root itself; target the existing
exclude matcher tests (the test file covering the exclude logic / the function
that evaluates excludes e.g., isExcluded or matchesExclude) and assert
normalized path handling (Windows separators) and explicit validation errors for
invalid patterns where applicable.
- Around line 150-155: Add an optional acceptance criterion that describes a
dry-run/preview validation tool (e.g., a CLI flag like --validate-excludes or
--list-excluded) that allows users to preview which files would be excluded
without performing create/update/delete actions; update the "Acceptance
criteria" section to include this bullet, clarify it is optional/enhancement,
and ensure it specifies expected behavior (preview output, non-destructive,
usable with glob and literal excludes).
- Around line 137-147: Add three additional risk items under the "Risks and
mitigations" section: 4) Unintended critical file exclusion — advise adding a
preview flag like --dry-run or --list-excluded to show what would be excluded
before syncing; 5) Literal filenames with glob metacharacters — document
escaping rules for doublestar or introduce/describe a literal: prefix to force
literal matching; and 6) Production rollback — specify a rollback strategy such
as a feature flag or environment variable to disable pattern matching and fall
back to literal-only mode in emergencies.
In `@internal/exclude/matcher.go`:
- Line 61: The MatchRule method currently declares an unused isDir parameter
(signature: Matcher.MatchRule(filePath string, _ bool) (bool, string)); remove
the unused boolean parameter from the method signature (change to
Matcher.MatchRule(filePath string) (bool, string)) and update all callers and
any tests to use the new signature, or alternatively implement directory-aware
logic inside Matcher.MatchRule to respect patterns ending with '/' by using the
isDir value — pick one approach consistently and adjust references to the
Matcher.MatchRule symbol accordingly.
- Around line 12-16: The rule struct has redundant fields literal and pattern
that are always the same; replace them with a single field (e.g., value string)
and keep isPattern bool to indicate whether value is a glob/pattern or a
literal, then update every place that constructs or reads rule.literal or
rule.pattern (e.g., parsers/constructors and matchers) to use rule.value instead
and preserve the existing isPattern semantics so matching logic in functions
that reference rule.literal/pattern continues to work correctly.
- Around line 125-127: The hasMeta function currently checks only for '*' and
'?' via strings.ContainsAny(pattern, "*?") and intentionally treats bracket
expressions like '[' and ']' as literal characters; add a brief comment above
hasMeta explaining this design decision (i.e., that '[' and ']' are
intentionally omitted so character classes are not considered meta) and
reference the function name hasMeta and the pattern parameter so future
maintainers understand why bracket expressions are not treated as meta patterns.
In `@internal/syncer/syncer.go`:
- Around line 159-169: The isExcluded method redundantly calls
s.matcher.MatchRule twice; simplify by only using the absolute path once. Keep
the nil check for s.matcher, then call s.matcher.MatchRule(absPath, isDir) and
return its (matched, rule) result directly instead of calling
MatchRule(relPath...). Update the function body in Syncer.isExcluded to remove
the fallback call and return the single MatchRule result.
In `@internal/watcher/watcher.go`:
- Around line 227-230: The call to context.Background() in addRecursive loses
cancellation and logging context; change the signature of Watcher.addRecursive
to accept a context (e.g., func (w *Watcher) addRecursive(ctx context.Context,
path string) error) and use that ctx when calling w.isExcluded and any other
context-using helpers, then update all callers of addRecursive to pass through
their ctx (or derive one) so cancellation and structured logging propagate
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e909d99-0b8d-4891-a306-715d1d6e90af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mddocs/implementation-plan-exclude-patterns.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
capcom6
force-pushed
the
codex/plan-implementation-for-roadmap-feature
branch
from
241ece7 to
f399f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Around line 23-33: The New function currently calls normalizePath on each raw
rule which strips leading slashes and misinterprets absolute rules; update New
to detect absolute rules (using filepath.IsAbs) and handle them explicitly: if a
rule is absolute, convert it to a path relative to absSourceRoot (use
filepath.Rel(absSourceRoot, raw)) and reject with an error if the absolute rule
is outside sourceRoot, then pass the resulting relative path into normalizePath;
alternatively, if absolute rules should be unsupported, return a clear error
from New when filepath.IsAbs(raw) is true instead of silently normalizing them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0262868-53d4-40d1-8da6-644379d53485
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
✅ Files skipped from review due to trivial changes (1)
- main.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/exclude/matcher.go (3)
79-93: Minor: avoid building the ancestor list when no pattern rules exist, and skip the literal-prefix check for pattern rules.Two small tweaks on the hot path (every sync/watch event calls this):
pathCandidateswalks the path for every call, even when all rules are literal.- For a pattern rule like
**/*.tmp, thenormalized == r.literal || HasPrefix(normalized, r.literal+"/")check on line 81 can never succeed and is pure overhead.Proposed tweak
- ancestors := pathCandidates(normalized) - for _, r := range m.rules { - if normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") { - return true, r.literal - } - - if r.isPattern { - for _, candidate := range ancestors { - matched, err := doublestar.Match(r.pattern, candidate) - if err == nil && matched { - return true, r.pattern - } - } - } - } + var ancestors []string + for _, r := range m.rules { + if !r.isPattern { + if normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") { + return true, r.literal + } + continue + } + if ancestors == nil { + ancestors = pathCandidates(normalized) + } + for _, candidate := range ancestors { + if matched, err := doublestar.Match(r.pattern, candidate); err == nil && matched { + return true, r.pattern + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 79 - 93, The pathCandidates list is built unconditionally and the literal-prefix check runs for pattern rules; change the logic in the match loop to: first, detect whether any m.rules have r.isPattern and only call pathCandidates(normalized) when at least one pattern exists; second, for each rule skip the literal equality/prefix check when r.isPattern is true (only perform normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") for non-pattern rules), and keep the existing doublestar.Match loop for pattern rules (returning r.pattern on match) so literal rules still return r.literal immediately.
12-16: Redundantliteral/patternfields onrule.Both fields are always assigned the same
normalizedvalue, sorulecarries duplicated state. A singlevalue string(plusisPattern) keeps the struct smaller and the intent clearer.Proposed simplification
type rule struct { - literal string - pattern string - isPattern bool + value string + isPattern bool } @@ - compiled = append(compiled, rule{ - literal: normalized, - pattern: normalized, - isPattern: isPattern, - }) + compiled = append(compiled, rule{ + value: normalized, + isPattern: isPattern, + })Update
MatchRuleaccordingly (r.valuein both the literal-prefix check and thedoublestar.Matchcall).Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 12 - 16, The rule struct currently duplicates state with literal and pattern both set to the same normalized value; replace these two fields with a single value string plus the existing isPattern bool, update all usages (including the MatchRule function) to read r.value for both the literal-prefix check and the doublestar.Match call, and remove the redundant fields and any code that sets both literal and pattern so the struct and related logic only store and use r.value and r.isPattern.
73-73:isDirparameter is accepted but ignored.Both
MatchandMatchRuletake anisDir bool, callers ininternal/syncer/syncer.goandinternal/watcher/watcher.gopass it, but the implementation discards it via_ bool. Either:
- drop the parameter from the public API to avoid misleading callers into thinking directory-specific rules (e.g., trailing-slash rules à la
.gitignore) are honored, or- actually use it — e.g., only allow a rule ending in
/to match whenisDiris true, which also restores the "match directories only" semantics lost tofilepath.Cleanon line 31.Given the watcher currently hard-codes
falseon line 261 ofinternal/watcher/watcher.go, whichever path you pick, the caller contract should be made explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` at line 73, Matcher.Match and Matcher.MatchRule currently accept an isDir bool but ignore it (signature uses `_ bool`), so either remove the parameter from both APIs or honor it; to fix, decide on one approach and make caller contract explicit: either remove isDir from Matcher.Match and Matcher.MatchRule signatures and update all callers to stop passing it, or implement directory-only matching (e.g., treat rules ending with '/' as matching only when isDir is true and preserve other semantics lost by filepath.Clean) by using the passed isDir in both Matcher.Match and Matcher.MatchRule and update the caller that currently passes a hard-coded false to pass the actual directory flag. Ensure you update both methods (Matcher.Match, Matcher.MatchRule) and all callers to be consistent with the chosen contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Line 31: The code uses filepath.Clean on strings.TrimSpace(raw) which silently
removes a trailing slash and thus loses directory-only intent; preserve that
intent by detecting whether the original raw string (before TrimSpace/Clean)
ends with a path separator (e.g. strings.HasSuffix(raw, "/") or
os.PathSeparator) and carry that flag forward instead of letting filepath.Clean
drop it; update the logic around the cleanedRaw assignment (the
filepath.Clean(strings.TrimSpace(raw)) expression) and downstream handling that
currently ignores the isDir parameter (see the comment near line 73) so that
directory-only matches are honored when the original pattern had a trailing
slash, and ensure any canonicalization still uses cleanedRaw for normalization
while relying on the new flag for directory-only semantics.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 79-93: The pathCandidates list is built unconditionally and the
literal-prefix check runs for pattern rules; change the logic in the match loop
to: first, detect whether any m.rules have r.isPattern and only call
pathCandidates(normalized) when at least one pattern exists; second, for each
rule skip the literal equality/prefix check when r.isPattern is true (only
perform normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/")
for non-pattern rules), and keep the existing doublestar.Match loop for pattern
rules (returning r.pattern on match) so literal rules still return r.literal
immediately.
- Around line 12-16: The rule struct currently duplicates state with literal and
pattern both set to the same normalized value; replace these two fields with a
single value string plus the existing isPattern bool, update all usages
(including the MatchRule function) to read r.value for both the literal-prefix
check and the doublestar.Match call, and remove the redundant fields and any
code that sets both literal and pattern so the struct and related logic only
store and use r.value and r.isPattern.
- Line 73: Matcher.Match and Matcher.MatchRule currently accept an isDir bool
but ignore it (signature uses `_ bool`), so either remove the parameter from
both APIs or honor it; to fix, decide on one approach and make caller contract
explicit: either remove isDir from Matcher.Match and Matcher.MatchRule
signatures and update all callers to stop passing it, or implement
directory-only matching (e.g., treat rules ending with '/' as matching only when
isDir is true and preserve other semantics lost by filepath.Clean) by using the
passed isDir in both Matcher.Match and Matcher.MatchRule and update the caller
that currently passes a hard-coded false to pass the actual directory flag.
Ensure you update both methods (Matcher.Match, Matcher.MatchRule) and all
callers to be consistent with the chosen contract.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ead20beb-99eb-4dfa-93ca-b31c0a4032fe
📒 Files selected for processing (4)
internal/exclude/errors.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.go
✅ Files skipped from review due to trivial changes (1)
- internal/exclude/matcher_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/syncer/syncer.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exclude/matcher.go (1)
147-149:hasMetamisses doublestar's character-class and brace metacharacters.
doublestartreats[,],{,}as glob metacharacters, buthasMetaonly detects*?. A rule likefile[0-9].logor{a,b}/creturns false fromhasMeta, causing the pattern to skip validation and be treated as a literal match—silently failing to match as a glob pattern.♻️ Suggested change
func hasMeta(pattern string) bool { - return strings.ContainsAny(pattern, "*?") + // Mirror the metacharacters recognized by doublestar: *, ?, [..], {..}. + return strings.ContainsAny(pattern, "*?[{") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 147 - 149, hasMeta currently only checks for "*" and "?" using strings.ContainsAny, but doublestar also treats "[", "]", "{", "}" as glob metacharacters; update hasMeta to detect those additional characters so patterns like "file[0-9].log" or "{a,b}/c" are recognized as globs. Modify the function hasMeta to include the bracket and brace characters in the character set checked (keep using strings.ContainsAny or equivalent) so it returns true for any of "*?[]{}". Ensure you update the code paths that call hasMeta (e.g., the glob validation/branching logic) so they will now treat those patterns as globs rather than literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Line 164: The file internal/exclude/matcher.go is not correctly formatted
(EOF/newline or import grouping) causing golangci-lint to fail; fix by running
goimports -w internal/exclude/matcher.go (or gofmt -s -w
internal/exclude/matcher.go) to normalize imports and add the trailing newline,
then re-run the linter/CI; ensure the final commit includes the formatted
matcher.go changes.
- Around line 85-103: The directory-only guard in the loop over m.rules (in
matcher.go) is too broad: the check "if r.isDir && !isDir { continue }" prevents
descendant literal-prefix matches (e.g., r.literal "node_modules" should exclude
"node_modules/foo.js"); change the logic in the rule iteration so the r.isDir &&
!isDir branch only skips when the current path is an exact match of the rule
(compare normalized == r.literal), but still allow the literal-prefix check
(strings.HasPrefix(normalized, r.literal+"/")) to run for descendants; keep the
existing pattern handling (r.isPattern with doublestar.Match over ancestors)
unchanged.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 147-149: hasMeta currently only checks for "*" and "?" using
strings.ContainsAny, but doublestar also treats "[", "]", "{", "}" as glob
metacharacters; update hasMeta to detect those additional characters so patterns
like "file[0-9].log" or "{a,b}/c" are recognized as globs. Modify the function
hasMeta to include the bracket and brace characters in the character set checked
(keep using strings.ContainsAny or equivalent) so it returns true for any of
"*?[]{}". Ensure you update the code paths that call hasMeta (e.g., the glob
validation/branching logic) so they will now treat those patterns as globs
rather than literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b991017-5b05-4c99-b283-f8f676a85632
📒 Files selected for processing (1)
internal/exclude/matcher.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exclude/matcher.go (1)
29-44:⚠️ Potential issue | 🟠 MajorAbsolute exclude rules still silently misbehave.
filepath.ToSlashpreserves the leading/, so a rule like/repo/dist/**is stored as/repo/dist/**and is then matched against cleaned relative paths likedist/foo.js— it will never match. Either relativize absolute rules againstsourceRootup front (rejecting rules outside the root), or reject absolute rules with a clear error fromNew. Also considerstrings.TrimSpaceand skipping empty entries so accidental blank lines in config don't produce a no-op rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 44, Trim each raw rule (use strings.TrimSpace) and skip empty entries before calling filepath.ToSlash in the code that builds the compiled []rule (the constructor New or the function that creates compiled). After normalizing, detect absolute rules (leading "/") and either relativize them against the package's sourceRoot parameter (rejecting and returning an error if the absolute path is outside sourceRoot) or return a clear ErrInvalidPattern-style error for absolute rules; update the compiled append logic for rule{value: normalized, isPattern: hasMeta(normalized)} accordingly so absolute rules no longer silently fail. Ensure validation still uses doublestar.ValidatePattern and keep references to ErrInvalidPattern, rule, hasMeta, and the New constructor for locating changes.
🧹 Nitpick comments (3)
internal/exclude/matcher.go (1)
36-38: Remove the commented-out**prefixing block.Dead/commented code should not ship — either re-enable it with a rationale or delete it. Leaving it in invites drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 36 - 38, Remove the dead commented block that checks for a "**" prefix (the lines using strings.HasPrefix(normalized, "**") and normalized = "/" + normalized) in internal/exclude/matcher.go; either delete these commented lines entirely so no commented-out logic remains around the normalized variable, or restore them as active code with a short rationale comment if the "**" prefix handling is required (update any related tests in functions referencing normalized accordingly).internal/syncer/syncer.go (2)
101-138: Deduplicate the per-entry exclusion block.
childAbsPath/childRelPathare constructed twice and the exclusion-check + debug-log pattern is copy-pasted across the dir and file branches. Hoisting it above thefile.IsDir()switch tightens the loop and keeps the log/skip behavior consistent. Same pattern also appears inSync(lines 49-55) and could be extracted into a small helper likeskipIfExcluded(ctx, abs, rel string) boolthat returns true after logging.Proposed refactor
for _, file := range files { select { case <-ctx.Done(): return nil default: } - if file.IsDir() { - childAbsPath := filepath.Join(absPath, file.Name()) - childRelPath := filepath.Join(relPath, file.Name()) - if matched, rule := s.isExcluded(childAbsPath, childRelPath); matched { - s.logger.Debug(ctx, "Excluded path skipped", logger.Fields{ - "path": childRelPath, - "rule": rule, - }) - continue - } - - if dErr := s.syncDir(ctx, childAbsPath, childRelPath); dErr != nil { - return dErr - } - - continue - } - childAbsPath := filepath.Join(absPath, file.Name()) childRelPath := filepath.Join(relPath, file.Name()) if matched, rule := s.isExcluded(childAbsPath, childRelPath); matched { s.logger.Debug(ctx, "Excluded path skipped", logger.Fields{ "path": childRelPath, "rule": rule, }) continue } + if file.IsDir() { + if dErr := s.syncDir(ctx, childAbsPath, childRelPath); dErr != nil { + return dErr + } + continue + } + if fErr := s.syncFile(ctx, childAbsPath, childRelPath); fErr != nil { return fErr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 101 - 138, The loop duplicates construction of childAbsPath/childRelPath and the exclusion-check+debug log in both branches; hoist childAbsPath := filepath.Join(absPath, file.Name()) and childRelPath := filepath.Join(relPath, file.Name()) to the top of the per-file loop, call s.isExcluded(childAbsPath, childRelPath) once and continue if matched (logging the rule), and then call s.syncDir(ctx, childAbsPath, childRelPath) or s.syncFile(ctx, childAbsPath, childRelPath) based on file.IsDir(); also extract the same pattern used in Sync into a small helper like skipIfExcluded(ctx, abs, rel string) bool that calls s.isExcluded and s.logger.Debug so both syncer.go loop and Sync use that helper to avoid duplication.
160-170: TherelPathfallback is likely redundant.
MatchRulealready convertsabsPathto a path relative tosourceRootviafilepath.Relinternally, so tryingabsPathfirst and thenrelPathdouble-normalizes. If the absolute-path call succeeds in relativization, the second call just repeats the same work; if it fails (path outside source), retrying withrelPathwon't produce a different match. Consider passing onlyabsPathand dropping the fallback — or, if the fallback exists to paper over the Windows separator bug flagged inmatcher.go, fixing that root cause is preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 160 - 170, The fallback to calling s.matcher.MatchRule(relPath) in Syncer.isExcluded is redundant because MatchRule already relativizes absPath; remove the second call and have isExcluded only call and return the result of s.matcher.MatchRule(absPath) (i.e., change isExcluded to call MatchRule(absPath) and return that bool/string), remove the now-unused relPath parameter from isExcluded and update all callers accordingly; if the fallback was intended to address the Windows-separator issue in matcher.go, instead fix that normalization in matcher.MatchRule so relPath fallback is unnecessary, then remove the fallback call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Around line 3-11: Replace the stdlib log usage and import with the injected
logger by adding a logger.Logger parameter to New (the matcher constructor used
by the watcher/syncer) and store it on the matcher struct, remove the "log"
import from internal/exclude/matcher.go, and change the noisy log.Printf calls
in MatchRule (and any other places) to use the stored logger at Debug level (or
remove them entirely) so linting passes and structured logging is used
consistently.
- Around line 57-67: The MatchRule function uses filepath.ToSlash on filePath
before calling filepath.Rel, causing separator mismatch with m.sourceRoot on
Windows; fix by first computing relPath := filepath.Rel(m.sourceRoot, filePath)
(use original OS separators), only if err == nil set normalized = relPath, then
convert normalized to forward slashes via filepath.ToSlash and then call
path.Clean; update the logic in Matcher.MatchRule (references:
Matcher.MatchRule, m.sourceRoot, normalized, filepath.Rel, filepath.ToSlash,
path.Clean) so relative paths are computed with OS separators and normalized
afterwards.
---
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 29-44: Trim each raw rule (use strings.TrimSpace) and skip empty
entries before calling filepath.ToSlash in the code that builds the compiled
[]rule (the constructor New or the function that creates compiled). After
normalizing, detect absolute rules (leading "/") and either relativize them
against the package's sourceRoot parameter (rejecting and returning an error if
the absolute path is outside sourceRoot) or return a clear
ErrInvalidPattern-style error for absolute rules; update the compiled append
logic for rule{value: normalized, isPattern: hasMeta(normalized)} accordingly so
absolute rules no longer silently fail. Ensure validation still uses
doublestar.ValidatePattern and keep references to ErrInvalidPattern, rule,
hasMeta, and the New constructor for locating changes.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 36-38: Remove the dead commented block that checks for a "**"
prefix (the lines using strings.HasPrefix(normalized, "**") and normalized = "/"
+ normalized) in internal/exclude/matcher.go; either delete these commented
lines entirely so no commented-out logic remains around the normalized variable,
or restore them as active code with a short rationale comment if the "**" prefix
handling is required (update any related tests in functions referencing
normalized accordingly).
In `@internal/syncer/syncer.go`:
- Around line 101-138: The loop duplicates construction of
childAbsPath/childRelPath and the exclusion-check+debug log in both branches;
hoist childAbsPath := filepath.Join(absPath, file.Name()) and childRelPath :=
filepath.Join(relPath, file.Name()) to the top of the per-file loop, call
s.isExcluded(childAbsPath, childRelPath) once and continue if matched (logging
the rule), and then call s.syncDir(ctx, childAbsPath, childRelPath) or
s.syncFile(ctx, childAbsPath, childRelPath) based on file.IsDir(); also extract
the same pattern used in Sync into a small helper like skipIfExcluded(ctx, abs,
rel string) bool that calls s.isExcluded and s.logger.Debug so both syncer.go
loop and Sync use that helper to avoid duplication.
- Around line 160-170: The fallback to calling s.matcher.MatchRule(relPath) in
Syncer.isExcluded is redundant because MatchRule already relativizes absPath;
remove the second call and have isExcluded only call and return the result of
s.matcher.MatchRule(absPath) (i.e., change isExcluded to call MatchRule(absPath)
and return that bool/string), remove the now-unused relPath parameter from
isExcluded and update all callers accordingly; if the fallback was intended to
address the Windows-separator issue in matcher.go, instead fix that
normalization in matcher.MatchRule so relPath fallback is unnecessary, then
remove the fallback call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02fba150-69a4-4cdb-8413-71045c8dc19f
📒 Files selected for processing (4)
internal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exclude/matcher_test.go
Comment on lines
+3
to
+11
| import ( | ||
| "fmt" | ||
| "log" | ||
| "path" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/bmatcuk/doublestar/v4" | ||
| ) |
There was a problem hiding this comment.
Replace log with the injected logger to unblock CI.
depguard is failing the lint job on this import (import 'log' is not allowed from list 'non-main files'). The log.Printf calls at lines 66-67 also fire on every MatchRule invocation — once per watcher event and once per traversed entry — which is both noisy and unstructured. Take a logger.Logger in New (the watcher/syncer already thread one through) and drop these to Debug, or remove them entirely.
🧰 Tools
🪛 GitHub Actions: Go
[error] 5-5: golangci-lint (depguard): import 'log' is not allowed from list 'non-main files': Use log/slog instead (see https://go.dev/blog/slog).
🪛 GitHub Check: Lint
[failure] 5-5:
import 'log' is not allowed from list 'non-main files': Use log/slog instead, see https://go.dev/blog/slog (depguard)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/exclude/matcher.go` around lines 3 - 11, Replace the stdlib log
usage and import with the injected logger by adding a logger.Logger parameter to
New (the matcher constructor used by the watcher/syncer) and store it on the
matcher struct, remove the "log" import from internal/exclude/matcher.go, and
change the noisy log.Printf calls in MatchRule (and any other places) to use the
stored logger at Debug level (or remove them entirely) so linting passes and
structured logging is used consistently.
Comment on lines
+57
to
+67
| func (m *Matcher) MatchRule(filePath string) (bool, string) { | ||
| normalized := filepath.ToSlash(filePath) | ||
| relPath, err := filepath.Rel(m.sourceRoot, normalized) | ||
| if err == nil { | ||
| normalized = relPath | ||
| } | ||
|
|
||
| normalized = path.Clean(normalized) | ||
|
|
||
| log.Printf("Normalizing %s to %s", filePath, normalized) | ||
| log.Printf("Matching %s against %v", normalized, m.rules) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "matcher.go" | head -20Repository: capcom6/sftp-sync
Length of output: 90
🏁 Script executed:
cat -n ./internal/exclude/matcher.goRepository: capcom6/sftp-sync
Length of output: 2505
🏁 Script executed:
grep -r "MatchRule\|Matcher" . --include="*.go" -n | grep -E "(test|Test)" | head -20Repository: capcom6/sftp-sync
Length of output: 451
🏁 Script executed:
cat -n ./internal/exclude/matcher_test.goRepository: capcom6/sftp-sync
Length of output: 3861
🏁 Script executed:
cat -n ./internal/exclude/matcher.go | sed -n '1,50p'Repository: capcom6/sftp-sync
Length of output: 1364
🏁 Script executed:
go doc filepath.RelRepository: capcom6/sftp-sync
Length of output: 675
🏁 Script executed:
rg "filepath\.Rel" --type go -B 2 -A 2Repository: capcom6/sftp-sync
Length of output: 1030
🏁 Script executed:
cat -n ./internal/watcher/watcher.go | grep -A 5 -B 5 "filepath.Rel"Repository: capcom6/sftp-sync
Length of output: 391
🏁 Script executed:
cat -n ./internal/syncer/syncer.go | grep -A 5 -B 5 "filepath.Rel"Repository: capcom6/sftp-sync
Length of output: 518
🏁 Script executed:
python3 << 'EOF'
import os
import sys
# Simulate what happens on Windows with filepath.Rel and mismatched separators
# This demonstrates the core issue
# On Windows:
# m.sourceRoot = "C:\repo" (from filepath.Abs)
# After ToSlash: "C:/repo"
# filePath = "C:/repo/src/main.go" (after ToSlash)
# The issue is that filepath.Rel expects both arguments in the OS's native format
# When you mix separators, it can fail or produce unexpected results
print("===== Windows path separator mismatch scenario =====")
print()
print("Case 1: Correct (both with backslashes)")
print(' filepath.Rel("C:\\repo", "C:\\repo\\src\\main.go")')
print(" Result: src\\main.go (expected)")
print()
print("Case 2: Problematic (mismatched separators)")
print(' filepath.Rel("C:\\repo", "C:/repo/src/main.go")')
print(" Result: Error or bogus path like ..\\..\\C:\\repo\\src\\main.go")
print()
print("This is exactly what the code does currently:")
print(" 1. normalized = filepath.ToSlash(filePath) // converts to /")
print(" 2. filepath.Rel(m.sourceRoot, normalized) // m.sourceRoot has \\")
print()
print("The proposed fix addresses this by:")
print(" 1. Computing relative path with OS separators first")
print(" 2. Converting to forward slashes AFTER getting the relative path")
EOFRepository: capcom6/sftp-sync
Length of output: 726
🏁 Script executed:
# Let me also check if there are any integration tests or how the code is tested
find . -type f -name "*test*.go" | xargs grep -l "Match\|Matcher" | head -5Repository: capcom6/sftp-sync
Length of output: 95
Fix Windows path separator mismatch in filepath.Rel call.
On Windows, m.sourceRoot contains backslashes (from filepath.Abs at line 24), but normalized is converted to forward slashes via filepath.ToSlash at line 58. Calling filepath.Rel(m.sourceRoot, normalized) with mismatched separators will fail, causing the code to silently fall back to matching against absolute paths instead of relative ones. This breaks exclude rule matching on Windows.
Compute the relative path first using OS separators, then convert to forward slashes:
Proposed fix
-func (m *Matcher) MatchRule(filePath string) (bool, string) {
- normalized := filepath.ToSlash(filePath)
- relPath, err := filepath.Rel(m.sourceRoot, normalized)
- if err == nil {
- normalized = relPath
- }
-
- normalized = path.Clean(normalized)
-
- log.Printf("Normalizing %s to %s", filePath, normalized)
- log.Printf("Matching %s against %v", normalized, m.rules)
+func (m *Matcher) MatchRule(filePath string) (bool, string) {
+ candidate := filePath
+ if filepath.IsAbs(candidate) {
+ if rel, err := filepath.Rel(m.sourceRoot, candidate); err == nil {
+ candidate = rel
+ }
+ }
+ normalized := path.Clean(filepath.ToSlash(candidate))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/exclude/matcher.go` around lines 57 - 67, The MatchRule function
uses filepath.ToSlash on filePath before calling filepath.Rel, causing separator
mismatch with m.sourceRoot on Windows; fix by first computing relPath :=
filepath.Rel(m.sourceRoot, filePath) (use original OS separators), only if err
== nil set normalized = relPath, then convert normalized to forward slashes via
filepath.ToSlash and then call path.Clean; update the logic in Matcher.MatchRule
(references: Matcher.MatchRule, m.sourceRoot, normalized, filepath.Rel,
filepath.ToSlash, path.Clean) so relative paths are computed with OS separators
and normalized afterwards.
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
1 participant
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.
Motivation
*,**,?) so users can ignore groups of files likenode_modules/**or**/*.tmpinstead of enumerating paths.Description
internal/exclude/matcher.gowhich implementsNewandMatch/MatchRuleusinggithub.com/bmatcuk/doublestar/v4for doublestar-style glob matching and path normalization.internal/cli/commands/sync/sync.goand updatingmain.goand flag help text to mention glob support.internal/watcherandinternal/syncerso watcher registration and sync operations early-return for excluded paths and log which rule caused the skip.README.md, adddocs/implementation-plan-exclude-patterns.md, updatego.modwith the new dependency, and include unit tests ininternal/exclude/matcher_test.gocovering literal and pattern cases plus normalization and absolute-path boundaries.Testing
go test ./internal/exclude -vand passed.go test ./...andgo build ./...and succeeded.Codex Task
Summary by CodeRabbit
New Features
Documentation
Tests
Chores