Skip to content
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

Fix relative file path bug #11

Merged
merged 2 commits into from
Mar 13, 2023
Merged

Conversation

Nehliin
Copy link
Contributor

@Nehliin Nehliin commented Mar 12, 2023

The documentation states that the file path should be relative to the torrent name but any sub directory structures were ignored. This fixes that so files in sub directories have paths with the structure intact.

Also the TorrentBuilder sets the name to the last component to the path according to the docs which seems wrong. The name should be either the file name in the single file case or a directory name in the multi file case according to BEP-3. Overall the entire name field in the Torrent struct seems very fragile. I'd suggest having a separate "path" filed that is an actual path to the root directory of the torrent which all file paths are relative to. The name field can still exist and may or may not match the path.

@ttlajus
Copy link
Owner

ttlajus commented Mar 12, 2023

Thank you for reporting this! It is indeed a bug.

However, note that path here has already been canonicalized, so simply storing it as-is does not give us the relative path, even though that does preserve the directory hierarchy.

I'll fix it tomorrow, or you can give it another try if you wish : )


Also the TorrentBuilder sets the name to the last component to the path according to the docs which seems wrong. The name should be either the file name in the single file case or a directory name in the multi file case according to BEP-3.

Aren't they the same? My code simply calls std::path::Path::file_name(), which

Returns the final component of the Path, if there is one.
If the path is a normal file, this is the file name. If it’s the path of a directory, this is the directory name.


Overall the entire name field in the Torrent struct seems very fragile. I'd suggest having a separate "path" filed that is an actual path to the root directory of the torrent which all file paths are relative to. The name field can still exist and may or may not match the path.

Torrent files can also be inspected without being downloaded. This is actually the primary use case for this library. Assigning a local path makes little sense in this case.

If you do want to download torrents, the torrent client should have its own struct to manage all the info (metadata from Torrent, data about local files, data about trackers/peers, etc).

@Nehliin
Copy link
Contributor Author

Nehliin commented Mar 12, 2023

However, note that path here has already been canonicalized, so simply storing it as-is does not give us the relative path, even though that does preserve the directory hierarchy.

Yeah I was primarily interested in making it relative as in subdirectories etc remained but you are right it's not actually a relative path. I can fix that.

Aren't hey the same? My code simply calls std::path::Path::file_name(), which

ah didn't look at code the implementation should have done that.

Torrent files can also be inspected without being downloaded. This is actually the primary use case for this library. Assigning a local path makes little sense in this case.

If you do want to download torrents, the torrent client should have its own struct to manage all the info (metadata from Torrent, data about local files, data about trackers/peers, etc).

My point was mainly about the potential disconnect between the directory structure and name when constructing the Torrents via TorrentBuilder but fair enough I can see that the disconnect might be desirable in certain scenarios and can assume the name will always be the root (file or directory) and leave it at that. It also makes more sense after my first comment was clarified 👍 .

@ttlajus ttlajus merged commit b03a4cf into ttlajus:master Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants