Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the whitespace checking/fixing scripts against edge cases (invalid Unicode output, missing/empty results artifacts, and incorrect diff invocation) and adjusts the wrapper behavior to avoid running the fixer when the checker produced no usable output.
Changes:
- Replaced Unicode warning output with ASCII-safe text in the whitespace checker.
- Ensured
check-results.logexists even when there are zero commits to report, and switched parsing to use the in-memory$log. - Adjusted diff invocation in the fixer and gated running the fixer based on checker outcome/artifacts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Build/Agent/fix-whitespace.ps1 | Updates the fallback git diff call used to determine files to fix |
| Build/Agent/check-whitespace.ps1 | Makes results logging resilient to empty output and replaces Unicode warning text |
| Build/Agent/check-and-fix-whitespace.ps1 | Cleans stale results and gates fixer execution based on checker outcome |
| $base = Get-BaseRef | ||
| Write-Host "Fixing whitespace for files changed since $base..HEAD" | ||
| $fixFiles = git diff --name-only "$base"..HEAD | ||
| $fixFiles = git diff --name-only $base HEAD |
There was a problem hiding this comment.
Changing from git diff <base>..<head> semantics to git diff <base> <head> can alter which changes/files are selected (notably when the base ref is not a strict ancestor, since A..B uses the merge-base). If the intent is still 'files changed since base', consider passing a single revision-range argument (e.g., \"$base..HEAD\") rather than two separate commit arguments.
| $fixFiles = git diff --name-only $base HEAD | |
| $fixFiles = git diff --name-only "$base..HEAD" |
| $null = $log | Tee-Object -FilePath check-results.log | ||
| $log | Out-Host | ||
| if (-not (Test-Path -LiteralPath 'check-results.log')) { | ||
| New-Item -ItemType File -Path 'check-results.log' -Force | Out-Null |
There was a problem hiding this comment.
Use -LiteralPath for New-Item here (to match Test-Path -LiteralPath and avoid provider/path parsing surprises). This keeps path handling consistent and reduces edge-case failures with special characters.
| New-Item -ItemType File -Path 'check-results.log' -Force | Out-Null | |
| New-Item -ItemType File -LiteralPath 'check-results.log' -Force | Out-Null |
| if ($ec -eq 0 -or (Test-Path -LiteralPath 'check-results.log')) { | ||
| & "$PSScriptRoot/fix-whitespace.ps1" | ||
| } | ||
| else { | ||
| Write-Error 'Whitespace checker failed before producing results; skipping fixer.' | ||
| } |
There was a problem hiding this comment.
Using only file existence as a proxy for 'checker produced results' becomes less reliable now that the checker may create an empty check-results.log as a placeholder. To better match the intent ('skip fixer after failures without results'), consider checking for non-empty results (e.g., file length > 0) or using a more explicit success/results signal from the checker.
Summary
check-results.logValidation
./Build/Agent/check-and-fix-whitespace.ps1./Build/Agent/commit-messages.ps1CI: Full local checkThis change is