git: worktree, Don't delete local untracked files when resetting worktree #1540
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refines the worktree reset functionality so that local untracked files are preserved when resetting the worktree, addressing issue #53.
- Updates the test in worktree_test.go to verify that untracked files (e.g. "foo") remain unaltered.
- Modifies the ResetSparsely, resetIndex, and resetWorktree functions in worktree.go to track removed files and adjust the file checkout logic based on untracked status.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| worktree_test.go | Replaces a simple IsClean assertion with a loop to validate specific file statuses. |
| worktree.go | Returns removed file information from resetIndex and uses it in resetWorktree to decide which files to check out. |
| continue | ||
| } | ||
| if st.Worktree != Unmodified || st.Staging != Unmodified { | ||
| s.Fail("file %s not unmodified", file) |
There was a problem hiding this comment.
It appears that s.Fail is being used with a format string; consider using s.Failf to properly format error messages.
| s.Fail("file %s not unmodified", file) | |
| s.Failf("file %s not unmodified", file) |
Copilot uses AI. Check for mistakes.
| // only checkout an untracked file if it is in the list of files | ||
| // a reset should leave untracked files alone | ||
| file := nameFromAction(&ch) | ||
| if status.File(file).Worktree != Untracked || inFiles(files, file) { |
There was a problem hiding this comment.
[nitpick] The compound conditional here could be refactored into a well-named variable or helper function to enhance readability.
| if status.File(file).Worktree != Untracked || inFiles(files, file) { | |
| if shouldCheckoutChange(status, file, files) { |
Copilot uses AI. Check for mistakes.
| // only checkout an untracked file if it is in the list of files | ||
| // a reset should leave untracked files alone | ||
| file := nameFromAction(&ch) | ||
| if status.File(file).Worktree != Untracked || inFiles(files, file) { |
There was a problem hiding this comment.
Can we use ch instead of status? I think something like
action, err := ch.Action()
if err != nil {
return err
}
if action != merkletrie.Insert {
// checkout the file
}Would work since "Untracked" would mean that the file appears in worktree but not in staging.
For reference, Status() also works this way internally.
Lines 93 to 118 in 93ed29d
This way we can avoid re-doing computations that we already have done.
| // only checkout an untracked file if it is in the list of files | ||
| // a reset should leave untracked files alone | ||
| file := nameFromAction(&ch) | ||
| if status.File(file).Worktree != Untracked || inFiles(files, file) { |
There was a problem hiding this comment.
Is there any specific reason that we check inFiles here? I think it is already done in block above.
I wasn't paying attention to the comment. Sorry for the confusion.
Fixes #53