perf: fixed issues with argument handling, command lookup & file pro… #23574
Conversation
📝 WalkthroughWalkthroughThe pre-commit hook script has been updated to improve its robustness and clarity. Modifications include enhanced quoting for handling special characters, improved conditional checks using idiomatic Bash patterns, and a more reliable command presence check using Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Pre-commit Hook
participant Checker as f_check_cmds
participant Processor as File Processor
participant Formatter as Tools (gofmt, misspell, goimports)
Script->>Checker: Validate required commands using "command -v"
Checker-->>Script: Return command availability status
Script->>Script: Check if STAGED_GO_FILES exists using [[ -n ]]
Script->>Processor: Loop over each staged file
Processor->>Processor: Verify file pattern (.pb.go$) using refined regex
Processor->>Formatter: Execute formatting commands with quoted filenames
Formatter-->>Processor: Return command output
Processor-->>Script: File processing complete
Script->>Script: Evaluate overall exit status with f_exit_success
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
contrib/githooks/pre-commit (4)
8-10: Consider using printf for better format control.While the current implementation is safer with quoted arguments, using
printfwould provide better control over output formatting and special character handling.- echo "$@" >&2 + printf '%s\n' "$@" >&2
20-26: LGTM! More reliable command checking.The change to use
command -vinstead ofwhichis a significant improvement as it's more reliable and POSIX compliant. The variable quoting also enhances safety.Consider enhancing the error message to be more informative:
- f_exit_success "Couldn't find '$cmd', skipping" + f_exit_success "Required command '$cmd' not found. Please install it to enable pre-commit checks."
32-35: Consider adding file existence checks.While the script handles file patterns correctly, it would be more robust to verify file existence before processing.
for file in $STAGED_GO_FILES; do + if [[ ! -f "$file" ]]; then + f_echo_stderr "Warning: '$file' not found, skipping" + continue + fi if [[ "$file" =~ vendor/ ]] || [[ "$file" =~ tests/mocks/ ]] || [[ "$file" =~ \.pb\.go$ ]]; then
37-41: LGTM! Safer command execution.The addition of quotes around
"$file"in all commands ensures safe handling of filenames with spaces or special characters.Consider adding error handling for the git add command:
- git add "$file" + if ! git add "$file"; then + f_echo_stderr "Warning: Failed to stage changes for '$file'" + fi
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/githooks/pre-commit(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
contrib/githooks/pre-commit (2)
12-18: LGTM! Improved argument handling.The change to use
[[ -n "$*" ]]is a more idiomatic way to check for non-empty arguments in bash. The function correctly handles the exit status and argument passing.
30-31: LGTM! Better empty check.The change to use
[[ -n "$STAGED_GO_FILES" ]]is a more idiomatic way to check for non-empty strings in bash.
Description
This PR addresses several issues in the script to ensure better functionality and compatibility.
Key changes include:
f_exit_success: Fixed how arguments are passed tof_echo_stderrand ensuredexit 0works correctly after output.whichwithcommand -v: Switched fromwhichtocommand -vfor better compatibility and reliability.STAGED_GO_FILES: Improved the check to handle cases where the variable might contain spaces or be empty..pb.goregex: Updated regex to match only files ending with.pb.goand avoid false positives.These changes make the script more robust and prevent errors related to file handling and command lookup.
Author Checklist
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
I have...
Summary by CodeRabbit
Chores
Bug Fixes