-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Choose the longest valid mount point #97
Conversation
e1203bd
to
da8ce63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
I gave it a go to make the code a little more idiomatic as well if you want to take a look.
However, I am a bit anxious about the performance impliciations of this change and would prefer if the mount-points that are passed in would be sorted to begin with.
Then the function could be simpler and continue using the first match, while also being less wasteful.
Is this something you would want to do for practice?
Thanks for the quick response! I can give it a shot, but wouldn't this just increase the time complexity? Right now this runs in O(n) time, and a sort would bring that up to O(n log n). |
Thanks! The idea was to pre-sort the mount-points, as this functions seems to be called in a couple of places and potentially many items. If it makes no sense to you, I'd understand, it can certainly be merged as is without anybody filing complaints about performance. |
Oh, I see what you're saying. I think that's smarter. |
Ok, how does this look? |
73ca3ec
to
8cb3f75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this should work.
I will create a new patch release soon after merging.
And here it is: https://github.com/Byron/trash-rs/releases/tag/v3.2.1 |
Awesome, thank you! |
If the user has a drive mounted at
/run
and/run/media/test
, and they are trashing a file/run/media/test/remove-me
, depending on the order the mount point iterates through, it could incorrectly choose/run
as the mount point, leading to a failed trash.This simple pull request takes the longest valid mount point and uses that for the trash directory. This should prevent the issue above.
Note: I have not done too much rust development, so I'm not entirely sure that this is the simplest way to solve this. Please let me know if you see of anything that can be improved.
Related: #57