fix(cli): fix path printed by "kopia snapshot fix" commands #4638
Conversation
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
jkowalski
approved these changes
Jun 2, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4638 +/- ##
==========================================
+ Coverage 75.86% 76.40% +0.53%
==========================================
Files 470 529 +59
Lines 37301 40160 +2859
==========================================
+ Hits 28299 30683 +2384
- Misses 7071 7457 +386
- Partials 1931 2020 +89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
70b8b69 to
e76141c
Compare
Contributor
Author
|
Updated to (hopefully) fix lint errors. |
The dirRelativePath variable is actually the path to the file being checked, but was treated as if it was the path to the parent directory, causing the filename to be duplicated in log messages.
e76141c to
b70be5f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes duplicated filenames in the output of kopia snapshot fix commands by using the full file path instead of reconstructing it manually.
- Renamed
dirRelativePathtopathFromRootin bothrewriteEntrymethods. - Updated log and verifier calls to use
pathFromRootdirectly, removing manual concatenation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cli/command_snapshot_fix_remove_files.go | Renamed parameter and adjusted log messages to use the full path from the root. |
| cli/command_snapshot_fix_invalid_files.go | Similarly renamed parameter and updated verifier, log, and callback to use full path. |
Comments suppressed due to low confidence (3)
cli/command_snapshot_fix_remove_files.go:30
- [nitpick] Consider renaming
pathFromRootto a more concise name likeentryPathorfilePathto clearly reflect that this is the full file path being processed.
func (c *commandSnapshotFixRemoveFiles) rewriteEntry(ctx context.Context, pathFromRoot string, ent *snapshot.DirEntry) (*snapshot.DirEntry, error) {
cli/command_snapshot_fix_invalid_files.go:35
- [nitpick] Consider renaming
pathFromRootto a concise name likeentryPathorfilePathfor consistency and readability.
func (c *commandSnapshotFixInvalidFiles) rewriteEntry(ctx context.Context, pathFromRoot string, ent *snapshot.DirEntry) (*snapshot.DirEntry, error) {
cli/command_snapshot_fix_remove_files.go:30
- Add unit tests for
rewriteEntryto verify that the correct full file paths are logged when removing files, ensuring this fix is covered by tests.
func (c *commandSnapshotFixRemoveFiles) rewriteEntry(ctx context.Context, pathFromRoot string, ent *snapshot.DirEntry) (*snapshot.DirEntry, error) {
| for _, id := range c.removeObjectIDs { | ||
| if ent.ObjectID.String() == id { | ||
| log(ctx).Infof("will remove file %v", path.Join(dirRelativePath, ent.Name)) | ||
| log(ctx).Infof("will remove file %v", pathFromRoot) |
There was a problem hiding this comment.
Use %s instead of %v when formatting a string path in Infof to make the intention explicit and avoid relying on default formatting.
Suggested change
| log(ctx).Infof("will remove file %v", pathFromRoot) | |
| log(ctx).Infof("will remove file %s", pathFromRoot) |
|
|
||
| if matched { | ||
| log(ctx).Infof("will remove file %v", path.Join(dirRelativePath, ent.Name)) | ||
| log(ctx).Infof("will remove file %v", pathFromRoot) |
There was a problem hiding this comment.
Use %s instead of %v when formatting a string path in Infof to make the intention explicit and avoid relying on default formatting.
Suggested change
| log(ctx).Infof("will remove file %v", pathFromRoot) | |
| log(ctx).Infof("will remove file %s", pathFromRoot) |
| if err := c.verifier.VerifyFile(ctx, ent.ObjectID, fname); err != nil { | ||
| log(ctx).Warnf("removing invalid file %v due to: %v", fname, err) | ||
| if err := c.verifier.VerifyFile(ctx, ent.ObjectID, pathFromRoot); err != nil { | ||
| log(ctx).Warnf("removing invalid file %v due to: %v", pathFromRoot, err) |
There was a problem hiding this comment.
Use %s instead of %v when logging a string path to make the formatting clearer and more intentional.
Suggested change
| log(ctx).Warnf("removing invalid file %v due to: %v", pathFromRoot, err) | |
| log(ctx).Warnf("removing invalid file %s due to: %v", pathFromRoot, err) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The
dirRelativePathvariable is actually the path to the file being checked, but was treated as if it was the path to the parent directory, causing the filename to be duplicated in log messages. This PR fixes that.Here is the output of
remove-fileson stock Kopia 0.20.1:Evidently, the last path component is duplicated. The paths printed do not actually exist, but the paths printed minus the duplicated last path component do exist.
For demonstration of correctness of the fix, here is the output after adding a log statement to print
dirRelativePathandent.Name:It can be seen that the path is the path of the file being checked, not the parent directory.