-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
make the save path for part files configurable #8119
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
Conversation
b7d78ea to
adc0235
Compare
|
Responding to #7194 (comment) Yes, you make a good point. A relative path makes more sense. |
|
would it be possible for you to make it default to .parts ? if everyone has a different directory it'll be annoying when bug reports happen. in fact i would honestly suggest just hardcoding it to .parts |
Then it would break backwards compatibility. part files for torrents downloaded with the previous version would not be picked up. |
adc0235 to
d21bed8
Compare
|
Oh, I think we misunderstood each other. I would still suggest being able to turn the utilization of a subdir on and off, however:
|
|
the way this feature is "turned off" right now (in this patch) is to leave the |
|
I strongly suggest making part_file_dir non-configurable:
The reasons for that are as follows: What do we win by making this configurable? Honestly, nothing that I can see. One could say it's for customization but we weren't able to customize the names of the parts files either and they were just being spewed into the save directory, so this is better in every way even without customization. What do we lose?
|
|
oh also, when initializing, checking a boolean is loads faster than checking if a string is empty... especially with thousands of torrents, that will help |
Backwards compatibility
I don't believe you. But even if it's slightly faster, it makes no difference in this context.
Very marginally. The complicated parts are not related to these checks. Can you point out some specific complexity in this patch that you're concerned about?
Their respective part files are stored in different directories. The name of that directory is stored in the resume data though. |
I think you misunderstood me. I suggest making it possible to turn using subdirs on and off and default to off. That ensures backwards compatibility like you want. Configuring the name of the subdir does not provide any backwards compatibility, since currently there is no subdir at all. I suggest making the name of the subdir NOT configurable, hardcode it, and decide whether it's being used by using a boolean flag. The flag is what makes it backwards compatible. |
|
@cheater I still think that since we've already gotten into this, we need to make it as flexible as possible (as I mentioned, just let you set both relative and absolute paths). This still does not oblige the frontend application to provide all these possibilities to the user, but it at least makes it easier to further modify this behavior if necessary (without having to make new changes to libtorrent, which is much more difficult to do). |
What? No. You probably came up with this and then didn't read my messages properly. I explain it three times. Let me explain it in another way that frames things the way that you frame it. I suggest:
I'm not asking to "change from one hardcoded setting to another".
Front-end validation is a bug introducing antipattern. I've been trying to explain this earlier on. The choices should be limited by the data structures in use, not by the front end UI. This means that data on disk cannot assume values that would be impossible to represent with the UI. |
The ".parts" directory name you suggested is the hard coded option.
You're suggesting adding another hard coded name, after saying you don't like the first hard coded name. I believe @glassez's point is that someone else may have an equally strong argument for a different name, and then we have 3 hardcoded options. and so on.
You say this, but you don't really make a case for it. Compare it to other APIs. |
d21bed8 to
c2096bb
Compare
|
Hey man, I don't think you understand what front-end validation means when talking about bad patterns ... anyways, don't worry about it. Do what you want. I don't care if the subdir is called ".parts", "smurfs", "vodka", or anything else. I was just trying to save people a future headache, but nvm. |
|
a client of a shared library isn't necessarily a "front-end". |
It's you who talk about "front-end validation" so you could clarify what you mean. I just said that it's a good idea to have library flexibly configurable (in those aspects where it doesn't require unnecessary complexity). @cheater |
c2096bb to
7ab44d4
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.
Pull request overview
This pull request adds a configurable subdirectory path for part files in torrents. Part files store partial pieces for files with zero priority, and previously they were always stored directly in the download directory. This feature allows users to organize these part files into a subdirectory.
Changes:
- Added
part_file_dirfield toadd_torrent_paramsto specify a relative subdirectory path for part files - Updated storage implementations (posix_storage and mmap_storage) to handle the custom part file directory
- Added serialization/deserialization support for the new field in resume data
- Updated Python bindings to expose the new field
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| include/libtorrent/add_torrent_params.hpp | Added part_file_dir field with documentation |
| include/libtorrent/storage_defs.hpp | Added part_file_dir parameter to storage_params constructor, changed path to string_view |
| include/libtorrent/aux_/torrent.hpp | Added m_part_file_dir member variable |
| include/libtorrent/aux_/posix_storage.hpp | Added m_part_file_dir member to posix_storage |
| include/libtorrent/aux_/mmap_storage.hpp | Added m_part_file_dir member to mmap_storage |
| include/libtorrent/aux_/storage_utils.hpp | Updated delete_files signature to take full part file path |
| src/torrent.cpp | Initialize and validate m_part_file_dir, updated debug logging |
| src/session_handle.cpp | Added validation that part_file_dir is not an absolute path |
| src/posix_storage.cpp | Implemented part file directory handling in all operations |
| src/mmap_storage.cpp | Implemented part file directory handling in all operations |
| src/storage_utils.cpp | Updated delete_files to accept full part file path |
| src/write_resume_data.cpp | Added serialization of part_file_dir |
| src/read_resume_data.cpp | Added deserialization of part_file_dir |
| src/create_torrent.cpp | Added empty part_file_dir parameter to storage_params construction |
| test/test_storage.cpp | Added comprehensive tests for part file directory functionality and move operations |
| test/test_read_resume.cpp | Added roundtrip test for part_file_dir |
| test/test_disk_io.cpp | Added empty part_file_dir parameter |
| test/make_torrent.cpp | Added empty part_file_dir parameter |
| tools/disk_io_stress_test.cpp | Added empty part_file_dir parameter |
| examples/connection_tester.cpp | Added empty part_file_dir parameter |
| bindings/python/src/session.cpp | Added Python binding support for part_file_dir |
| ChangeLog | Documented the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7ab44d4 to
7fb193f
Compare
No description provided.