Add a homeDir field in LocalFileSystem for removefile to check #4543
+274
−11
Conversation
This file contains 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
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
679049a
to
00c5853
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
b3203e6
to
8a42558
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
b97e001
to
e40d223
Compare
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
f748cb1
to
681381f
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
3c5bb2b
to
8a31a89
Compare
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4543 +/- ##
==========================================
+ Coverage 87.34% 87.36% +0.02%
==========================================
Files 1348 1349 +1
Lines 56508 56639 +131
Branches 7112 7131 +19
==========================================
+ Hits 49357 49484 +127
- Misses 6981 6983 +2
- Partials 170 172 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
acquamarin
reviewed
Nov 19, 2024
| auto subStr = subPath.generic_string(); | ||
|
|
||
| // Check if subStr starts with baseStr and ensure proper boundaries | ||
| return subStr.starts_with(baseStr) && |
There was a problem hiding this comment.
I don't think the string based comparison is safe?
I will let @mewim comment on this
There was a problem hiding this comment.
I think we already guaranteed the path is a standard form absolute path that contains no .. or . , also checked the length of two path and their ending characters?
There was a problem hiding this comment.
Changed to a safer solution , which uses filesystem::relative to check the relation between two absolute dirs, should be safe enough
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
6a06a2a
to
6c089de
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
3e0e0f0
to
ec6fd02
Compare
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
f33c7c1
to
3019481
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
a0c2d49
to
dab9e8c
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
7961376
to
e488e21
Compare
Benchmark ResultMaster commit hash:
|
Tests are added via c api |
acquamarin
reviewed
Nov 21, 2024
|
|
||
| // Cleanup: Remove directories after the test | ||
| std::filesystem::remove_all("/tmp/test2"); | ||
| std::filesystem::remove_all("/tmp/dbHome/test2"); |
There was a problem hiding this comment.
Can you also add those test cases:
- Delete directories with edge cases.
- Test whether it works as expected on windows since windows paths can have a mixture of '\' and '/'.
For example: homedir: "C:\Desktop\dir"
Path to delete: "C:\Desktop/dir/test1" - Test whether we error in wild card pattern paths. (e.g. path to delete: '/tmp/kuzu*.test')
- Test when we delete a path with homedirectory symbol (e.g. path to delete "~/test")
There was a problem hiding this comment.
added more tests, we do not support wild card patter for safety.
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
aa25126
to
3572b8a
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
dac964e
to
a54fa52
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
698804b
to
f6e7168
Compare
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
2cf9207
to
50b7e91
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
995bcf4
to
7955894
Compare
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
a3870c9
to
f78dd3c
Compare
SterlingT3485
force-pushed
the
sterling_remove_only_db
branch
from
d564220
to
6ff8697
Compare
Benchmark ResultMaster commit hash:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Added this for
removeFileIfExists(const std::string& path)to only remove the files under our DBpathFixes #4529
Contributor agreement