Skip to content

Warn Windows users about backslash path separators in glob patterns - #1539

Open
subaru-ye wants to merge 1 commit into
alibaba:mainfrom
subaru-ye:fix/issue-1463-windows-separator-warning
Open

subaru-ye wants to merge 1 commit into
alibaba:mainfrom
subaru-ye:fix/issue-1463-windows-separator-warning

Conversation

@subaru-ye

Copy link
Copy Markdown

Summary

  • warn Windows users when --exclude contains probable backslash path separators
  • apply the same warning to selected rule.json include and exclude patterns
  • preserve supported doublestar escapes and existing matching semantics
  • quote warning values safely to prevent control characters from affecting terminal output

Testing

  • make check
  • make test
  • make coverage — 91.5%
  • ocr review --audience agent — 0 findings across 3 selected items

AI usage disclosure

This change was developed with assistance from OpenAI Codex (GPT-5). The required automated review was run with OpenCodeReview using DeepSeek Flash. I reviewed and understand all generated changes.

Fixes #1463

@github-actions

Copy link
Copy Markdown
Contributor

✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

@NanaseInori NanaseInori 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.

Reviewed all six changed files and #1463. This implements the issue's warning-only option: matching semantics and pattern contents remain unchanged. The issue search also found the later #1550 alternative; that overlap does not create a dependency for this earlier implementation.

The CLI path warns before appending its patterns, while rule loading warns only for the selected include/exclude layer. Both use stderr, leaving machine-readable stdout alone. The helper consumes trusted escape pairs together, detects dangling backslashes, and uses %q for user-controlled pattern text. No blocking defect found in the warning-only behavior.

I independently ran an isolated Go harness containing the exact new classifier/output algorithm: all 16 supplied classifier cases passed; the Windows branch emitted exactly three safely quoted warning lines for the mixed fixture; the Linux branch emitted none. This does not exercise doublestar itself or substitute for the repository/Windows integration suite.

One non-blocking test improvement: TestBuildFileFilter_WarnsOnlySelectedLayer searches for the raw backslash pattern, while the warning quotes it with doubled backslashes. Assert an empty buffer, or search for strconv.Quote(pattern), so that test would actually fail if an inactive layer were accidentally warned. The implementation currently returns after the selected layer, so this is test strength rather than a production defect.

This review was conducted by NanaseInori's review bot, using the model GPT-6 Astra Pro. If you need a human review, please manually @NanaseInori.

This branch has not been deployed

No deployments
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.

Windows-style path separators in --exclude and rule.json include/exclude patterns

2 participants