Fix some file renaming woes (probably) #20128

calumapplepie · 2023-12-16T04:43:57Z

Hullo, new contributer, wanted to avoid others losing data the way I just did!

I added a few new checks that, to my knowlege, will prevent users from renaming files on top of files that exist. While this can be useful for reseeding, it causes instant data loss if qbittorrent has downloaded any part of the existing file.

I also do some cleanup on messages and exists() function.

Reject the PR if you'd prefer to move the functionality to a different place, but I had the itch to hack something up to fix this, and I couldn't resist.

Probably closes a few issues, but I wasn't sure which, and I should get back to final projects.

luzpaz · 2023-12-16T09:02:31Z

Thanks! Can you look at https://github.com/qbittorrent/qBittorrent/issues?q=is%3Aopen+is%3Aissue+label%3A%22Data+loss%22 and see which tickets this PR addresses ?

github-actions · 2024-02-15T00:14:25Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

calumapplepie · 2024-02-15T16:06:56Z

Not stale, I just don’t have the bandwidth to hunt through issues in an unfamiliar repo.

github-actions · 2024-04-16T00:14:25Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

calumapplepie · 2024-04-16T10:49:57Z

Not stale

github-actions · 2024-06-20T00:15:45Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

calumapplepie · 2024-06-20T01:22:24Z

Still not stale <3

Chocobo1 · 2024-06-20T06:15:20Z

src/base/bittorrent/abstractfilestorage.cpp

@@ -53,7 +55,7 @@ void BitTorrent::AbstractFileStorage::renameFile(const Path &oldPath, const Path
        if ((renamingFileIndex < 0) && (path == oldPath))
            renamingFileIndex = i;
        else if (path == newPath)
-            throw RuntimeError(tr("The file already exists: '%1'.").arg(newPath.toString()));
+            throw RuntimeError(tr("The file already exists in this torrent: '%1'.").arg(newPath.toString()));


This message is more confusing. There is no indication of what torrent it is referring to.

Chocobo1 · 2024-06-20T06:16:23Z

src/base/path.cpp

@@ -130,7 +130,7 @@ bool Path::isRelative() const

 bool Path::exists() const
 {
-    return !isEmpty() && QFileInfo::exists(m_pathStr);
+    return !isValid() && QFileInfo::exists(m_pathStr);


This change is excessive. Please revert it.

Chocobo1 · 2024-06-20T06:16:32Z

src/base/bittorrent/abstractfilestorage.cpp

@@ -70,6 +72,7 @@ void BitTorrent::AbstractFileStorage::renameFolder(const Path &oldFolderPath, co
        throw RuntimeError(tr("The new path is invalid: '%1'.").arg(newFolderPath.toString()));
    if (newFolderPath.isAbsolute())
        throw RuntimeError(tr("Absolute path isn't allowed: '%1'.").arg(newFolderPath.toString()));
+


Please don't make unrelated changes.

Suggested change

Chocobo1 · 2024-06-20T06:19:01Z

src/base/bittorrent/abstractfilestorage.cpp

@@ -81,7 +84,7 @@ void BitTorrent::AbstractFileStorage::renameFolder(const Path &oldFolderPath, co
        if (path.hasAncestor(oldFolderPath))
            renamingFileIndexes.append(i);
        else if (path.hasAncestor(newFolderPath))
-            throw RuntimeError(tr("The folder already exists: '%1'.").arg(newFolderPath.toString()));
+            throw RuntimeError(tr("The folder already exists in this torrent: '%1'.").arg(newFolderPath.toString()));


This message is more confusing. There is no indication of what torrent it is referring to.

Suggested change

throw RuntimeError(tr("The folder already exists in this torrent: '%1'.").arg(newFolderPath.toString()));

throw RuntimeError(tr("The folder path is the same: '%1'.").arg(newFolderPath.toString()));

Chocobo1 · 2024-06-20T06:20:07Z

src/base/bittorrent/abstractfilestorage.cpp

@@ -90,6 +93,6 @@ void BitTorrent::AbstractFileStorage::renameFolder(const Path &oldFolderPath, co
    for (const int index : renamingFileIndexes)
    {
        const Path newFilePath = newFolderPath / oldFolderPath.relativePathOf(filePath(index));
-        renameFile(index, newFilePath);
+        renameFile(index, newFilePath); // note; if file would be overwritten, we note that is true here


The comment audience is unclear and doesn't bear any context for the developer. Just don't add it.

Suggested change

renameFile(index, newFilePath); // note; if file would be overwritten, we note that is true here

renameFile(index, newFilePath);

Chocobo1 · 2024-06-20T06:28:21Z

src/base/bittorrent/abstractfilestorage.cpp

@@ -53,7 +55,7 @@ void BitTorrent::AbstractFileStorage::renameFile(const Path &oldPath, const Path
        if ((renamingFileIndex < 0) && (path == oldPath))
            renamingFileIndex = i;
        else if (path == newPath)
-            throw RuntimeError(tr("The file already exists: '%1'.").arg(newPath.toString()));
+            throw RuntimeError(tr("The file already exists in this torrent: '%1'.").arg(newPath.toString()));


Perhaps:

Suggested change

throw RuntimeError(tr("The file already exists in this torrent: '%1'.").arg(newPath.toString()));

throw RuntimeError(tr("The file path is the same: '%1'.").arg(newPath.toString()));

github-actions · 2024-08-20T00:16:48Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

github-actions · 2024-08-27T00:17:19Z

This PR was closed because it has been stalled for some time with no activity.

calumapplepie added 3 commits December 15, 2023 23:25

Check that a path is valid before verifying existence

5f8f94d

Check for file already existing before performing rename

4a1169c

Document how folder renames are protected against

b8861b2

github-actions bot added the Stale label Feb 15, 2024

github-actions bot removed the Stale label Feb 16, 2024

github-actions bot added the Stale label Apr 16, 2024

luzpaz added PR waiting review and removed Stale labels Apr 16, 2024

xavier2k6 removed the PR waiting review label Apr 20, 2024

xavier2k6 requested a review from a team April 20, 2024 09:24

github-actions bot added the Stale label Jun 20, 2024

Chocobo1 requested changes Jun 20, 2024

View reviewed changes

github-actions bot removed the Stale label Jun 21, 2024

github-actions bot added the Stale label Aug 20, 2024

github-actions bot closed this Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some file renaming woes (probably) #20128

Fix some file renaming woes (probably) #20128

calumapplepie commented Dec 16, 2023

luzpaz commented Dec 16, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2024

calumapplepie commented Feb 15, 2024

github-actions bot commented Apr 16, 2024

calumapplepie commented Apr 16, 2024 via email

github-actions bot commented Jun 20, 2024

calumapplepie commented Jun 20, 2024

Chocobo1 Jun 20, 2024

Chocobo1 Jun 20, 2024 •

edited

Loading

Chocobo1 Jun 20, 2024

Chocobo1 Jun 20, 2024

Chocobo1 Jun 20, 2024

Chocobo1 Jun 20, 2024

Chocobo1 Jun 20, 2024

github-actions bot commented Aug 20, 2024

github-actions bot commented Aug 27, 2024

	throw RuntimeError(tr("The folder already exists in this torrent: '%1'.").arg(newFolderPath.toString()));
	throw RuntimeError(tr("The folder path is the same: '%1'.").arg(newFolderPath.toString()));

	renameFile(index, newFilePath); // note; if file would be overwritten, we note that is true here
	renameFile(index, newFilePath);

	throw RuntimeError(tr("The file already exists in this torrent: '%1'.").arg(newPath.toString()));
	throw RuntimeError(tr("The file path is the same: '%1'.").arg(newPath.toString()));

Fix some file renaming woes (probably) #20128

Fix some file renaming woes (probably) #20128

Conversation

calumapplepie commented Dec 16, 2023

luzpaz commented Dec 16, 2023 • edited Loading

github-actions bot commented Feb 15, 2024

calumapplepie commented Feb 15, 2024

github-actions bot commented Apr 16, 2024

calumapplepie commented Apr 16, 2024 via email

github-actions bot commented Jun 20, 2024

calumapplepie commented Jun 20, 2024

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 20, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 20, 2024

github-actions bot commented Aug 27, 2024

luzpaz commented Dec 16, 2023 •

edited

Loading

Chocobo1 Jun 20, 2024 •

edited

Loading