refactor: handle prepare-commit-msg hook installation errors#249
refactor: handle prepare-commit-msg hook installation errors#249appleboy merged 1 commit intoappleboy:mainfrom
prepare-commit-msg hook installation errors#249Conversation
- Add specific error handling for `prepare-commit-msg` hook installation Signed-off-by: doodleEsc <[email protected]>
WalkthroughAdjusts InstallHook() in git/git.go to ignore os.IsNotExist errors when checking the hook target path, allowing hook creation if the target file is missing. Existing-file errors still stop execution; other unexpected errors are returned unchanged. No other logic or API surface changes. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant G as InstallHook()
participant FS as FileSystem
C->>G: InstallHook(target)
G->>FS: IsFile(target)
alt Error is os.IsNotExist
note right of G: Target missing → continue
G->>FS: Create hook file and set permissions
G-->>C: Success/creation result
else File exists
note right of G: Existing hook
G-->>C: Error "hook file ... exist"
else Other error
note right of G: Unexpected error
G-->>C: Propagate error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @doodleEsc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the error handling logic for the prepare-commit-msg Git hook installation. The change ensures that the installation process correctly distinguishes between actual errors and cases where the hook file simply does not exist, preventing unnecessary failures and improving the robustness of the hook installation.
Highlights
- Refined Error Handling for Git Hook Installation: Modified the
InstallHookfunction ingit/git.goto refine error handling when checking for the existence of theprepare-commit-msghook file. Previously, any error fromfile.IsFilewould halt the process; now,os.IsNotExisterrors are specifically ignored, allowing the installation to proceed if the file simply doesn't exist.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
#248 pr for this issue |
There was a problem hiding this comment.
Code Review
This pull request correctly adds error handling for the prepare-commit-msg hook installation, specifically to ignore os.ErrNotExist when checking if the hook file exists. My review includes a suggestion to refactor the new logic for improved readability and maintainability.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
- Coverage 21.96% 6.79% -15.18%
==========================================
Files 8 33 +25
Lines 346 1870 +1524
==========================================
+ Hits 76 127 +51
- Misses 264 1734 +1470
- Partials 6 9 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
git/git.go (4)
205-207: Avoid TOCTOU: create the hook atomically with O_EXCL.Between the existence check and WriteFile, another process could create the file. Use an atomic create to prevent clobbering and surface an accurate “already exists” error.
-// Write the hook file with executable permissions (0o755) -return os.WriteFile(target, content, 0o755) //nolint:gosec +// Write the hook file with executable permissions (0o755) atomically +f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o755) //nolint:gosec +if err != nil { + if os.IsExist(err) { + return fmt.Errorf("hook file already exists: %s", target) + } + return err +} +defer f.Close() +if _, err := f.Write(content); err != nil { + return err +} +return nil
197-197: Nit: standardize and clarify error messages (include path).Clearer user-facing errors help troubleshooting.
- return errors.New("hook file prepare-commit-msg exist") + return fmt.Errorf("hook file already exists: %s", target)- return errors.New("hook file prepare-commit-msg is not exist") + return fmt.Errorf("hook file does not exist: %s", target)Also applies to: 223-223
218-221: UninstallHook: treat os.IsNotExist consistently with InstallHook.Mirror the improved install behavior so a missing hook produces a friendly “does not exist” message instead of propagating a raw error.
exists, err := file.IsFile(target) if err != nil { - return err + if os.IsNotExist(err) { + return fmt.Errorf("hook file does not exist: %s", target) + } + return err }
8-8: Prefer filepath over path for OS-native paths.path is for slash-separated paths; filepath handles OS-specific separators. Low risk, improves portability on Windows.
-import ( +import ( "errors" "fmt" "os" "os/exec" - "path" + "path/filepath" "strconv" "strings"- target := path.Join(strings.TrimSpace(string(hookPath)), HookPrepareCommitMessageTemplate) + target := filepath.Join(strings.TrimSpace(string(hookPath)), HookPrepareCommitMessageTemplate)- target := path.Join(strings.TrimSpace(string(hookPath)), HookPrepareCommitMessageTemplate) + target := filepath.Join(strings.TrimSpace(string(hookPath)), HookPrepareCommitMessageTemplate)Also applies to: 191-191, 217-217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
git/git.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
git/git.go (2)
cmd/hook.go (1)
init(10-13)git/hook.go (1)
init(29-33)
🔇 Additional comments (1)
git/git.go (1)
193-195: Good fix: ignore os.IsNotExist so a missing hook can be created.This aligns behavior with intent: a nonexistent target should not abort installation, while other errors still surface.
|
@doodleEsc Thanks. |
prepare-commit-msghook installationSummary by CodeRabbit