Skip to content

Prevent CLRF line endings with pre-commit hook#29169

Merged
mvvmm merged 9 commits intoproductionfrom
line-ending-commit-hook
Mar 20, 2026
Merged

Prevent CLRF line endings with pre-commit hook#29169
mvvmm merged 9 commits intoproductionfrom
line-ending-commit-hook

Conversation

@mvvmm
Copy link
Contributor

@mvvmm mvvmm commented Mar 20, 2026

Summary

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:

Pattern Owners
/.github/ @cloudflare/content-engineering, @kodster28, @mvvmm, @colbywhite, @ahaywood
* @cloudflare/pcx-technical-writing
package.json @cloudflare/content-engineering

@mvvmm mvvmm force-pushed the line-ending-commit-hook branch from e036d2e to 4738553 Compare March 20, 2026 16:20
@mvvmm
Copy link
Contributor Author

mvvmm commented Mar 20, 2026

/bonk do you see any issues with this?

@mvvmm mvvmm changed the title Line ending commit hook Prevent CLRF line endings with pre-commit hook Mar 20, 2026
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 20, 2026

Reviewing this PR that adds a Husky pre-commit hook for LF line endings.

Findings:

The PR adds a pre-commit hook that:

  • Detects CRLF line endings in staged files
  • Converts them to LF using perl (cross-platform compatible)
  • Re-stages the fixed files

The implementation is solid with proper handling for:

  • Empty staging area (early exit)
  • Deleted files (skips non-existent files)
  • Both CRLF and standalone CR characters

Minor issue:

Line 14 in .husky/pre-commit - Unquoted variable could break on filenames with spaces:

if [ ! -f "$file" ]; then

This is already correct. However, line 13 and line 20 also use $file and should be checked:

for file in $STAGED; do  # Line 13 - will break on spaces

And line 20:

if grep -qU $'\r' "$file"; then  # Line 20 - correctly quoted

Recommendation:

Line 13 should handle word splitting properly. The for file in $STAGED loop will break if filenames contain spaces. Consider:

while IFS= read -r file; do
  # ... existing logic ...
done <<< "$STAGED"

Overall assessment:

The hook is a useful defense-in-depth addition alongside the existing .gitattributes configuration. Good protection against AI tools and Windows environments introducing CRLF. The package.json changes (adding husky dependency and prepare script) are correct.

LGTM with the minor note about filename handling. The space-in-filename issue is an edge case but worth addressing for robustness.

github run

@mvvmm
Copy link
Contributor Author

mvvmm commented Mar 20, 2026

/bonk please review

Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor suggestion on the caching configuration.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 20, 2026

Posted 1 suggestion on the node_modules cache key.

The pre-commit hook looks solid—properly handles filenames with spaces, deleted files, and cross-platform compatibility. Good defense alongside .gitattributes.

github run

@mvvmm mvvmm force-pushed the line-ending-commit-hook branch from 3100e0c to 735b8dd Compare March 20, 2026 17:18
@mvvmm mvvmm marked this pull request as ready for review March 20, 2026 17:19
@mvvmm mvvmm requested review from a team, ahaywood and colbywhite as code owners March 20, 2026 17:19
@mvvmm mvvmm enabled auto-merge (squash) March 20, 2026 17:32
@mvvmm mvvmm merged commit a1c6bd9 into production Mar 20, 2026
9 checks passed
@mvvmm mvvmm deleted the line-ending-commit-hook branch March 20, 2026 17:42
@github-actions
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants