Feat: patcch record command#13
Open
RohitRavindra-dev wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a ChangesPatch recording
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PatchRecordCommand
participant PatchService
participant Filesystem
participant Git
User->>PatchRecordCommand: patch record path
PatchRecordCommand->>Filesystem: ValidateDevLocalFilesystem()
PatchRecordCommand->>PatchService: Run(path)
PatchService->>Filesystem: PathExists(path)
PatchService->>Git: Detect changed files
Git-->>PatchService: Changed file paths
PatchService-->>PatchRecordCommand: Status or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/filesystem/utils.go (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove the doc comment clarity.
The comment
// bool: isDir err: if path doesnt existsis grammatically incomplete and omits the apostrophe. Consider a proper Go doc comment starting with the function name.📝 Suggested doc comment
-// bool: isDir err: if path doesnt exists +// PathExists returns true if the path is a directory, false if it is a file. +// It returns an error if the path does not exist or stat fails. func PathExists(path string) (bool, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filesystem/utils.go` at line 12, Update the doc comment above the associated function in utils.go to start with that function’s name and clearly describe the returned boolean and error, including the correct “doesn’t exist” grammar.internal/service/patch/patcher.go (1)
11-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove excessive blank lines between statements.
Every statement is separated by a blank line, which hurts readability. Go convention is to group related statements without blank lines between them.
📝 Suggested cleanup
func listFilesToPatch(path string, isDir bool) ([]string, error) { - filesToPatch := []string{} - if isDir { filesWithChanges, err := git.ListFilesWithChangesInDir(path) if err != nil { return nil, err } filesToPatch = append(filesToPatch, filesWithChanges...) } else { hasChanges, err := git.FileHasChanges(path) if err != nil { return nil, err } if hasChanges { filesToPatch = append(filesToPatch, path) } } - return filesToPatch, nil - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/patch/patcher.go` around lines 11 - 56, Remove the unnecessary blank lines separating related statements in listFilesToPatch and Run, including around conditionals, error handling, and return statements. Keep blank lines only where they meaningfully separate distinct logical sections, without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/git/utils.go`:
- Around line 21-31: Update executeGitCommandWithOutput to accept a
context.Context parameter and construct the command with exec.CommandContext
instead of exec.Command, propagating the context so callers can cancel or
time-limit hung git operations. Update all callers to pass their appropriate
context while preserving the existing output and error formatting.
- Around line 59-78: Update ListFilesWithChangesInDir to normalize Git’s CRLF
output before returning filenames, ensuring entries split from filesWithChanges
do not retain trailing carriage returns. Preserve the existing empty-result
behavior and error propagation.
In `@internal/service/patch/patcher.go`:
- Around line 35-56: Update Run to remove the trailing exclamation mark from the
empty-path error so it satisfies ST1005, and change the status message before
completion to describe recording the patch rather than applying it. Keep the
existing control flow and file listing unchanged.
---
Nitpick comments:
In `@internal/filesystem/utils.go`:
- Line 12: Update the doc comment above the associated function in utils.go to
start with that function’s name and clearly describe the returned boolean and
error, including the correct “doesn’t exist” grammar.
In `@internal/service/patch/patcher.go`:
- Around line 11-56: Remove the unnecessary blank lines separating related
statements in listFilesToPatch and Run, including around conditionals, error
handling, and return statements. Keep blank lines only where they meaningfully
separate distinct logical sections, without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91775ae9-19c4-4206-a397-35c36967fcad
📒 Files selected for processing (6)
cmd/patch.gocmd/patch_record.gointernal/filesystem/utils.gointernal/git/utils.gointernal/service/patch/patcher.gotodos.md
Comment on lines
+21
to
+31
| // TODO make this the only git command util | ||
| func executeGitCommandWithOutput(args []string) (string, error) { | ||
| cmd := exec.Command("git", args...) | ||
|
|
||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return string(out), fmt.Errorf("failed to run git %v: %w\n%s", args, err, out) | ||
| } | ||
|
|
||
| return string(out), nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use exec.CommandContext for timeout/cancellation support.
exec.Command without a context means a hung git process will block the CLI indefinitely. The noctx linter also flags this. Consider accepting a context.Context (or at minimum a timeout) so callers can cancel long-running git operations.
🔒️ Proposed fix
-// TODO make this the only git command util
-func executeGitCommandWithOutput(args []string) (string, error) {
- cmd := exec.Command("git", args...)
+// TODO make this the only git command util
+func executeGitCommandWithOutput(ctx context.Context, args []string) (string, error) {
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ cmd := exec.CommandContext("git", args...)🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 23-23: os/exec.Command must not be called. use os/exec.CommandContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/git/utils.go` around lines 21 - 31, Update
executeGitCommandWithOutput to accept a context.Context parameter and construct
the command with exec.CommandContext instead of exec.Command, propagating the
context so callers can cancel or time-limit hung git operations. Update all
callers to pass their appropriate context while preserving the existing output
and error formatting.
Source: Linters/SAST tools
Comment on lines
+59
to
+78
| func ListFilesWithChangesInDir(dirpath string) ([]string, error) { | ||
| filesWithChanges, err := executeGitCommandWithOutput([]string{ | ||
| "diff", | ||
| "--name-only", | ||
| "--", | ||
| dirpath, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| filesWithChanges = strings.TrimSpace(filesWithChanges) | ||
|
|
||
| if filesWithChanges == "" { | ||
| return []string{}, nil | ||
| } | ||
|
|
||
| return strings.Split(filesWithChanges, "\n"), nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle carriage returns in filenames on Windows.
strings.Split(filesWithChanges, "\n") will leave trailing \r characters in filenames when git runs on Windows (CRLF line endings). This could cause downstream path mismatches. Consider using strings.Fields or trimming \r from each entry.
💚 Proposed fix
return strings.Split(filesWithChanges, "\n"), nil
+ // Alternative: handles \r\n on Windows
+ // return strings.Fields(filesWithChanges), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/git/utils.go` around lines 59 - 78, Update ListFilesWithChangesInDir
to normalize Git’s CRLF output before returning filenames, ensuring entries
split from filesWithChanges do not retain trailing carriage returns. Preserve
the existing empty-result behavior and error propagation.
Comment on lines
+35
to
+56
| func Run(path string) error { | ||
|
|
||
| if path == "" { | ||
| return fmt.Errorf("[Error] path is empty!") | ||
| } | ||
|
|
||
| isDir, err := filesystem.PathExists(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| filesToPatch, err := listFilesToPatch(path, isDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(filesToPatch) == 0 { | ||
| return fmt.Errorf("[Error] no files with changes at indicated path: %s", path) | ||
| } | ||
| fmt.Printf("[Started] applying patch to files: \n\t\t- %s\n", strings.Join(filesToPatch, "\n\t\t- ")) | ||
| fmt.Println("[Completed] recording new patch into devlocal") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix error string punctuation and misleading status message.
Two issues:
- Line 38:
"[Error] path is empty!"ends with!— staticcheck ST1005 requires error strings not end with punctuation or newlines. - Line 53: The message says "applying patch to files" but this is the
recordcommand, notapply. The message is misleading to users.
💚 Proposed fixes
- return fmt.Errorf("[Error] path is empty!")
+ return fmt.Errorf("[Error] path is empty")- fmt.Printf("[Started] applying patch to files: \n\t\t- %s\n", strings.Join(filesToPatch, "\n\t\t- "))
- fmt.Println("[Completed] recording new patch into devlocal")
+ fmt.Printf("[Started] recording patch for files: \n\t\t- %s\n", strings.Join(filesToPatch, "\n\t\t- "))
+ fmt.Println("[Completed] recording new patch into devlocal")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func Run(path string) error { | |
| if path == "" { | |
| return fmt.Errorf("[Error] path is empty!") | |
| } | |
| isDir, err := filesystem.PathExists(path) | |
| if err != nil { | |
| return err | |
| } | |
| filesToPatch, err := listFilesToPatch(path, isDir) | |
| if err != nil { | |
| return err | |
| } | |
| if len(filesToPatch) == 0 { | |
| return fmt.Errorf("[Error] no files with changes at indicated path: %s", path) | |
| } | |
| fmt.Printf("[Started] applying patch to files: \n\t\t- %s\n", strings.Join(filesToPatch, "\n\t\t- ")) | |
| fmt.Println("[Completed] recording new patch into devlocal") | |
| return nil | |
| } | |
| func Run(path string) error { | |
| if path == "" { | |
| return fmt.Errorf("[Error] path is empty") | |
| } | |
| isDir, err := filesystem.PathExists(path) | |
| if err != nil { | |
| return err | |
| } | |
| filesToPatch, err := listFilesToPatch(path, isDir) | |
| if err != nil { | |
| return err | |
| } | |
| if len(filesToPatch) == 0 { | |
| return fmt.Errorf("[Error] no files with changes at indicated path: %s", path) | |
| } | |
| fmt.Printf("[Started] recording patch for files: \n\t\t- %s\n", strings.Join(filesToPatch, "\n\t\t- ")) | |
| fmt.Println("[Completed] recording new patch into devlocal") | |
| return nil | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 38-38: ST1005: error strings should not end with punctuation or newlines
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/patch/patcher.go` around lines 35 - 56, Update Run to remove
the trailing exclamation mark from the empty-path error so it satisfies ST1005,
and change the status message before completion to describe recording the patch
rather than applying it. Keep the existing control flow and file listing
unchanged.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
patch record <file-or-directory>command to identify and record Git changes.Bug Fixes